ES6: Add Object.getOwnPropertySymbols

ES6: Add Object.getOwnPropertySymbols

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-object.getownpropertysymbols

This allows you to get the symbols used as property keys for an object.

  var object = {};
  var sym = Symbol();
  object[sym] = 42;
  assert(Object.getOwnPropertySymbols(object)[0] === sym);

This is only available with --harmony-symbols

BUG=v8:3049
R=rossberg@chromium.org, rossberg
LOG=Y

Committed: https://code.google.com/p/v8/source/detail?r=18520

[arv (Not doing code reviews), 2013-12-09 18:07:29]

A few questions

1. Right now this handles interceptors using symbols. Is that even possible?

2. The spec has a shared internal function called GetOwnPropertyKeys (O, Type).
We could do the same and share more code but it would involve more runtime
branches.

3. The runtime function %GetLocalPropertyNames takes a boolean. true means only
strings and false means strings and symbols. I see that some mirroring functions
uses the latter. Would it be OK to change the runtime function to only include
symbols when passed false and change the mirrorring function? That would mean
that the ordering would change. Another option is to use a tri state (bitmask)
but that seems a bit of an overkill.

Thanks for reviewing

[arv (Not doing code reviews), 2013-12-09 19:46:41]

Ouch, this leaks private symbols. Let me fix that.

[rossberg, 2013-12-10 10:39:53]

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1041: function GetSymbolsInArray(propertyKeys) {
Nit: perhaps call this FilterPublicSymbols?

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1045: if (IS_SYMBOL(propertyKeys[i]) &&
!%SymbolIsPrivate(propertyKeys[i])) {
It might make sense to introduce an IS_PRIVATE macro for the latter.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1060: // FIXME: Proxies use a shared trap for String and Symbol
keys.
Please use TODO(arv); same below

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1072: // FIXME: Have the runtime do the filtering.
Yeah, I would very much prefer to have this implemented efficiently right away,
at least for ordinary properties (for interceptor properties it matters less).
Just turn the Boolean flag into a three-way flag.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1100: if (propertySet[symbol] === true) {
Could combine this with the previous if.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1494: // getOwnPropertySymbols is added in symbol.js
Good to have that comment here. While you're at it, can you add the same for
.observe?

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
File test/mjsunit/harmony/symbols.js (right):

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:301: function TestKeySymbols(obj) {
Let's call this TestGetOwnPropertySymbols

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:305: assertEquals("symbol", typeof syms[i])
Also assert that insertion order is maintained (which implies that they are all
different).

And please add a test that ensures that inherited symbols are not included.

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:365: function
TestGetOwnPropertySymbolsWithPrivateSymbols() {
Nit: Move this next to the other test.

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:367: var obj = {}
Hm, the private symbol is never added to obj. ;)

Also, maybe have a mixture of both public and private symbols, and check that
insertion order still holds after non-trivial filtering.

[arv (Not doing code reviews), 2013-12-11 00:16:26]

I didn't have time to do the refactoring of the runtime function yet. I'll get
to it tomorrow.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1041: function GetSymbolsInArray(propertyKeys) {
On 2013/12/10 10:39:53, rossberg wrote:
> Nit: perhaps call this FilterPublicSymbols?

Done.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1045: if (IS_SYMBOL(propertyKeys[i]) &&
!%SymbolIsPrivate(propertyKeys[i])) {
On 2013/12/10 10:39:53, rossberg wrote:
> It might make sense to introduce an IS_PRIVATE macro for the latter.

Done.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1060: // FIXME: Proxies use a shared trap for String and Symbol
keys.
On 2013/12/10 10:39:53, rossberg wrote:
> Please use TODO(arv); same below

Done.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1100: if (propertySet[symbol] === true) {
On 2013/12/10 10:39:53, rossberg wrote:
> Could combine this with the previous if.

I also don't think this comment is correct. propertySet is an object with a null
[[Prototype]] so there is no toString on it. I'll update the original case too.

https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode...
src/v8natives.js:1494: // getOwnPropertySymbols is added in symbol.js
On 2013/12/10 10:39:53, rossberg wrote:
> Good to have that comment here. While you're at it, can you add the same for
> .observe?

Done.

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
File test/mjsunit/harmony/symbols.js (right):

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:301: function TestKeySymbols(obj) {
On 2013/12/10 10:39:53, rossberg wrote:
> Let's call this TestGetOwnPropertySymbols

Done.

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:305: assertEquals("symbol", typeof syms[i])
On 2013/12/10 10:39:53, rossberg wrote:
> Also assert that insertion order is maintained (which implies that they are
all
> different).

You sent me off on goose chase there. I finally figured out that we only use
insertion order when %HasFastProperties is true

Done.

> And please add a test that ensures that inherited symbols are not included.

Done.

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:365: function
TestGetOwnPropertySymbolsWithPrivateSymbols() {
On 2013/12/10 10:39:53, rossberg wrote:
> Nit: Move this next to the other test.

I kept it here because this test does not take an obj like the other test. I
also added one more test down here so it feels less out of place.

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:367: var obj = {}
On 2013/12/10 10:39:53, rossberg wrote:
> Hm, the private symbol is never added to obj. ;)
> 
> Also, maybe have a mixture of both public and private symbols, and check that
> insertion order still holds after non-trivial filtering.

Doh!

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

PTAL

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5920
src/runtime.cc:5920: for (int i = 0; i < res->length(); i++) {
This is ugly. What is the correct way to do this?

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

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
File test/mjsunit/harmony/symbols.js (right):

https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:305: assertEquals("symbol", typeof syms[i])
On 2013/12/11 00:16:26, arv wrote:
> On 2013/12/10 10:39:53, rossberg wrote:
> > Also assert that insertion order is maintained (which implies that they are
> all
> > different).
> 
> You sent me off on goose chase there. I finally figured out that we only use
> insertion order when %HasFastProperties is true

Ah, sorry about that. I should have known.

https://codereview.chromium.org/108083005/diff/90001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/108083005/diff/90001/src/macros.py#newcode162
src/macros.py:162: macro IS_PRIVATE(arg) = (%SymbolIsPrivate(arg));
Nit: s/arg/sym/

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5750
src/runtime.cc:5750: if (name->IsSymbol() &&
!static_cast<Symbol*>(name)->is_private()) {
Use Symbol::cast(name) here (and below).

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

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5861
src/runtime.cc:5861: if ((key_type & Runtime::PROPERTY_KEY_SYMBOL) &&
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.

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5917
src/runtime.cc:5917: if ((key_type & Runtime::PROPERTY_KEY_SYMBOL) &&
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.

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5919
src/runtime.cc:5919: Handle<FixedArray> res =
isolate->factory()->NewFixedArray(result->Length());
Nit: line length

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5920
src/runtime.cc:5920: for (int i = 0; i < res->length(); i++) {
On 2013/12/11 23:51:12, arv wrote:
> This is ugly. What is the correct way to do this?

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

https://codereview.chromium.org/108083005/diff/90001/test/mjsunit/harmony/sym...
File test/mjsunit/harmony/symbols.js (right):

https://codereview.chromium.org/108083005/diff/90001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:276: function TestKeyHas(obj) {
Doh, how t.f. did this ever work???

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

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5861
src/runtime.cc:5861: if ((key_type & Runtime::PROPERTY_KEY_SYMBOL) &&
On 2013/12/12 15:37:59, rossberg wrote:
> 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.

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.

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

PTAL

https://codereview.chromium.org/108083005/diff/90001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/108083005/diff/90001/src/macros.py#newcode162
src/macros.py:162: macro IS_PRIVATE(arg) = (%SymbolIsPrivate(arg));
On 2013/12/12 15:37:59, rossberg wrote:
> Nit: s/arg/sym/

Done.

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/108083005/diff/90001/src/runtime.cc#newcode5750
src/runtime.cc:5750: if (name->IsSymbol() &&
!static_cast<Symbol*>(name)->is_private()) {
On 2013/12/12 15:37:59, rossberg wrote:
> Use Symbol::cast(name) here (and below).
> 
> But I had actually hoped that this whole function would be unnecessary. See
> below.

Done.

https://codereview.chromium.org/108083005/diff/90001/test/mjsunit/harmony/sym...
File test/mjsunit/harmony/symbols.js (right):

https://codereview.chromium.org/108083005/diff/90001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:276: function TestKeyHas(obj) {
On 2013/12/12 15:37:59, rossberg wrote:
> Doh, how t.f. did this ever work???

There is a global variable called obj which happens to be the exact same.

for (var in objs) {
  var obj = objs[i];
  TestKeyHas();
}

[rossberg, 2013-12-18 16:50:12]

Looking quite good now, just two more comments.

https://codereview.chromium.org/108083005/diff/130001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/108083005/diff/130001/src/objects.cc#newcode5915
src/objects.cc:5915: if (key->IsSymbol()) {
I would prefer to turn this inside out and do the filter checks first and the
(more expensive) instance type checks only if necessary.

https://codereview.chromium.org/108083005/diff/130001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/108083005/diff/130001/src/v8natives.js#newcod...
src/v8natives.js:1041: function FilterKeyNames(array, symbolsOnly) {
Why can't we fold this into the loop at line 1108, and avoid duplicating the
work?

[arv (Not doing code reviews), 2013-12-18 18:07:12]

PTAL

https://codereview.chromium.org/108083005/diff/130001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/108083005/diff/130001/src/objects.cc#newcode5915
src/objects.cc:5915: if (key->IsSymbol()) {
On 2013/12/18 16:50:12, rossberg wrote:
> I would prefer to turn this inside out and do the filter checks first and the
> (more expensive) instance type checks only if necessary.

Done.

However, these filters are not mutually exclusive so I can either call IsSymbol
once for each case or I could do it once by adding another bitmask check.

https://codereview.chromium.org/108083005/diff/130001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/108083005/diff/130001/src/v8natives.js#newcod...
src/v8natives.js:1041: function FilterKeyNames(array, symbolsOnly) {
On 2013/12/18 16:50:12, rossberg wrote:
> Why can't we fold this into the loop at line 1108, and avoid duplicating the
> work?

Done.

That would make more sense. And now I understand what you mean with your
previous comment.

[arv (Not doing code reviews), 2013-12-18 18:10:02]

https://codereview.chromium.org/108083005/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/108083005/diff/150001/src/objects.cc#newcode5915
src/objects.cc:5915: if ((filter & SYMBOLIC) && key->IsSymbol()) {
One option here would be:

if (filter & (SYMBOLIC | SYMBOLIC | STRING)) {
  bool is_symbol = key->IsSymbol();
  
  ...
}
return false;

[rossberg, 2013-12-19 10:28:04]

LGTM

I'll try to land once the tree allows it (given the long line of CLs currently
waiting that may not happen before my vacation, though -- just in case you
wonder ;) ).

https://codereview.chromium.org/108083005/diff/130001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/108083005/diff/130001/src/v8natives.js#newcod...
src/v8natives.js:1041: function FilterKeyNames(array, symbolsOnly) {
On 2013/12/18 18:07:12, arv wrote:
> On 2013/12/18 16:50:12, rossberg wrote:
> > Why can't we fold this into the loop at line 1108, and avoid duplicating the
> > work?
> 
> Done.
> 
> That would make more sense. And now I understand what you mean with your
> previous comment.

Yes, sorry if that was too cryptic.

https://codereview.chromium.org/108083005/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/108083005/diff/150001/src/objects.cc#newcode5915
src/objects.cc:5915: if ((filter & SYMBOLIC) && key->IsSymbol()) {
On 2013/12/18 18:10:02, arv wrote:
> One option here would be:
> 
> if (filter & (SYMBOLIC | SYMBOLIC | STRING)) {
>   bool is_symbol = key->IsSymbol();
>   
>   ...
> }
> return false;

I leave that up to you, I'm fine either way.

[rossberg, 2013-12-20 12:16:56]

Hm, I get plenty of assertion failures when running the test suite on a debug
build...

[arv (Not doing code reviews), 2014-01-06 23:41:50]

PTAL

Here are the issues that was causing debug failures.

1. I added an assert that the filter was within range. The fuzzer of course
tries random things outside this range.
2. A few tests still passed treu to %GetLocalPropertyNames
3. But strangest of all was that I had to move the definition of
ObjectGetOwnPropertySymbols into symbol.js. One of the asserts fail if we define
ObjectGetOwnPropertySymbols in v8Natives.js and install it in symbol.js.

[rossberg, 2014-01-09 12:01:27]

LGTM with small nits.

(Next time please upload a suitable diff if you rebase. ;) )

https://codereview.chromium.org/108083005/diff/210001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/108083005/diff/210001/src/runtime.cc#newcode5747
src/runtime.cc:5747: // args[1]: int
Maybe say that this int is PropertyAttributes.

https://codereview.chromium.org/108083005/diff/210001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/108083005/diff/210001/src/v8natives.js#newcod...
src/v8natives.js:1073: var namedInterceptorNames =
Nit: obsolete line wrap

[arv (Not doing code reviews), 2014-01-09 14:16:09]

PTAL

Sorry about the ugly interdiff.

[rossberg, 2014-01-09 15:57:43]

Committed patchset #12 manually as r18520 (presubmit successful).