Repository: source-academy/js-slang
Pull request #118 (2018-11-08 02:07:01)

Refactor for statement check into rules
The for statement requiring all 3 parts should be refactored out of the interpreter and into rules, avoiding duplication of such a check for the metacircular interpreter once it uses the same parser as the Source interpreter.

coveralls: ## Pull Request Test Coverage Report for [Build 278](https://coveralls.io/builds/19974717)

* **16** of **17**   **(94.12%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.08%**) to **52.617%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [src/rules/forStatementMustHaveAllParts.ts](https://coveralls.io/builds/19974717/source?filename=src%2Frules%2FforStatementMustHaveAllParts.ts#L21) | 15 | 16 | 93.75%
<!-- | **Total:** | **16** | **17** | **94.12%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/19974717/badge)](https://coveralls.io/builds/19974717) |
| :-- | --: |
| Change from base [Build 266](https://coveralls.io/builds/19931948): |  0.08% |
| Covered Lines: | 994 |
| Relevant Lines: | 1764 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/src/createContext.ts b/src/createContext.ts
index 3686acc5..50678b0c 100644
--- a/src/createContext.ts
+++ b/src/createContext.ts
@@ -50,7 +50,15 @@ export const ensureGlobalEnvironmentExist = (context: Context) => {
 
 const defineSymbol = (context: Context, name: string, value: Value) => {
   const globalFrame = context.runtime.frames[0]
-  globalFrame.environment[name] = value
+  Object.defineProperty(
+    globalFrame.environment,
+    name,
+    {
+      value,
+      writable: false,
+      enumerable: true
+    }
+  )
 }
 
 export const importExternalSymbols = (context: Context, externalSymbols: string[]) => {
@@ -143,7 +151,7 @@ export const importBuiltins = (context: Context, externalBuiltIns: CustomBuiltIn
       function () {
         return context.metaCircularParser
           .parse.apply(context.metaCircularParser, arguments)
-      });
+      })
     defineSymbol(context, 'apply_in_underlying_javascript', function(
       fun: Function,
       args: Value
diff --git a/src/interpreter.ts b/src/interpreter.ts
index 0af46d41..d5262efc 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -86,6 +86,7 @@ function defineVariable(context: Context, name: string, value: Value, constant=f
 
   return frame
 }
+
 function* visit(context: Context, node: es.Node) {
   context.runtime.nodes.unshift(node)
   yield context
@@ -321,15 +322,6 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
     return new BreakValue()
   },
   ForStatement: function*(node: es.ForStatement, context: Context) {
-    // Check that all 3 expressions are not empty in a for loop
-    const missing_parts = ["init", "test", "update"].filter(
-      part => node[part] === null
-    );
-    if (missing_parts.length > 0) {
-      const error = new errors.EmptyForExpression(node, missing_parts);
-      handleError(context, error);
-    }
-
     // Create a new block scope for the loop variables
     const loopFrame = createBlockFrame(context, "forLoopFrame")
     pushFrame(context, loopFrame)
diff --git a/src/rules/forStatementMustHaveAllParts.ts b/src/rules/forStatementMustHaveAllParts.ts
new file mode 100644
index 00000000..6a3de415
--- /dev/null
+++ b/src/rules/forStatementMustHaveAllParts.ts
@@ -0,0 +1,44 @@
+import { stripIndent } from 'common-tags'
+import * as es from 'estree'
+
+import { ErrorSeverity, ErrorType, Rule, SourceError } from '../types'
+
+export class ForStatmentMustHaveAllParts implements SourceError {
+  public type = ErrorType.SYNTAX
+  public severity = ErrorSeverity.ERROR
+
+  constructor(public node: es.ForStatement, private missingParts :string[]) {}
+
+  get location() {
+    return this.node.loc!
+  }
+
+  public explain() {
+    return `Missing ${this.missingParts.join(', ')} expression${this.missingParts.length === 1 ? '' : 's'} in for statement.`
+  }
+
+  public elaborate() {
+    return stripIndent`
+      This for statement requires all three parts (initialiser, test, update) to be present.
+    `
+  }
+}
+
+const forStatementMustHaveAllParts: Rule<es.ForStatement> = {
+  name: 'for-statement-must-have-all-parts',
+
+  checkers: {
+    ForStatement(node: es.ForStatement) {
+      const missingParts = ["init", "test", "update"].filter(
+        part => node[part] === null
+      );
+      if (missingParts.length > 0) {
+        return [new ForStatmentMustHaveAllParts(node, missingParts)]
+      } else {
+        return []
+      }
+    }
+  }
+}
+
+export default forStatementMustHaveAllParts
diff --git a/src/rules/index.ts b/src/rules/index.ts
index 7ef3213f..c5bde15b 100644
--- a/src/rules/index.ts
+++ b/src/rules/index.ts
@@ -14,6 +14,7 @@ import noNonEmptyList from './noNonEmptyList'
 import noUnspecifiedLiteral from './noUnspecifiedLiteral'
 import noUnspecifiedOperator from './noUnspecifiedOperator'
 import singleVariableDeclaration from './singleVariableDeclaration'
+import forStatementMustHaveAllParts from './forStatementMustHaveAllParts'
 
 const rules: Array<Rule<es.Node>> = [
   bracesAroundIfElse,
@@ -27,7 +28,8 @@ const rules: Array<Rule<es.Node>> = [
   noDeclareReserved,
   noDeclareMutable,
   noUnspecifiedLiteral,
-  noUnspecifiedOperator
+  noUnspecifiedOperator,
+  forStatementMustHaveAllParts
 ]
 
 export default rules


Test patch:
diff --git a/src/__tests__/__snapshots__/disallowed_syntax.ts.snap b/src/__tests__/__snapshots__/disallowed_syntax.ts.snap
index f1d6c96e..6579a98c 100644
--- a/src/__tests__/__snapshots__/disallowed_syntax.ts.snap
+++ b/src/__tests__/__snapshots__/disallowed_syntax.ts.snap
@@ -1,11 +1,11 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`Cannot leave blank expressions in for loops 1`] = `"Line 2: For statement cannot have empty init expression."`;
+exports[`Cannot leave blank expressions in for loops 1`] = `"Line 2: Missing init expression in for statement."`;
 
-exports[`Cannot leave blank expressions in for loops 2`] = `"Line 2: For statement cannot have empty test expression."`;
+exports[`Cannot leave blank expressions in for loops 2`] = `"Line 2: Missing test expression in for statement."`;
 
-exports[`Cannot leave blank expressions in for loops 3`] = `"Line 2: For statement cannot have empty update expression."`;
+exports[`Cannot leave blank expressions in for loops 3`] = `"Line 2: Missing update expression in for statement."`;
 
-exports[`Cannot leave blank expressions in for loops 4`] = `"Line 2: For statement cannot have empty init,test,update expressions."`;
+exports[`Cannot leave blank expressions in for loops 4`] = `"Line 2: Missing init, test, update expressions in for statement."`;
 
 exports[`Cannot use update expressions 1`] = `"Line 3: Update expressions are not allowed"`;
diff --git a/src/__tests__/__snapshots__/interpreter-errors.ts.snap b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
index 429aef8d..6b7585b8 100644
--- a/src/__tests__/__snapshots__/interpreter-errors.ts.snap
+++ b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
@@ -27,6 +27,58 @@ Array [
 ]
 `;
 
+exports[`Error when assigning to builtin 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Error when assigning to builtin 2`] = `
+Array [
+  ConstAssignment {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 11,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "name": "map",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`Error when assigning to builtin 3`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Error when assigning to builtin 4`] = `
+Array [
+  ConstAssignment {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 17,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "name": "undefined",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
 exports[`Error when assigning to property on undefined 1`] = `
 Object {
   "status": "error",
diff --git a/src/__tests__/interpreter-errors.ts b/src/__tests__/interpreter-errors.ts
index 39644f92..330133bd 100644
--- a/src/__tests__/interpreter-errors.ts
+++ b/src/__tests__/interpreter-errors.ts
@@ -15,6 +15,34 @@ test("Undefined variable error is thrown", () => {
   })
 })
 
+test('Error when assigning to builtin', () => {
+  const code = `
+    map = 5;
+   `;
+  const context = mockContext(3)
+  const promise = runInContext(code, context, { scheduler: 'preemptive' })
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot()
+    expect(obj.status).toBe('error')
+    expect(context.errors).toMatchSnapshot()
+    expect(parseError(context.errors)).toBe('Line 2: Cannot assign new value to constant map')
+  })
+})
+
+test('Error when assigning to builtin', () => {
+  const code = `
+    undefined = 5;
+   `;
+  const context = mockContext(3)
+  const promise = runInContext(code, context, { scheduler: 'preemptive' })
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot()
+    expect(obj.status).toBe('error')
+    expect(context.errors).toMatchSnapshot()
+    expect(parseError(context.errors)).toBe('Line 2: Cannot assign new value to constant undefined')
+  })
+})
+
 test('Error when assigning to property on undefined', () => {
   const code = `
     undefined.prop = 123;
@@ -100,4 +128,4 @@ test('In case a function ever returns null, should throw an error as well', () =
     expect(context.errors).toMatchSnapshot()
     expect(parseError(context.errors)).toBe('Line 3: Cannot read property prop of null')
   })
-})
\ No newline at end of file
+})