Detect read-write conflicts

Summary:
If one additional function reads a property that is written by another additional function, it becomes potentially dependent on the other function running before it does. Additional functions are supposed to be independent of each other, so we now detect this and report errors.
Closes https://github.com/facebook/prepack/pull/850

Differential Revision: D5529672

Pulled By: hermanventer

fbshipit-source-id: f810d27d1fbf03d6538d70e61a1d9e7c0fe55309

scripts/test-error-handler.js

@@ -54,6 +54,7 @@ function runTest(name: string, code: string): boolean {
   console.log(chalk.inverse(name));
 
   let recover = code.includes("// recover-from-errors");
+  let additionalFunctions = code.includes("// additional functions");
 
   let expectedErrors = code.match(/\/\/\s*expected errors:\s*(.*)/);
   invariant(expectedErrors);
@@ -64,13 +65,15 @@ function runTest(name: string, code: string): boolean {
 
   let errors = [];
   try {
-    prepackFileSync(name, {
+    let options = {
       internalDebug: false,
       mathRandomSeed: "0",
       onError: errorHandler.bind(null, recover ? "Recover" : "Fail", errors),
       serialize: true,
       speculate: true,
-    });
+    };
+    if (additionalFunctions) (options: any).additionalFunctions = ["additional1", "additional2"];
+    prepackFileSync(name, options);
     if (!recover) {
       console.log(chalk.red("Serialization succeeded though it should have failed"));
       return false;
@@ -88,7 +91,7 @@ function runTest(name: string, code: string): boolean {
       let expected = expectedErrors[i][prop];
       let actual = (errors[i]: any)[prop];
       if (prop === "location") {
-        delete actual.filename;
+        if (actual) delete actual.filename;
         actual = JSON.stringify(actual);
         expected = JSON.stringify(expected);
       }

scripts/test-runner.js

@@ -93,6 +93,7 @@ function runTest(name, code, options, args) {
     compatibility,
     speculate,
     delayUnsupportedRequires,
+    onError: diag => "Fail",
     internalDebug: true,
     serialize: true,
     uniqueSuffix: "",
@@ -100,7 +101,7 @@ function runTest(name, code, options, args) {
   if (code.includes("// additional functions")) options.additionalFunctions = ["additional1", "additional2"];
   if (code.includes("// throws introspection error")) {
     try {
-      let realmOptions = { serialize: true, compatibility, uniqueSuffix: "" };
+      let realmOptions = { serialize: true, compatibility, uniqueSuffix: "", errorHandler: diag => "Fail" };
       let realm = construct_realm(realmOptions);
       initializeGlobals(realm);
       let serializerOptions = {
@@ -259,8 +260,8 @@ function run(args) {
     //only run specific tests if desired
     if (!test.name.includes(args.filter)) continue;
 
-    total++;
     for (let delayInitializations of [false, true]) {
+      total++;
       let options = { delayInitializations: delayInitializations };
       if (runTest(test.name, test.file, options, args)) passed++;
       else failed++;

src/methods/join.js

@@ -28,11 +28,11 @@ import {
 } from "../completions.js";
 import { TypesDomain, ValuesDomain } from "../domains/index.js";
 import { Reference } from "../environment.js";
-import { cloneDescriptor, equalDescriptors, IsDataDescriptor, StrictEqualityComparison } from "../methods/index.js";
+import { cloneDescriptor, IsDataDescriptor, StrictEqualityComparison } from "../methods/index.js";
 import { construct_empty_effects } from "../realm.js";
 import { Generator } from "../utils/generator.js";
 import type { SerializationContext } from "../utils/generator.js";
-import { AbstractValue, ConcreteValue, Value } from "../values/index.js";
+import { AbstractValue, Value } from "../values/index.js";
 
 import invariant from "../invariant.js";
 import * as t from "babel-types";
@@ -589,44 +589,3 @@ export function joinBooleans(v1: void | boolean, v2: void | boolean): void | boo
     return v1 || v2;
   }
 }
-
-// Creates a single map that intersects maps m1 and m2 using the given intersect operator for values.
-export function intersectMaps<K, V>(
-  m1: Map<K, void | V>,
-  m2: Map<K, void | V>,
-  intersect: (K, void | V, void | V) => V
-): Map<K, void | V> {
-  let m3: Map<K, void | V> = new Map();
-  m1.forEach((val1, key, map1) => {
-    if (!m2.has(key)) return;
-    let val2 = m2.get(key);
-    let val3 = intersect(key, val1, val2);
-    m3.set(key, val3);
-  });
-  return m3;
-}
-
-export function intersectBindings(realm: Realm, e1: Effects, e2: Effects): Effects {
-  let [c1, g1, b1, p1, o1] = e1;
-  let [c2, g2, b2, p2, o2] = e2;
-
-  // Neither set of effects is allowed to be forked or abrupt
-  invariant(c1 instanceof Value && c2 instanceof Value);
-
-  // ignore generators
-  g1;
-  g2;
-  // intersect bindings
-  let conflict = realm.intrinsics.empty;
-  let b3 = intersectMaps(b1, b2, (b, v1, v2) => (v1 === v2 ? v1 : conflict));
-  let eqVal = (v1, v2) =>
-    v1 === v2 ||
-    (v1 instanceof ConcreteValue && v2 instanceof ConcreteValue && StrictEqualityComparison(realm, v1, v2));
-  let eqDesc = (d1, d2) => d1 && d2 && equalDescriptors(d1, d2) && eqVal(d1.value, d2.value);
-  let p3 = intersectMaps(p1, p2, (b, d1, d2) => (eqDesc(d1, d2) ? d1 : { value: conflict }));
-  // ignore created objects
-  o1;
-  o2;
-
-  return [realm.intrinsics.empty, new Generator(realm), b3, p3, new Set()];
-}

src/methods/properties.js

@@ -985,6 +985,7 @@ export function OrdinaryGetOwnProperty(realm: Realm, O: ObjectValue, P: Property
     }
     return undefined;
   }
+  realm.callReportPropertyAccess(existingBinding);
   if (!existingBinding.descriptor) return undefined;
 
   // 3. Let D be a newly created Property Descriptor with no fields.

src/realm.js

@@ -172,7 +172,7 @@ export class Realm {
   modifiedBindings: void | Bindings;
   modifiedProperties: void | PropertyBindings;
   createdObjects: void | CreatedObjects;
-  reportPropertyModification: void | (PropertyBinding => void);
+  reportPropertyAccess: void | (PropertyBinding => void);
 
   currentLocation: ?BabelNodeSourceLocation;
   nextContextLocation: ?BabelNodeSourceLocation;
@@ -543,6 +543,12 @@ export class Realm {
     return binding;
   }
 
+  callReportPropertyAccess(binding: PropertyBinding): void {
+    if (this.reportPropertyAccess !== undefined) {
+      this.reportPropertyAccess(binding);
+    }
+  }
+
   // Record the current value of binding in this.modifiedProperties unless
   // there is already an entry for binding.
   recordModifiedProperty(binding: PropertyBinding): void {
@@ -550,10 +556,8 @@ export class Realm {
       // This only happens during speculative execution and is reported elsewhere
       throw new FatalError("Trying to modify a property in read-only realm");
     }
+    this.callReportPropertyAccess(binding);
     if (this.modifiedProperties !== undefined && !this.modifiedProperties.has(binding)) {
-      if (this.reportPropertyModification !== undefined) {
-        this.reportPropertyModification(binding);
-      }
       this.modifiedProperties.set(binding, cloneDescriptor(binding.descriptor));
     }
   }

src/serializer/functions.js

@@ -9,13 +9,12 @@
 
 /* @flow */
 
-import type { BabelNodeCallExpression, BabelNodeIdentifier } from "babel-types";
+import type { BabelNodeCallExpression, BabelNodeIdentifier, BabelNodeSourceLocation } from "babel-types";
 import { CompilerDiagnostic, FatalError } from "../errors.js";
 import invariant from "../invariant.js";
-import { intersectBindings } from "../methods/join.js";
 import { type PropertyBindings, Realm } from "../realm.js";
 import type { PropertyBinding } from "../types.js";
-import { EmptyValue, FunctionValue, Value } from "../values/index.js";
+import { FunctionValue, Value } from "../values/index.js";
 import * as t from "babel-types";
 
 export class Functions {
@@ -52,6 +51,7 @@ export class Functions {
     }
 
     // check that functions are independent
+    let conflicts: Set<BabelNodeSourceLocation> = new Set();
     for (let call1 of calls) {
       let e1 = this.realm.evaluateNodeForEffectsInGlobalEnv(call1);
       if (!(e1[0] instanceof Value)) {
@@ -67,44 +67,39 @@ export class Functions {
       }
       for (let call2 of calls) {
         if (call1 === call2) continue;
-        let e2 = this.realm.evaluateNodeForEffectsInGlobalEnv(call2);
-        if (!(e2[0] instanceof Value)) continue; // will report error in outer loop
-        let e3 = intersectBindings(this.realm, e1, e2);
-        let hasWriteConflicts = false;
-        for (let pb of e3[3].values()) {
-          invariant(pb !== undefined); // guaranteed by intersectBindings
-          if (pb.value instanceof EmptyValue) {
-            hasWriteConflicts = true;
-            break;
-          }
-        }
-        if (hasWriteConflicts) this.reportWriteConflicts(e3[3], call1, call2);
+        this.reportWriteConflicts(conflicts, e1[3], call1, call2);
       }
     }
+    if (conflicts.size > 0) throw new FatalError();
   }
 
-  reportWriteConflicts(pbs: PropertyBindings, call1: BabelNodeCallExpression, call2: BabelNodeCallExpression) {
+  reportWriteConflicts(
+    conflicts: Set<BabelNodeSourceLocation>,
+    pbs: PropertyBindings,
+    call1: BabelNodeCallExpression,
+    call2: BabelNodeCallExpression
+  ) {
     let fname = "";
-    let oldReporter = this.realm.reportPropertyModification;
-    this.realm.reportPropertyModification = (pb: PropertyBinding) => {
-      if (pbs.has(pb)) {
+    let oldReporter = this.realm.reportPropertyAccess;
+    this.realm.reportPropertyAccess = (pb: PropertyBinding) => {
+      let location = this.realm.currentLocation;
+      if (!location) return; // happens only when accessing an additional function property
+      if (pbs.has(pb) && !conflicts.has(location)) {
         let error = new CompilerDiagnostic(
-          `Property write conflicts with write in additional function ${fname}`,
-          this.realm.currentLocation,
+          `Property access conflicts with write in additional function ${fname}`,
+          location,
           "PP1003",
           "FatalError"
         );
         this.realm.handleError(error);
+        conflicts.add(location);
       }
     };
     try {
-      fname = ((call2.callee: any): BabelNodeIdentifier).name;
-      this.realm.evaluateNodeForEffectsInGlobalEnv(call1);
       fname = ((call1.callee: any): BabelNodeIdentifier).name;
       this.realm.evaluateNodeForEffectsInGlobalEnv(call2);
     } finally {
-      this.realm.reportPropertyModification = oldReporter;
+      this.realm.reportPropertyAccess = oldReporter;
     }
-    throw new FatalError();
   }
 }

test/error-handler/bad-functions.js

@@ -1,6 +1,6 @@
 // additional functions
-// throws introspection error
-
+// recover-from-errors
+// expected errors: [{"location":null,"severity":"FatalError","errorCode":"PP1002","message":"Additional function additional1 may terminate abruptly"}]
 var wildcard = global.__abstract ? global.__abstract("number", 123, "123") : 123;
 global.a = "";
 

test/error-handler/bad-functions2.js

@@ -1,5 +1,6 @@
 // additional functions
-// throws introspection error
+// recover-from-errors
+// expected errors: [{"location":null,"severity":"FatalError","errorCode":"PP1002","message":"Additional function additional2 may terminate abruptly"}]
 
 var wildcard = global.__abstract ? global.__abstract("number", 123, "123") : 123;
 global.a = "";
@@ -10,7 +11,6 @@ function additional1() {
 
 function additional2() {
   if (wildcard) throw new Exception();
-  global.a = "foo";
 }
 
 inspect = function() {

test/error-handler/missing-functions.js

@@ -0,0 +1,9 @@
+// additional functions
+// recover-from-errors
+// expected errors: [{"location":null,"severity":"FatalError","errorCode":"PP1001","message":"Additional function additional2 not defined in the global object"}]
+
+function additional1() {}
+
+inspect = function() {
+  return "foo";
+}

test/error-handler/write-in-conflict.js

@@ -0,0 +1,17 @@
+// additional functions
+// recover-from-errors
+// expected errors: [{"location":{"start":{"line":10,"column":20},"end":{"line":10,"column":26},"identifierName":"global","source":"test/error-handler/write-in-conflict.js"},"severity":"FatalError","errorCode":"PP1003"}]
+
+function additional1() {
+  global.a = "foo";
+}
+
+function additional2() {
+  global.b = "a" in global;
+}
+
+inspect = function() {
+  additional2();
+  additional1();
+  return global.b;
+}

test/error-handler/write-write-conflict.js

@@ -0,0 +1,19 @@
+// additional functions
+// recover-from-errors
+// expected errors: [{"location":{"start":{"line":12,"column":13},"end":{"line":12,"column":18},"source":"test/error-handler/write-write-conflict.js"},"severity":"FatalError","errorCode":"PP1003"},{"location":{"start":{"line":8,"column":13},"end":{"line":8,"column":18},"source":"test/error-handler/write-write-conflict.js"},"severity":"FatalError","errorCode":"PP1003"}]
+
+global.a = "";
+
+function additional1() {
+  global.a = "foo";
+}
+
+function additional2() {
+  global.a = "bar";
+}
+
+inspect = function() {
+  additional2();
+  additional1();
+  return global.a;
+}

test/error-handler/write-write-conflict2.js

@@ -0,0 +1,19 @@
+// additional functions
+// recover-from-errors
+// expected errors: [{"location":{"start":{"line":12,"column":13},"end":{"line":12,"column":18},"source":"test/error-handler/write-write-conflict2.js"},"severity":"FatalError","errorCode":"PP1003"},{"location":{"start":{"line":8,"column":9},"end":{"line":8,"column":15},"identifierName":"global","source":"test/error-handler/write-write-conflict2.js"},"severity":"FatalError","errorCode":"PP1003"}]
+
+global.a = "";
+
+function additional1() {
+  delete global.a;
+}
+
+function additional2() {
+  global.a = "bar";
+}
+
+inspect = function() {
+  additional2();
+  additional1();
+  return global.a;
+}

test/error-handler/write-write-conflict3.js

@@ -0,0 +1,19 @@
+// additional functions
+// recover-from-errors
+// expected errors: [{"location":{"start":{"line":12,"column":9},"end":{"line":12,"column":15},"identifierName":"global","source":"test/error-handler/write-write-conflict3.js"},"severity":"FatalError","errorCode":"PP1003"},{"location":{"start":{"line":8,"column":13},"end":{"line":8,"column":18},"source":"test/error-handler/write-write-conflict3.js"},"severity":"FatalError","errorCode":"PP1003"}]
+
+global.a = "";
+
+function additional1() {
+  global.a = "foo";
+}
+
+function additional2() {
+  delete global.a;
+}
+
+inspect = function() {
+  additional2();
+  additional1();
+  return global.a;
+}

test/error-handler/write-write-conflict4.js

@@ -0,0 +1,19 @@
+// additional functions
+// recover-from-errors
+// expected errors: [{"location":{"start":{"line":12,"column":13},"end":{"line":12,"column":18},"source":"test/error-handler/write-write-conflict4.js"},"severity":"FatalError","errorCode":"PP1003"},{"location":{"start":{"line":8,"column":13},"end":{"line":8,"column":18},"source":"test/error-handler/write-write-conflict4.js"},"severity":"FatalError","errorCode":"PP1003"}]
+
+global.a = "";
+
+function additional1() {
+  global.a = "foo";
+}
+
+function additional2() {
+  global.a = "foo";
+}
+
+inspect = function() {
+  additional2();
+  additional1();
+  return global.a + global.b;
+}

test/error-handler/write-write-unknown-prop-conflict.js

@@ -0,0 +1,20 @@
+// additional functions
+// recover-from-errors
+// expected errors: [{"location":{"start":{"line":13,"column":16},"end":{"line":13,"column":21},"source":"test/error-handler/write-write-unknown-prop-conflict.js"},"severity":"FatalError","errorCode":"PP1003"},{"location":{"start":{"line":9,"column":16},"end":{"line":9,"column":21},"source":"test/error-handler/write-write-unknown-prop-conflict.js"},"severity":"FatalError","errorCode":"PP1003"}]
+
+let i = global.__abstract ? __abstract("string", "x") : "x";
+global.a = { x: "" }
+
+function additional1() {
+  global.a[i] = "foo";
+}
+
+function additional2() {
+  global.a[i] = "bar";
+}
+
+inspect = function() {
+  additional2();
+  additional1();
+  return global.a.x;
+}

test/serializer/additional-functions/missing-functions.js

@@ -1,8 +0,0 @@
-// additional functions
-// throws introspection error
-
-function additional1() {}
-
-inspect = function() {
-  return "foo";
-}

test/serializer/additional-functions/write-in-conflict.js

@@ -1,18 +1,17 @@
 // additional functions
 // throws introspection error
 
-global.a = "";
-
 function additional1() {
-  global.a = "foo";
+  a = "foo";
 }
 
 function additional2() {
-  delete global.a;
+  return "a" in global;
 }
 
+global.__residual ? __residual("void", additional1) : additional1();
+x = additional2();
+
 inspect = function() {
-  additional2();
-  additional1();
-  return global.a;
+  return x;
 }

test/serializer/additional-functions/write-write-conflict.js

@@ -1,18 +0,0 @@
-// additional functions
-// throws introspection error
-
-global.a = "";
-
-function additional1() {
-  global.a = "foo";
-}
-
-function additional2() {
-  global.a = "bar";
-}
-
-inspect = function() {
-  additional2();
-  additional1();
-  return global.a;
-}

test/serializer/additional-functions/write-write-conflict2.js

@@ -1,18 +0,0 @@
-// additional functions
-// throws introspection error
-
-global.a = "";
-
-function additional1() {
-  delete global.a;
-}
-
-function additional2() {
-  global.a = "bar";
-}
-
-inspect = function() {
-  additional2();
-  additional1();
-  return global.a;
-}

test/serializer/additional-functions/write-write-noconflict2.js

@@ -1,17 +0,0 @@
-// additional functions
-
-global.a = "";
-
-function additional1() {
-  global.a = "foo";
-}
-
-function additional2() {
-  global.a = "foo";
-}
-
-inspect = function() {
-  additional2();
-  additional1();
-  return global.a + global.b;
-}