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

src/objects.cc

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/objects.h"
 
 #include <cmath>
 #include <iomanip>
 #include <sstream>
 
@@ skipping 4768 matching lines @@
   }
 
   return JSObject::GetFunctionRealm(Handle<JSObject>::cast(receiver));
 }
 
 
 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());
 }
 
 
 MaybeHandle<Object> JSProxy::GetTrap(Handle<JSProxy> proxy,
                                      Handle<String> trap) {
   DCHECK(!proxy->IsRevoked());
   Isolate* isolate = proxy->GetIsolate();
@@ skipping 2093 matching lines @@
 
 
 // TODO(jkummerow): Any chance to unify this with
 // "MaybeHandle<Object> GetOwnProperty()" in runtime-object.cc?
 
 // ES6 9.1.5.1
 // Returns true on success; false if there was an exception or no property.
 // 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);
 
   if (!maybe.IsJust()) return false;
   PropertyAttributes attrs = maybe.FromJust();
   if (attrs == ABSENT) return false;
   DCHECK(!isolate->has_pending_exception());
 
   // 3. Let D be a newly created Property Descriptor with no fields.
@@ skipping 28 matching lines @@
   desc->set_configurable((attrs & DONT_DELETE) == 0);
   // 9. Return D.
   DCHECK(PropertyDescriptor::IsAccessorDescriptor(desc) !=
          PropertyDescriptor::IsDataDescriptor(desc));
   return true;
 }
 
 
 // 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);
   // 3. If handler is null, throw a TypeError exception.
   if (proxy->IsRevoked()) {
     isolate->Throw(*isolate->factory()->NewTypeError(
         MessageTemplate::kProxyRevoked, trap_name));
     return false;
   }
   // 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(
       isolate, trap,
       Object::GetMethod(Handle<JSReceiver>::cast(handler), trap_name), false);
   // 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);
   // 9. If Type(trapResultObj) is neither Object nor Undefined, throw a
   //    TypeError exception.
   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()) {
     // 11a. If targetDesc is undefined, return undefined.
     if (target_desc.is_empty()) return false;
     // 11b. If targetDesc.[[Configurable]] is false, throw a TypeError
     //      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).
     Maybe<bool> maybe_extensible = JSReceiver::IsExtensible(target);
     if (maybe_extensible.IsNothing()) return false;
     bool extensible_target = maybe_extensible.FromJust();
     // 11d. (Assert)
     // 11e. If extensibleTarget is false, throw a TypeError exception.
     if (!extensible_target) {
       isolate->Throw(*isolate->factory()->NewTypeError(
@@ skipping 11 matching lines @@
   if (!PropertyDescriptor::ToPropertyDescriptor(isolate, trap_result_obj,
                                                 desc)) {
     DCHECK(isolate->has_pending_exception());
     return false;
   }
   // 14. Call CompletePropertyDescriptor(resultDesc).
   PropertyDescriptor::CompletePropertyDescriptor(isolate, desc);
   // 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());
     return false;
   }
   // 17. If resultDesc.[[Configurable]] is false, then
   if (!desc->configurable()) {
     // 17a. If targetDesc is undefined or targetDesc.[[Configurable]] is true:
     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;
     }
   }
   // 18. Return resultDesc.
   return true;
 }
 
 
 bool JSObject::ReferencesObjectFromElements(FixedArray* elements,
                                             ElementsKind kind,
@@ skipping 1213 matching lines @@
     // Internalize on the fly so we can use pointer identity later.
     next = isolate->factory()->InternalizeName(Handle<Name>::cast(next));
     list->set(index, *next);
     // 7e. Set index to index + 1. (See loop header.)
   }
   // 8. Return list.
   return list;
 }
 
 
+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,
                               Enumerability enum_policy,
                               KeyAccumulator* accumulator) {
   // 1. Let handler be the value of the [[ProxyHandler]] internal slot of O.
   Handle<Object> handler(proxy->handler(), isolate);
   // 2. If handler is null, throw a TypeError exception.
   if (proxy->IsRevoked()) {
     isolate->Throw(*isolate->factory()->NewTypeError(
@@ skipping 54 matching lines @@
     bool found = JSReceiver::GetOwnPropertyDescriptor(
         isolate, target, handle(target_keys->get(i), isolate), &desc);
     if (isolate->has_pending_exception()) return false;
     // 14b. If desc is not undefined and desc.[[Configurable]] is false, then
     if (found && !desc.configurable()) {
       // 14b i. Append key as an element of targetNonconfigurableKeys.
       target_nonconfigurable_keys->set(nonconfigurable_keys_length,
                                        target_keys->get(i));
       nonconfigurable_keys_length++;
       // The key was moved, null it out in the original list.
       target_keys->set(i, Smi::FromInt(0));

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

I'd prefer working on target_configurable_keys instead since we don't use
target_keys afterwards. Same would apply further up.

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

As discussed, I disagree. At this point we're iterating over all the target
keys. The distinction between configurable and nonconfigurable keys only becomes
valid after this loop.

     } else {
       // 14c. Else,
       // 14c i. Append key as an element of targetConfigurableKeys.
       // (No-op, just keep it in |target_keys|.)
     }
   }
   accumulator->NextPrototype();  // Prepare for accumulating keys.
   // 15. If extensibleTarget is true and targetNonconfigurableKeys is empty,
   //     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;
   }
   // 16. Let uncheckedResultKeys be a new List which is a copy of trapResult.
   Zone set_zone;
   const int kPresent = 1;
   const int kGone = 0;
   IdentityMap<int> unchecked_result_keys(isolate->heap(), &set_zone);
   int unchecked_result_keys_size = trap_result->length();
   for (int i = 0; i < trap_result->length(); ++i) {
@@ skipping 10 matching lines @@
       isolate->Throw(*isolate->factory()->NewTypeError(
           MessageTemplate::kProxyTrapResultMustInclude, handle(key, isolate)));
       return false;
     }
     // 17b. Remove key from uncheckedResultKeys.
     *found = kGone;
     unchecked_result_keys_size--;
   }
   // 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;
   }
   // 19. Repeat, for each key that is an element of targetConfigurableKeys:
   for (int i = 0; i < target_configurable_keys->length(); ++i) {
     Object* key = target_configurable_keys->get(i);
     if (key->IsSmi()) continue;  // Zapped entry, was nonconfigurable.
     // 19a. If key is not an element of uncheckedResultKeys, throw a
     //      TypeError exception.
     int* found = unchecked_result_keys.Find(key);
     if (found == nullptr || *found == kGone) {
       isolate->Throw(*isolate->factory()->NewTypeError(
           MessageTemplate::kProxyTrapResultMustInclude, handle(key, isolate)));
       return false;
     }
     // 19b. Remove key from uncheckedResultKeys.
     *found = kGone;
     unchecked_result_keys_size--;
   }
   // 20. If uncheckedResultKeys is not empty, throw a TypeError exception.
   if (unchecked_result_keys_size != 0) {
     DCHECK_GT(unchecked_result_keys_size, 0);
     isolate->Throw(*isolate->factory()->NewTypeError(
         MessageTemplate::kProxyTargetNotExtensible));
     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;
 }
 
 
 MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
                                             KeyCollectionType type,
                                             KeyFilter filter,
                                             GetKeysConversion keys_conversion,
                                             Enumerability enum_policy) {
@@ skipping 10526 matching lines @@
   if (cell->value() != *new_value) {
     cell->set_value(*new_value);
     Isolate* isolate = cell->GetIsolate();
     cell->dependent_code()->DeoptimizeDependentCodeGroup(
         isolate, DependentCode::kPropertyCellChangedGroup);
   }
 }
 
 }  // namespace internal
 }  // namespace v8