Repository: source-academy/js-slang
Pull request #362 (2019-10-18 11:27:58)

Improve error messages, fix naming bug and assignment bug in native
Fixes https://github.com/source-academy/cadet-frontend/issues/963

When js functions throw their own errors (e.g. head(xs) will throw if xs is not a pair), it gets wrapped in an ExceptionError. If these functions are nested down, they each get wrapped in their own ExceptionError, causing the [object Object] error message.

Now, we check whether the error has already been wrapped before wrapping the error.

This also improves many of the Line -1 messages that occur.

This also fixes the bug here: https://sourceacademy.nus.edu.sg/playground#chap=3&exec=1000&ext=NONE&prgrm=GYVwdgxgLglg9mABAdwE4EMAOAKAlIgbwChFTEiBfIA

and the bug here:

https://sourceacademy.nus.edu.sg/playground#chap=3&exec=1000&ext=NONE&prgrm=DYUwLgBAhhC8EAcoEsBOBuAUDeS3qA

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

* **15** of **15**   **(100.0%)**  changed or added relevant lines in **3** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.1%**) to **88.899%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/26395859/badge)](https://coveralls.io/builds/26395859) |
| :-- | --: |
| Change from base [Build 1741](https://coveralls.io/builds/26105112): |  0.1% |
| Covered Lines: | 2188 |
| Relevant Lines: | 2397 |

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

chrisgzf: can this be merged soon? many students are facing the `[object Object]` errors on piazza

Patch:
diff --git a/package.json b/package.json
index fed05137..bd6c4bc2 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "js-slang",
-  "version": "0.3.3",
+  "version": "0.3.5",
   "description": "Javascript-based interpreter for slang, written in Typescript",
   "author": {
     "name": "Source Academy",
diff --git a/src/index.ts b/src/index.ts
index 821596fd..1e72ab6e 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -201,10 +201,19 @@ export async function runInContext(
         value: sandboxedEval(transpiled)
       } as Result)
     } catch (error) {
-      if (error instanceof RuntimeSourceError || error instanceof ExceptionError) {
+      if (error instanceof RuntimeSourceError) {
         context.errors.push(error)
         return resolvedErrorPromise
       }
+      if (error instanceof ExceptionError) {
+        // if we know the location of the error, just throw it
+        if (error.location.start.line !== -1) {
+          context.errors.push(error)
+          return resolvedErrorPromise
+        } else {
+          error = error.error // else we try to get the location from source map
+        }
+      }
       const errorStack = error.stack
       const match = /<anonymous>:(\d+):(\d+)/.exec(errorStack)
       if (match === null) {
diff --git a/src/interpreter.ts b/src/interpreter.ts
index c3217984..57bc6fbc 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -619,7 +619,7 @@ export function* apply(
         )
 
         const loc = node ? node.loc! : constants.UNKNOWN_LOCATION
-        if (!(e instanceof errors.RuntimeSourceError)) {
+        if (!(e instanceof errors.RuntimeSourceError || e instanceof errors.ExceptionError)) {
           // The error could've arisen when the builtin called a source function which errored.
           // If the cause was a source error, we don't want to include the error.
           // However if the error came from the builtin itself, we need to handle it.
diff --git a/src/transpiler.ts b/src/transpiler.ts
index b00c71ec..b99d5357 100644
--- a/src/transpiler.ts
+++ b/src/transpiler.ts
@@ -364,7 +364,7 @@ export function checkForUndefinedVariablesAndTransformAssignmentsToPropagateBack
       if (previousVariablesToAst.has(name)) {
         const lastAncestor: es.Node = ancestors[ancestors.length - 2]
         const { isConstant, variableLocationId } = previousVariablesToAst.get(name)!
-        if (lastAncestor.type === 'AssignmentExpression') {
+        if (lastAncestor.type === 'AssignmentExpression' && lastAncestor.left === identifier) {
           // if this is an assignment expression, we want to propagate back the change
           if (isConstant) {
             throw new ConstAssignment(identifier, name)
@@ -717,7 +717,7 @@ function getDeclarationsToAccessTranspilerInternals(): es.VariableDeclaration[]
           ),
           'get'
         ),
-        [create.literal(name)]
+        [create.literal(key)]
       )
     }
     return create.declaration(name, kind, value)
diff --git a/src/utils/operators.ts b/src/utils/operators.ts
index 06154611..6bc24bda 100644
--- a/src/utils/operators.ts
+++ b/src/utils/operators.ts
@@ -4,7 +4,8 @@ import {
   CallingNonFunctionValue,
   ExceptionError,
   GetInheritedPropertyError,
-  InvalidNumberOfArguments
+  InvalidNumberOfArguments,
+  RuntimeSourceError
 } from '../interpreter-errors'
 import { PotentialInfiniteLoopError, PotentialInfiniteRecursionError } from '../native-errors'
 import { callExpression, locationDummyNode } from './astCreator'
@@ -32,7 +33,12 @@ export function callIfFuncAndRightArgs(
       try {
         return candidate(...args)
       } catch (error) {
-        throw new ExceptionError(error, dummy.loc!)
+        // if we already handled the error, simply pass it on
+        if (!(error instanceof RuntimeSourceError || error instanceof ExceptionError)) {
+          throw new ExceptionError(error, dummy.loc!)
+        } else {
+          throw error
+        }
       }
     } else {
       const expectedLength = candidate.transformedFunction.length
@@ -163,7 +169,17 @@ export const callIteratively = (f: any, ...args: any[]) => {
         )
       }
     }
-    const res = f(...args)
+    let res
+    try {
+      res = f(...args)
+    } catch (error) {
+      // if we already handled the error, simply pass it on
+      if (!(error instanceof RuntimeSourceError || error instanceof ExceptionError)) {
+        throw new ExceptionError(error, dummy.loc!)
+      } else {
+        throw error
+      }
+    }
     if (res === null || res === undefined) {
       return res
     } else if (res.isTail === true) {


Test patch:
diff --git a/src/__tests__/__snapshots__/interpreter-errors.ts.snap b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
index 9c1539fb..2ce1fdda 100644
--- a/src/__tests__/__snapshots__/interpreter-errors.ts.snap
+++ b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
@@ -67,6 +67,195 @@ map(f, list(1, 2));",
 }
 `;
 
+exports[`Cascading js errors work properly 1: expectParsedError 1`] = `
+Object {
+  "alertResult": Array [],
+  "code": "function make_alternating_stream(stream) {
+  return pair(head(stream), () => make_alternating_stream(
+                                    negate_whole_stream(
+                                        stream_tail(stream))));
+}
+
+function negate_whole_stream(stream) {
+    return pair(-head(stream), () => negate_whole_stream(stream_tail(stream)));
+}
+
+const ones = pair(1, () => ones);
+eval_stream(make_alternating_stream(enum_stream(1, 9)), 9);",
+  "displayResult": Array [],
+  "errors": Array [
+    ExceptionError {
+      "error": [Error: head(xs) expects a pair as argument xs, but encountered null],
+      "location": SourceLocation {
+        "end": Position {
+          "column": 29,
+          "line": 8,
+        },
+        "start": Position {
+          "column": 17,
+          "line": 8,
+        },
+      },
+      "severity": "Error",
+      "type": "Runtime",
+    },
+  ],
+  "parsedErrors": "Line 8: Error: head(xs) expects a pair as argument xs, but encountered null",
+  "result": undefined,
+  "resultStatus": "error",
+  "transpiled": "const native = $$NATIVE_STORAGE;
+const callIfFuncAndRightArgs = native.operators.get(\\"callIfFuncAndRightArgs\\");
+const boolOrErr = native.operators.get(\\"boolOrErr\\");
+const wrap = native.operators.get(\\"wrap\\");
+const unaryOp = native.operators.get(\\"unaryOp\\");
+const binaryOp = native.operators.get(\\"binaryOp\\");
+const throwIfTimeout = native.operators.get(\\"throwIfTimeout\\");
+const setProp = native.operators.get(\\"setProp\\");
+const getProp = native.operators.get(\\"getProp\\");
+let lastStatementResult = undefined;
+const globals = $NATIVE_STORAGE.globals;
+(( <globals redacted> ) => {
+  return (() => {
+    {
+      {
+        const make_alternating_stream = wrap(stream => {
+          return {
+            isTail: true,
+            function: pair,
+            functionName: \\"pair\\",
+            arguments: [callIfFuncAndRightArgs(head, 2, 14, stream), wrap(() => ({
+              isTail: true,
+              function: make_alternating_stream,
+              functionName: \\"make_alternating_stream\\",
+              arguments: [callIfFuncAndRightArgs(negate_whole_stream, 3, 36, callIfFuncAndRightArgs(stream_tail, 4, 40, stream))],
+              line: 2,
+              column: 34
+            }), \\"() => make_alternating_stream(negate_whole_stream(stream_tail(stream)))\\")],
+            line: 2,
+            column: 9
+          };
+        }, \\"function make_alternating_stream(stream) {\\\\n  return pair(head(stream), () => make_alternating_stream(negate_whole_stream(stream_tail(stream))));\\\\n}\\");
+        const negate_whole_stream = wrap(stream => {
+          return {
+            isTail: true,
+            function: pair,
+            functionName: \\"pair\\",
+            arguments: [unaryOp(\\"-\\", callIfFuncAndRightArgs(head, 8, 17, stream), 8, 16), wrap(() => ({
+              isTail: true,
+              function: negate_whole_stream,
+              functionName: \\"negate_whole_stream\\",
+              arguments: [callIfFuncAndRightArgs(stream_tail, 8, 57, stream)],
+              line: 8,
+              column: 37
+            }), \\"() => negate_whole_stream(stream_tail(stream))\\")],
+            line: 8,
+            column: 11
+          };
+        }, \\"function negate_whole_stream(stream) {\\\\n  return pair(-head(stream), () => negate_whole_stream(stream_tail(stream)));\\\\n}\\");
+        const ones = callIfFuncAndRightArgs(pair, 11, 13, 1, wrap(() => ({
+          isTail: false,
+          value: ones
+        }), \\"() => ones\\"));
+        lastStatementResult = eval(\\"callIfFuncAndRightArgs(eval_stream, 12, 0, callIfFuncAndRightArgs(make_alternating_stream, 12, 12, callIfFuncAndRightArgs(enum_stream, 12, 36, 1, 9)), 9);\\");
+        globals.variables.set(\\"make_alternating_stream\\", {
+          kind: \\"const\\",
+          getValue: () => {
+            return make_alternating_stream;
+          }
+        });
+        globals.variables.set(\\"negate_whole_stream\\", {
+          kind: \\"const\\",
+          getValue: () => {
+            return negate_whole_stream;
+          }
+        });
+        globals.variables.set(\\"ones\\", {
+          kind: \\"const\\",
+          getValue: () => {
+            return ones;
+          }
+        });
+      }
+    }
+    return lastStatementResult;
+  })();
+})();
+",
+  "visualiseListResult": Array [],
+}
+`;
+
+exports[`Cascading js errors work properly: expectParsedError 1`] = `
+Object {
+  "alertResult": Array [],
+  "code": "function h(p) {
+  return head(p);
+}
+
+h(null);",
+  "displayResult": Array [],
+  "errors": Array [
+    ExceptionError {
+      "error": [Error: head(xs) expects a pair as argument xs, but encountered null],
+      "location": SourceLocation {
+        "end": Position {
+          "column": 16,
+          "line": 2,
+        },
+        "start": Position {
+          "column": 9,
+          "line": 2,
+        },
+      },
+      "severity": "Error",
+      "type": "Runtime",
+    },
+  ],
+  "parsedErrors": "Line 2: Error: head(xs) expects a pair as argument xs, but encountered null",
+  "result": undefined,
+  "resultStatus": "error",
+  "transpiled": "const native = $$NATIVE_STORAGE;
+const callIfFuncAndRightArgs = native.operators.get(\\"callIfFuncAndRightArgs\\");
+const boolOrErr = native.operators.get(\\"boolOrErr\\");
+const wrap = native.operators.get(\\"wrap\\");
+const unaryOp = native.operators.get(\\"unaryOp\\");
+const binaryOp = native.operators.get(\\"binaryOp\\");
+const throwIfTimeout = native.operators.get(\\"throwIfTimeout\\");
+const setProp = native.operators.get(\\"setProp\\");
+const getProp = native.operators.get(\\"getProp\\");
+let lastStatementResult = undefined;
+const globals = $NATIVE_STORAGE.globals;
+(( <globals redacted> ) => {
+  return (() => {
+    {
+      {
+        const h = wrap(p => {
+          return {
+            isTail: true,
+            function: head,
+            functionName: \\"head\\",
+            arguments: [p],
+            line: 2,
+            column: 9
+          };
+        }, \\"function h(p) {\\\\n  return head(p);\\\\n}\\");
+        lastStatementResult = eval(\\"callIfFuncAndRightArgs(h, 5, 0, null);\\");
+        globals.variables.set(\\"h\\", {
+          kind: \\"const\\",
+          getValue: () => {
+            return h;
+          }
+        });
+      }
+    }
+    return lastStatementResult;
+  })();
+})();
+",
+  "visualiseListResult": Array [],
+}
+`;
+
 exports[`Error when accessing inherited property of object: expectParsedError 1`] = `
 Object {
   "alertResult": Array [],
@@ -3097,6 +3286,28 @@ Object {
   "parsedErrors": "Line 1: Expected number on right hand side of operation, got string.",
   "result": undefined,
   "resultStatus": "error",
+  "transpiled": "const native = $$NATIVE_STORAGE;
+const callIfFuncAndRightArgs = native.operators.get(\\"callIfFuncAndRightArgs\\");
+const boolOrErr = native.operators.get(\\"boolOrErr\\");
+const wrap = native.operators.get(\\"wrap\\");
+const unaryOp = native.operators.get(\\"unaryOp\\");
+const binaryOp = native.operators.get(\\"binaryOp\\");
+const throwIfTimeout = native.operators.get(\\"throwIfTimeout\\");
+const setProp = native.operators.get(\\"setProp\\");
+const getProp = native.operators.get(\\"getProp\\");
+let lastStatementResult = undefined;
+const globals = $NATIVE_STORAGE.globals;
+(( <globals redacted> ) => {
+  return (() => {
+    {
+      {
+        lastStatementResult = eval(\\"binaryOp(\\\\\\"*\\\\\\", 12, 'string', 1, 0);\\");
+      }
+    }
+    return lastStatementResult;
+  })();
+})();
+",
   "visualiseListResult": Array [],
 }
 `;
diff --git a/src/__tests__/__snapshots__/transpiled-code.ts.snap b/src/__tests__/__snapshots__/transpiled-code.ts.snap
index 983f2c45..0210652a 100644
--- a/src/__tests__/__snapshots__/transpiled-code.ts.snap
+++ b/src/__tests__/__snapshots__/transpiled-code.ts.snap
@@ -2,9 +2,9 @@
 
 exports[`Ensure no name clashes 1`] = `
 "const native0 = $$NATIVE_STORAGE;
-const callIfFuncAndRightArgs0 = native.operators.get(\\"callIfFuncAndRightArgs0\\");
-const boolOrErr0 = native.operators.get(\\"boolOrErr0\\");
-const wrap90 = native.operators.get(\\"wrap90\\");
+const callIfFuncAndRightArgs0 = native.operators.get(\\"callIfFuncAndRightArgs\\");
+const boolOrErr0 = native.operators.get(\\"boolOrErr\\");
+const wrap90 = native.operators.get(\\"wrap\\");
 const unaryOp = native.operators.get(\\"unaryOp\\");
 const binaryOp = native.operators.get(\\"binaryOp\\");
 const throwIfTimeout = native.operators.get(\\"throwIfTimeout\\");
diff --git a/src/__tests__/interpreter-errors.ts b/src/__tests__/interpreter-errors.ts
index 826f93e6..bd6b8b2b 100644
--- a/src/__tests__/interpreter-errors.ts
+++ b/src/__tests__/interpreter-errors.ts
@@ -963,6 +963,43 @@ test('Type error with <number> * <nonnumber>, error line at <number>, not <nonnu
     *
     'string';
     `,
-    { chapter: 1 }
+    { chapter: 1, native: true }
   ).toMatchInlineSnapshot(`"Line 1: Expected number on right hand side of operation, got string."`)
 })
+
+test('Cascading js errors work properly 1', () => {
+  return expectParsedError(
+    stripIndent`
+    function make_alternating_stream(stream) {
+      return pair(head(stream), () => make_alternating_stream(
+                                        negate_whole_stream(
+                                            stream_tail(stream))));
+    }
+
+    function negate_whole_stream(stream) {
+        return pair(-head(stream), () => negate_whole_stream(stream_tail(stream)));
+    }
+
+    const ones = pair(1, () => ones);
+    eval_stream(make_alternating_stream(enum_stream(1, 9)), 9);
+    `,
+    { chapter: 3, native: true }
+  ).toMatchInlineSnapshot(
+    `"Line 8: Error: head(xs) expects a pair as argument xs, but encountered null"`
+  )
+})
+
+test('Cascading js errors work properly', () => {
+  return expectParsedError(
+    stripIndent`
+    function h(p) {
+      return head(p);
+    }
+
+    h(null);
+    `,
+    { chapter: 2, native: true }
+  ).toMatchInlineSnapshot(
+    `"Line 2: Error: head(xs) expects a pair as argument xs, but encountered null"`
+  )
+})
diff --git a/src/__tests__/native.ts b/src/__tests__/native.ts
index 9eae9513..abbc462c 100644
--- a/src/__tests__/native.ts
+++ b/src/__tests__/native.ts
@@ -81,3 +81,33 @@ test('test proper setting of variables in an outer scope', async () => {
   expect(result.status).toBe('finished')
   expect((result as Finished).value).toBe('new')
 })
+
+test('using internal names still work', async () => {
+  const context = mockContext(3)
+  const result = await runInContext(
+    stripIndent`
+    const boolOrErr = 1;
+    function wrap() {
+      return boolOrErr;
+    }
+    wrap();
+  `,
+    context
+  )
+  expect(result.status).toBe('finished')
+  expect((result as Finished).value).toBe(1)
+})
+
+test('assigning a = b where b was from a previous program call works', async () => {
+  const context = mockContext(3)
+  const result = await runInContext(
+    stripIndent`
+    let b = null;
+    b = pair;
+    b = 1;
+  `,
+    context
+  )
+  expect(result.status).toBe('finished')
+  expect((result as Finished).value).toBe(1)
+})
diff --git a/src/stdlib/__tests__/__snapshots__/list.ts.snap b/src/stdlib/__tests__/__snapshots__/list.ts.snap
index 8c0a6dde..3e2f2124 100644
--- a/src/stdlib/__tests__/__snapshots__/list.ts.snap
+++ b/src/stdlib/__tests__/__snapshots__/list.ts.snap
@@ -22,8 +22,7 @@ Object {
       "type": "Runtime",
     },
   ],
-  "parsedErrors": "native:\\"Line -1: Error: head(xs) expects a pair as argument xs, but encountered null\\"
-interpreted:\\"Line 147: Error: head(xs) expects a pair as argument xs, but encountered null\\"",
+  "parsedErrors": "Line 147: Error: head(xs) expects a pair as argument xs, but encountered null",
   "result": undefined,
   "resultStatus": "error",
   "transpiled": "const native = $$NATIVE_STORAGE;
diff --git a/src/stdlib/__tests__/list.ts b/src/stdlib/__tests__/list.ts
index 97184434..f3943022 100644
--- a/src/stdlib/__tests__/list.ts
+++ b/src/stdlib/__tests__/list.ts
@@ -524,10 +524,9 @@ describe('These tests are reporting weird line numbers, as list functions are no
     list_ref(list(1, 2, 3), 3);
   `,
       { chapter: 2, native: true }
-    ).toMatchInlineSnapshot(`
-"native:\\"Line -1: Error: head(xs) expects a pair as argument xs, but encountered null\\"
-interpreted:\\"Line 147: Error: head(xs) expects a pair as argument xs, but encountered null\\""
-`)
+    ).toMatchInlineSnapshot(
+      `"Line 147: Error: head(xs) expects a pair as argument xs, but encountered null"`
+    )
   })
 
   test('bad index error list_ref', () => {