Handle non-enumerable property shadowing

Summary: Gather the nonenumerable properties at each level of the prototype chain, and add them to the dedup'ing set.

Reviewed By: jpporto

Differential Revision: D43645420

fbshipit-source-id: 3eeb3cfcd6711f0a24d62ba7443a9b4f446e4d24

include/hermes/VM/JSObject.h

@@ -599,6 +599,18 @@ class JSObject : public GCCell {
             !onlyEnumerable));
   }
 
+  /// Same as getOwnPropertyNames, however, *only* include non-enumerable
+  /// properties, and skip enumerable properties.
+  /// \returns a JSArray containing the names.
+  static CallResult<Handle<JSArray>> getOwnNonEnumerablePropertyNames(
+      Handle<JSObject> selfHandle,
+      Runtime &runtime) {
+    return getOwnPropertyKeys(
+        selfHandle,
+        runtime,
+        OwnKeys::Flags().plusIncludeNonSymbols().plusIncludeNonEnumerable());
+  }
+
   /// Return a list of property symbols keys belonging to this object.
   /// The order of properties follows ES2015 - insertion order.
   /// \returns a JSArray containing the symbols.

lib/VM/JSObject.cpp

@@ -3042,6 +3042,15 @@ CallResult<uint32_t> appendAllPropertyNames(
   // strings (SymbolIDs) or index names.
   llvh::SmallSet<SymbolID::RawType, 4> dedupNames;
   llvh::SmallSet<double, 4> dedupIdxNames;
+  // For the SymbolIDs we collect for the non enumerable properties, they are
+  // not stored in the resulting BigStorage. Therefore it's not safe to store
+  // the raw types in the SmallSet. We put them in this ArrayStorage to make the
+  // gc aware we are still holding reference to them.
+  CallResult<HermesValue> keepaliveRes = ArrayStorage::create(runtime, 0, 0);
+  // We can use keepaliveRes without checking its ExecutionResult since it will
+  // never fail.
+  MutableHandle<ArrayStorage> keepalive =
+      runtime.makeMutableHandle(vmcast<ArrayStorage>(*keepaliveRes));
   // Add the current value of prop and/or propIdHandle to correct set(s).
   auto addToDedup = [&dedupIdxNames, &dedupNames, &runtime](
                         Handle<> prop, Handle<SymbolID> propIdHandle) {
@@ -3133,6 +3142,44 @@ CallResult<uint32_t> appendAllPropertyNames(
         ++size;
       }
     }
+    // Now we must handle non-enumerable property shadowing. Get all the
+    // non-enumerable properties in this object, and add them to the sets we are
+    // maintaining. Therefore, if a parent object later on has a property with
+    // the same name as an non-enumerable property defined in the child object,
+    // the child object will shadow the parent's enumerable property.
+    // We don't support property shadowing with proxy objects.
+    if (LLVM_LIKELY(!head->isProxyObject())) {
+      auto nonEnumerablePropsRes =
+          JSObject::getOwnNonEnumerablePropertyNames(head, runtime);
+      if (LLVM_UNLIKELY(cr == ExecutionStatus::EXCEPTION)) {
+        return ExecutionStatus::EXCEPTION;
+      }
+      auto nonEnumerableProps = *nonEnumerablePropsRes;
+      auto flushTo = gcScope.createMarker();
+      if (ArrayStorage::ensureCapacity(
+              keepalive,
+              runtime,
+              keepalive->size() + nonEnumerableProps->getEndIndex()) ==
+          ExecutionStatus::EXCEPTION) {
+        return ExecutionStatus::EXCEPTION;
+      }
+      for (unsigned i = 0, e = nonEnumerableProps->getEndIndex(); i < e; ++i) {
+        gcScope.flushToMarker(flushTo);
+        prop = nonEnumerableProps->at(runtime, i).unboxToHV(runtime);
+        if (prop->isString()) {
+          CallResult<Handle<SymbolID>> symRes =
+              runtime.getIdentifierTable().getSymbolHandleFromPrimitive(
+                  runtime,
+                  runtime.makeHandle<StringPrimitive>(prop->getString()));
+          if (LLVM_UNLIKELY(symRes == ExecutionStatus::EXCEPTION)) {
+            return ExecutionStatus::EXCEPTION;
+          }
+          propIdHandle = *symRes;
+          ArrayStorage::push_back(keepalive, runtime, propIdHandle);
+        }
+        addToDedup(prop, propIdHandle);
+      }
+    }
     // Continue to follow the prototype chain.
     CallResult<PseudoHandle<JSObject>> parentRes =
         JSObject::getPrototypeOf(head, runtime);

test/hermes/for_in.js

@@ -127,3 +127,23 @@ for (var prop in child) {
 //CHECK: dup
 //CHECK: b
 //CHECK: a
+
+// Check that non-enumerable child properties shadow parent 
+// enumerable properties of the same name.
+var parent = {hide: "value", propA: 5};
+var child = Object.create(parent);
+Object.defineProperty(child, 'hide', {enumerable: false});
+child.propB = 5;
+for (var prop in child) {
+  print(prop);
+}
+//CHECK: propB
+//CHECK: propA
+var grandchild = Object.create(child);
+grandchild.propC = 10;
+for (var prop in grandchild) {
+  print(prop);
+}
+//CHECK: propC
+//CHECK: propB
+//CHECK: propA