Refactor assignment on partial or possibly deleted property (#2389)

Summary:
The goal of this refactoring is to move more of the special case logic around properties as deeply as possible so that when we expand the object model, we don't have to add special cases through all the indirections. This will be clear in a follow up where I try to model the multiple identities.

This reverts the fix in #1183. That fix is no longer needed for its original purpose because those getters are now modeled as pure and the unnecessary accesses gets eliminated by the serializer.

I also remove the `Receiver.$Delete(P)` hack in `OrdinarySet` when a property might have been deleted. I'll need that because this hack doesn't work on objects that only allow weak deletions (such as object set templates). Instead, I model possibly deleted properties in ValidateAndApplyPropertyDescriptor.

It turns out that much of it doesn't matter if it has been deleted or not since you can't delete a non-configurable property.

The React environment calls Set/DefineOwnProperty on partials during initialization. These must read to get the current descriptor. However, a temporal read of the actual value can't be emitted at this point. So I emit a placeholder value that shouldn't be serialized. This is something the environment initialization must already consider. We can't have it read partial values before there is an environment to consider.
Pull Request resolved: https://github.com/facebook/prepack/pull/2389

Differential Revision: D9274895

Pulled By: sebmarkbage

fbshipit-source-id: b8e8aee697121552243f7a67b7c19c62768f4729

src/methods/properties.js

@@ -180,6 +180,13 @@ function havocDescriptor(realm: Realm, desc: Descriptor) {
 
 // Determines if an object with parent O may create its own property P.
 function parentPermitsChildPropertyCreation(realm: Realm, O: ObjectValue, P: PropertyKeyValue): boolean {
+  if (O.isSimpleObject()) {
+    // Simple object always allow property creation since there are no setters.
+    // Object.prototype is considered simple even though __proto__ is a setter.
+    // TODO: That is probably the incorrect assumption but that is implied everywhere.
+    return true;
+  }
+
   let ownDesc = O.$GetOwnProperty(P);
   let ownDescValue = !ownDesc
     ? realm.intrinsics.undefined
@@ -249,7 +256,7 @@ export class PropertiesImplementation {
       Havoc.value(realm, V);
       // The receiver might leak to a getter so if it's not already havoced, we need to havoc it.
       Havoc.value(realm, Receiver);
-      if (realm.generator) {
+      if (realm.generator !== undefined) {
         realm.generator.emitPropertyAssignment(Receiver, StringKey(P), V);
       }
       return true;
@@ -261,9 +268,7 @@ export class PropertiesImplementation {
     invariant(IsPropertyKey(realm, P), "expected property key");
 
     // 2. Let ownDesc be ? O.[[GetOwnProperty]](P).
-    let ownDesc;
-    let existingBinding = InternalGetPropertiesMap(O, P).get(InternalGetPropertiesKey(P));
-    if (existingBinding !== undefined || !(O.isPartialObject() && O.isSimpleObject())) ownDesc = O.$GetOwnProperty(P);
+    let ownDesc = O.$GetOwnProperty(P);
     let ownDescValue = !ownDesc
       ? realm.intrinsics.undefined
       : ownDesc.value === undefined
@@ -387,10 +392,7 @@ export class PropertiesImplementation {
         if (!(Receiver instanceof ObjectValue)) return false;
 
         // c. Let existingDescriptor be ? Receiver.[[GetOwnProperty]](P).
-        let existingDescriptor;
-        let binding = InternalGetPropertiesMap(Receiver, P).get(InternalGetPropertiesKey(P));
-        if (binding !== undefined || !(Receiver.isPartialObject() && Receiver.isSimpleObject()))
-          existingDescriptor = Receiver.$GetOwnProperty(P);
+        let existingDescriptor = Receiver.$GetOwnProperty(P);
         if (existingDescriptor !== undefined) {
           if (existingDescriptor.descriptor1 === ownDesc) existingDescriptor = ownDesc;
           else if (existingDescriptor.descriptor2 === ownDesc) existingDescriptor = ownDesc;
@@ -429,11 +431,11 @@ export class PropertiesImplementation {
 
           // iv. Return ? Receiver.[[DefineOwnProperty]](P, valueDesc).
           if (weakDeletion || existingDescValue.mightHaveBeenDeleted()) {
-            // At this point we are not actually sure that Receiver actually has
-            // a property P, however, if it has, we are sure that its a data property,
-            // and that redefining the property with valueDesc will not change the
-            // attributes of the property, so we delete it to make things nice for $DefineOwnProperty.
-            Receiver.$Delete(P);
+            // At this point we are not sure that Receiver actually has a property P.
+            // If, however, it has -> P. If, however, it has, we are sure that its a
+            // data property, and that redefining the property with valueDesc will not
+            // change the attributes of the property, so we can reuse the existing
+            // descriptor.
             valueDesc = existingDescriptor;
             valueDesc.value = V;
           }
@@ -700,7 +702,7 @@ export class PropertiesImplementation {
     if (!desc) {
       ensureIsNotFinal(realm, O, P);
       if (!realm.ignoreLeakLogic && O.mightBeHavocedObject()) {
-        if (realm.generator) {
+        if (realm.generator !== undefined) {
           realm.generator.emitPropertyDelete(O, StringKey(P));
         }
       }
@@ -711,7 +713,7 @@ export class PropertiesImplementation {
     if (desc.configurable) {
       ensureIsNotFinal(realm, O, P);
       if (O.mightBeHavocedObject()) {
-        if (realm.generator) {
+        if (realm.generator !== undefined) {
           realm.generator.emitPropertyDelete(O, StringKey(P));
         }
         return true;
@@ -839,7 +841,7 @@ export class PropertiesImplementation {
         ensureIsNotFinal(realm, O, P);
         if (!realm.ignoreLeakLogic && O.mightBeHavocedObject()) {
           havocDescriptor(realm, Desc);
-          if (realm.generator) {
+          if (realm.generator !== undefined) {
             realm.generator.emitDefineProperty(O, StringKey(P), Desc);
           }
           return true;
@@ -883,7 +885,6 @@ export class PropertiesImplementation {
       // e. Return true.
       return true;
     }
-    this.ThrowIfMightHaveBeenDeleted(current.value);
 
     // 3. Return true, if every field in Desc is absent.
     if (!Object.keys(Desc).length) return true;
@@ -913,8 +914,12 @@ export class PropertiesImplementation {
       return true;
     }
 
+    let mightHaveBeenDeleted = current.value instanceof Value && current.value.mightHaveBeenDeleted();
+
     // 5. If the [[Configurable]] field of current is false, then
     if (!current.configurable) {
+      invariant(!mightHaveBeenDeleted, "a non-configurable property can't be deleted");
+
       // a. Return false, if the [[Configurable]] field of Desc is true.
       if (Desc.configurable) return false;
 
@@ -928,7 +933,7 @@ export class PropertiesImplementation {
       ensureIsNotFinal(realm, O, P);
       if (!realm.ignoreLeakLogic && O.mightBeHavocedObject()) {
         havocDescriptor(realm, Desc);
-        if (realm.generator) {
+        if (realm.generator !== undefined) {
           realm.generator.emitDefineProperty(O, StringKey(P), Desc);
         }
         return true;
@@ -952,9 +957,6 @@ export class PropertiesImplementation {
         // Preserve the existing values of the converted property's [[Configurable]] and [[Enumerable]] attributes and set the rest of the property's attributes to their default values.
         if (O !== undefined) {
           invariant(P !== undefined);
-          let key = InternalGetPropertiesKey(P);
-          let propertyBinding = InternalGetPropertiesMap(O, P).get(key);
-          invariant(propertyBinding !== undefined);
           delete current.writable;
           delete current.value;
           current.get = realm.intrinsics.undefined;
@@ -965,9 +967,6 @@ export class PropertiesImplementation {
         // i. If O is not undefined, convert the property named P of object O from an accessor property to a data property. Preserve the existing values of the converted property's [[Configurable]] and [[Enumerable]] attributes and set the rest of the property's attributes to their default values.
         if (O !== undefined) {
           invariant(P !== undefined);
-          let key = InternalGetPropertiesKey(P);
-          let propertyBinding = InternalGetPropertiesMap(O, P).get(key);
-          invariant(propertyBinding !== undefined);
           delete current.get;
           delete current.set;
           current.writable = false;
@@ -1007,6 +1006,25 @@ export class PropertiesImplementation {
       }
     }
 
+    if (mightHaveBeenDeleted) {
+      // If the property might have been deleted, we need to ensure that either
+      // the new descriptor overrides any existing values, or always results in
+      // the default value.
+      let unknownEnumerable = !("enumerable" in Desc) && !!current.enumerable;
+      let unknownWritable = !("writable" in Desc) && !!current.writable;
+      if (unknownEnumerable || unknownWritable) {
+        let error = new CompilerDiagnostic(
+          "unknown descriptor attributes on deleted property",
+          realm.currentLocation,
+          "PP0038",
+          "RecoverableError"
+        );
+        if (realm.handleError(error) !== "Recover") {
+          throw new FatalError();
+        }
+      }
+    }
+
     // 10. If O is not undefined, then
     if (O !== undefined) {
       invariant(P !== undefined);
@@ -1041,9 +1059,7 @@ export class PropertiesImplementation {
     invariant(O instanceof ObjectValue);
 
     // 1. Let current be ? O.[[GetOwnProperty]](P).
-    let current;
-    let binding = InternalGetPropertiesMap(O, P).get(InternalGetPropertiesKey(P));
-    if (binding !== undefined || !(O.isPartialObject() && O.isSimpleObject())) current = O.$GetOwnProperty(P);
+    let current = O.$GetOwnProperty(P);
 
     // 2. Let extensible be the value of the [[Extensible]] internal slot of O.
     let extensible = O.getExtensible();
@@ -1382,7 +1398,6 @@ export class PropertiesImplementation {
           if (P instanceof StringValue) P = P.value;
           if (typeof P === "string") {
             // In this case it is safe to defer the property access to runtime (at this point in time)
-            invariant(realm.generator);
             let absVal;
             function createAbstractPropertyValue(type: typeof Value) {
               invariant(typeof P === "string");
@@ -1394,7 +1409,7 @@ export class PropertiesImplementation {
                   createOperationDescriptor("ABSTRACT_PROPERTY"),
                   { kind: AbstractValue.makeKind("property", P) }
                 );
-              } else {
+              } else if (realm.generator !== undefined) {
                 return AbstractValue.createTemporalFromBuildFunction(
                   realm,
                   type,
@@ -1402,6 +1417,18 @@ export class PropertiesImplementation {
                   createOperationDescriptor("ABSTRACT_PROPERTY"),
                   { skipInvariant: true, isPure: true }
                 );
+              } else {
+                // During environment initialization we'll call Set and DefineOwnProperty
+                // to initialize objects. Since these needs to introspect the descriptor,
+                // we need some kind of value as its placeholder. This value should never
+                // leak to the serialized environment.
+                return AbstractValue.createFromBuildFunction(
+                  realm,
+                  type,
+                  [O._templateFor || O, new StringValue(realm, P)],
+                  createOperationDescriptor("ABSTRACT_PROPERTY"),
+                  { kind: "environment initialization expression" }
+                );
               }
             }
             if (O.isTransitivelySimple()) {

src/serializer/ResidualHeapVisitor.js

@@ -1001,6 +1001,8 @@ export class ResidualHeapVisitor {
   visitAbstractValue(val: AbstractValue): void {
     if (val.kind === "sentinel member expression") {
       this.logger.logError(val, "expressions of type o[p] are not yet supported for partially known o and unknown p");
+    } else if (val.kind === "environment initialization expression") {
+      this.logger.logError(val, "reads during environment initialization should never leak to serialization");
     } else if (val.kind === "conditional") {
       this._visitAbstractValueConditional(val);
       return;

src/values/AbstractValue.js

@@ -59,6 +59,7 @@ export type AbstractValueKind =
   | "explicit conversion to object"
   | "check for known property"
   | "sentinel member expression"
+  | "environment initialization expression"
   | "template for property name condition"
   | "template for prototype member expression"
   | "this"

src/values/ObjectValue.js

@@ -403,6 +403,7 @@ export default class ObjectValue extends ConcreteValue {
   }
 
   isSimpleObject(): boolean {
+    if (this === this.$Realm.intrinsics.ObjectPrototype) return true;
     if (!this._isSimple.mightNotBeTrue()) return true;
     if (this.isPartialObject()) return false;
     if (this.symbols.size > 0) return false;
@@ -413,7 +414,6 @@ export default class ObjectValue extends ConcreteValue {
       if (!desc.writable) return false;
     }
     if (this.$Prototype instanceof NullValue) return true;
-    if (this.$Prototype === this.$Realm.intrinsics.ObjectPrototype) return true;
     invariant(this.$Prototype);
     return this.$Prototype.isSimpleObject();
   }

test/error-handler/PropertyAttributeConflict.js

@@ -0,0 +1,10 @@
+// recover-from-errors
+// expected errors: [{location: {"start":{"line":7,"column":41},"end":{"line":7,"column":42},"source":"test/error-handler/PropertyAttributeConflict.js"}, errorCode: "PP0038", severity: "RecoverableError", message: "unknown descriptor attributes on deleted property"}]
+
+var c = __abstract("boolean", "(true)");
+var obj = { x: 1 };
+if (c) delete obj.x;
+Object.defineProperty(obj, "x", { value: 2 });
+inspect = function() {
+  return Object.getOwnPropertyDescriptor(obj, "x").enumerable;
+};

test/serializer/abstract/DefineDeletedProperty.js

@@ -0,0 +1,9 @@
+var c = global.__abstract ? __abstract("boolean", "(true)") : true;
+var obj = {};
+Object.defineProperty(obj, "x", { value: 1, enumerable: true, writable: true, configurable: true });
+if (c) delete obj.x;
+Object.defineProperty(obj, "x", { enumerable: true, writable: true, configurable: false });
+
+inspect = function() {
+  return Object.getOwnPropertyDescriptor(obj, "x");
+};

test/serializer/abstract/PropertyAttributeConflict.js

@@ -0,0 +1,9 @@
+// throws introspection error
+
+var c = global.__abstract ? __abstract("boolean", "(true)") : true;
+var obj = { x: 1 };
+if (c) delete obj.x;
+Object.defineProperty(obj, "x", { value: 2 });
+inspect = function() {
+  return Object.getOwnPropertyDescriptor(obj, "x").enumerable;
+};