Fixed bugs involving unsafe atemporals and improved abstract concrete unions (#2513)

Summary:
Release notes: Fixed bugs that could cause generated code to throw

A refactor of my previous attempt to address #2327, which I had to abandon because it was incompatible with the current implementation of certain modeling primitives. Like the previous version, this PR is an intermediate fix that will be refined in a follow up PR. It consists of three changes:

1) It adds a new helper that discharges values from a union after deriving the abstract value in it if needed.

2) It makes conditionally temporal values safe using a helper called `convertToTemporalIfArgsAreTemporal`.

3) It makes the the abstract and concrete members explicit in the interface to Abstract Concrete Unions. Presently, two code sites make the assumption that the abstract member is the first element of `args`, while all others traverse the list to locate it.

Follow ups to come:
1) Update `convertToTemporalIfArgsAreTemporal` to produce a conditional value left to be simplified by the serializer.
2) Enforce the protocol *temporal args should imply temporal results* more generally (#2489).

Fixes #2327 and #2406
Pull Request resolved: https://github.com/facebook/prepack/pull/2513

Differential Revision: D9636173

Pulled By: sb98052

fbshipit-source-id: 22d63dfb9d0da4b1f6eba4e2f6f88760f5eb03ca

src/evaluators/BinaryExpression.js

@@ -196,7 +196,9 @@ export function computeBinary(
 
   let resultType;
   const compute = () => {
-    if (lval instanceof AbstractValue || rval instanceof AbstractValue) {
+    let lvalIsAbstract = lval instanceof AbstractValue;
+    let rvalIsAbstract = rval instanceof AbstractValue;
+    if (lvalIsAbstract || rvalIsAbstract) {
       // If the left-hand side of an instanceof operation is a primitive,
       // and the right-hand side is a simple object (it does not have [Symbol.hasInstance]),
       // then the result should always compute to `false`.
@@ -208,10 +210,21 @@ export function computeBinary(
       ) {
         return realm.intrinsics.false;
       }
+
       try {
         // generate error if binary operation might throw or have side effects
         resultType = getPureBinaryOperationResultType(realm, op, lval, rval, lloc, rloc);
-        return AbstractValue.createFromBinaryOp(realm, op, lval, rval, loc);
+        let result = AbstractValue.createFromBinaryOp(realm, op, lval, rval, loc);
+
+        if ((op === "in" || op === "instanceof") && result instanceof AbstractValue && rvalIsAbstract)
+          // This operation is a conditional atemporal
+          // See #2327
+          result = AbstractValue.convertToTemporalIfArgsAreTemporal(
+            realm,
+            result,
+            [rval] /* throwing does not depend upon lval */
+          );
+        return result;
       } catch (x) {
         if (x instanceof FatalError) {
           // There is no need to revert any effects, because the above operation is pure.

src/intrinsics/ecma262/StringPrototype.js

@@ -574,7 +574,10 @@ export default function(realm: Realm, obj: ObjectValue): ObjectValue {
     let O = RequireObjectCoercible(realm, context);
 
     if (O instanceof AbstractValue && O.getType() === StringValue) {
-      return AbstractValue.createFromTemplate(realm, sliceTemplateSrc, StringValue, [O, start, end]);
+      // This operation is a conditional atemporal
+      // See #2327
+      let absVal = AbstractValue.createFromTemplate(realm, sliceTemplateSrc, StringValue, [O, start, end]);
+      return AbstractValue.convertToTemporalIfArgsAreTemporal(realm, absVal, [O]);
     }
 
     // 2. Let S be ? ToString(O).
@@ -608,7 +611,10 @@ export default function(realm: Realm, obj: ObjectValue): ObjectValue {
     let O = RequireObjectCoercible(realm, context);
 
     if (O instanceof AbstractValue && O.getType() === StringValue) {
-      return AbstractValue.createFromTemplate(realm, splitTemplateSrc, ObjectValue, [O, separator, limit]);
+      // This operation is a conditional atemporal
+      // See #2327
+      let absVal = AbstractValue.createFromTemplate(realm, splitTemplateSrc, ObjectValue, [O, separator, limit]);
+      return AbstractValue.convertToTemporalIfArgsAreTemporal(realm, absVal, [O]);
     }
 
     // 2. If separator is neither undefined nor null, then

src/intrinsics/prepack/utils.js

@@ -11,13 +11,13 @@
 
 import type { Realm } from "../../realm.js";
 import {
-  Value,
   AbstractValue,
   ConcreteValue,
   FunctionValue,
-  StringValue,
   ObjectValue,
+  StringValue,
   UndefinedValue,
+  Value,
 } from "../../values/index.js";
 import { DisablePlaceholderSuffix } from "../../utils/PreludeGenerator.js";
 import { ValuesDomain } from "../../domains/index.js";
@@ -122,7 +122,6 @@ export function createAbstract(
     result.functionResultType = functionResultType;
   }
 
-  if (additionalValues.length > 0)
-    result = AbstractValue.createAbstractConcreteUnion(realm, result, ...additionalValues);
+  if (additionalValues.length > 0) result = AbstractValue.createAbstractConcreteUnion(realm, result, additionalValues);
   return result;
 }

src/methods/get.js

@@ -329,7 +329,10 @@ export function OrdinaryGetPartial(
   Receiver: Value
 ): Value {
   if (Receiver instanceof AbstractValue && Receiver.getType() === StringValue && P === "length") {
-    return AbstractValue.createFromTemplate(realm, lengthTemplateSrc, NumberValue, [Receiver]);
+    let absVal = AbstractValue.createFromTemplate(realm, lengthTemplateSrc, NumberValue, [Receiver]);
+    // This operation is a conditional atemporal
+    // See #2327
+    return AbstractValue.convertToTemporalIfArgsAreTemporal(realm, absVal, [Receiver]);
   }
 
   if (!(P instanceof AbstractValue)) return O.$Get(P, Receiver);

src/methods/properties.js

@@ -1495,12 +1495,10 @@ export class PropertiesImplementation {
               absVal = createAbstractPropertyValue(ObjectValue);
               invariant(absVal instanceof AbstractObjectValue);
               absVal.makeSimple("transitive");
-              absVal = AbstractValue.createAbstractConcreteUnion(
-                realm,
-                absVal,
+              absVal = AbstractValue.createAbstractConcreteUnion(realm, absVal, [
                 realm.intrinsics.undefined,
-                realm.intrinsics.null
-              );
+                realm.intrinsics.null,
+              ]);
             } else {
               absVal = createAbstractPropertyValue(Value);
             }
@@ -1568,12 +1566,11 @@ export class PropertiesImplementation {
       if (O.isIntrinsic() && O.isPartialObject()) {
         if (value instanceof AbstractValue) {
           let savedUnion;
-          let savedIndex;
           if (value.kind === "abstractConcreteUnion") {
+            // TODO: Simplify this code by using helpers from the AbstractValue factory
+            // instead of deriving values directly.
             savedUnion = value;
-            savedIndex = savedUnion.args.findIndex(e => e instanceof AbstractValue);
-            invariant(savedIndex >= 0);
-            value = savedUnion.args[savedIndex];
+            value = savedUnion.args[0];
             invariant(value instanceof AbstractValue);
           }
           if (value.kind !== "resolved") {
@@ -1588,10 +1585,10 @@ export class PropertiesImplementation {
               skipInvariant: true,
             });
             if (savedUnion !== undefined) {
-              invariant(savedIndex !== undefined);
-              let args = savedUnion.args.slice(0);
-              args[savedIndex] = value;
-              value = AbstractValue.createAbstractConcreteUnion(realm, ...args);
+              invariant(value instanceof AbstractValue);
+              let concreteValues = (savedUnion.args.filter(e => e instanceof ConcreteValue): any);
+              invariant(concreteValues.length === savedUnion.args.length - 1);
+              value = AbstractValue.createAbstractConcreteUnion(realm, value, concreteValues);
             }
             if (functionResultType !== undefined) {
               invariant(value instanceof AbstractObjectValue);

src/realm.js

@@ -1599,7 +1599,8 @@ export class Realm {
   rebuildObjectProperty(object: Value, key: string, propertyValue: Value, path: string): void {
     if (!(propertyValue instanceof AbstractValue)) return;
     if (propertyValue.kind === "abstractConcreteUnion") {
-      let absVal = propertyValue.args.find(e => e instanceof AbstractValue);
+      invariant(propertyValue.args.length >= 2);
+      let absVal = propertyValue.args[0];
       invariant(absVal instanceof AbstractValue);
       propertyValue = absVal;
     }

src/serializer/ResidualHeapSerializer.js

@@ -2068,9 +2068,9 @@ export class ResidualHeapSerializer {
   _serializeAbstractValueHelper(val: AbstractValue): BabelNodeExpression {
     let serializedArgs = val.args.map((abstractArg, i) => this.serializeValue(abstractArg));
     if (val.kind === "abstractConcreteUnion") {
-      let abstractIndex = val.args.findIndex(v => v instanceof AbstractValue);
-      invariant(abstractIndex >= 0 && abstractIndex < val.args.length);
-      return serializedArgs[abstractIndex];
+      invariant(val.args.length >= 2);
+      invariant(val.args[0] instanceof AbstractValue);
+      return serializedArgs[0];
     }
     if (val.kind === "explicit conversion to object") {
       let ob = serializedArgs[0];

src/utils/ConcreteModelConverter.js

@@ -58,7 +58,7 @@ export function concretize(realm: Realm, val: Value): ConcreteValue {
   }
   invariant(val instanceof AbstractValue);
   if (val.kind === "abstractConcreteUnion") {
-    invariant(val.args.length > 0);
+    invariant(val.args.length >= 2);
     return concretize(realm, val.args[0]);
   }
   const type = val.types.getType();

src/utils/simplifier.js

@@ -267,7 +267,7 @@ function simplify(realm, value: Value, isCondition: boolean = false): Value {
     case "abstractConcreteUnion": {
       // The union of an abstract value with one or more concrete values.
       if (realm.pathConditions.isEmpty()) return value;
-      let [abstractValue, ...concreteValues] = value.args;
+      let [abstractValue, concreteValues] = AbstractValue.dischargeValuesFromUnion(realm, value);
       invariant(abstractValue instanceof AbstractValue);
       let remainingConcreteValues = [];
       for (let concreteValue of concreteValues) {
@@ -277,7 +277,7 @@ function simplify(realm, value: Value, isCondition: boolean = false): Value {
       }
       if (remainingConcreteValues.length === 0) return abstractValue;
       if (remainingConcreteValues.length === concreteValues.length) return value;
-      return AbstractValue.createAbstractConcreteUnion(realm, abstractValue, ...remainingConcreteValues);
+      return AbstractValue.createAbstractConcreteUnion(realm, abstractValue, remainingConcreteValues);
     }
     default:
       return value;

src/values/AbstractValue.js

@@ -368,6 +368,9 @@ export default class AbstractValue extends Value {
     return false;
   }
 
+  isTemporal(): boolean {
+    return this.$Realm.getTemporalOperationEntryFromDerivedValue(this) !== undefined;
+  }
   // todo: abstract values should never be of type UndefinedValue or NullValue, assert this
   mightBeFalse(): boolean {
     let valueType = this.getType();
@@ -884,16 +887,69 @@ export default class AbstractValue extends Value {
     }
   }
 
+  static convertToTemporalIfArgsAreTemporal(realm: Realm, val: AbstractValue, condArgs?: Array<Value>): AbstractValue {
+    if (condArgs === undefined) condArgs = val.args;
+
+    let temporalArg = condArgs.find(arg => arg.isTemporal());
+    if (temporalArg !== undefined) {
+      let realmGenerator = realm.generator;
+      invariant(realmGenerator !== undefined);
+      invariant(val.operationDescriptor !== undefined);
+      return realmGenerator.deriveAbstract(val.types, val.values, val.args, val.operationDescriptor);
+    } else {
+      return val;
+    }
+  }
+
+  static dischargeValuesFromUnion(realm: Realm, union: AbstractValue): [AbstractValue, Array<ConcreteValue>] {
+    invariant(union instanceof AbstractValue && union.kind === "abstractConcreteUnion");
+    let abstractValue = union.args[0];
+    invariant(abstractValue instanceof AbstractValue);
+
+    let concreteValues = (union.args.filter(e => e instanceof ConcreteValue): any);
+    invariant(concreteValues.length === union.args.length - 1);
+
+    if (!abstractValue.isTemporal()) {
+      // We make the abstract value in an abstract concrete union temporal, as it is predicated
+      // on the conditions that preclude the concrete values in the union. The type invariant
+      // also only applies in that condition, so it is skipped when deriving the value
+      // See #2327
+      let realmGenerator = realm.generator;
+
+      invariant(realmGenerator !== undefined);
+      invariant(abstractValue.operationDescriptor !== undefined);
+      abstractValue = realmGenerator.deriveAbstract(
+        abstractValue.types,
+        abstractValue.values,
+        abstractValue.args,
+        abstractValue.operationDescriptor,
+        {
+          isPure: true,
+          skipInvariant: true,
+        }
+      );
+    }
+
+    return [abstractValue, concreteValues];
+  }
+
   // Creates a union of an abstract value with one or more concrete values.
   // The operation descriptor for the abstract values becomes the operation descriptor for the union.
   // Use this only to allow instrinsic abstract objects to be null and/or undefined.
-  static createAbstractConcreteUnion(realm: Realm, ...elements: Array<Value>): AbstractValue {
-    let concreteValues: Array<ConcreteValue> = (elements.filter(e => e instanceof ConcreteValue): any);
-    invariant(concreteValues.length > 0 && concreteValues.length === elements.length - 1);
-    let concreteSet = new Set(concreteValues);
-    let abstractValue = elements.find(e => e instanceof AbstractValue);
+  static createAbstractConcreteUnion(
+    realm: Realm,
+    abstractValue: AbstractValue,
+    concreteValues: Array<ConcreteValue>
+  ): AbstractValue {
+    invariant(concreteValues.length > 0);
     invariant(abstractValue instanceof AbstractValue);
+
+    let checkedConcreteValues = (concreteValues.filter(e => e instanceof ConcreteValue): any);
+    invariant(checkedConcreteValues.length === concreteValues.length);
+
+    let concreteSet: Set<ConcreteValue> = new Set(checkedConcreteValues);
     let values;
+
     if (!abstractValue.values.isTop()) {
       abstractValue.values.getElements().forEach(v => concreteSet.add(v));
       values = new ValuesDomain(concreteSet);
@@ -901,7 +957,7 @@ export default class AbstractValue extends Value {
       values = ValuesDomain.topVal;
     }
     let types = TypesDomain.topVal;
-    let [hash, operands] = hashCall("abstractConcreteUnion", abstractValue, ...concreteValues);
+    let [hash, operands] = hashCall("abstractConcreteUnion", abstractValue, ...checkedConcreteValues);
     let result = new AbstractValue(realm, types, values, hash, operands, createOperationDescriptor("SINGLE_ARG"), {
       kind: "abstractConcreteUnion",
     });

src/values/ConcreteValue.js

@@ -30,6 +30,9 @@ export default class ConcreteValue extends Value {
     super(realm, intrinsicName);
   }
 
+  isTemporal(): boolean {
+    return false;
+  }
   mightNotBeFalse(): boolean {
     return !this.mightBeFalse();
   }

src/values/Value.js

@@ -80,6 +80,10 @@ export default class Value {
     return !!this.intrinsicName;
   }
 
+  isTemporal() {
+    invariant(false, "abstract method; please override");
+  }
+
   isPartialObject(): boolean {
     return false;
   }

test/serializer/abstract/Issue2327InBinop.js

@@ -0,0 +1,11 @@
+// expected RecoverableError: PP0004
+
+let o = global.__abstractOrNull ? __abstractOrNull("object", "undefined") : undefined;
+let s = true;
+
+if (o != undefined) s = "foobar" in o;
+if (o != undefined) s = "foobar" in o;
+
+global.s = s;
+
+inspect = () => s; // Should be true

test/serializer/abstract/Issue2327InBinopUnconditional.js

@@ -0,0 +1,12 @@
+// Copies of "foobar" in:1
+// expected RecoverableError: PP0004
+
+let o = global.__abstract ? __abstract("object", "('foobar42')") : "foobar42";
+let s = true;
+
+if (o != undefined) s = "foobar" in o;
+if (o != undefined) s = "foobar" in o;
+
+global.s = s;
+
+inspect = () => s; // Should be true

test/serializer/abstract/Issue2327InstanceOfBinop.js

@@ -0,0 +1,11 @@
+// expected RecoverableError: PP0004
+
+let o = global.__abstractOrNull ? __abstractOrNull("object", "undefined") : undefined;
+let s = true;
+
+if (o != undefined) s = "foobar" instanceof o;
+if (o != undefined) s = "foobar" instanceof o;
+
+global.s = s;
+
+inspect = () => s; // Should be true

test/serializer/abstract/Issue2327InstanceOfBinopUnconditional.js

@@ -0,0 +1,12 @@
+// Copies of "foobar" instanceof:1
+// expected RecoverableError: PP0004
+
+let o = global.__abstract ? __abstract("object", "({})") : {};
+let s = true;
+
+if (o != undefined) s = "foobar" instanceof o;
+if (o != undefined) s = "foobar" instanceof o;
+
+global.s = s;
+
+inspect = () => s; // Should be true

test/serializer/abstract/Issue2327StringLength.js

@@ -0,0 +1,9 @@
+let o = global.__abstractOrNull ? __abstractOrNull("string", "undefined") : undefined;
+let s;
+
+if (o != undefined) s = 5 + o.length;
+if (o != undefined) s = 7 + o.length;
+
+global.s = s;
+
+inspect = () => s;

test/serializer/abstract/Issue2327StringLengthUnconditional.js

@@ -0,0 +1,11 @@
+// Copies of length:1
+
+let o = global.__abstract ? __abstract("string", "('xyz')") : "xyz";
+let s;
+
+if (o != undefined) s = 5 + o.length;
+if (o != undefined) s = 7 + o.length;
+
+global.s = s;
+
+inspect = () => s;

test/serializer/abstract/Issue2327StringSlice.js

@@ -0,0 +1,9 @@
+let o = global.__abstractOrNull ? __abstractOrNull("string", "undefined") : undefined;
+let s = "ok";
+
+if (o != undefined) s = "X" + o.slice(3);
+if (o != undefined) s = "Y" + o.slice(3);
+
+global.s = s;
+
+inspect = () => s;

test/serializer/abstract/Issue2327StringSliceUnconditional.js

@@ -0,0 +1,13 @@
+// Copies of slice\(:1
+
+(function() {
+  let o = global.__abstract ? __abstract("string", "('x y z')") : "x y z";
+  let c = global.__abstract ? __abstract("boolean", "true") : true;
+  let s = "ok";
+
+  if (c) s = "X" + o.slice(3);
+  if (c) s = "Y" + o.slice(3);
+
+  global.s = s;
+  inspect = () => s;
+})();

test/serializer/abstract/Issue2327StringSplit.js

@@ -0,0 +1,9 @@
+let o = global.__abstractOrNull ? __abstractOrNull("string", "undefined") : undefined;
+let s = "ok";
+
+if (o != undefined) s = o.split();
+if (o != undefined) s = o.split();
+
+global.s = s;
+
+inspect = () => s;

test/serializer/abstract/Optional3.js

@@ -1,4 +1,4 @@
-// Copies of f;:1
+// Copies of f;:2
 function f() {
   return 123;
 }