Add support for computed property names in object literals

Add support for computed property names in object literals

R=rossberg@chromium.org
TEST=mjsunit/object-literal-computed-names.js
LOG=N

[wingo, 2014-06-11 15:35:29]

Seemed like a good idea.  Should it be behind a flag?  What am I forgetting to
test?

There's a pending TODO that I don't plan on doing: computed names for
getters/setters.

[arv (Not doing code reviews), 2014-06-11 19:28:29]

https://codereview.chromium.org/332443002/diff/1/test/mjsunit/object-literal-...
File test/mjsunit/object-literal-computed-names.js (right):

https://codereview.chromium.org/332443002/diff/1/test/mjsunit/object-literal-...
test/mjsunit/object-literal-computed-names.js:26: var buffer = '/* ' + (' ' *
4096) + '*/';
(' ' * 4096)

This doesn't do what you think it does.

Do this instead,

Array(4096 + 1).join(' ')

or --harmony_strings and ' '.repeat(4096)

https://codereview.chromium.org/332443002/diff/1/test/mjsunit/object-literal-...
test/mjsunit/object-literal-computed-names.js:47: 
Can you also add a test that ensures that a computed property name that computes
to "__proto__" does not set the [[Prototype]]?

var a = 42;
var b = {['__proto__']: a};
assertEquals(Object.getPrototypeOf(b), Object.prototype);
assertEquals(b.__proto__, a);
assertTrue(b.hasOwnProperty('__proto__'));

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-__proto__-property-...

[wingo, 2014-06-12 09:25:55]

On 2014/06/11 19:28:29, arv wrote:
>
https://codereview.chromium.org/332443002/diff/1/test/mjsunit/object-literal-...
> File test/mjsunit/object-literal-computed-names.js (right):
> 
>
https://codereview.chromium.org/332443002/diff/1/test/mjsunit/object-literal-...
> test/mjsunit/object-literal-computed-names.js:26: var buffer = '/* ' + (' ' *
> 4096) + '*/';
> (' ' * 4096)
> 
> This doesn't do what you think it does.

How embarrassing!  Thanks for the note.

> Can you also add a test that ensures that a computed property name that
computes
> to "__proto__" does not set the [[Prototype]]?

Hum, this points out a problem in the patch -- before, the set of properties in
an object was statically known, and so you could create a new object literal by
cloning a template and then updating properties that needed to be computed at
runtime.  Now with computed property names that isn't the case.  In my patch we
still create a template for the first few properties -- the ones that precede
any computed property name -- but subsequent properties are not in the template.
 This of course opens up the possibility of interference from Object.prototype
:/

I'll have to revise the patch.  Thanks again for the feedback, Erik.

[rossberg, 2014-06-12 12:40:10]

On 2014/06/12 09:25:55, wingo wrote:
> Hum, this points out a problem in the patch -- before, the set of properties
in
> an object was statically known, and so you could create a new object literal
by
> cloning a template and then updating properties that needed to be computed at
> runtime.  Now with computed property names that isn't the case.  In my patch
we
> still create a template for the first few properties -- the ones that precede
> any computed property name -- but subsequent properties are not in the
template.
>  This of course opens up the possibility of interference from Object.prototype
> :/

Hm, trying to understand what you mean. As far as the spec is concerned, all
properties are defined, not set, so the prototype should never interfere. Are
you saying that it could yet potentially do in the current implementation?

[wingo, 2014-06-12 13:11:01]

On 2014/06/12 12:40:10, rossberg wrote:
> On 2014/06/12 09:25:55, wingo wrote:
> > Hum, this points out a problem in the patch -- before, the set of properties
> in
> > an object was statically known, and so you could create a new object literal
> by
> > cloning a template and then updating properties that needed to be computed
at
> > runtime.  Now with computed property names that isn't the case.  In my patch
> we
> > still create a template for the first few properties -- the ones that
precede
> > any computed property name -- but subsequent properties are not in the
> template.
> >  This of course opens up the possibility of interference from
Object.prototype
> > :/
> 
> Hm, trying to understand what you mean. As far as the spec is concerned, all
> properties are defined, not set, so the prototype should never interfere. Are
> you saying that it could yet potentially do in the current implementation?

For each object literal in the source, the current implementation lazily
prepares a "boilerplate object" when the code is first reached.  In ES5.1, the
set of properties is known, so the runtime can create a template object that has
all the properties, and whose map has the properties in the correct
left-to-right insertion order.  The only thing that needs to happen at runtime
is computing those properties that are not constant, and since they are already
in the object, we just use Runtime_SetProperty to update the object.

However with computed property names we don't know the set of properties ahead
of time, and so we can't create a boilerplate object that has all of them
defined already.  So for ES6 object literals with computed properties -- call it
the dynamic object literal case -- we have to do something like PutOwnProperty
at runtime, instead of SetProperty.

Computed property names are the only way to get dynamic object literals.  The
current code is fine for ES5.1.

Note that all properties to the right of the first computed property have to be
added via PutOwnProperty, at least in the baseline case, in order to preserve
insertion order.

[rossberg, 2014-06-12 14:23:46]

On 2014/06/12 13:11:01, wingo wrote:
> On 2014/06/12 12:40:10, rossberg wrote:
> > On 2014/06/12 09:25:55, wingo wrote:
> > > Hum, this points out a problem in the patch -- before, the set of
properties
> > in
> > > an object was statically known, and so you could create a new object
literal
> > by
> > > cloning a template and then updating properties that needed to be computed
> at
> > > runtime.  Now with computed property names that isn't the case.  In my
patch
> > we
> > > still create a template for the first few properties -- the ones that
> precede
> > > any computed property name -- but subsequent properties are not in the
> > template.
> > >  This of course opens up the possibility of interference from
> Object.prototype
> > > :/
> > 
> > Hm, trying to understand what you mean. As far as the spec is concerned, all
> > properties are defined, not set, so the prototype should never interfere.
Are
> > you saying that it could yet potentially do in the current implementation?
> 
> For each object literal in the source, the current implementation lazily
> prepares a "boilerplate object" when the code is first reached.  In ES5.1, the
> set of properties is known, so the runtime can create a template object that
has
> all the properties, and whose map has the properties in the correct
> left-to-right insertion order.  The only thing that needs to happen at runtime
> is computing those properties that are not constant, and since they are
already
> in the object, we just use Runtime_SetProperty to update the object.
> 
> However with computed property names we don't know the set of properties ahead
> of time, and so we can't create a boilerplate object that has all of them
> defined already.  So for ES6 object literals with computed properties -- call
it
> the dynamic object literal case -- we have to do something like PutOwnProperty
> at runtime, instead of SetProperty.
> 
> Computed property names are the only way to get dynamic object literals.  The
> current code is fine for ES5.1.

Actually, it's not. I only just saw that we generate invocations to SetProperty
for computed properties, and that is already bogus for ES5. Consider:

Object.defineProperty(Object.prototype, 'x', {set: print, get: print});
{x: 6}  // prints 6!


> Note that all properties to the right of the first computed property have to
be
> added via PutOwnProperty, at least in the baseline case, in order to preserve
> insertion order.

Yes. Basically, I was wondering whether there is anything more to fixing this
than just replacing the call to SetProperty with (a runtime wrapper of)
SetOwnPropertyIgnoreAttributes in your CL.

[wingo, 2014-06-12 14:29:52]

Updated to initialize dynamic part of object literals via a new runtime call. 
Nastiest code I've written in a while...

[rossberg, 2014-06-12 14:38:32]

On 2014/06/12 14:23:46, rossberg wrote:
> Actually, it's not. I only just saw that we generate invocations to
SetProperty
> for computed properties, and that is already bogus for ES5. Consider:
> 
> Object.defineProperty(Object.prototype, 'x', {set: print, get: print});
> {x: 6}  // prints 6!

Actually, strike that. I made the stupid JS mistake of not parenthesising an
object literal 8-}. When fixed, the example actually works. Michael had to
remind that it does because even the computed properties are already
preinstalled on the boilerplate.

[wingo, 2014-06-12 14:47:20]

On 2014/06/12 14:38:32, rossberg wrote:
> I made the stupid JS mistake of not parenthesising an
> object literal 8-}.

I also made that mistake for a while and was wondering why gdb wouldn't break in
the object-literal parsing code :)

[Michael Starzinger, 2014-06-13 08:14:19]

Drive-by comments.

https://codereview.chromium.org/332443002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/preparser.h#newcode1607
src/preparser.h:1607: is_computed_name = true;
Shouldn't the detection of this syntax live behind a flag for now?

https://codereview.chromium.org/332443002/diff/20001/test/mjsunit/object-lite...
File test/mjsunit/object-literal-computed-names.js (right):

https://codereview.chromium.org/332443002/diff/20001/test/mjsunit/object-lite...
test/mjsunit/object-literal-computed-names.js:5: // Test computed properties
syntax.
This file should definitely go into the "harmony" or one of the "esN"
sub-directories.

[marja, 2014-06-13 14:10:37]

https://codereview.chromium.org/332443002/diff/20001/test/mjsunit/object-lite...
File test/mjsunit/object-literal-computed-names.js (right):

https://codereview.chromium.org/332443002/diff/20001/test/mjsunit/object-lite...
test/mjsunit/object-literal-computed-names.js:25: // function is really big.
This is not lazily parsed (i.e., parsed with PreParser), see
https://codereview.chromium.org/329413002/ for explanation why.

Pls verify for realz that PreParser gets invoked when you think it is (you
probably cannot verify it from the outside, but you can verify it by inspecting
what the code does).

Also, pls add tests ("RunParserSyncTest") in test-parsing (see
TEST(InvalidLeftHandSide) for an example) to verify that Parser and PreParser
accept the same language and produce the same errors.

[rossberg, 2014-06-13 15:19:25]

https://codereview.chromium.org/332443002/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/ast.h#newcode1455
src/ast.h:1455: bool is_computed_name);
Is this flag needed? Isn't it implied by the type of key Expression node?

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1704: // Object literals have two parts.  The
"static" part on the left contains no
I'm not convinced that it's best to duplicate this whole loop, given that it
shares a lot of (sub)cases. I found the previous version with the phase flag
somewhat more pleasant.

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1761:
accessor_table.lookup(key->AsLiteral())->second;
Is this correct? Can't accessors have computed property names as well?

https://codereview.chromium.org/332443002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/preparser.h#newcode1607
src/preparser.h:1607: is_computed_name = true;
On 2014/06/13 08:14:18, Michael Starzinger wrote:
> Shouldn't the detection of this syntax live behind a flag for now?

I agree, we should have --harmony-object-literals (which later might also
include short method forms).

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

https://codereview.chromium.org/332443002/diff/20001/src/runtime.cc#newcode5498
src/runtime.cc:5498: static MaybeHandle<Object> PutOwnElement(Isolate* isolate,
Nit: SetOwnElement

https://codereview.chromium.org/332443002/diff/20001/src/runtime.cc#newcode5521
src/runtime.cc:5521: static MaybeHandle<Object> PutOwnProperty(Isolate* isolate,
Nit: SetOwnProperty

https://codereview.chromium.org/332443002/diff/20001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/runtime.h#newcode228
src/runtime.h:228: F(PutOwnProperty, 3, 1) \
Let's call this SetOwnProperty for consistency.

[wingo, 2014-06-16 08:32:27]

Updated patch, PTAL.  I can give another go at the code generation stuff.

Marja thank you very much for pointing out my embarrassing not-working hack.  I
have fixed it and tested again in GDB to ensure that we hit the pre-parsing
case.

https://codereview.chromium.org/332443002/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/ast.h#newcode1455
src/ast.h:1455: bool is_computed_name);
On 2014/06/13 15:19:24, rossberg wrote:
> Is this flag needed? Isn't it implied by the type of key Expression node?

The flag is needed.  ['foo']:42 has a Literal as the expression node.  We can't
just treat it as foo:42 because of computed names don't participate in the
early-error-on-duplicate-key stuff.

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1704: // Object literals have two parts.  The
"static" part on the left contains no
On 2014/06/13 15:19:24, rossberg wrote:
> I'm not convinced that it's best to duplicate this whole loop, given that it
> shares a lot of (sub)cases. I found the previous version with the phase flag
> somewhat more pleasant.

I tried to do this at first, but ran into problems with accessors.  Notably, in
the original code accessors are not set until the end, to allow the
AccessorsTable to accumulate both the getter and the setter for a value.  These
values are then set together in a post-pass.  

However in the computed properties case we need to set the "get" and "set"
accessors separately, when we see them.

I also had problems getting the switch fallthrough logic to do the right thing. 
I can give it another go if you think it's the right thing.

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1761:
accessor_table.lookup(key->AsLiteral())->second;
On 2014/06/13 15:19:24, rossberg wrote:
> Is this correct? Can't accessors have computed property names as well?

According to spec, yes, but not in the current patch.  I can add a TODO.

https://codereview.chromium.org/332443002/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/preparser.h#newcode1607
src/preparser.h:1607: is_computed_name = true;
On 2014/06/13 15:19:24, rossberg wrote:
> On 2014/06/13 08:14:18, Michael Starzinger wrote:
> > Shouldn't the detection of this syntax live behind a flag for now?
> 
> I agree, we should have --harmony-object-literals (which later might also
> include short method forms).

Done.

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

https://codereview.chromium.org/332443002/diff/20001/src/runtime.cc#newcode5498
src/runtime.cc:5498: static MaybeHandle<Object> PutOwnElement(Isolate* isolate,
On 2014/06/13 15:19:24, rossberg wrote:
> Nit: SetOwnElement

Done.

https://codereview.chromium.org/332443002/diff/20001/src/runtime.cc#newcode5521
src/runtime.cc:5521: static MaybeHandle<Object> PutOwnProperty(Isolate* isolate,
On 2014/06/13 15:19:24, rossberg wrote:
> Nit: SetOwnProperty

Done.

https://codereview.chromium.org/332443002/diff/20001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/runtime.h#newcode228
src/runtime.h:228: F(PutOwnProperty, 3, 1) \
On 2014/06/13 15:19:25, rossberg wrote:
> Let's call this SetOwnProperty for consistency.

Done.

https://codereview.chromium.org/332443002/diff/20001/test/mjsunit/object-lite...
File test/mjsunit/object-literal-computed-names.js (right):

https://codereview.chromium.org/332443002/diff/20001/test/mjsunit/object-lite...
test/mjsunit/object-literal-computed-names.js:5: // Test computed properties
syntax.
On 2014/06/13 08:14:19, Michael Starzinger wrote:
> This file should definitely go into the "harmony" or one of the "esN"
> sub-directories.

Done.

[marja, 2014-06-16 08:35:41]

On 2014/06/16 08:32:27, wingo wrote:
> Updated patch, PTAL.  I can give another go at the code generation stuff.
> 
> Marja thank you very much for pointing out my embarrassing not-working hack. 
I
> have fixed it and tested again in GDB to ensure that we hit the pre-parsing
> case.

I think you should add test cases to test-parsing.cc (see the cases which call
RunParserSyncTest) - they will surely invoke the PreParser and test that it
produces the same errors as the Parser (and accepts the same language).

[wingo, 2014-06-16 09:35:53]

On 2014/06/16 08:35:41, marja wrote:
> On 2014/06/16 08:32:27, wingo wrote:
> > Updated patch, PTAL.  I can give another go at the code generation stuff.
> > 
> > Marja thank you very much for pointing out my embarrassing not-working hack.

> I
> > have fixed it and tested again in GDB to ensure that we hit the pre-parsing
> > case.
> 
> I think you should add test cases to test-parsing.cc (see the cases which call
> RunParserSyncTest) - they will surely invoke the PreParser and test that it
> produces the same errors as the Parser (and accepts the same language).

Uf, sorry for missing this part of your comment previously.  Done.

[rossberg, 2014-06-18 14:13:40]

https://codereview.chromium.org/332443002/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/332443002/diff/20001/src/ast.h#newcode1455
src/ast.h:1455: bool is_computed_name);
On 2014/06/16 08:32:27, wingo wrote:
> On 2014/06/13 15:19:24, rossberg wrote:
> > Is this flag needed? Isn't it implied by the type of key Expression node?
> 
> The flag is needed.  ['foo']:42 has a Literal as the expression node.  We
can't
> just treat it as foo:42 because of computed names don't participate in the
> early-error-on-duplicate-key stuff.

Apparently, it was decided to drop duplicate checking altogether at the last
meeting (scroll down to Conclusion/Resolution):

http://esdiscuss.org/notes/2014-06-06#rest-properties-and-spread-properties-s...

Not sure I like that, but hey, it's JavaScript.

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/332443002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1704: // Object literals have two parts.  The
"static" part on the left contains no
On 2014/06/16 08:32:27, wingo wrote:
> On 2014/06/13 15:19:24, rossberg wrote:
> > I'm not convinced that it's best to duplicate this whole loop, given that it
> > shares a lot of (sub)cases. I found the previous version with the phase flag
> > somewhat more pleasant.
> 
> I tried to do this at first, but ran into problems with accessors.  Notably,
in
> the original code accessors are not set until the end, to allow the
> AccessorsTable to accumulate both the getter and the setter for a value. 
These
> values are then set together in a post-pass.  
> 
> However in the computed properties case we need to set the "get" and "set"
> accessors separately, when we see them.
> 
> I also had problems getting the switch fallthrough logic to do the right
thing. 
> I can give it another go if you think it's the right thing.

Perhaps this problem also goes away with the spec change quoted above?

https://codereview.chromium.org/332443002/diff/60001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/332443002/diff/60001/src/preparser.h#newcode1613
src/preparser.h:1613: }
Add a fall-through comment

[wingo, 2014-06-26 09:03:34]

See https://codereview.chromium.org/352173004/ for work to relax duplicate
checking.  Once that is resolved I'll revisit this CL.