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

Issue:
Function body identifiers are created in a different frame from function parameters
Wasn't detected till now since the only time this matters is when a variable in the function body is declared with the same name as one of the function arguments. This is a SyntaxError in Javascript, so the parser catches that early. 

However, for debugging/environment visualising, we need them to be in the same frame.

Pull request description:
Create only one frame for both function body and parameters
Fixes #207 

Patch:
diff --git a/src/closure.ts b/src/closure.ts
index 158f7bbf..51d401db 100644
--- a/src/closure.ts
+++ b/src/closure.ts
@@ -4,18 +4,12 @@ import * as es from 'estree'
 
 import { apply } from './interpreter'
 import { Context, Environment, Value } from './types'
-import * as create from './utils/astCreator'
+import { blockArrowFunction, callExpression, identifier, returnStatement } from './utils/astCreator'
 
 const closureToJS = (value: Closure, context: Context, klass: string) => {
   function DummyClass(this: Closure) {
     const args: Value[] = Array.prototype.slice.call(arguments)
-    const gen = apply(
-      context,
-      value,
-      args,
-      create.callExpression(create.identifier(klass), args),
-      this
-    )
+    const gen = apply(context, value, args, callExpression(identifier(klass), args), this)
     let it = gen.next()
     while (!it.done) {
       it = gen.next()
@@ -58,45 +52,16 @@ export default class Closure extends Callable {
     function isExpressionBody(body: es.BlockStatement | es.Expression): body is es.Expression {
       return body.type !== 'BlockStatement'
     }
+    const functionBody = isExpressionBody(node.body)
+      ? [returnStatement(node.body, node.body.loc!)]
+      : node.body
+    const closure = new Closure(
+      blockArrowFunction(node.params as es.Identifier[], functionBody, node.loc!),
+      environment,
+      context
+    )
 
-    let closure = null
-    if (isExpressionBody(node.body)) {
-      closure = new Closure(
-        {
-          type: 'FunctionExpression',
-          loc: node.loc,
-          id: null,
-          params: node.params,
-          body: {
-            type: 'BlockStatement',
-            loc: node.body.loc,
-            body: [
-              {
-                type: 'ReturnStatement',
-                loc: node.body.loc,
-                argument: node.body
-              }
-            ]
-          } as es.BlockStatement
-        } as es.FunctionExpression,
-        environment,
-        context
-      )
-    } else {
-      closure = new Closure(
-        {
-          type: 'FunctionExpression',
-          loc: node.loc,
-          id: null,
-          params: node.params,
-          body: node.body
-        } as es.FunctionExpression,
-        environment,
-        context
-      )
-    }
-
-    // Set the closure's nod to point back at the original one
+    // Set the closure's node to point back at the original one
     closure.originalNode = node
 
     return closure
@@ -115,16 +80,12 @@ export default class Closure extends Callable {
   /** The original node that created this Closure */
   public originalNode: es.Function
 
-  constructor(
-    public node: es.FunctionExpression,
-    public environment: Environment,
-    context: Context
-  ) {
+  constructor(public node: es.Function, public environment: Environment, context: Context) {
     super(function(this: any, ...args: any[]) {
       return funJS.apply(this, args)
     })
     this.originalNode = node
-    if (this.node.id) {
+    if (this.node.type === 'FunctionDeclaration' && this.node.id !== null) {
       this.functionName = this.node.id.name
     } else {
       this.functionName = `Anonymous${++Closure.lambdaCtr}`
diff --git a/src/interpreter.ts b/src/interpreter.ts
index 2ee15847..161584c5 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -4,7 +4,7 @@ import Closure from './closure'
 import * as constants from './constants'
 import * as errors from './interpreter-errors'
 import { Context, Environment, Frame, Value } from './types'
-import { createNode } from './utils/node'
+import { conditionalExpression, literal, primitive } from './utils/astCreator'
 import { evaluateBinaryExpression, evaluateUnaryExpression } from './utils/operators'
 import * as rttc from './utils/rttc'
 
@@ -33,7 +33,7 @@ const createEnvironment = (
   if (callExpression) {
     environment.callExpression = {
       ...callExpression,
-      arguments: args.map(a => createNode(a) as es.Expression)
+      arguments: args.map(primitive)
     }
   }
   closure.node.params.forEach((param, index) => {
@@ -204,19 +204,9 @@ function* getArgs(context: Context, call: es.CallExpression) {
 
 function transformLogicalExpression(node: es.LogicalExpression): es.ConditionalExpression {
   if (node.operator === '&&') {
-    return {
-      type: 'ConditionalExpression',
-      test: node.left,
-      consequent: node.right,
-      alternate: createNode(false)
-    } as es.ConditionalExpression
+    return conditionalExpression(node.left, node.right, literal(false), node.loc!)
   } else {
-    return {
-      type: 'ConditionalExpression',
-      test: node.left,
-      consequent: createNode(true),
-      alternate: node.right
-    } as es.ConditionalExpression
+    return conditionalExpression(node.left, literal(true), node.right, node.loc!)
   }
 }
 
@@ -233,6 +223,23 @@ function* reduceIf(node: es.IfStatement | es.ConditionalExpression, context: Con
 
 export type Evaluator<T extends es.Node> = (node: T, context: Context) => IterableIterator<Value>
 
+function* evaluateBlockSatement(context: Context, node: es.BlockStatement) {
+  hoistFunctionsAndVariableDeclarationsIdentifiers(context, node)
+  let result
+  for (const statement of node.body) {
+    result = yield* evaluate(statement, context)
+    if (
+      result instanceof ReturnValue ||
+      result instanceof TailCallReturnValue ||
+      result instanceof BreakValue ||
+      result instanceof ContinueValue
+    ) {
+      break
+    }
+  }
+  return result
+}
+
 /**
  * WARNING: Do not use object literal shorthands, e.g.
  *   {
@@ -466,7 +473,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
   *FunctionDeclaration(node: es.FunctionDeclaration, context: Context) {
     const id = node.id as es.Identifier
     // tslint:disable-next-line:no-any
-    const closure = new Closure(node as any, currentEnvironment(context), context)
+    const closure = new Closure(node, currentEnvironment(context), context)
     defineVariable(context, id.name, closure, true)
     return undefined
   },
@@ -540,19 +547,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
     // Create a new environment (block scoping)
     const environment = createBlockEnvironment(context, 'blockEnvironment')
     pushEnvironment(context, environment)
-    hoistFunctionsAndVariableDeclarationsIdentifiers(context, node)
-
-    for (const statement of node.body) {
-      result = yield* evaluate(statement, context)
-      if (
-        result instanceof ReturnValue ||
-        result instanceof TailCallReturnValue ||
-        result instanceof BreakValue ||
-        result instanceof ContinueValue
-      ) {
-        break
-      }
-    }
+    result = yield* evaluateBlockSatement(context, node)
     popEnvironment(context)
     return result
   },
@@ -598,7 +593,7 @@ export function* apply(
         pushEnvironment(context, environment)
         total++
       }
-      result = yield* evaluate(fun.node.body, context)
+      result = yield* evaluateBlockSatement(context, fun.node.body as es.BlockStatement)
       if (result instanceof TailCallReturnValue) {
         fun = result.callee
         node = result.node
diff --git a/src/utils/astCreator.ts b/src/utils/astCreator.ts
index c451e707..29493eac 100644
--- a/src/utils/astCreator.ts
+++ b/src/utils/astCreator.ts
@@ -64,13 +64,15 @@ export const expressionStatement = (expression: es.Expression): es.ExpressionSta
 
 export const blockArrowFunction = (
   params: es.Identifier[],
-  body: es.Statement[] | es.BlockStatement
+  body: es.Statement[] | es.BlockStatement,
+  loc?: es.SourceLocation
 ): es.ArrowFunctionExpression => ({
   type: 'ArrowFunctionExpression',
   expression: false,
   generator: false,
   params,
-  body: Array.isArray(body) ? blockStatement(body) : body
+  body: Array.isArray(body) ? blockStatement(body) : body,
+  loc
 })
 
 export const blockStatement = (body: es.Statement[]): es.BlockStatement => ({
@@ -78,9 +80,13 @@ export const blockStatement = (body: es.Statement[]): es.BlockStatement => ({
   body
 })
 
-export const returnStatement = (argument: es.Expression): es.ReturnStatement => ({
+export const returnStatement = (
+  argument: es.Expression,
+  loc?: es.SourceLocation
+): es.ReturnStatement => ({
   type: 'ReturnStatement',
-  argument
+  argument,
+  loc
 })
 
 export const property = (key: string, value: es.Expression): es.Property => ({
@@ -139,3 +145,8 @@ export const arrayExpression = (elements: es.Expression[]): es.ArrayExpression =
   type: 'ArrayExpression',
   elements
 })
+
+// primitive: undefined is a possible value
+export const primitive = (value: any): es.Expression => {
+  return value === undefined ? identifier('undefined') : literal(value)
+}
diff --git a/src/utils/node.ts b/src/utils/node.ts
deleted file mode 100644
index 19e7ddf8..00000000
--- a/src/utils/node.ts
+++ /dev/null
@@ -1,28 +0,0 @@
-/**
- * Utility functions to work with the AST (Abstract Syntax Tree)
- */
-import * as es from 'estree'
-import { Value } from '../types'
-const createLiteralNode = (value: {}): es.Node => {
-  if (typeof value === 'undefined') {
-    return {
-      type: 'Identifier',
-      name: 'undefined'
-    } as es.Node
-  } else {
-    return {
-      type: 'Literal',
-      value,
-      raw: value
-    } as es.SimpleLiteral
-  }
-}
-/**
- * Create an AST node from a Source value.
- *
- * @param value any valid Source value (number/string/boolean/Closure)
- * @returns {Node}
- */
-export const createNode = (value: Value): es.Node => {
-  return createLiteralNode(value)
-}


Test patch:
diff --git a/src/__tests__/__snapshots__/environment.ts.snap b/src/__tests__/__snapshots__/environment.ts.snap
new file mode 100644
index 00000000..98cce4b0
--- /dev/null
+++ b/src/__tests__/__snapshots__/environment.ts.snap
@@ -0,0 +1,240 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Function params and body identifiers are in the same environment 1`] = `
+Array [
+  Object {
+    "callExpression": Object {
+      "arguments": Array [
+        Object {
+          "loc": undefined,
+          "type": "Literal",
+          "value": 2,
+        },
+      ],
+      "callee": Node {
+        "end": 83,
+        "loc": SourceLocation {
+          "end": Position {
+            "column": 1,
+            "line": 6,
+          },
+          "start": Position {
+            "column": 0,
+            "line": 6,
+          },
+        },
+        "name": "f",
+        "start": 82,
+        "type": "Identifier",
+      },
+      "end": 86,
+      "loc": SourceLocation {
+        "end": Position {
+          "column": 4,
+          "line": 6,
+        },
+        "start": Position {
+          "column": 0,
+          "line": 6,
+        },
+      },
+      "start": 82,
+      "type": "CallExpression",
+    },
+    "head": Object {
+      "x": 2,
+      "y": 1,
+    },
+    "name": "f",
+    "tail": Object {
+      "head": Object {
+        "Infinity": Infinity,
+        "NaN": NaN,
+        "accumulate": [Function],
+        "append": [Function],
+        "apply_in_underlying_javascript": [Function],
+        "array_length": [Function],
+        "build_list": [Function],
+        "display": [Function],
+        "draw_data": [Function],
+        "enum_list": [Function],
+        "equal": [Function],
+        "error": [Function],
+        "f": [Function],
+        "filter": [Function],
+        "for_each": [Function],
+        "head": [Function],
+        "is_array": [Function],
+        "is_boolean": [Function],
+        "is_function": [Function],
+        "is_list": [Function],
+        "is_null": [Function],
+        "is_number": [Function],
+        "is_pair": [Function],
+        "is_string": [Function],
+        "is_undefined": [Function],
+        "length": [Function],
+        "list": [Function],
+        "list_ref": [Function],
+        "list_to_string": [Function],
+        "map": [Function],
+        "math_E": 2.718281828459045,
+        "math_LN10": 2.302585092994046,
+        "math_LN2": 0.6931471805599453,
+        "math_LOG10E": 0.4342944819032518,
+        "math_LOG2E": 1.4426950408889634,
+        "math_PI": 3.141592653589793,
+        "math_SQRT1_2": 0.7071067811865476,
+        "math_SQRT2": 1.4142135623730951,
+        "math_abs": [Function],
+        "math_acos": [Function],
+        "math_acosh": [Function],
+        "math_asin": [Function],
+        "math_asinh": [Function],
+        "math_atan": [Function],
+        "math_atan2": [Function],
+        "math_atanh": [Function],
+        "math_cbrt": [Function],
+        "math_ceil": [Function],
+        "math_clz32": [Function],
+        "math_cos": [Function],
+        "math_cosh": [Function],
+        "math_exp": [Function],
+        "math_expm1": [Function],
+        "math_floor": [Function],
+        "math_fround": [Function],
+        "math_hypot": [Function],
+        "math_imul": [Function],
+        "math_log": [Function],
+        "math_log10": [Function],
+        "math_log1p": [Function],
+        "math_log2": [Function],
+        "math_max": [Function],
+        "math_min": [Function],
+        "math_pow": [Function],
+        "math_random": [Function],
+        "math_round": [Function],
+        "math_sign": [Function],
+        "math_sin": [Function],
+        "math_sinh": [Function],
+        "math_sqrt": [Function],
+        "math_tan": [Function],
+        "math_tanh": [Function],
+        "math_trunc": [Function],
+        "member": [Function],
+        "pair": [Function],
+        "parse": [Function],
+        "parse_int": [Function],
+        "prompt": [Function],
+        "raw_display": [Function],
+        "remove": [Function],
+        "remove_all": [Function],
+        "reverse": [Function],
+        "runtime": [Function],
+        "set_head": [Function],
+        "set_tail": [Function],
+        "stringify": [Function],
+        "tail": [Function],
+        "undefined": undefined,
+      },
+      "name": "global",
+      "tail": null,
+    },
+    "thisContext": undefined,
+  },
+  Object {
+    "head": Object {
+      "Infinity": Infinity,
+      "NaN": NaN,
+      "accumulate": [Function],
+      "append": [Function],
+      "apply_in_underlying_javascript": [Function],
+      "array_length": [Function],
+      "build_list": [Function],
+      "display": [Function],
+      "draw_data": [Function],
+      "enum_list": [Function],
+      "equal": [Function],
+      "error": [Function],
+      "f": [Function],
+      "filter": [Function],
+      "for_each": [Function],
+      "head": [Function],
+      "is_array": [Function],
+      "is_boolean": [Function],
+      "is_function": [Function],
+      "is_list": [Function],
+      "is_null": [Function],
+      "is_number": [Function],
+      "is_pair": [Function],
+      "is_string": [Function],
+      "is_undefined": [Function],
+      "length": [Function],
+      "list": [Function],
+      "list_ref": [Function],
+      "list_to_string": [Function],
+      "map": [Function],
+      "math_E": 2.718281828459045,
+      "math_LN10": 2.302585092994046,
+      "math_LN2": 0.6931471805599453,
+      "math_LOG10E": 0.4342944819032518,
+      "math_LOG2E": 1.4426950408889634,
+      "math_PI": 3.141592653589793,
+      "math_SQRT1_2": 0.7071067811865476,
+      "math_SQRT2": 1.4142135623730951,
+      "math_abs": [Function],
+      "math_acos": [Function],
+      "math_acosh": [Function],
+      "math_asin": [Function],
+      "math_asinh": [Function],
+      "math_atan": [Function],
+      "math_atan2": [Function],
+      "math_atanh": [Function],
+      "math_cbrt": [Function],
+      "math_ceil": [Function],
+      "math_clz32": [Function],
+      "math_cos": [Function],
+      "math_cosh": [Function],
+      "math_exp": [Function],
+      "math_expm1": [Function],
+      "math_floor": [Function],
+      "math_fround": [Function],
+      "math_hypot": [Function],
+      "math_imul": [Function],
+      "math_log": [Function],
+      "math_log10": [Function],
+      "math_log1p": [Function],
+      "math_log2": [Function],
+      "math_max": [Function],
+      "math_min": [Function],
+      "math_pow": [Function],
+      "math_random": [Function],
+      "math_round": [Function],
+      "math_sign": [Function],
+      "math_sin": [Function],
+      "math_sinh": [Function],
+      "math_sqrt": [Function],
+      "math_tan": [Function],
+      "math_tanh": [Function],
+      "math_trunc": [Function],
+      "member": [Function],
+      "pair": [Function],
+      "parse": [Function],
+      "parse_int": [Function],
+      "prompt": [Function],
+      "raw_display": [Function],
+      "remove": [Function],
+      "remove_all": [Function],
+      "reverse": [Function],
+      "runtime": [Function],
+      "set_head": [Function],
+      "set_tail": [Function],
+      "stringify": [Function],
+      "tail": [Function],
+      "undefined": undefined,
+    },
+    "name": "global",
+    "tail": null,
+  },
+]
+`;
diff --git a/src/__tests__/environment.ts b/src/__tests__/environment.ts
new file mode 100644
index 00000000..c70e2fed
--- /dev/null
+++ b/src/__tests__/environment.ts
@@ -0,0 +1,25 @@
+import { stripIndent } from 'common-tags'
+import { Program } from 'estree'
+import { evaluate } from '../interpreter'
+import { mockContext } from '../mocks/context'
+import { parse } from '../parser'
+
+test('Function params and body identifiers are in the same environment', () => {
+  const code = stripIndent`
+  function f(x) {
+    const y = 1;
+    // 13 steps to evaluate until here
+    return x;
+  }
+  f(2);
+  `
+  const context = mockContext(4)
+  const parsed = parse(code, context)
+  const it = evaluate((parsed as any) as Program, context)
+  const stepsToComment = 13 // manually counted magic number
+  for (let i = 0; i < stepsToComment; i += 1) {
+    it.next()
+  }
+  expect(context.runtime.environments).toMatchSnapshot()
+  expect(context.runtime.environments[0].head).toMatchObject({ x: 2, y: 1 })
+})