Repository: source-academy/js-slang
Pull request #141 (2018-12-08 03:06:27)

Only function DECLARATIONS need to come first in function bodies
small clarification, regarding the restriction on placing local function declarations


Patch:
diff --git a/doc/source_return.tex b/doc/source_return.tex
index 6e4bbc37..d9e5e568 100644
--- a/doc/source_return.tex
+++ b/doc/source_return.tex
@@ -8,6 +8,6 @@ $\textbf{\texttt{return}}$ and $\textit{expression}$ in return statements.
   $(\ \textit{name}\ | \ \textbf{\texttt{(}}\ \textit{parameters}\ \textbf{\texttt{)}}\ )$
                                                and
 $\texttt{\textbf{=>}}$ in function definition expressions.
-\item Local functions within an outer function
+\item Local function declarations within an outer function
   must precede all other statements in body of the outer function.
 \end{itemize}
diff --git a/src/interpreter-errors.ts b/src/interpreter-errors.ts
index 4490f572..712abf44 100644
--- a/src/interpreter-errors.ts
+++ b/src/interpreter-errors.ts
@@ -7,13 +7,31 @@ import { UNKNOWN_LOCATION } from './constants'
 import { toString } from './interop'
 import { ErrorSeverity, ErrorType, SourceError, Value } from './types'
 
-export class InterruptedError implements SourceError {
+export class RuntimeSourceError implements SourceError {
+  public type = ErrorType.RUNTIME
+  public severity = ErrorSeverity.ERROR
+  public location: es.SourceLocation
+
+  constructor(node?: es.Node) {
+    this.location = node ? node.loc! : UNKNOWN_LOCATION
+  }
+
+  public explain() {
+    return ''
+  }
+
+  public elaborate() {
+    return this.explain()
+  }
+}
+
+export class InterruptedError extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(node: es.Node) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -40,7 +58,7 @@ export class ExceptionError implements SourceError {
   }
 }
 
-export class MaximumStackLimitExceeded implements SourceError {
+export class MaximumStackLimitExceeded extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
@@ -57,7 +75,7 @@ export class MaximumStackLimitExceeded implements SourceError {
   }
 
   constructor(node: es.Node, private calls: es.CallExpression[]) {
-    this.location = node ? node.loc! : UNKNOWN_LOCATION
+    super(node)
   }
 
   public explain() {
@@ -73,17 +91,13 @@ export class MaximumStackLimitExceeded implements SourceError {
   }
 }
 
-export class CallingNonFunctionValue implements SourceError {
+export class CallingNonFunctionValue extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(private callee: Value, node?: es.Node) {
-    if (node) {
-      this.location = node.loc!
-    } else {
-      this.location = UNKNOWN_LOCATION
-    }
+    super(node)
   }
 
   public explain() {
@@ -95,13 +109,13 @@ export class CallingNonFunctionValue implements SourceError {
   }
 }
 
-export class UndefinedVariable implements SourceError {
+export class UndefinedVariable extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(public name: string, node: es.Node) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -113,13 +127,13 @@ export class UndefinedVariable implements SourceError {
   }
 }
 
-export class UnassignedVariable implements SourceError {
+export class UnassignedVariable extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(public name: string, node: es.Node) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -131,13 +145,13 @@ export class UnassignedVariable implements SourceError {
   }
 }
 
-export class InvalidNumberOfArguments implements SourceError {
+export class InvalidNumberOfArguments extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(node: es.Node, private expected: number, private got: number) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -149,13 +163,13 @@ export class InvalidNumberOfArguments implements SourceError {
   }
 }
 
-export class VariableRedeclaration implements SourceError {
+export class VariableRedeclaration extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(node: es.Node, private name: string) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -167,13 +181,13 @@ export class VariableRedeclaration implements SourceError {
   }
 }
 
-export class ConstAssignment implements SourceError {
+export class ConstAssignment extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(node: es.Node, private name: string) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -185,13 +199,13 @@ export class ConstAssignment implements SourceError {
   }
 }
 
-export class EmptyForExpression implements SourceError {
+export class EmptyForExpression extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(node: es.Node, private missing: string[]) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -205,13 +219,13 @@ export class EmptyForExpression implements SourceError {
 
 }
 
-export class GetPropertyError implements SourceError {
+export class GetPropertyError extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(node: es.Node, private obj: es.Node, private prop: string) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -223,13 +237,13 @@ export class GetPropertyError implements SourceError {
   }
 }
 
-export class SetPropertyError implements SourceError {
+export class SetPropertyError extends RuntimeSourceError {
   public type = ErrorType.RUNTIME
   public severity = ErrorSeverity.ERROR
   public location: es.SourceLocation
 
   constructor(node: es.Node, private obj: es.Node, private prop: string) {
-    this.location = node.loc!
+    super(node)
   }
 
   public explain() {
@@ -239,4 +253,4 @@ export class SetPropertyError implements SourceError {
   public elaborate() {
     return 'TODO'
   }
-}
\ No newline at end of file
+}
diff --git a/src/interpreter.ts b/src/interpreter.ts
index 519f38e1..8c03ef26 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -614,8 +614,14 @@ export function* apply(
         const globalFrame = context.runtime.frames[context.runtime.frames.length - 1]
         context.runtime.frames = [globalFrame]
         const loc = node ? node.loc! : constants.UNKNOWN_LOCATION
-        handleError(context, new errors.ExceptionError(e, loc))
+        if (!(e instanceof errors.RuntimeSourceError)) {
+          // 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.
+          handleError(context, new errors.ExceptionError(e, loc))
+        }
         result = undefined
+        throw e
       }
     } else {
       handleError(context, new errors.CallingNonFunctionValue(fun, node))


Test patch:
diff --git a/src/__tests__/__snapshots__/interpreter-errors.ts.snap b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
index 6b7585b8..eef991a7 100644
--- a/src/__tests__/__snapshots__/interpreter-errors.ts.snap
+++ b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
@@ -1,5 +1,31 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`Builtins don't create additional errors when it's not their fault 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Builtins don't create additional errors when it's not their fault 2`] = `
+Array [
+  UndefinedVariable {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 14,
+        "line": 3,
+      },
+      "start": Position {
+        "column": 13,
+        "line": 3,
+      },
+    },
+    "name": "a",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
 exports[`Error when accessing property on undefined 1`] = `
 Object {
   "status": "error",
@@ -214,6 +240,61 @@ Array [
 ]
 `;
 
+exports[`Nice errors when errors occur inside builtins 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Nice errors when errors occur inside builtins 2`] = `
+Array [
+  ExceptionError {
+    "error": [Error: parse_int expects two arguments a string s, and a positive integer i between 2 and 36, inclusive.],
+    "location": SourceLocation {
+      "end": Position {
+        "column": 19,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`Nice errors when errors occur inside builtins 3`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Nice errors when errors occur inside builtins 4`] = `
+Array [
+  ExceptionError {
+    "error": [Error: Parse error on line 1:
+'
+-^
+Expecting ';', ')', '}', '=', '+', '-', '*', '/', '%', '&&', '||', '===', '!==', '>', '<', '>=', '<=', '=>', '[', ']', '.', '?', ':', 'EmptyString', 'QuotedString', 'QuotedStringEscape', ',', got '1'],
+    "location": SourceLocation {
+      "end": Position {
+        "column": 14,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
 exports[`Undefined variable error is thrown 1`] = `
 Object {
   "status": "error",
diff --git a/src/__tests__/interpreter-errors.ts b/src/__tests__/interpreter-errors.ts
index 330133bd..dc041928 100644
--- a/src/__tests__/interpreter-errors.ts
+++ b/src/__tests__/interpreter-errors.ts
@@ -129,3 +129,51 @@ test('In case a function ever returns null, should throw an error as well', () =
     expect(parseError(context.errors)).toBe('Line 3: Cannot read property prop of null')
   })
 })
+
+test('Nice errors when errors occur inside builtins', () => {
+  const code = `
+    parse_int("10");
+   `;
+  const context = mockContext(4)
+  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: Error: parse_int expects two arguments a string s, and a positive integer i between 2 and 36, inclusive.')
+  })
+})
+
+test('Nice errors when errors occur inside builtins', () => {
+  const code = `
+    parse("'");
+   `;
+  const context = mockContext(4)
+  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: Error: Parse error on line 1:
+'
+-^
+Expecting ';', ')', '}', '=', '+', '-', '*', '/', '%', '&&', '||', '===', '!==', '>', '<', '>=', '<=', '=>', '[', ']', '.', '?', ':', 'EmptyString', 'QuotedString', 'QuotedStringEscape', ',', got '1'`)
+  });
+})
+
+test("Builtins don't create additional errors when it's not their fault", () => {
+  const code = `
+    function f(x) {
+      return a;
+    }
+    map(f, list(1, 2));
+   `;
+  const context = mockContext(4)
+  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 3: Name a not declared')
+  });
+})
\ No newline at end of file