[proxies] Implement Symbol/DONT_ENUM filtering for GetKeys()

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 335ffd27ee996154af93db1a629e9e2005772d73..c292eac1d558ebf3255a969c28478421db3c671f 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -4786,7 +4786,8 @@ Maybe<PropertyAttributes> JSProxy::GetPropertyAttributes(LookupIterator* it) {
   Isolate* isolate = it->isolate();
   HandleScope scope(isolate);
   PropertyDescriptor desc;
-  bool found = JSProxy::GetOwnPropertyDescriptor(it, &desc);
+  bool found = JSProxy::GetOwnPropertyDescriptor(
+      isolate, it->GetHolder<JSProxy>(), it->GetName(), &desc);
   if (isolate->has_pending_exception()) return Nothing<PropertyAttributes>();
   if (!found) return Just(ABSENT);
   return Just(desc.ToAttributes());
@@ -6900,12 +6901,13 @@ bool JSReceiver::GetOwnPropertyDescriptor(Isolate* isolate,
 // static
 bool JSReceiver::GetOwnPropertyDescriptor(LookupIterator* it,
                                           PropertyDescriptor* desc) {
+  Isolate* isolate = it->isolate();
   // "Virtual" dispatch.
   if (it->IsFound() && it->GetHolder<JSReceiver>()->IsJSProxy()) {
-    return JSProxy::GetOwnPropertyDescriptor(it, desc);
+    return JSProxy::GetOwnPropertyDescriptor(isolate, it->GetHolder<JSProxy>(),
+                                             it->GetName(), desc);
   }
 
-  Isolate* isolate = it->isolate();
   // 1. (Assert)
   // 2. If O does not have an own property with key P, return undefined.
   Maybe<PropertyAttributes> maybe = JSObject::GetPropertyAttributes(it);
@@ -6954,13 +6956,11 @@ bool JSReceiver::GetOwnPropertyDescriptor(LookupIterator* it,
 
 // ES6 9.5.5
 // static
-bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
+bool JSProxy::GetOwnPropertyDescriptor(Isolate* isolate, Handle<JSProxy> proxy,
+                                       Handle<Name> name,
                                        PropertyDescriptor* desc) {
-  Handle<JSProxy> proxy = it->GetHolder<JSProxy>();
-  Isolate* isolate = it->isolate();
   Handle<String> trap_name =
       isolate->factory()->getOwnPropertyDescriptor_string();
-  Handle<Name> property_name = it->GetName();
   // 1. (Assert)
   // 2. Let handler be the value of the [[ProxyHandler]] internal slot of O.
   Handle<Object> handler(proxy->handler(), isolate);
@@ -6973,10 +6973,9 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   // 4. Assert: Type(handler) is Object.
   DCHECK(handler->IsJSReceiver());
   // If the handler is not null, the target can't be null either.
-  DCHECK(it->GetHolder<JSProxy>()->target()->IsJSReceiver());
+  DCHECK(proxy->target()->IsJSReceiver());
   // 5. Let target be the value of the [[ProxyTarget]] internal slot of O.
-  Handle<JSReceiver> target(
-      JSReceiver::cast(it->GetHolder<JSProxy>()->target()), isolate);
+  Handle<JSReceiver> target(JSReceiver::cast(proxy->target()), isolate);
   // 6. Let trap be ? GetMethod(handler, "getOwnPropertyDescriptor").
   Handle<Object> trap;
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
@@ -6985,12 +6984,11 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   // 7. If trap is undefined, then
   if (trap->IsUndefined()) {
     // 7a. Return target.[[GetOwnProperty]](P).
-    return JSReceiver::GetOwnPropertyDescriptor(isolate, target, property_name,
-                                                desc);
+    return JSReceiver::GetOwnPropertyDescriptor(isolate, target, name, desc);
   }
   // 8. Let trapResultObj be ? Call(trap, handler, «target, P»).
   Handle<Object> trap_result_obj;
-  Handle<Object> args[] = {target, property_name};
+  Handle<Object> args[] = {target, name};
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
       isolate, trap_result_obj,
       Execution::Call(isolate, trap, handler, arraysize(args), args), false);
@@ -6999,13 +6997,12 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   if (!trap_result_obj->IsJSReceiver() && !trap_result_obj->IsUndefined()) {
     isolate->Throw(*isolate->factory()->NewTypeError(
         MessageTemplate::kProxyHandlerReturned, handler, trap_result_obj,
-        property_name));
+        name));
     return false;
   }
   // 10. Let targetDesc be ? target.[[GetOwnProperty]](P).
   PropertyDescriptor target_desc;
-  JSReceiver::GetOwnPropertyDescriptor(isolate, target, property_name,
-                                       &target_desc);
+  JSReceiver::GetOwnPropertyDescriptor(isolate, target, name, &target_desc);
   if (isolate->has_pending_exception()) return false;
   // 11. If trapResultObj is undefined, then
   if (trap_result_obj->IsUndefined()) {
@@ -7015,7 +7012,7 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
     //      exception.
     if (!target_desc.configurable()) {
       isolate->Throw(*isolate->factory()->NewTypeError(
-          MessageTemplate::kProxyTargetPropNotConfigurable, property_name));
+          MessageTemplate::kProxyTargetPropNotConfigurable, name));
       return false;
     }
     // 11c. Let extensibleTarget be ? IsExtensible(target).
@@ -7047,7 +7044,7 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
   // 15. Let valid be IsCompatiblePropertyDescriptor (extensibleTarget,
   //     resultDesc, targetDesc).
   bool valid = IsCompatiblePropertyDescriptor(isolate, extensible_target, desc,
-                                              &target_desc, property_name);
+                                              &target_desc, name);
   // 16. If valid is false, throw a TypeError exception.
   if (!valid) {
     DCHECK(isolate->has_pending_exception());
@@ -7059,7 +7056,7 @@ bool JSProxy::GetOwnPropertyDescriptor(LookupIterator* it,
     if (target_desc.is_empty() || target_desc.configurable()) {
       // 17a i. Throw a TypeError exception.
       isolate->Throw(*isolate->factory()->NewTypeError(
-          MessageTemplate::kRedefineDisallowed, property_name));
+          MessageTemplate::kRedefineDisallowed, name));
       return false;
     }
   }
@@ -8293,9 +8290,39 @@ static MaybeHandle<FixedArray> CreateListFromArrayLike_StringSymbol(
 }
 
 
+MaybeHandle<FixedArray> FilterProxyKeys(Isolate* isolate, Handle<JSProxy> owner,
+                                        Handle<FixedArray> keys,
+                                        KeyFilter filter,
+                                        Enumerability enum_policy) {
+  if (filter == INCLUDE_SYMBOLS && enum_policy == IGNORE_ENUMERABILITY) {
+    // Nothing to do.
+    return keys;
+  }
+  int store_position = 0;
+  for (int i = 0; i < keys->length(); ++i) {
+    Handle<Name> key(Name::cast(keys->get(i)), isolate);
+    if (filter == SKIP_SYMBOLS && key->IsSymbol()) continue;  // Skip this key.
+    if (enum_policy == RESPECT_ENUMERABILITY) {
+      PropertyDescriptor desc;
+      bool found =
+          JSProxy::GetOwnPropertyDescriptor(isolate, owner, key, &desc);
+      if (isolate->has_pending_exception()) return MaybeHandle<FixedArray>();
+      if (!found || !desc.enumerable()) continue;  // Skip this key.
+    }
+    // Keep this key.
+    if (store_position != i) {
+      keys->set(store_position, *key);
+    }
+    store_position++;
+  }
+  if (store_position == 0) return isolate->factory()->empty_fixed_array();
+  keys->Shrink(store_position);

[Camillo Bruni, 2015/12/01 14:39:39]

we could actually avoid shrinking it by passing the definite size to
AddKeysFromProxy then again we don't care too much for proxies I guess :)

[Jakob Kummerow, 2015/12/02 09:42:56]

Acknowledged.

+  return keys;
+}
+
+
 // ES6 9.5.12
 // Returns "false" in case of exception.
-// TODO(jkummerow): |filter| and |enum_policy| are currently ignored.
 // static
 bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
                               Handle<JSProxy> proxy, KeyFilter filter,
@@ -8382,6 +8409,10 @@ bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
   //     then:
   if (extensible_target && nonconfigurable_keys_length == 0) {
     // 15a. Return trapResult.
+    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+        isolate, trap_result,
+        FilterProxyKeys(isolate, proxy, trap_result, filter, enum_policy),
+        false);

[Camillo Bruni, 2015/12/01 14:39:39]

I wonder whether it would not make more sense to do the filtering in
GetKeys_Internal?

[Jakob Kummerow, 2015/12/02 09:42:56]

Done -- moved it into the AddKeysFromProxy call right below. (There's no way we
could do it from GetKeys_Internal.)

     accumulator->AddKeysFromProxy(trap_result);
     return true;
   }
@@ -8412,6 +8443,10 @@ bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
   }
   // 18. If extensibleTarget is true, return trapResult.
   if (extensible_target) {
+    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+        isolate, trap_result,
+        FilterProxyKeys(isolate, proxy, trap_result, filter, enum_policy),
+        false);
     accumulator->AddKeysFromProxy(trap_result);
     return true;
   }
@@ -8439,6 +8474,9 @@ bool JSProxy::OwnPropertyKeys(Isolate* isolate, Handle<JSReceiver> receiver,
     return false;
   }
   // 21. Return trapResult.
+  ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+      isolate, trap_result,
+      FilterProxyKeys(isolate, proxy, trap_result, filter, enum_policy), false);
   accumulator->AddKeysFromProxy(trap_result);
   return true;
 }