Fixed multiple ordering bugs involving unsafe a-temporal abstract values

src/evaluators/BinaryExpression.js

@@ -194,11 +194,28 @@ 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) {
       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);
+
+        // We have determined that it is safe to assume the type of this operation,
+        // but there is one last check. If the operation can throw, then our assumption
+        // may rely on a dynamic condition. We express this condition as an implication,
+        // rval is temporal => this operation is temporal
+        // See #2327
+        if ((op === "in" || op === "instanceof") && (rvalIsAbstract && rval.isTemporal()))
+          return AbstractValue.createTemporalFromBuildFunction(
+            realm,
+            resultType,
+            [lval, rval],
+            createOperationDescriptor("BINARY_EXPRESSION", { op }),
+            { isPure: true, skipInvariant: true }
+          );
+        else return AbstractValue.createFromBinaryOp(realm, op, lval, rval, loc);
       } catch (x) {
         if (x instanceof FatalError) {
           // There is no need to revert any effects, because the above operation is pure.

src/intrinsics/ecma262/JSON.js

@@ -317,8 +317,11 @@ const JSONParse = buildExpressionTemplate(JSONParseStr);
 function InternalJSONClone(realm: Realm, val: Value): Value {
   if (val instanceof AbstractValue) {
     if (val instanceof AbstractObjectValue) {
-      let strVal = AbstractValue.createFromTemplate(realm, JSONStringify, StringValue, [val], JSONStringifyStr);
-      let obVal = AbstractValue.createFromTemplate(realm, JSONParse, ObjectValue, [strVal], JSONParseStr);
+      let strVal;
+      let obVal;
+
+      strVal = AbstractValue.createFromTemplate(realm, JSONStringify, StringValue, [val], JSONStringifyStr);
+      obVal = AbstractValue.createFromTemplate(realm, JSONParse, ObjectValue, [strVal], JSONParseStr);
       obVal.values = new ValuesDomain(new Set([InternalCloneObject(realm, val.getTemplate())]));
       return obVal;
     }

src/intrinsics/ecma262/Math.js

@@ -167,6 +167,9 @@ export default function(realm: Realm): ObjectValue {
           let template = buildExpressionTemplate(templateSource);
           buildMathTemplates.set(name, (r = { template, templateSource }));
         }
+        /* Functions in the Math module cannot throw, so if we ever end up supporting these calls on AbstractValues,
+        using createFromTemplate instead of createTemporalFromTemplate should be safe.
+        This makes the function amenable to CSE, which is a good thing for Math.* */
         return AbstractValue.createFromTemplate(realm, r.template, NumberValue, args, r.templateSource);
       }
 

src/intrinsics/ecma262/StringPrototype.js

@@ -577,7 +577,14 @@ export default function(realm: Realm, obj: ObjectValue): ObjectValue {
     let O = RequireObjectCoercible(realm, context);
 
     if (O instanceof AbstractValue && O.getType() === StringValue) {
-      return AbstractValue.createFromTemplate(realm, sliceTemplate, StringValue, [O, start, end], sliceTemplateSrc);
+      // If O is temporal, this operation is also temporal. See #2327.
+      if (O.isTemporal())
+        return AbstractValue.createTemporalFromTemplate(realm, sliceTemplate, StringValue, [O, start, end], {
+          isPure: true,
+          skipInvariant: true,
+        });
+      else
+        return AbstractValue.createFromTemplate(realm, sliceTemplate, StringValue, [O, start, end], sliceTemplateSrc);
     }
 
     // 2. Let S be ? ToString(O).
@@ -611,13 +618,20 @@ export default function(realm: Realm, obj: ObjectValue): ObjectValue {
     let O = RequireObjectCoercible(realm, context);
 
     if (O instanceof AbstractValue && O.getType() === StringValue) {
-      return AbstractValue.createFromTemplate(
-        realm,
-        splitTemplate,
-        ObjectValue,
-        [O, separator, limit],
-        splitTemplateSrc
-      );
+      // If O is temporal, this operation is also temporal. See #2327.
+      if (O.isTemporal())
+        return AbstractValue.createTemporalFromTemplate(realm, splitTemplate, ObjectValue, [O, separator, limit], {
+          isPure: true,
+          skipInvariant: true,
+        });
+      else
+        return AbstractValue.createFromTemplate(
+          realm,
+          splitTemplate,
+          ObjectValue,
+          [O, separator, limit],
+          splitTemplateSrc
+        );
     }
 
     // 2. If separator is neither undefined nor null, then

src/intrinsics/prepack/utils.js

@@ -29,7 +29,6 @@ import { Utils } from "../../singletons.js";
 import invariant from "../../invariant.js";
 
 const throwTemplateSrc = "(function(){throw new global.Error('abstract value defined at ' + A);})()";
-const throwTemplate = buildExpressionTemplate(throwTemplateSrc);
 
 export function parseTypeNameOrTemplate(
   realm: Realm,
@@ -77,9 +76,12 @@ export function createAbstract(
   let { type, template, functionResultType } = parseTypeNameOrTemplate(realm, typeNameOrTemplate);
   let optionsMap = options ? options.properties : new Map();
 
-  let result;
+  let abstractValue;
   let locString,
     loc = null;
+
+  let resultIsAbstractConcreteUnion = additionalValues.length > 0;
+
   for (let executionContext of realm.contextStack.slice().reverse()) {
     let caller = executionContext.caller;
     loc = executionContext.loc;
@@ -91,41 +93,60 @@ export function createAbstract(
     );
     if (locString !== undefined) break;
   }
+
+  if (locString === undefined) locString = "(unknown location)";
+
+  let locVal, kind, templateSrc, disablePlaceholdersOption;
+
   if (name === undefined) {
-    let locVal = new StringValue(realm, locString !== undefined ? locString : "(unknown location)");
-    let kind = AbstractValue.makeKind("abstractCounted", (realm.objectCount++).toString()); // need not be an object, but must be unique
-    result = AbstractValue.createFromTemplate(realm, throwTemplate, type, [locVal], kind);
+    locVal = [new StringValue(realm, locString)];
+    kind = AbstractValue.makeKind("abstractCounted", (realm.objectCount++).toString()); // need not be an object, but must be unique
+    templateSrc = throwTemplateSrc;
+    disablePlaceholdersOption = undefined;
   } else {
-    let kind = AbstractValue.makeKind("abstract", name);
+    locVal = [];
+    kind = AbstractValue.makeKind("abstract", name);
+    templateSrc = name;
+    disablePlaceholdersOption = {
+      disablePlaceholders: !!optionsMap.get("disablePlaceholders"),
+    };
     if (!optionsMap.get("allowDuplicateNames") && !realm.isNameStringUnique(name)) {
       let error = new CompilerDiagnostic("An abstract value with the same name exists", loc, "PP0019", "FatalError");
       realm.handleError(error);
       throw new FatalError();
     } else {
       realm.saveNameString(name);
     }
-    result = AbstractValue.createFromTemplate(
-      realm,
-      buildExpressionTemplate(name, { disablePlaceholders: !!optionsMap.get("disablePlaceholders") }),
-      type,
-      [],
-      kind
-    );
-    result.intrinsicName = name;
   }
 
-  if (template) result.values = new ValuesDomain(new Set([template]));
+  let buildTemplate = buildExpressionTemplate(templateSrc, disablePlaceholdersOption);
+
+  // Abstract values in an abstract concrete union are temporal, as they are 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.
+  if (resultIsAbstractConcreteUnion)
+    abstractValue = AbstractValue.createTemporalFromTemplate(realm, buildTemplate, type, locVal, {
+      skipInvariant: true,
+    });
+  else {
+    abstractValue = AbstractValue.createFromTemplate(realm, buildTemplate, type, locVal, kind);
+    if (name !== undefined) abstractValue.intrinsicName = name;
+  }
+
+  if (template) abstractValue.values = new ValuesDomain(new Set([template]));
+
   if (template && !(template instanceof FunctionValue)) {
     // why exclude functions?
     template.makePartial();
-    if (name !== undefined) realm.rebuildNestedProperties(result, name);
+    if (name !== undefined) realm.rebuildNestedProperties(abstractValue, name);
   }
+
   if (functionResultType) {
-    invariant(result instanceof AbstractObjectValue);
-    result.functionResultType = functionResultType;
+    invariant(abstractValue instanceof AbstractObjectValue);
+    abstractValue.functionResultType = functionResultType;
   }
 
-  if (additionalValues.length > 0)
-    result = AbstractValue.createAbstractConcreteUnion(realm, result, ...additionalValues);
-  return result;
+  if (resultIsAbstractConcreteUnion)
+    return AbstractValue.createAbstractConcreteUnion(realm, abstractValue, ...additionalValues);
+  else return abstractValue;
 }

src/methods/get.js

@@ -327,7 +327,14 @@ export function OrdinaryGetPartial(
   Receiver: Value
 ): Value {
   if (Receiver instanceof AbstractValue && Receiver.getType() === StringValue && P === "length") {
-    return AbstractValue.createFromTemplate(realm, lengthTemplate, NumberValue, [Receiver], lengthTemplateSrc);
+    if (Receiver.isTemporal()) {
+      return AbstractValue.createTemporalFromTemplate(realm, lengthTemplate, NumberValue, [Receiver], {
+        isPure: true,
+        skipInvariant: true,
+      });
+    } else {
+      return AbstractValue.createFromTemplate(realm, lengthTemplate, NumberValue, [Receiver], lengthTemplateSrc);
+    }
   }
 
   if (!(P instanceof AbstractValue)) return O.$Get(P, Receiver);

src/utils/simplifier.js

@@ -234,6 +234,8 @@ function simplify(realm, value: Value, isCondition: boolean = false): Value {
       }
       if (remainingConcreteValues.length === 0) return abstractValue;
       if (remainingConcreteValues.length === concreteValues.length) return value;
+
+      // This abstractValue was extracted from an abstract concrete union, and must be temporal
       return AbstractValue.createAbstractConcreteUnion(realm, abstractValue, ...remainingConcreteValues);
     }
     default:

src/values/AbstractValue.js

@@ -355,6 +355,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();
@@ -756,7 +759,13 @@ export default class AbstractValue extends Value {
   /* Note that the template is parameterized by the names A, B, C and so on.
      When the abstract value is serialized, the serialized operations are substituted
      for the corresponding parameters and the resulting template is parsed into an AST subtree
-     that is incorporated into the AST produced by the serializer. */
+     that is incorporated into the AST produced by the serializer.
+
+     NOTE: Only call this function if you can guarantee that the template you pass to it will not
+     generate code that is a-temporal. Code is a-temporal if it cannot throw or cause side effects.
+     If there is a possibility that it might exhibit this behavior, then use the safer
+     createTemporalFromTemplate call instead. */
+
   static createFromTemplate(
     realm: Realm,
     template: PreludeGenerator => ({}) => BabelNodeExpression,
@@ -878,6 +887,7 @@ export default class AbstractValue extends Value {
     let concreteSet = new Set(concreteValues);
     let abstractValue = elements.find(e => e instanceof AbstractValue);
     invariant(abstractValue instanceof AbstractValue);
+
     let values;
     if (!abstractValue.values.isTop()) {
       abstractValue.values.getElements().forEach(v => concreteSet.add(v));

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

@@ -73,6 +73,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;