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

Issue:
Statement after return executed
function f() {
    return (() => 1)();
    display("here");
}
f();

currently displays "here"

Pull request description:
Fix tail call early-return bug (resolves #124)
Credits to @blackening for finding the root cause.

Patch:
diff --git a/src/interpreter.ts b/src/interpreter.ts
index d27f7c6f..0af46d41 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -362,7 +362,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
         value = undefined
         break
       }
-      if (value instanceof ReturnValue) {
+      if (value instanceof ReturnValue || value instanceof TailCallReturnValue) {
         break
       }
       if (node.update) {
@@ -501,6 +501,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
       result = yield* evaluate(statement, context)
       if (
         result instanceof ReturnValue ||
+        result instanceof TailCallReturnValue ||
         result instanceof BreakValue ||
         result instanceof ContinueValue
       ) {
@@ -514,7 +515,7 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
     let result: Value
     for (const statement of node.body) {
       result = yield* evaluate(statement, context)
-      if (result instanceof ReturnValue) {
+      if (result instanceof ReturnValue || result instanceof TailCallReturnValue) {
         break
       }
     }


Test patch:
diff --git a/src/__tests__/__snapshots__/return-regressions.ts.snap b/src/__tests__/__snapshots__/return-regressions.ts.snap
new file mode 100644
index 00000000..ef617a1f
--- /dev/null
+++ b/src/__tests__/__snapshots__/return-regressions.ts.snap
@@ -0,0 +1,113 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`Bare early returns in for loops work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`Bare early returns in if statements work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`Bare early returns in while loops work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`Bare early returns work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`Calling unreachable results in error 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Calling unreachable results in error 2`] = `
+Array [
+  TypeError {
+    "expected": "number",
+    "got": "boolean",
+    "location": SourceLocation {
+      "end": Position {
+        "column": 21,
+        "line": 3,
+      },
+      "start": Position {
+        "column": 13,
+        "line": 3,
+      },
+    },
+    "severity": "Error",
+    "side": " on right hand side of operation",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`Recursive call early returns in for loops work 1`] = `
+Object {
+  "status": "finished",
+  "value": 3,
+}
+`;
+
+exports[`Recursive call early returns in if statements work 1`] = `
+Object {
+  "status": "finished",
+  "value": 3,
+}
+`;
+
+exports[`Recursive call early returns in while loops work 1`] = `
+Object {
+  "status": "finished",
+  "value": 3,
+}
+`;
+
+exports[`Recursive call early returns work 1`] = `
+Object {
+  "status": "finished",
+  "value": 3,
+}
+`;
+
+exports[`Tail call early returns in for loops work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`Tail call early returns in if statements work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`Tail call early returns in while loops work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
+exports[`Tail call early returns work 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
diff --git a/src/__tests__/return-regressions.ts b/src/__tests__/return-regressions.ts
new file mode 100644
index 00000000..22deaeba
--- /dev/null
+++ b/src/__tests__/return-regressions.ts
@@ -0,0 +1,365 @@
+/**
+ * This file contains tests for regressions that TCO may have caused.
+ * Please reference Issue #124 (https://github.com/source-academy/js-slang/issues/124)
+ */
+
+import { mockContext } from "../mocks/context";
+import { runInContext } from "../index";
+import { Finished } from "../types";
+
+// This is bad practice. Don't do this!
+test("Calling unreachable results in error", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function f() {
+      unreachable();
+      return 0;
+    }
+    f();
+  `
+  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();
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Bare early returns work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function f() {
+      return 1;
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext();
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Recursive call early returns work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      return id(1) + id(2);
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext();
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(3);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Tail call early returns work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      return id(1);
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext();
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Bare early returns in if statements work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function f() {
+      if (true) {
+        return 1;
+        unreachable();
+      } else {}
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext();
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Recursive call early returns in if statements work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      if (true) {
+        return id(1) + id(2);
+        unreachable();
+      } else {}
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext();
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(3);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Tail call early returns in if statements work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      if (true) {
+        return id(1);
+        unreachable();
+      } else {}
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext();
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Bare early returns in while loops work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function f() {
+      while (true) {
+        return 1;
+        unreachable();
+      }
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Recursive call early returns in while loops work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      while (true) {
+        return id(1) + id(2);
+        unreachable();
+      }
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(3);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Tail call early returns in while loops work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      while (true) {
+        return id(1);
+        unreachable();
+      }
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Bare early returns in for loops work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function f() {
+      for (let i = 0; i < 100; i = i + 1) {
+        return i+1;
+        unreachable();
+      }
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Recursive call early returns in for loops work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      for (let i = 0; i < 100; i = i + 1) {
+        return id(i+1) + id(i+2);
+      }
+      return 0;
+    }
+    f();
+  `
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(3);
+  });
+});
+
+// This is bad practice. Don't do this!
+test("Tail call early returns in for loops work", () => {
+  const code = `
+    function unreachable() {
+      return 1 < true; // Will cause an error
+    }
+    function id(x) {
+      return x;
+    }
+    function f() {
+      for (let i = 0; i < 100; i = i + 1) {
+        return id(i+1);
+        unreachable();
+      }
+      unreachable();
+      return 0;
+      unreachable();
+    }
+    f();
+  `
+  const context = mockContext(3);
+  const promise = runInContext(code, context, { scheduler: "preemptive" });
+  return promise.then(obj => {
+    expect(obj).toMatchSnapshot();
+    expect(context.errors).toEqual([]);
+    expect(obj.status).toBe("finished");
+    expect((obj as Finished).value).toBe(1);
+  });
+});