ES6: Add support for method shorthand in object literals

ES6: Add support for method shorthand in object literals

This is governed by the harmony-object-literals flag.

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

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

[arv (Not doing code reviews), 2014-08-15 23:00:18]

This depends on https://codereview.chromium.org/458613004/

[arv (Not doing code reviews), 2014-08-15 23:03:27]

FYI

[arv (Not doing code reviews), 2014-08-20 22:08:29]

This is now ready for review.

What other things should I test?

https://codereview.chromium.org/477263002/diff/40001/src/ast.h
File src/ast.h (left):

https://codereview.chromium.org/477263002/diff/40001/src/ast.h#oldcode2364
src/ast.h:2364: enum KindFlag {
I initially added kConciseMethod to this enum but since we eventually want to
support generator methods a single enum is no longer suitable.

[arv (Not doing code reviews), 2014-08-21 16:57:08]

Ping.

[arv (Not doing code reviews), 2014-08-21 19:52:01]

Adding Marja for parser changes

I initially was going to do the StrictFormalParameters in a follow up but while
waiting for feedback I accidentally implemented it.

[marja, 2014-08-22 08:22:47]

https://codereview.chromium.org/477263002/diff/80001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/477263002/diff/80001/src/parser.h#newcode577
src/parser.h:577: FunctionLiteral::IsGeneratorFlag is_generator,
Are these flag mutually exclusive; Afaik an arrow function cannot be a
generator, so, it would make sense to have a three-value flag (normal, arrow,
generator) instead of 2 bools.

How does IsConcise relate to them? Looks like it cannot be an arrow function and
it cannot be a generator either?

So would it make sense to have a 4-value flag (normal, arrow, generator,
concise) here?

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3394: const char* context_data[][2] = {{"({",
"});"},
Is there a reason why you do

({ stuff });

and not just

{ stuff }

?

[rossberg, 2014-08-22 09:34:31]

https://codereview.chromium.org/477263002/diff/80001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/477263002/diff/80001/src/parser.h#newcode577
src/parser.h:577: FunctionLiteral::IsGeneratorFlag is_generator,
On 2014/08/22 08:22:46, marja wrote:
> Are these flag mutually exclusive; Afaik an arrow function cannot be a
> generator, so, it would make sense to have a three-value flag (normal, arrow,
> generator) instead of 2 bools.
> 
> How does IsConcise relate to them? Looks like it cannot be an arrow function
and
> it cannot be a generator either?
> 
> So would it make sense to have a 4-value flag (normal, arrow, generator,
> concise) here?

Generator methods can be concise, too. But that's the only possible overlap,
AFAICT. It's probably better to just extend the existing function kind enum with
both ConciseMethod and ConciseGeneratorMethod, because that's clearer and
excludes all invalid combinations by construction. (Although, in fact, there
doesn't even seem to be much of a need for distinguishing generators from
concise generators at all, so perhaps we can just completely avoid making that a
separate case.)

Likewise, it would make sense to change SharedFunctionInfo, FastNewClosureStub
and other relevant places to also use this enum instead of separate flags (in
particular, the stub will eventually need to handle arrow functions as well).

(Alternatively, we should at least have assertions in various places that
exclude all invalid combinations of the separated flags.)

https://codereview.chromium.org/477263002/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/477263002/diff/80001/src/preparser.h#newcode1864
src/preparser.h:1864: switch (next) {
Perhaps add a TODO that if both harmony_object_literals and harmony_generators
is active, you need to allow for a '*' before this.

https://codereview.chromium.org/477263002/diff/80001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/477263002/diff/80001/src/v8natives.js#newcode...
src/v8natives.js:1772: ? ''
Hm, does this need another case for IsConcise && IsGenerator?

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3394: const char* context_data[][2] = {{"({",
"});"},
On 2014/08/22 08:22:46, marja wrote:
> Is there a reason why you do
> 
> ({ stuff });
> 
> and not just
> 
> { stuff }
> 
> ?

Yes, because then it would be a block, not an object. ;) Because JavaScript.

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3397: const char* statement_data[] = {
Nit: statement_data seems like the wrong name in this case; method_data?

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3416: const char* statement_data[] = {
Nit: name_data?

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3488: const char* statement_data[] = {
Nit: parameter_data?

[arv (Not doing code reviews), 2014-08-22 23:16:03]

I ended up having to mostly rewrite ParsePropertyDefinition again to better
support numeric names.

I also changed the FunctionLiteral::KindFlag to FunctionKind  which is a bit
mask that is used throughout the code base.

I'm on vacation next week so take your time.

https://codereview.chromium.org/477263002/diff/80001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/477263002/diff/80001/src/parser.h#newcode577
src/parser.h:577: FunctionLiteral::IsGeneratorFlag is_generator,
On 2014/08/22 09:34:31, rossberg wrote:
> On 2014/08/22 08:22:46, marja wrote:
> > Are these flag mutually exclusive; Afaik an arrow function cannot be a
> > generator, so, it would make sense to have a three-value flag (normal,
arrow,
> > generator) instead of 2 bools.
> > 
> > How does IsConcise relate to them? Looks like it cannot be an arrow function
> and
> > it cannot be a generator either?
> > 
> > So would it make sense to have a 4-value flag (normal, arrow, generator,
> > concise) here?
> 
> Generator methods can be concise, too. But that's the only possible overlap,
> AFAICT. It's probably better to just extend the existing function kind enum
with
> both ConciseMethod and ConciseGeneratorMethod, because that's clearer and
> excludes all invalid combinations by construction. (Although, in fact, there
> doesn't even seem to be much of a need for distinguishing generators from
> concise generators at all, so perhaps we can just completely avoid making that
a
> separate case.)
> 
> Likewise, it would make sense to change SharedFunctionInfo, FastNewClosureStub
> and other relevant places to also use this enum instead of separate flags (in
> particular, the stub will eventually need to handle arrow functions as well).
> 
> (Alternatively, we should at least have assertions in various places that
> exclude all invalid combinations of the separated flags.)

I initially added kConciseMethod to FunctionLiteral::KindFlag but after
realizing that we will eventually need concise generator methods I changed to
individual flags.

Adding a kConciseGeneratorMethod when we need it seems like a cleaner solution.
I was not happy adding all these bool like parameters.

Looking forward, it seems like ES7 will support async functions, async arrows as
well as async concise functions and maybe even async generators. At that point
we would need to go back to individual flags but we can deal with that then.

https://codereview.chromium.org/477263002/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/477263002/diff/80001/src/preparser.h#newcode1864
src/preparser.h:1864: switch (next) {
On 2014/08/22 09:34:31, rossberg wrote:
> Perhaps add a TODO that if both harmony_object_literals and harmony_generators
> is active, you need to allow for a '*' before this.

Done.

I already have changes coming that changes this function further to handle class
elements correctly. I also plan to add support for `*method()`s.

https://codereview.chromium.org/477263002/diff/80001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/477263002/diff/80001/src/v8natives.js#newcode...
src/v8natives.js:1772: ? ''
On 2014/08/22 09:34:31, rossberg wrote:
> Hm, does this need another case for IsConcise && IsGenerator?

Eventually. Adding a todo for now.

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3397: const char* statement_data[] = {
On 2014/08/22 09:34:31, rossberg wrote:
> Nit: statement_data seems like the wrong name in this case; method_data?

Done.

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3416: const char* statement_data[] = {
On 2014/08/22 09:34:31, rossberg wrote:
> Nit: name_data?

Done.

https://codereview.chromium.org/477263002/diff/80001/test/cctest/test-parsing...
test/cctest/test-parsing.cc:3488: const char* statement_data[] = {
On 2014/08/22 09:34:31, rossberg wrote:
> Nit: parameter_data?

Done.

https://codereview.chromium.org/477263002/diff/100001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/477263002/diff/100001/src/ast.h#newcode2422
src/ast.h:2422: if (is_arrow()) return FunctionKind::kArrowFunction;
These are currently stored as a bitfield_. I could change this to store the
value as a FunctionKind (enum) with the trade-off that it would need more
memory.

I'm not sure what to optimize for here?

[rossberg, 2014-08-25 09:39:30]

I like this version much better. Moving forward, since you can easily construct
invalid kind combinations, let's add a couple more assertions, though (see
below).

https://codereview.chromium.org/477263002/diff/160001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode184
src/ast.h:184: // For generating IDs for AstNodes.
Please try to avoid rebasing while review is in progress.

https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2464
src/ast.h:2464: DCHECK(!(is_arrow() && is_concise_method()));
Perhaps factor these conditions into a function IsWellformedFunctionKind or
IsValidFunctionKind in globals.h, so that it can be reused in a couple of other
places.

https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2493
src/ast.h:2493: class IsArrow : public BitField<bool, 6, 1> {};
Please don't define overlapping bitfields. In particular, these ones silently
break when somebody changes the values in the definition of FunctionKind.

https://codereview.chromium.org/477263002/diff/160001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/globals.h#newcode768
src/globals.h:768: inline bool IsArrowFunction(FunctionKind kind) {
Each of these functions should assert that kind is well-formed.

https://codereview.chromium.org/477263002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/objects-inl.h#newco...
src/objects-inl.h:5598: void SharedFunctionInfo::set_kind(FunctionKind kind) {
Assert that kind is well-formed.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1873: typename ParserBase<Traits>::IdentifierT
ParserBase<Traits>::ParsePropertyName(
Nit: Given that it's called only in one place, I don't think this wrapper is
necessary.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1897: checker->CheckProperty(next, kValueProperty,
Can this check be factored out and moved after the if-chain, to avoid its
duplication?

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1903: // Concise Method
Nit: V8 typically uses a style where a comment like this is placed inside the
respective branch.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1937: LiteralT key = this->EmptyLiteral();
Nit: use ?: instead of `if` to avoid the redundant init.

https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3397: const char* object_literal_data[] = {
Hm, well, it's not really object literals in there...

https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3414: {"'use strict'; ({", "(x, y) {}});"},
Good idea, you should probably add that to your other tests as well, even though
it _should_ not matter there.

[arv (Not doing code reviews), 2014-09-02 14:42:48]

I might be slow to post another patch set. This week is going to be a bit crazy
for me.

https://codereview.chromium.org/477263002/diff/160001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2493
src/ast.h:2493: class IsArrow : public BitField<bool, 6, 1> {};
On 2014/08/25 09:39:29, rossberg wrote:
> Please don't define overlapping bitfields. In particular, these ones silently
> break when somebody changes the values in the definition of FunctionKind.

Yes, it does have a tight coupling but it leads to more concise and more
efficient code.

What would be your suggestion? To remove IsArrow, IsGenerator and
IsConciseMethod and then do another masking as needed?

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1873: typename ParserBase<Traits>::IdentifierT
ParserBase<Traits>::ParsePropertyName(
On 2014/08/25 09:39:30, rossberg wrote:
> Nit: Given that it's called only in one place, I don't think this wrapper is
> necessary.

This was done done for readability. The out params are don't care and since it
was being called from a context where we have is_getter and is_setter it got
confusing.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1897: checker->CheckProperty(next, kValueProperty,
On 2014/08/25 09:39:30, rossberg wrote:
> Can this check be factored out and moved after the if-chain, to avoid its
> duplication?

I had that at first but the call to CheckProperty depends on the current state
of the parser/scanner. I can try to refactor further.

[rossberg, 2014-09-08 14:08:59]

https://codereview.chromium.org/477263002/diff/160001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2493
src/ast.h:2493: class IsArrow : public BitField<bool, 6, 1> {};
On 2014/09/02 14:42:48, arv wrote:
> On 2014/08/25 09:39:29, rossberg wrote:
> > Please don't define overlapping bitfields. In particular, these ones
silently
> > break when somebody changes the values in the definition of FunctionKind.
> 
> Yes, it does have a tight coupling but it leads to more concise and more
> efficient code.
> 
> What would be your suggestion? To remove IsArrow, IsGenerator and
> IsConciseMethod and then do another masking as needed?

Yes. Just add some global helpers IsArrowKind, IsGeneratorKind and so on if you
want to avoid bit fiddling everywhere.

[arv (Not doing code reviews), 2014-09-09 22:24:27]

I'm back to normal schedule now.

I believe I fixed all the issues you pointed out.

PTAL

https://codereview.chromium.org/477263002/diff/160001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2464
src/ast.h:2464: DCHECK(!(is_arrow() && is_concise_method()));
On 2014/08/25 09:39:29, rossberg wrote:
> Perhaps factor these conditions into a function IsWellformedFunctionKind or
> IsValidFunctionKind in globals.h, so that it can be reused in a couple of
other
> places.

Done.

https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2493
src/ast.h:2493: class IsArrow : public BitField<bool, 6, 1> {};
On 2014/09/08 14:08:59, rossberg wrote:
> On 2014/09/02 14:42:48, arv wrote:
> > On 2014/08/25 09:39:29, rossberg wrote:
> > > Please don't define overlapping bitfields. In particular, these ones
> silently
> > > break when somebody changes the values in the definition of FunctionKind.
> > 
> > Yes, it does have a tight coupling but it leads to more concise and more
> > efficient code.
> > 
> > What would be your suggestion? To remove IsArrow, IsGenerator and
> > IsConciseMethod and then do another masking as needed?
> 
> Yes. Just add some global helpers IsArrowKind, IsGeneratorKind and so on if
you
> want to avoid bit fiddling everywhere.

Done.

https://codereview.chromium.org/477263002/diff/160001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/globals.h#newcode768
src/globals.h:768: inline bool IsArrowFunction(FunctionKind kind) {
On 2014/08/25 09:39:29, rossberg wrote:
> Each of these functions should assert that kind is well-formed.

Done.

https://codereview.chromium.org/477263002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/objects-inl.h#newco...
src/objects-inl.h:5598: void SharedFunctionInfo::set_kind(FunctionKind kind) {
On 2014/08/25 09:39:30, rossberg wrote:
> Assert that kind is well-formed.

Done.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1873: typename ParserBase<Traits>::IdentifierT
ParserBase<Traits>::ParsePropertyName(
On 2014/09/02 14:42:48, arv wrote:
> On 2014/08/25 09:39:30, rossberg wrote:
> > Nit: Given that it's called only in one place, I don't think this wrapper is
> > necessary.
> 
> This was done done for readability. The out params are don't care and since it
> was being called from a context where we have is_getter and is_setter it got
> confusing.

Removed.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1897: checker->CheckProperty(next, kValueProperty,
On 2014/09/02 14:42:48, arv wrote:
> On 2014/08/25 09:39:30, rossberg wrote:
> > Can this check be factored out and moved after the if-chain, to avoid its
> > duplication?
> 
> I had that at first but the call to CheckProperty depends on the current state
> of the parser/scanner. I can try to refactor further.

Decided not to change this.

1. The duplicate check will go away in ES6.
2. We want to check the name before we parse the expression/function body
3. The ObjectLiteralChecker depends on the current position of the scanner.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1903: // Concise Method
On 2014/08/25 09:39:30, rossberg wrote:
> Nit: V8 typically uses a style where a comment like this is placed inside the
> respective branch.

Done.

https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode...
src/preparser.h:1937: LiteralT key = this->EmptyLiteral();
On 2014/08/25 09:39:30, rossberg wrote:
> Nit: use ?: instead of `if` to avoid the redundant init.

Done.

https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3397: const char* object_literal_data[] = {
On 2014/08/25 09:39:30, rossberg wrote:
> Hm, well, it's not really object literals in there...

Done.

https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:3414: {"'use strict'; ({", "(x, y) {}});"},
On 2014/08/25 09:39:30, rossberg wrote:
> Good idea, you should probably add that to your other tests as well, even
though
> it _should_ not matter there.

Done.

[arv (Not doing code reviews), 2014-09-10 14:06:15]

PING

[rossberg, 2014-09-10 14:28:33]

lgtm

[arv (Not doing code reviews), 2014-09-10 15:35:13]

merge

[arv (Not doing code reviews), 2014-09-10 16:39:58]

Committed patchset #10 (id:220001) manually as 23846 (presubmit successful).