Repository: source-academy/js-slang
Pull request #75 (2018-10-01 15:54:13)

fix standalone block statement scoping
Fixed block statement scoping (so standalone block statements are now correctly scoped).

A downside now is that variables declared in loop blocks can now shadow loop variables, same as in ES6, but we should discourage this behaviour.
```
  function test(){
      for (let x = 0; x < 2; x = x + 1) {
        let x = 1;
        print(x);
      }
      return false;
  }
```

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

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

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/19283809/badge)](https://coveralls.io/builds/19283809) |
| :-- | --: |
| Change from base [Build 122](https://coveralls.io/builds/19269766): |  0.02% |
| Covered Lines: | 769 |
| Relevant Lines: | 1226 |

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

martin-henz: @remo5000 took a look and @openorclose has commented extensively. I think this is ready to go.

Patch:
diff --git a/src/interpreter.ts b/src/interpreter.ts
index 047a32ae..2ad5e3b8 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -425,18 +425,11 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
       handleError(context, error)
       return undefined
     }
-
-    // Create a new frame (block scoping)
-    const frame = createBlockFrame(context, "ifBlockFrame")
-    pushFrame(context, frame)
-
     if (test) {
       const result = yield* evaluate(node.consequent, context)
-      popFrame(context)
       return result
     } else if (node.alternate) {
       const result = yield* evaluate(node.alternate, context)
-      popFrame(context)
       return result
     } else {
       return undefined
@@ -468,13 +461,7 @@ 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
@@ -496,6 +483,11 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
   },
   BlockStatement: function*(node: es.BlockStatement, context: Context) {
     let result: Value
+
+    // Create a new frame (block scoping)
+    const frame = createBlockFrame(context, "blockFrame")
+    pushFrame(context, frame)
+
     for (const statement of node.body) {
       result = yield* evaluate(statement, context)
       if (
@@ -506,6 +498,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
         break
       }
     }
+    popFrame(context)
     return result
   },
   Program: function*(node: es.BlockStatement, context: Context) {


Test patch:
diff --git a/src/__tests__/__snapshots__/block_scoping.ts.snap b/src/__tests__/__snapshots__/block_scoping.ts.snap
index 5e41990c..c6a1330d 100644
--- a/src/__tests__/__snapshots__/block_scoping.ts.snap
+++ b/src/__tests__/__snapshots__/block_scoping.ts.snap
@@ -38,6 +38,13 @@ Object {
 }
 `;
 
+exports[`standalone block statements 1`] = `
+Object {
+  "status": "finished",
+  "value": true,
+}
+`;
+
 exports[`while loops use block scoping instead of function scoping 1`] = `
 Object {
   "status": "finished",
diff --git a/src/__tests__/block_scoping.ts b/src/__tests__/block_scoping.ts
index 4d30687d..d59aa942 100644
--- a/src/__tests__/block_scoping.ts
+++ b/src/__tests__/block_scoping.ts
@@ -2,6 +2,27 @@ import { mockContext } from "../mocks/context";
 import { parseError, runInContext } from "../index";
 import { Finished } from "../types";
 
+// This is bad practice. Don't do this!
+test("standalone block statements", () => {
+  const code = `
+    function test(){
+      const x = true;
+      {
+          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("const uses block scoping instead of function scoping", () => {
   const code = `