Implement ES6 symbol registry and predefined symbols

Implement ES6 symbol registry and predefined symbols

R=mstarzinger@chromium.org, arv@chromium.org
BUG=
LOG=Y

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

[sof, 2014-03-19 09:06:46]

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode100
src/symbol.js:100: throw MakeTypeError("not_a_symbol", ["Symbol.keyFor",
symbol]);
Looks like 'symbol' (%1) isn't used in the not_a_symbol format string.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = SymbolHook("@@create");
Just trying to understand.. where is the implicit addition to the global
registry for the well-known symbols given?

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

https://codereview.chromium.org/203243004/diff/20001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:414: assertFalse(Symbol.for("@@create") ===
Symbol.create)
Worth adding an initial test for keyFor() over well known symbols?

[Michael Starzinger, 2014-03-19 10:15:25]

LGTM if comments are addressed.

https://codereview.chromium.org/203243004/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/203243004/diff/20001/src/runtime.cc#newcode640
src/runtime.cc:640: HandleScope scope(isolate);
nit: Not that it makes much of a difference, but this can be within a
SealHandleScope here.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode66
src/symbol.js:66: function SymbolRegistry() {
nit: This naming makes it look like this is the implementation of
Symbol.registry, which doesn't exist. Could we instead rename it to
GetSymbolRegistry and move it up to around line 37 to clarify that it's an
internal helper?

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode77
src/symbol.js:77: function SymbolHook(key) {
nit: Likewise for GetSymbolHook here.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode98
src/symbol.js:98: function SymbolKeyFor(symbol) {
Is it specified what "Symbol.keyFor(Symbol.create)" should return?

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

https://codereview.chromium.org/203243004/diff/20001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:414: assertFalse(Symbol.for("@@create") ===
Symbol.create)
On 2014/03/19 09:06:47, sof wrote:
> Worth adding an initial test for keyFor() over well known symbols?

+1

[rossberg, 2014-03-20 10:40:13]

https://codereview.chromium.org/203243004/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/203243004/diff/20001/src/runtime.cc#newcode640
src/runtime.cc:640: HandleScope scope(isolate);
On 2014/03/19 10:15:25, Michael Starzinger wrote:
> nit: Not that it makes much of a difference, but this can be within a
> SealHandleScope here.

Done.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode66
src/symbol.js:66: function SymbolRegistry() {
On 2014/03/19 10:15:25, Michael Starzinger wrote:
> nit: This naming makes it look like this is the implementation of
> Symbol.registry, which doesn't exist. Could we instead rename it to
> GetSymbolRegistry and move it up to around line 37 to clarify that it's an
> internal helper?

Done.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode77
src/symbol.js:77: function SymbolHook(key) {
On 2014/03/19 10:15:25, Michael Starzinger wrote:
> nit: Likewise for GetSymbolHook here.

Renamed to InternalSymbol.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode98
src/symbol.js:98: function SymbolKeyFor(symbol) {
On 2014/03/19 10:15:25, Michael Starzinger wrote:
> Is it specified what "Symbol.keyFor(Symbol.create)" should return?

Simply undefined, since it's not in the (public) registry. I added a test.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode100
src/symbol.js:100: throw MakeTypeError("not_a_symbol", ["Symbol.keyFor",
symbol]);
On 2014/03/19 09:06:47, sof wrote:
> Looks like 'symbol' (%1) isn't used in the not_a_symbol format string.

Done.

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = SymbolHook("@@create");
On 2014/03/19 09:06:47, sof wrote:
> Just trying to understand.. where is the implicit addition to the global
> registry for the well-known symbols given?

Hm, I'm not sure what you are asking. The well-known symbols are not supposed to
be in the (public) registry. Or did I miss something?

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

https://codereview.chromium.org/203243004/diff/20001/test/mjsunit/harmony/sym...
test/mjsunit/harmony/symbols.js:414: assertFalse(Symbol.for("@@create") ===
Symbol.create)
On 2014/03/19 10:15:25, Michael Starzinger wrote:
> On 2014/03/19 09:06:47, sof wrote:
> > Worth adding an initial test for keyFor() over well known symbols?
> 
> +1

Done. (But note that it just returns undefined.)

[sof, 2014-03-20 10:49:06]

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = SymbolHook("@@create");
On 2014/03/20 10:40:13, rossberg wrote:
> On 2014/03/19 09:06:47, sof wrote:
> > Just trying to understand.. where is the implicit addition to the global
> > registry for the well-known symbols given?
> 
> Hm, I'm not sure what you are asking. The well-known symbols are not supposed
to
> be in the (public) registry. Or did I miss something?

ok, good - thought not. But the lookup in InternalSymbol (nee SymbolHook) is
confusing to me -- why is registry.for being consulted?

[rossberg, 2014-03-20 11:05:20]

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = SymbolHook("@@create");
On 2014/03/20 10:49:06, sof wrote:
> On 2014/03/20 10:40:13, rossberg wrote:
> > On 2014/03/19 09:06:47, sof wrote:
> > > Just trying to understand.. where is the implicit addition to the global
> > > registry for the well-known symbols given?
> > 
> > Hm, I'm not sure what you are asking. The well-known symbols are not
supposed
> to
> > be in the (public) registry. Or did I miss something?
> 
> ok, good - thought not. But the lookup in InternalSymbol (nee SymbolHook) is
> confusing to me -- why is registry.for being consulted?

Because that's a bug. :) Thanks for pointing out! Fixed

[rossberg, 2014-03-20 12:26:38]

Committed patchset #4 manually as r20118.

[arv (Not doing code reviews), 2014-03-22 17:15:49]

The important functionality: LGTM

I'm not sure that we should install Symbol.x if we do not support x, where x is
a certain feature. For example, one might expect us to support @@create because
we have Symbol.create. I think adding these once we support the feature would
allow code to use feature detection.

There are some details that need more work and expanded tests.

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode68
src/symbol.js:68: if (!('internal' in registry)) {
Maybe this could have been 

if (IS_UNDEFINED(registry.internal)) {

since the in operator is slow.

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = InternalSymbol("@@create");
Doesn't '@' in a property name cause the object to become slow?

The [[Description]] should be "Symbol.create".

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-well-known-symbols

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode138
src/symbol.js:138: %SetProperty($Symbol, "create", symbolCreate, DONT_ENUM);
Also, non writable, non configurable.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-symbol.create

[rossberg, 2014-03-24 10:25:46]

On 2014/03/22 17:15:49, arv wrote:
> I'm not sure that we should install Symbol.x if we do not support x, where x
is
> a certain feature. For example, one might expect us to support @@create
because
> we have Symbol.create. I think adding these once we support the feature would
> allow code to use feature detection.

I agree. But not having them would make testing the underlying machinery that
this CL introduces somewhat difficult. I consider disabling them before we ship
symbols.

> There are some details that need more work and expanded tests.

Addressed in new CL: https://codereview.chromium.org/208423013


> https://codereview.chromium.org/203243004/diff/50010/src/symbol.js
> File src/symbol.js (right):
> 
> https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode68
> src/symbol.js:68: if (!('internal' in registry)) {
> Maybe this could have been 
> 
> if (IS_UNDEFINED(registry.internal)) {
> 
> since the in operator is slow.
> 
> https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode121
> src/symbol.js:121: var symbolCreate = InternalSymbol("@@create");
> Doesn't '@' in a property name cause the object to become slow?
> 
> The [[Description]] should be "Symbol.create".
> 
> http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-well-known-symbols
> 
> https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode138
> src/symbol.js:138: %SetProperty($Symbol, "create", symbolCreate, DONT_ENUM);
> Also, non writable, non configurable.
> 
> http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-symbol.create

[rossberg, 2014-03-24 10:25:53]

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode68
src/symbol.js:68: if (!('internal' in registry)) {
On 2014/03/22 17:15:49, arv wrote:
> Maybe this could have been 
> 
> if (IS_UNDEFINED(registry.internal)) {
> 
> since the in operator is slow.

Done.

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = InternalSymbol("@@create");
On 2014/03/22 17:15:49, arv wrote:
> Doesn't '@' in a property name cause the object to become slow?

Objects should be completely ignorant to symbol descriptions. Am I missing
something?

> The [[Description]] should be "Symbol.create".
> 
> http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-well-known-symbols

Done.

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode138
src/symbol.js:138: %SetProperty($Symbol, "create", symbolCreate, DONT_ENUM);
On 2014/03/22 17:15:49, arv wrote:
> Also, non writable, non configurable.
> 
> http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-symbol.create

Done.

[arv (Not doing code reviews), 2014-03-24 15:18:57]

Thanks for the follow up.

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = InternalSymbol("@@create");
On 2014/03/24 10:25:53, rossberg wrote:
> On 2014/03/22 17:15:49, arv wrote:
> > Doesn't '@' in a property name cause the object to become slow?
> 
> Objects should be completely ignorant to symbol descriptions. Am I missing
> something?
> 
> > The [[Description]] should be "Symbol.create".
> > 
> >
http://people.mozilla.org/%257Ejorendorff/es6-draft.html#sec-well-known-symbols
> 
> Done.

You use the key on line 80. If we have a non identifier key we end up with a
slow object instead of an object with a hidden class for registry.internal.
However, I doubt this is an issue since the calls to InternalSymbol is limited.

[rossberg, 2014-03-24 15:37:56]

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js
File src/symbol.js (right):

https://codereview.chromium.org/203243004/diff/50010/src/symbol.js#newcode121
src/symbol.js:121: var symbolCreate = InternalSymbol("@@create");
On 2014/03/24 15:18:57, arv wrote:
> On 2014/03/24 10:25:53, rossberg wrote:
> > On 2014/03/22 17:15:49, arv wrote:
> > > Doesn't '@' in a property name cause the object to become slow?
> > 
> > Objects should be completely ignorant to symbol descriptions. Am I missing
> > something?
> > 
> > > The [[Description]] should be "Symbol.create".
> > > 
> > >
>
http://people.mozilla.org/%25257Ejorendorff/es6-draft.html#sec-well-known-sym...
> > 
> > Done.
> 
> You use the key on line 80. If we have a non identifier key we end up with a
> slow object instead of an object with a hidden class for registry.internal.
> However, I doubt this is an issue since the calls to InternalSymbol is
limited.

You're right. The dot will have the same effect, though.

But: I don't think it's an issue, quite the opposite -- this object is meant to
be a dictionary. It would actually be preferable to force this and the other
objects to be in dictionary mode from the get-go, to avoid unnecessary
transitions and polymorphism. I'll add that to the other CL.