[es6] fix nested object literals with computed property names

[es6] fix nested object literals with computed property names

Now, if a literal contains computed properties, it is non-simple and
must be build with the runtime. This ensures the correct behaviour.

BUG=v8:4387
LOG=N
R=adamk, rossberg, wingo

[caitp (gmail), 2015-08-24 15:56:27]

PTAL --- this is working locally.

I've not tested with computed properties in classes (it could be that there is
still a bug with nested class expressions in object literals or arrays?).

There may be a performance regression, but it would only because work was not
being done that needed to be done before.

[caitp (gmail), 2015-08-24 16:02:15]

On 2015/08/24 15:56:27, caitp wrote:
> PTAL --- this is working locally.
> 
> I've not tested with computed properties in classes (it could be that there is
> still a bug with nested class expressions in object literals or arrays?).
> 
> There may be a performance regression, but it would only because work was not
> being done that needed to be done before.

Seems to work with classes anyways (http://jsfiddle.net/1d4p6txn/ /
http://jsfiddle.net/jujhe1w8/) even in Canary, so probably won't worry about
that other than adding additional tests.

[wingo, 2015-08-24 16:04:40]

https://codereview.chromium.org/1307223002/diff/1/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/1307223002/diff/1/src/ast.cc#newcode437
src/ast.cc:437: bool is_simple = boilerplate_properties_ != 0 ||
properties()->length() == 0;
I am missing a bit of context.  Is boilerplate_properties always zero if there
are computed properties?  As an ignorant reader I would appreciate a
has_computed_properties() predicate :)

[caitp (gmail), 2015-08-24 16:08:27]

https://codereview.chromium.org/1307223002/diff/1/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/1307223002/diff/1/src/ast.cc#newcode437
src/ast.cc:437: bool is_simple = boilerplate_properties_ != 0 ||
properties()->length() == 0;
On 2015/08/24 16:04:40, wingo wrote:
> I am missing a bit of context.  Is boilerplate_properties always zero if there
> are computed properties?  As an ignorant reader I would appreciate a
> has_computed_properties() predicate :)

yeah, it can be organized that way for sure. I'm not totally sure why we don't
save the `has_computed_properties` flag in the literal, which is computed by the
parser already

[wingo, 2015-08-24 16:30:45]

lgtm

[adamk, 2015-08-24 18:00:53]

https://codereview.chromium.org/1307223002/diff/40001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1307223002/diff/40001/src/ast.h#newcode1493
src/ast.h:1493: return boilerplate_properties_ == 0 && properties_->length() >
0;
I don't think the LHS of this expression is correct: it only works for object
literals whose first property has a computed name. See the way the
number_of_boilerplate_properties is accumulated in ParseObjectLiteral(): it only
stops counting up after it hits the first computed name. I suspect test cases
where the first property is non-computed will keep failing with this patch.

[adamk, 2015-08-24 18:14:29]

https://codereview.chromium.org/1307223002/diff/40001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1307223002/diff/40001/src/ast.h#newcode1493
src/ast.h:1493: return boilerplate_properties_ == 0 && properties_->length() >
0;
On 2015/08/24 18:00:53, adamk wrote:
> I don't think the LHS of this expression is correct: it only works for object
> literals whose first property has a computed name. See the way the
> number_of_boilerplate_properties is accumulated in ParseObjectLiteral(): it
only
> stops counting up after it hits the first computed name. I suspect test cases
> where the first property is non-computed will keep failing with this patch.

Hmm, local testing suggests I'm wrong about this failing, so I'm not totally
sure what's going on.

[adamk, 2015-08-24 18:16:37]

https://codereview.chromium.org/1307223002/diff/40001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1307223002/diff/40001/src/ast.h#newcode1493
src/ast.h:1493: return boilerplate_properties_ == 0 && properties_->length() >
0;
On 2015/08/24 18:14:29, adamk wrote:
> On 2015/08/24 18:00:53, adamk wrote:
> > I don't think the LHS of this expression is correct: it only works for
object
> > literals whose first property has a computed name. See the way the
> > number_of_boilerplate_properties is accumulated in ParseObjectLiteral(): it
> only
> > stops counting up after it hits the first computed name. I suspect test
cases
> > where the first property is non-computed will keep failing with this patch.
> 
> Hmm, local testing suggests I'm wrong about this failing, so I'm not totally
> sure what's going on.

Sorry for the noise, I've confirmed that this is exactly the behavior (forgot to
negate the is_simple check so everything was getting marked non-simple).

[caitp (gmail), 2015-08-24 18:17:17]

On 2015/08/24 18:14:29, adamk wrote:
> https://codereview.chromium.org/1307223002/diff/40001/src/ast.h
> File src/ast.h (right):
> 
> https://codereview.chromium.org/1307223002/diff/40001/src/ast.h#newcode1493
> src/ast.h:1493: return boilerplate_properties_ == 0 && properties_->length() >
> 0;
> On 2015/08/24 18:00:53, adamk wrote:
> > I don't think the LHS of this expression is correct: it only works for
object
> > literals whose first property has a computed name. See the way the
> > number_of_boilerplate_properties is accumulated in ParseObjectLiteral(): it
> only
> > stops counting up after it hits the first computed name. I suspect test
cases
> > where the first property is non-computed will keep failing with this patch.
> 
> Hmm, local testing suggests I'm wrong about this failing, so I'm not totally
> sure what's going on.

Out for a bit, but I'll add more tests like this later. My reading of the parts
code is that if the has_computed_properties or whatever flag is set, it sets
boilerplate properties to 0. I'll add more tests in a bit

[caitp (gmail), 2015-08-24 18:29:50]

On 2015/08/24 18:17:17, caitp wrote:
> On 2015/08/24 18:14:29, adamk wrote:
> > https://codereview.chromium.org/1307223002/diff/40001/src/ast.h
> > File src/ast.h (right):
> > 
> > https://codereview.chromium.org/1307223002/diff/40001/src/ast.h#newcode1493
> > src/ast.h:1493: return boilerplate_properties_ == 0 && properties_->length()
>
> > 0;
> > On 2015/08/24 18:00:53, adamk wrote:
> > > I don't think the LHS of this expression is correct: it only works for
> object
> > > literals whose first property has a computed name. See the way the
> > > number_of_boilerplate_properties is accumulated in ParseObjectLiteral():
it
> > only
> > > stops counting up after it hits the first computed name. I suspect test
> cases
> > > where the first property is non-computed will keep failing with this
patch.
> > 
> > Hmm, local testing suggests I'm wrong about this failing, so I'm not totally
> > sure what's going on.
> 
> Out for a bit, but I'll add more tests like this later. My reading of the
parts
> code is that if the has_computed_properties or whatever flag is set, it sets
> boilerplate properties to 0. I'll add more tests in a bit

So you're saying it does fail that way in some cases?

[adamk, 2015-08-24 19:07:13]

On 2015/08/24 18:29:50, caitp wrote:
> On 2015/08/24 18:17:17, caitp wrote:
> > On 2015/08/24 18:14:29, adamk wrote:
> > > https://codereview.chromium.org/1307223002/diff/40001/src/ast.h
> > > File src/ast.h (right):
> > > 
> > >
https://codereview.chromium.org/1307223002/diff/40001/src/ast.h#newcode1493
> > > src/ast.h:1493: return boilerplate_properties_ == 0 &&
properties_->length()
> >
> > > 0;
> > > On 2015/08/24 18:00:53, adamk wrote:
> > > > I don't think the LHS of this expression is correct: it only works for
> > object
> > > > literals whose first property has a computed name. See the way the
> > > > number_of_boilerplate_properties is accumulated in ParseObjectLiteral():
> it
> > > only
> > > > stops counting up after it hits the first computed name. I suspect test
> > cases
> > > > where the first property is non-computed will keep failing with this
> patch.
> > > 
> > > Hmm, local testing suggests I'm wrong about this failing, so I'm not
totally
> > > sure what's going on.
> > 
> > Out for a bit, but I'll add more tests like this later. My reading of the
> parts
> > code is that if the has_computed_properties or whatever flag is set, it sets
> > boilerplate properties to 0. I'll add more tests in a bit
> 
> So you're saying it does fail that way in some cases?

Yes. For example:

out/Release/d8 -e "print(JSON.stringify([{name: 'ivan', [42]: 42 }]))"

still prints:

[{"name":"ivan"}]

with this patch applied.