Repository: source-academy/js-slang
Pull request #46 (2018-08-28 09:08:47)

regularizes the style guide

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

* **0** of **0**   changed or added relevant lines in **0** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at **51.771%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/18702853/badge)](https://coveralls.io/builds/18702853) |
| :-- | --: |
| Change from base [Build 76](https://coveralls.io/builds/18700379): |  0.0% |
| Covered Lines: | 666 |
| Relevant Lines: | 1194 |

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


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/package.json b/package.json
index d2b33091..e5456e04 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "js-slang",
-  "version": "0.1.6",
+  "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
diff --git a/src/stdlib/misc.ts b/src/stdlib/misc.ts
index 32e5bc5c..536864d0 100644
--- a/src/stdlib/misc.ts
+++ b/src/stdlib/misc.ts
@@ -49,7 +49,7 @@ export function parse_int(inputString: string, radix: number) {
     // the two arguments are provided, and parsed is not NaN
     return parsed
   } else {
-    throw new Error('parseInt expects two arguments a string s, and a positive integer i')
+    throw new Error('parse_int expects two arguments a string s, and a positive integer i')
   }
 }
 parse_int.__SOURCE__ = 'parse_int(s, i)'


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)
-  })
-})