ES6: Add Object.getOwnPropertySymbols

src/runtime.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 5723 matching lines @@
   Object* proto = obj->GetPrototype();
   while (proto->IsJSObject() &&
          JSObject::cast(proto)->map()->is_hidden_prototype()) {
     count++;
     proto = JSObject::cast(proto)->GetPrototype();
   }
   return count;
 }
 
 
+static Handle<FixedArray> FilterPublicSymbols(
+    Isolate* isolate, Handle<FixedArray> names) {
+  int length = names->length();
+  int symbol_count = 0;
+  for (int i = 0; i < length; i++) {
+    Object* name = names->get(i);
+    if (name->IsSymbol() && !static_cast<Symbol*>(name)->is_private()) {

[rossberg, 2013/12/12 15:37:59]

Use Symbol::cast(name) here (and below).

But I had actually hoped that this whole function would be unnecessary. See
below.

[arv (Not doing code reviews), 2013/12/16 21:51:21]

Done.

+      symbol_count++;
+    }
+  }
+
+  Handle<FixedArray> symbols = isolate->factory()->NewFixedArray(symbol_count);
+
+  if (symbol_count) {
+    int dest_pos = 0;
+    for (int i = 0; i < length; i++) {
+      Object* name = names->get(i);
+      if (name->IsSymbol() && !static_cast<Symbol*>(name)->is_private()) {
+        symbols->set(dest_pos++, name);
+      }
+    }
+  }
+
+  return symbols;
+}
+
+
 // Return the names of the local named properties.
 // args[0]: object
+// args[1]: int
 RUNTIME_FUNCTION(MaybeObject*, Runtime_GetLocalPropertyNames) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 2);
   if (!args[0]->IsJSObject()) {
     return isolate->heap()->undefined_value();
   }
+
   CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0);
-  CONVERT_BOOLEAN_ARG_CHECKED(include_symbols, 1);
-  PropertyAttributes filter = include_symbols ? NONE : SYMBOLIC;
+  CONVERT_SMI_ARG_CHECKED(key_type, 1);
+  PropertyAttributes filter =
+      (key_type & Runtime::PROPERTY_KEY_SYMBOL) ? NONE : SYMBOLIC;
 
   // Skip the global proxy as it has no properties and always delegates to the
   // real global object.
   if (obj->IsJSGlobalProxy()) {
     // Only collect names if access is permitted.
     if (obj->IsAccessCheckNeeded() &&
         !isolate->MayNamedAccess(*obj,
                                  isolate->heap()->undefined_value(),
                                  v8::ACCESS_KEYS)) {
       isolate->ReportFailedAccessCheck(*obj, v8::ACCESS_KEYS);
@@ skipping 56 matching lines @@
     int dest_pos = 0;
     for (int i = 0; i < total_property_count; i++) {
       Object* name = old_names->get(i);
       if (name == isolate->heap()->hidden_string()) {
         continue;
       }
       names->set(dest_pos++, name);
     }
   }
 
+  if ((key_type & Runtime::PROPERTY_KEY_SYMBOL) &&

[rossberg, 2013/12/12 15:37:59]

How about splitting PropertyAttributes SYMBOLIC filter into SYMBOLIC_PUBLIC and
SYMBOLIC_PRIVATE, so that you can avoid the filter post-processing altogether?
(Except for interceptors of course, which you have to post-process anyway, but
see below.)

You would only need to refine a couple of places in objects.cc accordingly,
namely Map::NumberOfDescribedProperties and JSObject::GetLocalPropertyNames, and
Dictionary<Shape, Key>::NumberOfElementsFilterAttributes.

[arv (Not doing code reviews), 2013/12/12 16:06:16]

I considered going down this route to handle string vs symbol but we currently
have 3 cases: strings, symbols and symbols+strings. If we also want to filter
out private symbols we need to add 2 bits to the bitmask.

I guess that is fine. I'll make the change and see what it looks like in the
end.

+      !(key_type & Runtime::PROPERTY_KEY_STRING)) {
+    names = FilterPublicSymbols(isolate, names);
+  }
+
   return *isolate->factory()->NewJSArrayWithElements(names);
 }
 
 
 // Return the names of the local indexed properties.
 // args[0]: object
 RUNTIME_FUNCTION(MaybeObject*, Runtime_GetLocalElementNames) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 1);
   if (!args[0]->IsJSObject()) {
@@ skipping 21 matching lines @@
   int result = 0;
   if (obj->HasNamedInterceptor()) result |= 2;
   if (obj->HasIndexedInterceptor()) result |= 1;
 
   return Smi::FromInt(result);
 }
 
 
 // Return property names from named interceptor.
 // args[0]: object
+// args[1]: int
 RUNTIME_FUNCTION(MaybeObject*, Runtime_GetNamedInterceptorPropertyNames) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 1);
   CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0);
+  CONVERT_SMI_ARG_CHECKED(key_type, 1);
 
   if (obj->HasNamedInterceptor()) {
     v8::Handle<v8::Array> result = GetKeysForNamedInterceptor(obj, obj);
-    if (!result.IsEmpty()) return *v8::Utils::OpenHandle(*result);
+    if (!result.IsEmpty()) {
+      if ((key_type & Runtime::PROPERTY_KEY_SYMBOL) &&

[rossberg, 2013/12/12 15:37:59]

I'm not sure I understand this. If somebody passed type = PROPERTY_KEY_STRING,
wouldn't you also need to filter all symbols?

I think it's better to leave this function alone and for the interceptor case,
do the filtering in v8natives' ObjectGetOwnPropertyKeys -- for interceptors,
that has to do a copy anyway, where it fits in nicely.

+          !(key_type & Runtime::PROPERTY_KEY_STRING)) {
+        Handle<FixedArray> res = isolate->factory()->NewFixedArray(result->Length());

[rossberg, 2013/12/12 15:37:59]

Nit: line length

+        for (int i = 0; i < res->length(); i++) {

[arv (Not doing code reviews), 2013/12/11 23:51:12]

This is ugly. What is the correct way to do this?

[rossberg, 2013/12/12 15:37:59]

You mean the copying? Only if you allow the Filter function to operate on
different array types (e.g. by templatising it).

+          res->set(i, *v8::Utils::OpenHandle(*result->Get(i)));
+        }
+        return *isolate->factory()->NewJSArrayWithElements(
+            FilterPublicSymbols(isolate, res));
+      }
+      return *v8::Utils::OpenHandle(*result);
+    }
   }
   return isolate->heap()->undefined_value();
 }
 
 
 // Return element names from indexed interceptor.
 // args[0]: object
 RUNTIME_FUNCTION(MaybeObject*, Runtime_GetIndexedInterceptorElementNames) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 1);
@@ skipping 9003 matching lines @@
     // Handle last resort GC and make sure to allow future allocations
     // to grow the heap without causing GCs (if possible).
     isolate->counters()->gc_last_resort_from_js()->Increment();
     isolate->heap()->CollectAllGarbage(Heap::kNoGCFlags,
                                        "Runtime::PerformGC");
   }
 }
 
 
 } }  // namespace v8::internal