[es7] refactor and fix Object.values() / Object.entries()

Previously, Object.values() and Object.entries() were piggy-backing on
Object.keys(). This meant that they would pre-filter non-enumerable properties,
violating the runtime behaviour of the methods. Unfortunately, this does not
match the current proposal text.

Also incorporates several tests verifying this behaviour based on tests included
in the ChakraCore implementation.

BUG=v8:4663
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, littledan@chromium.org

Review URL: https://codereview.chromium.org/1637753004

Cr-Commit-Position: refs/heads/master@{#33782}

src/builtins.cc

@@ -1777,22 +1777,10 @@ BUILTIN(ObjectValues) {
   Handle<JSReceiver> receiver;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, receiver,
                                      Object::ToObject(isolate, object));
-  Handle<FixedArray> keys;
+  Handle<FixedArray> values;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-      isolate, keys, JSReceiver::GetKeys(receiver, OWN_ONLY, ENUMERABLE_STRINGS,
-                                         CONVERT_TO_STRING));
-
-  for (int i = 0; i < keys->length(); ++i) {
-    auto key = Handle<Name>::cast(FixedArray::get(*keys, i, isolate));
-    Handle<Object> value;
-
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, value, Object::GetPropertyOrElement(receiver, key, STRICT));
-
-    keys->set(i, *value);
-  }
-
-  return *isolate->factory()->NewJSArrayWithElements(keys);
+      isolate, values, JSReceiver::GetOwnValues(receiver, ENUMERABLE_STRINGS));
+  return *isolate->factory()->NewJSArrayWithElements(values);
 }
 
 
@@ -1802,26 +1790,11 @@ BUILTIN(ObjectEntries) {
   Handle<JSReceiver> receiver;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, receiver,
                                      Object::ToObject(isolate, object));
-  Handle<FixedArray> keys;
+  Handle<FixedArray> entries;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-      isolate, keys, JSReceiver::GetKeys(receiver, OWN_ONLY, ENUMERABLE_STRINGS,
-                                         CONVERT_TO_STRING));
-
-  for (int i = 0; i < keys->length(); ++i) {
-    auto key = Handle<Name>::cast(FixedArray::get(*keys, i, isolate));
-    Handle<Object> value;
-
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, value, Object::GetPropertyOrElement(receiver, key, STRICT));
-
-    auto entry_storage = isolate->factory()->NewUninitializedFixedArray(2);
-    entry_storage->set(0, *key);
-    entry_storage->set(1, *value);
-    auto entry = isolate->factory()->NewJSArrayWithElements(entry_storage);
-    keys->set(i, *entry);
-  }
-
-  return *isolate->factory()->NewJSArrayWithElements(keys);
+      isolate, entries,
+      JSReceiver::GetOwnEntries(receiver, ENUMERABLE_STRINGS));
+  return *isolate->factory()->NewJSArrayWithElements(entries);
 }
 
 BUILTIN(ObjectGetOwnPropertyDescriptors) {

src/objects-inl.h

@@ -2375,7 +2375,8 @@ void FixedArray::set(int index, Smi* value) {
 void FixedArray::set(int index, Object* value) {
   DCHECK_NE(GetHeap()->fixed_cow_array_map(), map());
   DCHECK(IsFixedArray());
-  DCHECK(index >= 0 && index < this->length());
+  DCHECK_GE(index, 0);
+  DCHECK_LT(index, this->length());
   int offset = kHeaderSize + index * kPointerSize;
   WRITE_FIELD(this, offset, value);
   WRITE_BARRIER(GetHeap(), this, offset, value);

src/objects.cc

@@ -8944,6 +8944,64 @@ MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
   return keys;
 }
 
+MaybeHandle<FixedArray> GetOwnValuesOrEntries(Isolate* isolate,
+                                              Handle<JSReceiver> object,
+                                              PropertyFilter filter,
+                                              bool get_entries) {
+  PropertyFilter key_filter =
+      static_cast<PropertyFilter>(filter & ~ONLY_ENUMERABLE);
+  KeyAccumulator accumulator(isolate, OWN_ONLY, key_filter);
+  MAYBE_RETURN(GetKeys_Internal(isolate, object, object, OWN_ONLY, key_filter,
+                                &accumulator),
+               MaybeHandle<FixedArray>());
+  Handle<FixedArray> keys = accumulator.GetKeys(CONVERT_TO_STRING);
+  DCHECK(ContainsOnlyValidKeys(keys));
+
+  Handle<FixedArray> values_or_entries =
+      isolate->factory()->NewUninitializedFixedArray(keys->length());
+  int length = 0;
+
+  for (int i = 0; i < keys->length(); ++i) {
+    Handle<Name> key = Handle<Name>::cast(handle(keys->get(i), isolate));
+
+    if (filter & ONLY_ENUMERABLE) {
+      PropertyDescriptor descriptor;
+      Maybe<bool> did_get_descriptor = JSReceiver::GetOwnPropertyDescriptor(
+          isolate, object, key, &descriptor);
+      MAYBE_RETURN(did_get_descriptor, MaybeHandle<FixedArray>());
+      if (!did_get_descriptor.FromJust() || !descriptor.enumerable()) continue;
+    }
+
+    Handle<Object> value;
+    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+        isolate, value, JSReceiver::GetPropertyOrElement(object, key, STRICT),
+        MaybeHandle<FixedArray>());
+
+    if (get_entries) {
+      Handle<FixedArray> entry_storage =
+          isolate->factory()->NewUninitializedFixedArray(2);
+      entry_storage->set(0, *key);
+      entry_storage->set(1, *value);
+      value = isolate->factory()->NewJSArrayWithElements(entry_storage,
+                                                         FAST_ELEMENTS, 2);
+    }
+
+    values_or_entries->set(length, *value);
+    length++;
+  }
+  if (length < values_or_entries->length()) values_or_entries->Shrink(length);
+  return values_or_entries;
+}
+
+MaybeHandle<FixedArray> JSReceiver::GetOwnValues(Handle<JSReceiver> object,
+                                                 PropertyFilter filter) {
+  return GetOwnValuesOrEntries(object->GetIsolate(), object, filter, false);
+}
+
+MaybeHandle<FixedArray> JSReceiver::GetOwnEntries(Handle<JSReceiver> object,
+                                                  PropertyFilter filter) {
+  return GetOwnValuesOrEntries(object->GetIsolate(), object, filter, true);
+}
 
 bool Map::DictionaryElementsInPrototypeChainOnly() {
   if (IsDictionaryElementsKind(elements_kind())) {

src/objects.h

@@ -1959,6 +1959,12 @@ class JSReceiver: public HeapObject {
       Handle<JSReceiver> object, KeyCollectionType type, PropertyFilter filter,
       GetKeysConversion keys_conversion = KEEP_NUMBERS);
 
+  MUST_USE_RESULT static MaybeHandle<FixedArray> GetOwnValues(
+      Handle<JSReceiver> object, PropertyFilter filter);
+
+  MUST_USE_RESULT static MaybeHandle<FixedArray> GetOwnEntries(
+      Handle<JSReceiver> object, PropertyFilter filter);
+
   // Layout description.
   static const int kPropertiesOffset = HeapObject::kHeaderSize;
   static const int kHeaderSize = HeapObject::kHeaderSize + kPointerSize;

test/mjsunit/harmony/object-entries.js

@@ -3,10 +3,19 @@
 // found in the LICENSE file.
 
 // Flags: --harmony-object-values-entries --harmony-proxies --harmony-reflect
+// Flags: --allow-natives-syntax
 
 function TestMeta() {
   assertEquals(1, Object.entries.length);
   assertEquals(Function.prototype, Object.getPrototypeOf(Object.entries));
+  assertEquals("entries", Object.entries.name);
+
+  var descriptor = Object.getOwnPropertyDescriptor(Object, "entries");
+  assertTrue(descriptor.writable);
+  assertFalse(descriptor.enumerable);
+  assertTrue(descriptor.configurable);
+
+  assertThrows(() => new Object.entries({}), TypeError);
 }
 TestMeta();
 
@@ -36,10 +45,21 @@ function TestBasic() {
     ["b", 4]
   ], Object.entries(O));
   assertEquals(Object.entries(O), Object.keys(O).map(key => [key, O[key]]));
+
+  assertTrue(Array.isArray(Object.entries({})));
+  assertEquals(0, Object.entries({}).length);
 }
 TestBasic();
 
 
+function TestToObject() {
+  assertThrows(function() { Object.entries(); }, TypeError);
+  assertThrows(function() { Object.entries(null); }, TypeError);
+  assertThrows(function() { Object.entries(void 0); }, TypeError);
+}
+TestToObject();
+
+
 function TestOrder() {
   var O = {
     a: 1,
@@ -47,6 +67,8 @@ function TestOrder() {
   };
   O[456] = 123;
   Object.defineProperty(O, "HIDDEN", { enumerable: false, value: NaN });
+  var priv = %CreatePrivateSymbol("Secret");
+  O[priv] = 56;
 
   var log = [];
   var P = new Proxy(O, {
@@ -58,6 +80,10 @@ function TestOrder() {
       log.push(`[[Get]](${JSON.stringify(name)})`);
       return Reflect.get(target, name);
     },
+    getOwnPropertyDescriptor(target, name) {
+      log.push(`[[GetOwnProperty]](${JSON.stringify(name)})`);
+      return Reflect.getOwnPropertyDescriptor(target, name);
+    },
     set(target, name, value) {
       assertUnreachable();
     }
@@ -66,8 +92,158 @@ function TestOrder() {
   assertEquals([["456", 123], ["a", 1]], Object.entries(P));
   assertEquals([
     "[[OwnPropertyKeys]]",
+    "[[GetOwnProperty]](\"456\")",
     "[[Get]](\"456\")",
-    "[[Get]](\"a\")"
+    "[[GetOwnProperty]](\"a\")",
+    "[[Get]](\"a\")",
+    "[[GetOwnProperty]](\"HIDDEN\")"
   ], log);
 }
 TestOrder();
+
+
+function TestOrderWithDuplicates() {
+  var O = {
+    a: 1,
+    [Symbol.iterator]: null
+  };
+  O[456] = 123;
+  Object.defineProperty(O, "HIDDEN", { enumerable: false, value: NaN });
+  var priv = %CreatePrivateSymbol("Secret");
+  O[priv] = 56;
+
+  var log = [];
+  var P = new Proxy(O, {
+    ownKeys(target) {
+      log.push("[[OwnPropertyKeys]]");
+      return ["a", Symbol.iterator, "a", "456", "HIDDEN", "HIDDEN", "456"];
+    },
+    get(target, name) {
+      log.push(`[[Get]](${JSON.stringify(name)})`);
+      return Reflect.get(target, name);
+    },
+    getOwnPropertyDescriptor(target, name) {
+      log.push(`[[GetOwnProperty]](${JSON.stringify(name)})`);
+      return Reflect.getOwnPropertyDescriptor(target, name);
+    },
+    set(target, name, value) {
+      assertUnreachable();
+    }
+  });
+
+  assertEquals([
+    ["a", 1],
+    ["a", 1],
+    ["456", 123],
+    ["456", 123]
+  ], Object.entries(P));
+  assertEquals([
+    "[[OwnPropertyKeys]]",
+    "[[GetOwnProperty]](\"a\")",
+    "[[Get]](\"a\")",
+    "[[GetOwnProperty]](\"a\")",
+    "[[Get]](\"a\")",
+    "[[GetOwnProperty]](\"456\")",
+    "[[Get]](\"456\")",
+    "[[GetOwnProperty]](\"HIDDEN\")",
+    "[[GetOwnProperty]](\"HIDDEN\")",
+    "[[GetOwnProperty]](\"456\")",
+    "[[Get]](\"456\")"
+  ], log);
+}
+TestOrderWithDuplicates();
+
+
+function TestPropertyFilter() {
+  var object = { prop3: 30 };
+  object[2] = 40;
+  object["prop4"] = 50;
+  Object.defineProperty(object, "prop5", { value: 60, enumerable: true });
+  Object.defineProperty(object, "prop6", { value: 70, enumerable: false });
+  Object.defineProperty(object, "prop7", {
+      enumerable: true, get() { return 80; }});
+  var sym = Symbol("prop8");
+  object[sym] = 90;
+
+  values = Object.entries(object);
+  assertEquals(5, values.length);
+  assertEquals([
+    [ "2", 40 ],
+    [ "prop3", 30 ],
+    [ "prop4", 50 ],
+    [ "prop5", 60 ],
+    [ "prop7", 80 ]
+  ], values);
+}
+TestPropertyFilter();
+
+
+function TestWithProxy() {
+  var obj1 = {prop1:10};
+  var proxy1 = new Proxy(obj1, { });
+  assertEquals([ [ "prop1", 10 ] ], Object.entries(proxy1));
+
+  var obj2 = {};
+  Object.defineProperty(obj2, "prop2", { value: 20, enumerable: true });
+  Object.defineProperty(obj2, "prop3", {
+      get() { return 30; }, enumerable: true });
+  var proxy2 = new Proxy(obj2, {
+    getOwnPropertyDescriptor(target, name) {
+      return Reflect.getOwnPropertyDescriptor(target, name);
+    }
+  });
+  assertEquals([ [ "prop2", 20 ], [ "prop3", 30 ] ], Object.entries(proxy2));
+
+  var obj3 = {};
+  var count = 0;
+  var proxy3 = new Proxy(obj3, {
+    get(target, property, receiver) {
+      return count++ * 5;
+    },
+    getOwnPropertyDescriptor(target, property) {
+      return { configurable: true, enumerable: true };
+    },
+    ownKeys(target) {
+      return [ "prop0", "prop1", Symbol("prop2"), Symbol("prop5") ];
+    }
+  });
+  assertEquals([ [ "prop0", 0 ], [ "prop1", 5 ] ], Object.entries(proxy3));
+}
+TestWithProxy();
+
+
+function TestMutateDuringEnumeration() {
+  var aDeletesB = {
+    get a() {
+      delete this.b;
+      return 1;
+    },
+    b: 2
+  };
+  assertEquals([ [ "a", 1 ] ], Object.entries(aDeletesB));
+
+  var aRemovesB = {
+    get a() {
+      Object.defineProperty(this, "b", { enumerable: false });
+      return 1;
+    },
+    b: 2
+  };
+  assertEquals([ [ "a", 1 ] ], Object.entries(aRemovesB));
+
+  var aAddsB = { get a() { this.b = 2; return 1; } };
+  assertEquals([ [ "a", 1 ] ], Object.entries(aAddsB));
+
+  var aMakesBEnumerable = {};
+  Object.defineProperty(aMakesBEnumerable, "a", {
+    get() {
+      Object.defineProperty(this, "b", { enumerable: true });
+      return 1;
+    },
+    enumerable: true
+  });
+  Object.defineProperty(aMakesBEnumerable, "b", {
+      value: 2, configurable:true, enumerable: false });
+  assertEquals([ [ "a", 1 ], [ "b", 2 ] ], Object.entries(aMakesBEnumerable));
+}
+TestMutateDuringEnumeration();

test/mjsunit/harmony/object-values.js

@@ -3,10 +3,19 @@
 // found in the LICENSE file.
 
 // Flags: --harmony-object-values-entries --harmony-proxies --harmony-reflect
+// Flags: --allow-natives-syntax
 
 function TestMeta() {
   assertEquals(1, Object.values.length);
   assertEquals(Function.prototype, Object.getPrototypeOf(Object.values));
+  assertEquals("values", Object.values.name);
+
+  var descriptor = Object.getOwnPropertyDescriptor(Object, "values");
+  assertTrue(descriptor.writable);
+  assertFalse(descriptor.enumerable);
+  assertTrue(descriptor.configurable);
+
+  assertThrows(() => new Object.values({}), TypeError);
 }
 TestMeta();
 
@@ -27,10 +36,21 @@ function TestBasic() {
   Object.defineProperty(O, "HIDDEN", { enumerable: false, value: NaN });
   assertEquals([123, "ducks", 456, 1, 3, "quack", 2, 4], Object.values(O));
   assertEquals(Object.values(O), Object.keys(O).map(key => O[key]));
+
+  assertTrue(Array.isArray(Object.values({})));
+  assertEquals(0, Object.values({}).length);
 }
 TestBasic();
 
 
+function TestToObject() {
+  assertThrows(function() { Object.values(); }, TypeError);
+  assertThrows(function() { Object.values(null); }, TypeError);
+  assertThrows(function() { Object.values(void 0); }, TypeError);
+}
+TestToObject();
+
+
 function TestOrder() {
   var O = {
     a: 1,
@@ -38,6 +58,8 @@ function TestOrder() {
   };
   O[456] = 123;
   Object.defineProperty(O, "HIDDEN", { enumerable: false, value: NaN });
+  var priv = %CreatePrivateSymbol("Secret");
+  O[priv] = 56;
 
   var log = [];
   var P = new Proxy(O, {
@@ -49,6 +71,10 @@ function TestOrder() {
       log.push(`[[Get]](${JSON.stringify(name)})`);
       return Reflect.get(target, name);
     },
+    getOwnPropertyDescriptor(target, name) {
+      log.push(`[[GetOwnProperty]](${JSON.stringify(name)})`);
+      return Reflect.getOwnPropertyDescriptor(target, name);
+    },
     set(target, name, value) {
       assertUnreachable();
     }
@@ -57,8 +83,147 @@ function TestOrder() {
   assertEquals([123, 1], Object.values(P));
   assertEquals([
     "[[OwnPropertyKeys]]",
+    "[[GetOwnProperty]](\"456\")",
     "[[Get]](\"456\")",
-    "[[Get]](\"a\")"
+    "[[GetOwnProperty]](\"a\")",
+    "[[Get]](\"a\")",
+    "[[GetOwnProperty]](\"HIDDEN\")"
   ], log);
 }
 TestOrder();
+
+
+function TestOrderWithDuplicates() {
+  var O = {
+    a: 1,
+    [Symbol.iterator]: null
+  };
+  O[456] = 123;
+  Object.defineProperty(O, "HIDDEN", { enumerable: false, value: NaN });
+  O[priv] = 56;
+  var priv = %CreatePrivateSymbol("private");
+
+  var log = [];
+  var P = new Proxy(O, {
+    ownKeys(target) {
+      log.push("[[OwnPropertyKeys]]");
+      return [ "a", Symbol.iterator, "a", "456", "HIDDEN", "HIDDEN", "456" ];
+    },
+    get(target, name) {
+      log.push(`[[Get]](${JSON.stringify(name)})`);
+      return Reflect.get(target, name);
+    },
+    getOwnPropertyDescriptor(target, name) {
+      log.push(`[[GetOwnProperty]](${JSON.stringify(name)})`);
+      return Reflect.getOwnPropertyDescriptor(target, name);
+    },
+    set(target, name, value) {
+      assertUnreachable();
+    }
+  });
+
+  assertEquals([1, 1, 123, 123], Object.values(P));
+  assertEquals([
+    "[[OwnPropertyKeys]]",
+    "[[GetOwnProperty]](\"a\")",
+    "[[Get]](\"a\")",
+    "[[GetOwnProperty]](\"a\")",
+    "[[Get]](\"a\")",
+    "[[GetOwnProperty]](\"456\")",
+    "[[Get]](\"456\")",
+    "[[GetOwnProperty]](\"HIDDEN\")",
+    "[[GetOwnProperty]](\"HIDDEN\")",
+    "[[GetOwnProperty]](\"456\")",
+    "[[Get]](\"456\")",
+  ], log);
+}
+TestOrderWithDuplicates();
+
+
+function TestPropertyFilter() {
+  var object = { prop3: 30 };
+  object[2] = 40;
+  object["prop4"] = 50;
+  Object.defineProperty(object, "prop5", { value: 60, enumerable: true });
+  Object.defineProperty(object, "prop6", { value: 70, enumerable: false });
+  Object.defineProperty(object, "prop7", {
+      enumerable: true, get() { return 80; }});
+  var sym = Symbol("prop8");
+  object[sym] = 90;
+
+  values = Object.values(object);
+  assertEquals(5, values.length);
+  assertEquals([40,30,50,60,80], values);
+}
+TestPropertyFilter();
+
+
+function TestWithProxy() {
+  var obj1 = {prop1:10};
+  var proxy1 = new Proxy(obj1, { });
+  assertEquals([10], Object.values(proxy1));
+
+  var obj2 = {};
+  Object.defineProperty(obj2, "prop2", { value: 20, enumerable: true });
+  Object.defineProperty(obj2, "prop3", {
+      get() { return 30; }, enumerable: true });
+  var proxy2 = new Proxy(obj2, {
+    getOwnPropertyDescriptor(target, name) {
+      return Reflect.getOwnPropertyDescriptor(target, name);
+    }
+  });
+  assertEquals([20, 30], Object.values(proxy2));
+
+  var obj3 = {};
+  var count = 0;
+  var proxy3 = new Proxy(obj3, {
+    get(target, property, receiver) {
+      return count++ * 5;
+    },
+    getOwnPropertyDescriptor(target, property) {
+      return { configurable: true, enumerable: true };
+    },
+    ownKeys(target) {
+      return [ "prop0", "prop1", Symbol("prop2"), Symbol("prop5") ];
+    }
+  });
+  assertEquals([0, 5], Object.values(proxy3));
+}
+TestWithProxy();
+
+
+function TestMutateDuringEnumeration() {
+  var aDeletesB = {
+    get a() {
+      delete this.b;
+      return 1;
+    },
+    b: 2
+  };
+  assertEquals([1], Object.values(aDeletesB));
+
+  var aRemovesB = {
+    get a() {
+      Object.defineProperty(this, "b", { enumerable: false });
+      return 1;
+    },
+    b: 2
+  };
+  assertEquals([1], Object.values(aRemovesB));
+
+  var aAddsB = { get a() { this.b = 2; return 1; } };
+  assertEquals([1], Object.values(aAddsB));
+
+  var aMakesBEnumerable = {};
+  Object.defineProperty(aMakesBEnumerable, "a", {
+    get() {
+      Object.defineProperty(this, "b", { enumerable: true });
+      return 1;
+    },
+    enumerable: true
+  });
+  Object.defineProperty(aMakesBEnumerable, "b", {
+      value: 2, configurable:true, enumerable: false });
+  assertEquals([1, 2], Object.values(aMakesBEnumerable));
+}
+TestMutateDuringEnumeration();