Allow trailing commas in function parameter lists

Allow trailing commas in function parameter lists

Add a flag harmony_trailing_commas_in_parameters that allows trailing
commas in function parameter declaration lists and function call
parameter lists. Trailing commas are allowed in parenthetical lists like
`(a, b, c,)` only if the next token is `=>`, thereby making it an arrow
function declaration. Only 1 trailing comma is allowed, not `(a,,)`. A
trailing comma must follow a non-rest parameter, so `(,)` and `(...a,)`
are still SyntaxErrors. However, a trailing comma is allowed after a
spread parameter, e.g. `a(...b,);`.

Add parser tests for all of the above.

BUG=v8:5051
LOG=y

Committed: https://crrev.com/1ac0965542e714d6dd9318c46199704e035f3d9b
Cr-Commit-Position: refs/heads/master@{#37355}

[jwolfe, 2016-06-22 18:04:41]

jwolfe@igalia.com changed reviewers:
+ adamk@chromium.org

[jwolfe, 2016-06-22 18:04:42]

My confidence level on this patch could be classified as "WIP", although I don't
know of anything that's necessarily missing.

Please check my work on adding a flag. I tried following the examples of the
already existing flags, but I wouldn't be surprised if I missed something.

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1684: //   Expression ',' AssignmentExpression
If I'm supposed to update the grammar comments, I don't know what to do about
this one.

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2157: //   '(' (AssignmentExpression)*[','] ')'
Does this comment say that a trailing comma is already allowed?

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:3222: //    
FormalParameter[?Yield,?GeneratorParameter]
Should I update this comment to the new grammar? Really, this function
implements two different grammars depending on the
allow_harmony_trailing_commas_in_parameters flag

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

https://codereview.chromium.org/2094463002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:7824: { "'use strict';", "" },
Is this unnecessary? It looked like other tests were doing both strict and
sloppy, so I followed the examples. It shouldn't matter for this issue though.

https://codereview.chromium.org/2094463002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:7849: TEST(TrailingCommasInParametersErrors) {
should i combine these two tests? (effectively delete this line and the two
above it.)

[caitp (gmail), 2016-06-22 18:56:19]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2016-06-22 18:56:20]

Great work, here's a first round of comments, I'll take another look in a bit

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1684: //   Expression ',' AssignmentExpression
On 2016/06/22 18:04:41, jwolfe wrote:
> If I'm supposed to update the grammar comments, I don't know what to do about
> this one.

The grammar that needs changing here is
CoverParenthesizedExpressionAndArrowParameterList, not this one. Don't worry
about this one.

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1708: // a trailing comma is allowed at the end of an
arrow parameter list
Is this approach using PeekAhead() faster than just using the peek() and
ExpressionClassifier? We should try both and measure

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:2157: //   '(' (AssignmentExpression)*[','] ')'
On 2016/06/22 18:04:41, jwolfe wrote:
> Does this comment say that a trailing comma is already allowed?

I think this is just wrong, this doesn't even make sense as a YACC input. But
this line is very very old, so its origins are (to me) unknown, and long predate
trailing commas

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:3222: //    
FormalParameter[?Yield,?GeneratorParameter]
On 2016/06/22 18:04:41, jwolfe wrote:
> Should I update this comment to the new grammar? Really, this function
> implements two different grammars depending on the
> allow_harmony_trailing_commas_in_parameters flag

Honestly I'd prefer the jeffmo proposal just refactored itself to be similar to
the existing grammar.

Jeffmo says he thinks it was necessary to "cleanly" explain the trailing comma
--- I think we can express it in the comment using the well-known terms of the
current grammar though.

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:3249: if
(allow_harmony_trailing_commas_in_parameters() &&
For a lot of these, I feel like a WebKit-ism of defining helper macros which
more clearly explain what this is doing would be good, like
`breakIfTrailingComma()` or something.

I don't think it really fits into the chromium code style though, so I am fine
with it as-is.

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:820: FLAG_harmony_trailing_commas_in_parameters);
You need to do something similar in Parser::ParseLazyFunctionBodyWithPreParser()
so the preparser has the same flags

[jwolfe, 2016-06-23 18:43:00]

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

https://codereview.chromium.org/2094463002/diff/1/test/cctest/test-parsing.cc...
test/cctest/test-parsing.cc:7824: { "'use strict';", "" },
On 2016/06/22 18:04:41, jwolfe wrote:
> Is this unnecessary? It looked like other tests were doing both strict and
> sloppy, so I followed the examples. It shouldn't matter for this issue though.

Never mind. I've changed this in a later patch.

[adamk, 2016-06-23 22:08:50]

Haven't looked at the code yet, but a couple suggestions for tests to add.

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

https://codereview.chromium.org/2094463002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:7821: TEST(TrailingCommasInParameters) {
Please add test cases with more than one parameter.

https://codereview.chromium.org/2094463002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:7851: TEST(TrailingCommasInParametersErrors) {
Same here as above, please add tests with more than one parameter. Would also
like to see tests that parenthesized non-arrow expressions are properly
rejected.

[jwolfe, 2016-06-27 23:28:21]

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

https://codereview.chromium.org/2094463002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:7821: TEST(TrailingCommasInParameters) {
On 2016/06/23 22:08:49, adamk wrote:
> Please add test cases with more than one parameter.

Done.

https://codereview.chromium.org/2094463002/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:7851: TEST(TrailingCommasInParametersErrors) {
On 2016/06/23 22:08:49, adamk wrote:
> Same here as above, please add tests with more than one parameter. Would also
> like to see tests that parenthesized non-arrow expressions are properly
> rejected.

Done.

[adamk, 2016-06-28 00:39:47]

Can you add a few mjsunit tests for various kinds of functions that shows
'.length' is still properly recorded in the presence of trailing commas? I'm
convinced the code is right, but it seems like a good thing to have covered.

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2094463002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:1708: // a trailing comma is allowed at the end of an
arrow parameter list
On 2016/06/22 18:56:20, caitp wrote:
> Is this approach using PeekAhead() faster than just using the peek() and
> ExpressionClassifier? We should try both and measure

I have the same question :)

I'll be sad if this slows down the parser at all, actually.

https://codereview.chromium.org/2094463002/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (left):

https://codereview.chromium.org/2094463002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:3224: } while (!parameters->has_rest &&
Check(Token::COMMA));
Can't you keep this same do/while form and just add:

... && (!allow_harmony_trailing_commas_in_parameters() || peek() !=
Token::RPAREN)

Or did you rewrite it this way for clarity? I personally find the multiple
breaks a bit confusing.

[jwolfe, 2016-06-28 21:04:16]

https://codereview.chromium.org/2094463002/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (left):

https://codereview.chromium.org/2094463002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:3224: } while (!parameters->has_rest &&
Check(Token::COMMA));
On 2016/06/28 00:39:47, adamk wrote:
> Can't you keep this same do/while form and just add:
> 
> ... && (!allow_harmony_trailing_commas_in_parameters() || peek() !=
> Token::RPAREN)
> 
> Or did you rewrite it this way for clarity? I personally find the multiple
> breaks a bit confusing.

I rewrote it for clarity. I find multiple breaks much easier to understand than
a complicated while expression. So I guess this is just a matter of opinion.

It makes sense to me to make minimal changes to the source, so I think your way
has an advantage over mine. I will change it back if you think that's best.

[adamk, 2016-06-28 21:10:52]

https://codereview.chromium.org/2094463002/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (left):

https://codereview.chromium.org/2094463002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:3224: } while (!parameters->has_rest &&
Check(Token::COMMA));
On 2016/06/28 21:04:16, jwolfe wrote:
> On 2016/06/28 00:39:47, adamk wrote:
> > Can't you keep this same do/while form and just add:
> > 
> > ... && (!allow_harmony_trailing_commas_in_parameters() || peek() !=
> > Token::RPAREN)
> > 
> > Or did you rewrite it this way for clarity? I personally find the multiple
> > breaks a bit confusing.
> 
> I rewrote it for clarity. I find multiple breaks much easier to understand
than
> a complicated while expression. So I guess this is just a matter of opinion.
> 
> It makes sense to me to make minimal changes to the source, so I think your
way
> has an advantage over mine. I will change it back if you think that's best.

I'm OK with the multiple breaks too, mostly wanted to make sure I wasn't missing
something about the logic.

https://codereview.chromium.org/2094463002/diff/100001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2094463002/diff/100001/src/flag-definitions.h...
src/flag-definitions.h:214: V(harmony_trailing_commas_in_parameters,            
                 \
How about "harmony_trailing_commas" as the flag name, for the sake of brevity?
That might help the code read a little better too.

https://codereview.chromium.org/2094463002/diff/100001/test/mjsunit/arguments...
File test/mjsunit/arguments-extra-commas.js (right):

https://codereview.chromium.org/2094463002/diff/100001/test/mjsunit/arguments...
test/mjsunit/arguments-extra-commas.js:1: // Copyright 2016 the V8 project
authors. All rights reserved.
Also please put this file in mjsunit/harmony/ to reflect that it's for a
not-yet-launched feature.

https://codereview.chromium.org/2094463002/diff/100001/test/mjsunit/arguments...
test/mjsunit/arguments-extra-commas.js:1: // Copyright 2016 the V8 project
authors. All rights reserved.
For new files please use the shorter copyright notice (see most test files in
harmony/ for examples).

https://codereview.chromium.org/2094463002/diff/100001/test/mjsunit/arguments...
test/mjsunit/arguments-extra-commas.js:38: [f1, f2, f3].forEach(function(f) {
Sorry, I meant the "length" property on functions themselves, that is:

assertEquals(2, (function(a, b,){}).length)

[jwolfe, 2016-06-28 21:50:11]

All comments addressed except for the ExpressionClassifier question.

[adamk, 2016-06-28 21:51:53]

lgtm code-wise, thanks! Do you plan to do some performance analysis before
landing? Not strictly necessary, since the PeekAhead() is safely hidden behind
the flag.

[jwolfe, 2016-06-29 01:08:50]

On 2016/06/28 21:51:53, adamk wrote:
> lgtm code-wise, thanks! Do you plan to do some performance analysis before
> landing? Not strictly necessary, since the PeekAhead() is safely hidden behind
> the flag.

performance analysis was discussed in #v8, and it was decided that this patch
will get committed without doing the analysis.

[jwolfe, 2016-06-29 01:08:56]

The CQ bit was checked by jwolfe@igalia.com

[jwolfe, 2016-06-29 01:08:56]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2094463002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-29 01:09:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 01:38:08]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-29 01:39:21]

Description was changed from

==========
Allow trailing commas in function parameter lists

Add a flag harmony_trailing_commas_in_parameters that allows trailing
commas in function parameter declaration lists and function call
parameter lists. Trailing commas are allowed in parenthetical lists like
`(a, b, c,)` only if the next token is `=>`, thereby making it an arrow
function declaration. Only 1 trailing comma is allowed, not `(a,,)`. A
trailing comma must follow a non-rest parameter, so `(,)` and `(...a,)`
are still SyntaxErrors. However, a trailing comma is allowed after a
spread parameter, e.g. `a(...b,);`.

Add parser tests for all of the above.

BUG=v8:5051
LOG=y
==========

to

==========
Allow trailing commas in function parameter lists

Add a flag harmony_trailing_commas_in_parameters that allows trailing
commas in function parameter declaration lists and function call
parameter lists. Trailing commas are allowed in parenthetical lists like
`(a, b, c,)` only if the next token is `=>`, thereby making it an arrow
function declaration. Only 1 trailing comma is allowed, not `(a,,)`. A
trailing comma must follow a non-rest parameter, so `(,)` and `(...a,)`
are still SyntaxErrors. However, a trailing comma is allowed after a
spread parameter, e.g. `a(...b,);`.

Add parser tests for all of the above.

BUG=v8:5051
LOG=y

Committed: https://crrev.com/1ac0965542e714d6dd9318c46199704e035f3d9b
Cr-Commit-Position: refs/heads/master@{#37355}
==========

[commit-bot: I haz the power, 2016-06-29 01:39:23]

Patchset 9 (id:??) landed as
https://crrev.com/1ac0965542e714d6dd9318c46199704e035f3d9b
Cr-Commit-Position: refs/heads/master@{#37355}