Repository: source-academy/js-slang
Pull request #120 (2018-11-10 06:36:32)

Fix property getting and setting on undefined and null
Reading and writing values to `null` and `undefined` (or any other cases since I just wrapped it in a try catch) fixed.

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

* **20** of **22**   **(90.91%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.8%**) to **53.339%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [src/interpreter-errors.ts](https://coveralls.io/builds/20021780/source?filename=src%2Finterpreter-errors.ts#L204) | 12 | 14 | 85.71%
<!-- | **Total:** | **20** | **22** | **90.91%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/20021780/badge)](https://coveralls.io/builds/20021780) |
| :-- | --: |
| Change from base [Build 266](https://coveralls.io/builds/19931948): |  0.8% |
| Covered Lines: | 1013 |
| Relevant Lines: | 1775 |

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

martin-henz: Folks can you take a look at this? I don't have my hands on the interpreter so can't approve and merge. @yyc can you take a look?

Patch:
diff --git a/src/interpreter-errors.ts b/src/interpreter-errors.ts
index 56d20d2f..42049674 100644
--- a/src/interpreter-errors.ts
+++ b/src/interpreter-errors.ts
@@ -185,4 +185,40 @@ export class EmptyForExpression implements SourceError {
     return 'TODO'
   }
 
+}
+
+export class GetPropertyError implements SourceError {
+  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!
+  }
+
+  public explain() {
+    return `Cannot read property ${this.prop} of ${this.obj}`
+  }
+
+  public elaborate() {
+    return 'TODO'
+  }
+}
+
+export class SetPropertyError implements SourceError {
+  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!
+  }
+
+  public explain() {
+    return `Cannot assign property ${this.prop} of ${this.obj}`
+  }
+
+  public elaborate() {
+    return 'TODO'
+  }
 }
\ No newline at end of file
diff --git a/src/interpreter.ts b/src/interpreter.ts
index 4df480e3..d27f7c6f 100644
--- a/src/interpreter.ts
+++ b/src/interpreter.ts
@@ -383,16 +383,16 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
     if (obj instanceof Closure) {
       obj = obj.fun
     }
+    let prop
     if (node.computed) {
-      const prop = yield* evaluate(node.property, context)
-      return obj[prop]
+      prop = yield* evaluate(node.property, context)
     } else {
-      const name = (node.property as es.Identifier).name
-      if (name === 'prototype') {
-        return obj.prototype
-      } else {
-        return obj[name]
-      }
+      prop = (node.property as es.Identifier).name
+    }
+    try {
+      return obj[prop]
+    } catch {
+      handleError(context, new errors.GetPropertyError(node, obj, prop))
     }
   },
   AssignmentExpression: function*(node: es.AssignmentExpression, context: Context) {
@@ -406,7 +406,11 @@ export const evaluators: { [nodeType: string]: Evaluator<es.Node> } = {
         prop = (left.property as es.Identifier).name
       }
       const val = yield* evaluate(node.right, context)
-      obj[prop] = val
+      try {
+        obj[prop] = val
+      } catch {
+        handleError(context, new errors.SetPropertyError(node, obj, prop))
+      }
       return val
     }
     const id = node.left as es.Identifier


Test patch:
diff --git a/src/__tests__/__snapshots__/index.ts.snap b/src/__tests__/__snapshots__/index.ts.snap
index 6b8d1a69..bebf1b23 100644
--- a/src/__tests__/__snapshots__/index.ts.snap
+++ b/src/__tests__/__snapshots__/index.ts.snap
@@ -218,6 +218,13 @@ ArrowClosure {
 
 exports[`Cannot overwrite consts even when assignment is allowed 1`] = `"Line 4: Cannot assign new value to constant constant"`;
 
+exports[`Deep object assignment and retrieval 1`] = `
+Object {
+  "status": "finished",
+  "value": "string",
+}
+`;
+
 exports[`Empty code returns undefined 1`] = `
 Object {
   "status": "finished",
@@ -263,6 +270,13 @@ exports[`Simple inifinite recursion represents CallExpression well 1`] = `
   x(x => x(x))..  x(x => x(x))..  x(x => x(x)).."
 `;
 
+exports[`Simple object assignment and retrieval 1`] = `
+Object {
+  "status": "finished",
+  "value": 1,
+}
+`;
+
 exports[`Single boolean self-evaluates to itself 1`] = `
 Object {
   "status": "finished",
diff --git a/src/__tests__/__snapshots__/interpreter-errors.ts.snap b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
index 1bf6b341..429aef8d 100644
--- a/src/__tests__/__snapshots__/interpreter-errors.ts.snap
+++ b/src/__tests__/__snapshots__/interpreter-errors.ts.snap
@@ -1,5 +1,167 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`Error when accessing property on undefined 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Error when accessing property on undefined 2`] = `
+Array [
+  GetPropertyError {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 18,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "obj": undefined,
+    "prop": "prop",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`Error when assigning to property on undefined 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Error when assigning to property on undefined 2`] = `
+Array [
+  SetPropertyError {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 24,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "obj": undefined,
+    "prop": "prop",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`Error when assigning to property on variable with value undefined 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Error when assigning to property on variable with value undefined 2`] = `
+Array [
+  SetPropertyError {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 16,
+        "line": 3,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 3,
+      },
+    },
+    "obj": undefined,
+    "prop": "prop",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`Error when deeply accessing property on undefined 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Error when deeply accessing property on undefined 2`] = `
+Array [
+  GetPropertyError {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 18,
+        "line": 2,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 2,
+      },
+    },
+    "obj": undefined,
+    "prop": "prop",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`Error when deeply assigning to property on variable with value undefined 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`Error when deeply assigning to property on variable with value undefined 2`] = `
+Array [
+  GetPropertyError {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 10,
+        "line": 3,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 3,
+      },
+    },
+    "obj": undefined,
+    "prop": "prop",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
+exports[`In case a function ever returns null, should throw an error as well 1`] = `
+Object {
+  "status": "error",
+}
+`;
+
+exports[`In case a function ever returns null, should throw an error as well 2`] = `
+Array [
+  GetPropertyError {
+    "location": SourceLocation {
+      "end": Position {
+        "column": 15,
+        "line": 3,
+      },
+      "start": Position {
+        "column": 4,
+        "line": 3,
+      },
+    },
+    "obj": null,
+    "prop": "prop",
+    "severity": "Error",
+    "type": "Runtime",
+  },
+]
+`;
+
 exports[`Undefined variable error is thrown 1`] = `
 Object {
   "status": "error",
diff --git a/src/__tests__/index.ts b/src/__tests__/index.ts
index 321040a1..d0e1aa81 100644
--- a/src/__tests__/index.ts
+++ b/src/__tests__/index.ts
@@ -222,3 +222,36 @@ test('Multi-dimensional arrays display properly', () => {
     expect((obj as Finished).value).toBe('[1, function a() {}, 3, [() => 1, 5]]')
   })
 })
+
+test('Simple object assignment and retrieval', () => {
+  const code = `
+    const o = {};
+    o.a = 1;
+    o.a;
+   `;
+  const context = mockContext(4)
+  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(1)
+  })
+})
+
+test('Deep object assignment and retrieval', () => {
+  const code = `
+    const o = {};
+    o.a = {};
+    o.a.b = {};
+    o.a.b.c = "string";
+    o.a.b.c;
+   `;
+  const context = mockContext(4)
+  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("string")
+  })
+})
+
diff --git a/src/__tests__/interpreter-errors.ts b/src/__tests__/interpreter-errors.ts
index 115bc94a..39644f92 100644
--- a/src/__tests__/interpreter-errors.ts
+++ b/src/__tests__/interpreter-errors.ts
@@ -14,3 +14,90 @@ test("Undefined variable error is thrown", () => {
     expect(parseError(context.errors)).toBe("Line 2: Name im_undefined not declared")
   })
 })
+
+test('Error when assigning to property on undefined', () => {
+  const code = `
+    undefined.prop = 123;
+   `;
+  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: Cannot assign property prop of undefined')
+  })
+})
+
+test('Error when assigning to property on variable with value undefined', () => {
+  const code = `
+    const u = undefined;
+    u.prop = 123;
+   `;
+  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: Cannot assign property prop of undefined')
+  })
+})
+
+test('Error when deeply assigning to property on variable with value undefined', () => {
+  const code = `
+    const u = undefined;
+    u.prop.prop = 123;
+   `;
+  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: Cannot read property prop of undefined')
+  })
+})
+
+test('Error when accessing property on undefined', () => {
+  const code = `
+    undefined.prop;
+   `;
+  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: Cannot read property prop of undefined')
+  })
+})
+
+test('Error when deeply accessing property on undefined', () => {
+  const code = `
+    undefined.prop.prop;
+   `;
+  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: Cannot read property prop of undefined')
+  })
+})
+
+test('In case a function ever returns null, should throw an error as well', () => {
+  const code = `
+    const myNull = pair.constructor("return null;")();
+    myNull.prop;
+   `;
+  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: Cannot read property prop of null')
+  })
+})
\ No newline at end of file