Repository: source-academy/js-slang (ts)

Issue:
Critical for loop issues
a) Now the following valid es2015 code results in an error:

```javascript
function test(){
      for (let x = 0; x < 10; x = x + 1) {
        let x = 1; //Error on this line, should be valid
      }
}
test();
```

```let``` statements should still be block scoped in the body of a for loop.

b) Another problem:

```javascript
function test(){
      let timesRun = 0;
      for (let x = 0; x < 10; x = x + 1) {
        timesRun = timesRun + 1;
        x = x + 1;
      }
      return timesRun;
}
test();
```
The loop should only run five times as x is updated in the loop, but due to shallow copying of the environment variables into the new frame, x is not updated as it should be. 

Pull request description:
Refactor loop scoping (#65)
Resolves, if not completely fixes #65 (See comment by @martin-henz [here](https://github.com/source-academy/js-slang/issues/65#issuecomment-423804736))

Also thanks to @openorclose's suggestions, refactor the block scoping to be more DRY by putting it just in BlockStatement.



Patch:
diff --git a/doc/source_styleguide.tex b/doc/source_styleguide.tex
index 644a2818..bd330247 100644
--- a/doc/source_styleguide.tex
+++ b/doc/source_styleguide.tex
@@ -2,7 +2,7 @@
 
 \begin{document}
 	%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
-	\docheader{beta release}{Source}{Style Guide}
+	\docheader{Version 1.0}{Source}{Style Guide}
 	%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 
 This is the style guide for the language Source,
@@ -35,6 +35,17 @@ function make_abs_adder(x) {
 Note the four spaces before \lstinline{return}, and then indentation of the \lstinline{if}
 statement four spaces to the right of the preceding \lstinline{function}.
 
+Four space indent also applies when individual lines get too long and
+need to be broken at convenient places. Example:
+\begin{lstlisting}
+function frac_sum(a, b) {
+    return (a > b) ? 0
+        : 1 / (a * (a + 2))
+            +
+            frac_sum(a + 4, b);
+}
+\end{lstlisting}
+
 \section*{Line Length}
   Lines should be truncated such that they do not require excessive horizontal scrolling.
   As a guide, it should be \emph{no more than 80 characters}.
@@ -104,10 +115,10 @@ const x = 1 + 1;
 const x=1+1;
 
 // good style
-return (x === 0) ? "zero" : "not zero";
+return x === 0 ? "zero" : "not zero";
 
 // bad style
-return (x === 0)?"zero":"not zero";
+return x === 0?"zero":"not zero";
 \end{lstlisting}
 	
 	Do not leave a space between unary operators and the variable involved.
@@ -120,43 +131,68 @@ const negative_x = -x;
 const negative_x = - x;
 \end{lstlisting}
 
-\subsection*{Function Definitions}
+\subsection*{Function Definition Expressions}
 
 Keep the parameters and the body expression of a function definition expression
 in one line, if they are short enough.
 
 If they are lengthy, use
-indentation. The indentation starts just below the first character of the \textit{parameter},
-or the open parenthesis, if there are multiple parameters. If the body expression does not
-fit in one line, use indentation so that the subsequent lines start after the blank character
-after the arrow.
+indentation. The indentation starts four characters after first character of
+the \textit{parameter}, if there is only one,
+or four characters after the open parenthesis,
+if there are multiple parameters. If the body expression does not
+fit in one line, use indentation following the first line of the body
+expression. 
 
 \begin{lstlisting}
 // good style
 function count_buttons(garment) {
     return accumulate((sleaves, total) => sleaves + total,
-                      0, 
-		      map(jacket
-		          => is_checkered(jacket)
-		             ? count_buttons(jacket)
-                             : 1,
-			  garment));
+        0, 
+        map(jacket =>
+            is_checkered(jacket)
+                ? count_buttons(jacket)
+                : 1,
+                garment));
 }
 
-// bad style
+// good style
+function count_buttons(garment) {
+    return accumulate(
+        (sleaves, total) =>
+            delicate_calculation(sleaves + total),
+        0,
+        map(jacket =>
+            is_checkered(jacket)
+                ? count_buttons(jacket)
+                : 1,
+            garment));
+}
+
+// bad style: too much indentation
 function count_buttons(garment) {
-    return accumulate((sleaves, total)
-                      =>
-                      sleaves + total,
+    return accumulate((sleaves, total) =>
+                      delicate_calculation(sleaves + total),
                       0, 
-		      map(jacket
-		          => is_checkered(jacket)
-		          ? count_buttons(jacket)
+		      map(jacket =>
+                          is_checkered(jacket)
+                          ? count_buttons(jacket)
                           : 1,
-			  garment));
+                          garment));
 }
 
-
+// no newline allowed between parameters and =>
+function count_buttons(garment) {
+    return accumulate(
+        (sleaves, total) 
+            => delicate_calculation(sleaves + total),
+        0,
+        map(jacket 
+            => is_checkered(jacket)
+                ? count_buttons(jacket)
+                : 1,
+            garment));
+}
 \end{lstlisting}
 
 
@@ -170,30 +206,31 @@ const aspect_ratio = landscape ? 4 / 3 : 3 / 4;
 
 // bad style
 const aspect_ratio = landscape
-                   ? 4 / 3
-                   : 3 / 4;
+    ? 4 / 3
+    : 3 / 4;
 \end{lstlisting}
 
 If the \textit{consequent-expression} or \textit{alternative-expression} are lengthy, use
-indentation. The indentation starts just below the \textit{predicate}.
+indentation. The indentation is as usual four characters longer
+than the indentation of the previous line.
 
 \begin{lstlisting}
 // good style
 function A(x,y) {
     return y === 0
-           ? 0
-           : x === 0
-             ? 2 * y
-             : y === 1
-               ? 2
-               : A(x - 1, A(x, y - 1));
-
-// bad style
+        ? 0
+        : x === 0
+            ? 2 * y
+            : y === 1
+                ? 2
+                : A(x - 1, A(x, y - 1));
+
+// bad style: line too long
 function A(x,y) {
     return y === 0 ? 0 : x === 0 ? 2 * y : y === 1 ? 2 : A(x - 1, A(x, y - 1));
 }
 
-// bad style
+// bad style: too much indentation
 function A(x,y) {
     return y === 0
                ? 0
@@ -275,12 +312,14 @@ if (x === 1) {
 
   Clean up \emph{all trailing whitespace} before submitting your program.
 
-\section*{Variables}
-\subsection*{Naming}
-When naming variables, use \emph{underscores} to separate words. Examples: \lstinline{my_variable}, \lstinline{x}, \lstinline{rcross_bb}.
+\section*{Names}
+\subsection*{Choice of names}
+When naming constants or variables,
+use \emph{underscores} to separate words.
+Examples: \lstinline{my_variable}, \lstinline{x}, \lstinline{rcross_bb}.
 
 \subsection*{Nesting}
-Do not use the same variable name for nested scope. Examples:
+Do not use the same name for nested scope. Examples:
 \begin{lstlisting}
 // bad program
 const x = 1;
diff --git a/src/interpreter-errors.ts b/src/interpreter-errors.ts
index 49c6b41a..56d20d2f 100644
--- a/src/interpreter-errors.ts
+++ b/src/interpreter-errors.ts
@@ -105,7 +105,7 @@ export class UndefinedVariable implements SourceError {
   }
 
   public explain() {
-    return `Undefined Variable ${this.name}`
+    return `Name ${this.name} not declared`
   }
 
   public elaborate() {
@@ -148,3 +148,41 @@ export class VariableRedeclaration implements SourceError {
     return 'TODO'
   }
 }
+
+export class ConstAssignment implements SourceError {
+  public type = ErrorType.RUNTIME
+  public severity = ErrorSeverity.ERROR
+  public location: es.SourceLocation
+
+  constructor(node: es.Node, private name: string) {
+    this.location = node.loc!
+  }
+
+  public explain() {
+    return `Cannot assign new value to constant ${this.name}`
+  }
+
+  public elaborate() {
+    return 'TODO'
+  }
+}
+
+export class EmptyForExpression implements SourceError {
+  public type = ErrorType.RUNTIME
+  public severity = ErrorSeverity.ERROR
+  public location: es.SourceLocation
+
+  constructor(node: es.Node, private missing: string[]) {
+    this.location = node.loc!
+  }
+
+  public explain() {
+    const exp = this.missing.length > 1 ? 'expressions': 'expression'
+    return `For statement cannot have empty ${this.missing.join(',')} ${exp}.`
+  }
+
+  public elaborate() {
+    return 'TODO'
+  }
+
+}
\ No newline at end of file
diff --git a/src/interpreter.ts b/src/interpreter.ts
index 67655742..047a32ae 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -67,14 +67,22 @@ const handleError = (context: Context, error: SourceError) => {
   }
 }
 
-function defineVariable(context: Context, name: string, value: Value) {
+function defineVariable(context: Context, name: string, value: Value, constant=false) {
   const frame = context.runtime.frames[0]
 
   if (frame.environment.hasOwnProperty(name)) {
     handleError(context, new errors.VariableRedeclaration(context.runtime.nodes[0]!, name))
   }
 
-  frame.environment[name] = value
+  Object.defineProperty(
+    frame.environment,
+    name,
+    {
+      value,
+      writable: !constant,
+      enumerable: true
+    }
+  )
 
   return frame
 }
@@ -107,8 +115,13 @@ const setVariable = (context: Context, name: string, value: any) => {
   let frame: Frame | null = context.runtime.frames[0]
   while (frame) {
     if (frame.environment.hasOwnProperty(name)) {
-      frame.environment[name] = value
-      return
+      const descriptors = Object.getOwnPropertyDescriptors(frame.environment)
+      if(descriptors[name].writable) {
+        frame.environment[name] = value
+        return
+      }
+      const error = new errors.ConstAssignment(context.runtime.nodes[0]!, name)
+      handleError(context, error)
     } else {
       frame = frame.parent
     }
@@ -290,9 +303,10 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
   },
   VariableDeclaration: function*(node: es.VariableDeclaration, context: Context) {
     const declaration = node.declarations[0]
+    const constant = (node.kind == "const")
     const id = declaration.id as es.Identifier
     const value = yield* evaluate(declaration.init!, context)
-    defineVariable(context, id.name, value)
+    defineVariable(context, id.name, value, constant)
     return undefined
   },
   ContinueStatement: function*(node: es.ContinueStatement, context: Context) {
@@ -302,9 +316,18 @@ 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 frame = createBlockFrame(context, "forLoopFrame")
-    pushFrame(context, frame)
+    const loopFrame = createBlockFrame(context, "forLoopFrame")
+    pushFrame(context, loopFrame)
 
     if (node.init) {
       yield* evaluate(node.init, context)
@@ -315,9 +338,13 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
       // 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)
+      // We copy this as a const to avoid ES6 funkiness when mutating loop vars
+      // https://github.com/source-academy/js-slang/issues/65#issuecomment-425618227
+      const frame = createBlockFrame(context, "forBlockFrame")
       pushFrame(context, frame)
+      for(let name in loopFrame.environment) {
+        defineVariable(context, name, loopFrame.environment[name], true)
+      }
 
       value = yield* evaluate(node.body, context)
 
@@ -387,7 +414,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
     const id = node.id as es.Identifier
     // tslint:disable-next-line:no-any
     const closure = new Closure(node as any, currentFrame(context), context)
-    defineVariable(context, id.name, closure)
+    defineVariable(context, id.name, closure, true)
     return undefined
   },
   *IfStatement(node: es.IfStatement, context: Context) {


Test patch:
diff --git a/src/__tests__/__snapshots__/block_scoping.ts.snap b/src/__tests__/__snapshots__/block_scoping.ts.snap
index 47f6e8c8..5e41990c 100644
--- a/src/__tests__/__snapshots__/block_scoping.ts.snap
+++ b/src/__tests__/__snapshots__/block_scoping.ts.snap
@@ -1,5 +1,15 @@
 // 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 2`] = `"Line 2: For statement cannot have empty test expression."`;
+
+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 4`] = `"Line 2: For statement cannot have empty init,test,update expressions."`;
+
+exports[`Cannot overwrite loop variables within a block 1`] = `"Line 5: Cannot assign new value to constant x"`;
+
 exports[`const uses block scoping instead of function scoping 1`] = `
 Object {
   "status": "finished",
@@ -21,13 +31,6 @@ Object {
 }
 `;
 
-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",
diff --git a/src/__tests__/__snapshots__/index.ts.snap b/src/__tests__/__snapshots__/index.ts.snap
index 7964c125..f0924c2a 100644
--- a/src/__tests__/__snapshots__/index.ts.snap
+++ b/src/__tests__/__snapshots__/index.ts.snap
@@ -207,6 +207,8 @@ ArrowClosure {
 }
 `;
 
+exports[`Cannot overwrite consts even when assignment is allowed 1`] = `"Line 4: Cannot assign new value to constant constant"`;
+
 exports[`Empty code returns undefined 1`] = `
 Object {
   "status": "finished",
diff --git a/src/__tests__/__snapshots__/interpreter-errors.ts.snap b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
new file mode 100644
index 00000000..1bf6b341
--- /dev/null
+++ b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
@@ -0,0 +1,27 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Undefined variable error is thrown 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Undefined variable error is thrown 2`] = `
+Array [
+  UndefinedVariable {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 16,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "name": "im_undefined",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
diff --git a/src/__tests__/block_scoping.ts b/src/__tests__/block_scoping.ts
index e5f09768..4d30687d 100644
--- a/src/__tests__/block_scoping.ts
+++ b/src/__tests__/block_scoping.ts
@@ -1,5 +1,5 @@
 import { mockContext } from "../mocks/context";
-import { runInContext } from "../index";
+import { parseError, runInContext } from "../index";
 import { Finished } from "../types";
 
 // This is bad practice. Don't do this!
@@ -68,28 +68,6 @@ test("for loops use block scoping instead of function scoping", () => {
   });
 });
 
-// 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 = `
@@ -112,16 +90,16 @@ test("while loops use block scoping instead of function scoping", () => {
   });
 });
 
+//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/
 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]();
+    let z = [];
+    for (let x = 0; x < 2; x = x + 1) {
+      z[x] = () => x;
+    }
+    return z[1]();
   }
   test();
   `;
@@ -129,7 +107,68 @@ test("for loop `let` variables are copied into the block scope", () => {
   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);
+    expect(obj).toMatchSnapshot();
   });
-})
\ No newline at end of file
+});
+
+test("Cannot overwrite loop variables within a block", () => {
+  const code = `
+  function test(){
+      let z = [];
+      for (let x = 0; x < 2; x = x + 1) {
+        x = 1;
+      }
+      return false;
+  }
+  test();
+  `;
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe("error");
+    const errors = parseError(context.errors);
+    expect(errors).toMatchSnapshot();
+  });
+});
+
+test("Cannot leave blank expressions in for loops", () => {
+  const code = [
+    `
+    for(; i < 3; i++) {
+      break;
+    }
+  `,
+    `
+    for(let i = 0; ; i++) {
+      break;
+    }
+  `,
+    `
+    for(let i = 0; i < 3;) {
+      break;
+    }
+  `,
+    `
+    for(;;) {
+      break;
+    }
+  `
+  ];
+  const scheduler = "preemptive";
+  const promises = code.map(c => {
+    const context = mockContext(3);
+    return runInContext(c, context, { scheduler }).then(obj => ({
+      context,
+      obj
+    }));
+  });
+  return Promise.all(promises).then(results => {
+    results.map(res => {
+      const { context, obj } = res;
+      expect(obj.status).toBe("error");
+      const errors = parseError(context.errors);
+      expect(errors).toMatchSnapshot();
+    });
+  });
+});
diff --git a/src/__tests__/index.ts b/src/__tests__/index.ts
index 6092f48a..e3f4bf28 100644
--- a/src/__tests__/index.ts
+++ b/src/__tests__/index.ts
@@ -96,6 +96,42 @@ test(
   30000
 )
 
+test("Cannot overwrite consts even when assignment is allowed", () => {
+  const code = `
+  function test(){
+    const constant = 3;
+    constant = 4;
+    return constant;
+  }
+  test();
+  `;
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe("error");
+    const errors = parseError(context.errors)
+    expect(errors).toMatchSnapshot()
+  });
+});
+
+
+test("Can overwrite lets when assignment is allowed", () => {
+  const code = `
+  function test(){
+    let variable = false;
+    variable = true;
+    return variable;
+  }
+  test();
+  `;
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj.status).toBe('finished')
+    expect((obj as Finished).value).toBe(true)
+  });
+});
+
 test(
   'Inifinite recursion with list args represents CallExpression well',
   () => {
diff --git a/src/__tests__/interpreter-errors.ts b/src/__tests__/interpreter-errors.ts
new file mode 100644
index 00000000..115bc94a
--- /dev/null
+++ b/src/__tests__/interpreter-errors.ts
@@ -0,0 +1,16 @@
+import { mockContext } from "../mocks/context";
+import { runInContext, parseError } from "../index";
+
+test("Undefined variable error is thrown", () => {
+  const code = `
+    im_undefined;
+  `
+  const context = mockContext()
+  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: Name im_undefined not declared")
+  })
+})