Repository: source-academy/js-slang
Pull request #62 (2018-09-22 07:08:50)

Add block scoping for loops
As in title. 
Also added separate scoping for loop variables and loop blocks in for loops (see test cases, `block_scoping.ts`

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

* **11** of **11**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**3.8%**) to **55.61%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/19135792/badge)](https://coveralls.io/builds/19135792) |
| :-- | --: |
| Change from base [Build 84](https://coveralls.io/builds/19023067): |  3.8% |
| Covered Lines: | 714 |
| Relevant Lines: | 1205 |

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

openorclose: ```let``` declarations are not correctly freshly bound at each iteration of the loop. See following example.

```javascript
function test(){
      let z = [];
      for (let x = 0; x < 10; x = x + 1) {
        z[x] = () => x;
      }
      return z[5]();
}
test();
```

```test```` should return 5 but instead returns 10.
yyc: @openorclose Interesting, nice catch! Is this behaviour documented anywhere in the JS spec?
openorclose: [Link to the spec](https://www.ecma-international.org/ecma-262/6.0/#sec-for-statement-runtime-semantics-labelledevaluation).
[Mozilla blog on it](https://hacks.mozilla.org/2015/07/es6-in-depth-let-and-const/).

Motive for this new binding in es2015 most likely comes from wanting to bind event handlers dynamically more easily, rather than the boilerplate creation of an IIFE to trap the needed scope.
yyc: Fixed. Thanks @openorclose!

Patch:
diff --git a/package.json b/package.json
index 49c669c9..e5456e04 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "js-slang",
-  "version": "0.1.7",
+  "version": "0.1.8",
   "description": "Javascript-based interpreter for slang, written in Typescript",
   "author": {
     "name": "Source Academy",
diff --git a/src/interpreter.ts b/src/interpreter.ts
index 65b348ab..67655742 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -4,7 +4,7 @@ import * as es from 'estree'
 import * as constants from './constants'
 import { toJS } from './interop'
 import * as errors from './interpreter-errors'
-import { ArrowClosure, Closure, Context, ErrorSeverity, Frame, SourceError, Value } from './types'
+import { ArrowClosure, Closure, Context, ErrorSeverity, Frame, SourceError, Value, Environment } from './types'
 import { createNode } from './utils/node'
 import * as rttc from './utils/rttc'
 
@@ -42,20 +42,19 @@ const createFrame = (
   })
   return frame
 }
-
 const createBlockFrame = (
   context: Context,
-  vars: es.Identifier[],
-  args: Value[],
+  name = "blockFrame",
+  environment: Environment = {}
 ): Frame => {
   const frame: Frame = {
-    name: 'ifStatementBlock',
+    name,
     parent: currentFrame(context),
-    environment: {},
+    environment,
     thisContext: context
-  }
-  return frame
-}
+  };
+  return frame;
+};
 
 const handleError = (context: Context, error: SourceError) => {
   context.errors.push(error)
@@ -303,13 +302,27 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
     return new BreakValue()
   },
   ForStatement: function*(node: es.ForStatement, context: Context) {
+    // Create a new block scope for the loop variables
+    const frame = createBlockFrame(context, "forLoopFrame")
+    pushFrame(context, frame)
+
     if (node.init) {
       yield* evaluate(node.init, context)
     }
     let test = node.test ? yield* evaluate(node.test, context) : true
     let value
     while (test) {
+      // create block context and shallow copy loop frame environment
+      // see https://www.ecma-international.org/ecma-262/6.0/#sec-for-statement-runtime-semantics-labelledevaluation
+      // and https://hacks.mozilla.org/2015/07/es6-in-depth-let-and-const/
+      const bound_env = { ...currentFrame(context).environment }
+      const frame = createBlockFrame(context, "forBlockFrame", bound_env)
+      pushFrame(context, frame)
+
       value = yield* evaluate(node.body, context)
+
+      // Remove block context
+      popFrame(context);
       if (value instanceof ContinueValue) {
         value = undefined
       }
@@ -325,6 +338,9 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
       }
       test = node.test ? yield* evaluate(node.test, context) : true
     }
+
+    popFrame(context);
+
     if (value instanceof BreakValue) {
       return undefined
     }
@@ -384,7 +400,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
     }
 
     // Create a new frame (block scoping)
-    const frame = createBlockFrame(context, [], [])
+    const frame = createBlockFrame(context, "ifBlockFrame")
     pushFrame(context, frame)
 
     if (test) {
@@ -425,7 +441,13 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
       !(value instanceof BreakValue) &&
       !(value instanceof TailCallReturnValue)
     ) {
+      // Create a new frame (block scoping)
+      const frame = createBlockFrame(context, "whileBlockFrame")
+      pushFrame(context, frame)
+
       value = yield* evaluate(node.body, context)
+
+      popFrame(context)
     }
     if (value instanceof BreakValue) {
       return undefined


Test patch:
diff --git a/src/__tests__/__snapshots__/block_scoping.ts.snap b/src/__tests__/__snapshots__/block_scoping.ts.snap
new file mode 100644
index 00000000..47f6e8c8
--- /dev/null
+++ b/src/__tests__/__snapshots__/block_scoping.ts.snap
@@ -0,0 +1,43 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`const uses block scoping instead of function scoping 1`] = `
+Object {
+  "status": "finished",
+  "value": true,
+}
+`;
+
+exports[`for loop \`let\` variables are copied into the block scope 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`for loops use block scoping instead of function scoping 1`] = `
+Object {
+  "status": "finished",
+  "value": true,
+}
+`;
+
+exports[`interior of for loops don't affect loop variables 1`] = `
+Object {
+  "status": "finished",
+  "value": 4,
+}
+`;
+
+exports[`let uses block scoping instead of function scoping 1`] = `
+Object {
+  "status": "finished",
+  "value": true,
+}
+`;
+
+exports[`while loops use block scoping instead of function scoping 1`] = `
+Object {
+  "status": "finished",
+  "value": true,
+}
+`;
diff --git a/src/__tests__/__snapshots__/index.ts.snap b/src/__tests__/__snapshots__/index.ts.snap
index 7173af9a..7964c125 100644
--- a/src/__tests__/__snapshots__/index.ts.snap
+++ b/src/__tests__/__snapshots__/index.ts.snap
@@ -252,11 +252,4 @@ Object {
 }
 `;
 
-exports[`const uses block scoping instead of function scoping 1`] = `
-Object {
-  "status": "finished",
-  "value": true,
-}
-`;
-
 exports[`parseError for missing semicolon 1`] = `"Line 1: Missing semicolon at the end of statement"`;
diff --git a/src/__tests__/block_scoping.ts b/src/__tests__/block_scoping.ts
new file mode 100644
index 00000000..e5f09768
--- /dev/null
+++ b/src/__tests__/block_scoping.ts
@@ -0,0 +1,135 @@
+import { mockContext } from "../mocks/context";
+import { runInContext } from "../index";
+import { Finished } from "../types";
+
+// This is bad practice. Don't do this!
+test("const uses block scoping instead of function scoping", () => {
+  const code = `
+    function test(){
+      const x = true;
+      if(true) {
+          const x = false;
+      } else {
+          const x = false;
+      }
+      return x;
+    }
+    test();
+  `;
+  const context = mockContext();
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(true);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("let uses block scoping instead of function scoping", () => {
+  const code = `
+    function test(){
+      let x = true;
+      if(true) {
+          let x = false;
+      } else {
+          let x = false;
+      }
+      return x;
+    }
+    test();
+  `;
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe("finished");
+    expect(obj).toMatchSnapshot();
+    expect((obj as Finished).value).toBe(true);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("for loops use block scoping instead of function scoping", () => {
+  const code = `
+    function test(){
+      let x = true;
+      for (let x = 1; x > 0; x = x - 1) {
+      }
+      return x;
+    }
+    test();
+  `;
+  const context = mockContext(4);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe("finished");
+    expect(obj).toMatchSnapshot();
+    expect((obj as Finished).value).toBe(true);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("interior of for loops don't affect loop variables", () => {
+  const code = `
+    function test(){
+      let result = 0;
+      for (let x = 4; x > 0; x = x - 1) {
+        result = result + 1;
+        x = 0;
+      }
+      return result;
+    }
+    test();
+  `;
+  const context = mockContext(4);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe("finished");
+    expect(obj).toMatchSnapshot();
+    expect((obj as Finished).value).toBe(4);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("while loops use block scoping instead of function scoping", () => {
+  const code = `
+    function test(){
+      let x = true;
+      while (true) {
+        let x = false;
+        break;
+      }
+      return x;
+    }
+    test();
+  `;
+  const context = mockContext(4);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe("finished");
+    expect(obj).toMatchSnapshot();
+    expect((obj as Finished).value).toBe(true);
+  });
+});
+
+test("for loop `let` variables are copied into the block scope", () => {
+  // see https://www.ecma-international.org/ecma-262/6.0/#sec-for-statement-runtime-semantics-labelledevaluation
+  // and https://hacks.mozilla.org/2015/07/es6-in-depth-let-and-const/
+  const code = `
+  function test(){
+        let z = [];
+        for (let x = 0; x < 2; x = x + 1) {
+          z[x] = () => x;
+        }
+        return z[1]();
+  }
+  test();
+  `;
+  const context = mockContext(4);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe("finished");
+    expect(obj).toMatchSnapshot();
+    expect((obj as Finished).value).toBe(1);
+  });
+})
\ No newline at end of file
diff --git a/src/__tests__/index.ts b/src/__tests__/index.ts
index 4e57548e..6092f48a 100644
--- a/src/__tests__/index.ts
+++ b/src/__tests__/index.ts
@@ -131,26 +131,3 @@ test(
   },
   30000
 )
-
-// This is bad practice. Don't do this!
-test('const uses block scoping instead of function scoping', () => {
-  const code = `
-    function test(){
-      const x = true;
-      if(true) {
-          const x = false;
-      } else {
-          const x = false;
-      }
-      return x;
-    }
-    test();
-  `;
-  const context = mockContext()
-  const promise = runInContext(code, context, { scheduler: 'preemptive' })
-  return promise.then(obj => {
-    expect(obj).toMatchSnapshot()
-    expect(obj.status).toBe('finished')
-    expect((obj as Finished).value).toBe(true)
-  })
-})