André Bargull [:anba] Assignee
	Description • 8 years ago

Implement the stage 4 proposal "Trailing Commas in Function Param Lists".

https://github.com/tc39/proposals/blob/master/finished-proposals.md
https://jeffmo.github.io/es-trailing-function-commas/

	André Bargull [:anba] Assignee
	Comment 1 • 8 years ago

Similar to the other bug, let's start with feedback before the actual review.

	Tooru Fujisawa [:arai]
	Comment 2 • 8 years ago

Comment on attachment 8792576 [details] [diff] [review]
trailing_comma.patch

Review of attachment 8792576 [details] [diff] [review]:
-----------------------------------------------------------------

it might be better to keep the trailing comma in parse tree and Reflect.parse.

also, (not so important), it would be nice to say ES2017 in commit message,
and maybe jstests directory name (ecma_2017)?

::: js/src/frontend/Parser.cpp
@@ +6913,5 @@
>      if (!seq)
>          return null();
>      while (true) {
> +        // Tripledot is allowed iff we parse a
> +        // CoverParenthesizedExpressionAndArrowParameterList.

This code block is to handle trailing comma in parenthesized arrow function parameter, right?
it would be nice to add a comment here about it.

@@ +6923,5 @@
> +            if (tt == TOK_RP) {
> +                tokenStream.addModifierException(TokenStream::NoneIsOperand);
> +
> +                if (!tokenStream.getToken(&tt) || !tokenStream.peekTokenSameLine(&tt))
> +                    return null();

it should be better splitting this to

tokenStream.consumeKnownToken(TOK_RP, TokenStream::Operand);
if (!tokenStream.peekTokenSameLine(&tt))
    return null();

@@ +6926,5 @@
> +                if (!tokenStream.getToken(&tt) || !tokenStream.peekTokenSameLine(&tt))
> +                    return null();
> +                if (tt != TOK_ARROW) {
> +                    report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN,
> +                        "'=>' after argument list", TokenKindToDesc(tt));

I'm not sure if throwing a SyntaxError about arrow function for the following expression makes sense.

  js> (1,)+2
  typein:1:3 SyntaxError: expected '=>' after argument list, got '+':
  typein:1:3 (1,)+2
  typein:1:3 ...^

Also, it's reporting an error for TOK_RP position.
It would be better just saying |expected expression, got ')'|

	André Bargull [:anba] Assignee
	Comment 3 • 8 years ago

(In reply to Tooru Fujisawa [:arai] from comment #2)
> Comment on attachment 8792576 [details] [diff] [review]
> trailing_comma.patch
> 
> Review of attachment 8792576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it might be better to keep the trailing comma in parse tree and
> Reflect.parse.

The trailing comma is not kept for array and object literals, so I think we should omit it here, too.

> 
> also, (not so important), it would be nice to say ES2017 in commit message,
> and maybe jstests directory name (ecma_2017)?

I'm not sure if everyone agrees with the ecma_<year> scheme, I guess I need to do a poll on #jsapi first. :-)

> 
> ::: js/src/frontend/Parser.cpp
> @@ +6913,5 @@
> >      if (!seq)
> >          return null();
> >      while (true) {
> > +        // Tripledot is allowed iff we parse a
> > +        // CoverParenthesizedExpressionAndArrowParameterList.
> 
> This code block is to handle trailing comma in parenthesized arrow function
> parameter, right?
> it would be nice to add a comment here about it.

Okay, will do. I thought the "iff" plus CoverParenthesizedExpressionAndArrowParameterList give enough hints why this is needed. 

> 
> @@ +6923,5 @@
> > +            if (tt == TOK_RP) {
> > +                tokenStream.addModifierException(TokenStream::NoneIsOperand);
> > +
> > +                if (!tokenStream.getToken(&tt) || !tokenStream.peekTokenSameLine(&tt))
> > +                    return null();
> 
> it should be better splitting this to
> 
> tokenStream.consumeKnownToken(TOK_RP, TokenStream::Operand);
> if (!tokenStream.peekTokenSameLine(&tt))
>     return null();

Sure. I didn't see the consumeKnownToken method and simply copied the pattern from here: https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/js/src/frontend/Parser.cpp#4531


> 
> @@ +6926,5 @@
> > +                if (!tokenStream.getToken(&tt) || !tokenStream.peekTokenSameLine(&tt))
> > +                    return null();
> > +                if (tt != TOK_ARROW) {
> > +                    report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN,
> > +                        "'=>' after argument list", TokenKindToDesc(tt));
> 
> I'm not sure if throwing a SyntaxError about arrow function for the
> following expression makes sense.
> 
>   js> (1,)+2
>   typein:1:3 SyntaxError: expected '=>' after argument list, got '+':
>   typein:1:3 (1,)+2
>   typein:1:3 ...^
> 
> Also, it's reporting an error for TOK_RP position.

So we should probably fix it for |(...a);|, too? https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/js/src/frontend/Parser.cpp#9045-9049

js> (...a);
typein:1:5 SyntaxError: expected '=>' after argument list, got ';':
typein:1:5 (...a);
typein:1:5 .....^

	Tooru Fujisawa [:arai]
	Comment 4 • 8 years ago

(In reply to André Bargull from comment #3)
> > it might be better to keep the trailing comma in parse tree and
> > Reflect.parse.
> 
> The trailing comma is not kept for array and object literals, so I think we
> should omit it here, too.

I see :)


> Sure. I didn't see the consumeKnownToken method and simply copied the
> pattern from here:
> https://dxr.mozilla.org/mozilla-central/rev/
> f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/js/src/frontend/Parser.cpp#4531

that would also need a fix.
will file a bug.


> So we should probably fix it for |(...a);|, too?
> https://dxr.mozilla.org/mozilla-central/rev/
> f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/js/src/frontend/Parser.cpp#9045-9049
> 
> js> (...a);
> typein:1:5 SyntaxError: expected '=>' after argument list, got ';':
> typein:1:5 (...a);
> typein:1:5 .....^

oh... surely, the reported position is wrong there too.

but in this case tripledot mostly indicates it's arrow function parameter, I think.
or, perhaps we can report both possibilities in both case...

	André Bargull [:anba] Assignee
	Comment 5 • 8 years ago

(In reply to Tooru Fujisawa [:arai] from comment #4)
> oh... surely, the reported position is wrong there too.

I added |tokenStream.consumeKnownToken(next);| to advance the token stream to
the correct position [1]. Then I noticed that I need to guard consumeKnownToken
with |if (next != TOK_EOL)| to avoid an assertion failure in [2]. This made me
wonder if peekTokenSameLine is actually needed at this point [3], especially
because we don't use it here [4]. So what I did was to replace both calls to
peekTokenSameLine (in [3] and in the new code) with peekToken, and moved all
line break checks to [5].
Additionally I've changed the error message in [6] to "no line break is allowed before '=>'".

[1] https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/js/src/frontend/Parser.cpp#9046
[2] https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/js/src/frontend/TokenStream.h#640
[3] https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/js/src/frontend/Parser.cpp#9043
[4] https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/js/src/frontend/Parser.cpp#8939
[5] https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/js/src/frontend/Parser.cpp#7294
[6] https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/js/src/frontend/Parser.cpp#7295-7296

> but in this case tripledot mostly indicates it's arrow function parameter, I think.
> or, perhaps we can report both possibilities in both case...

After sleeping it over, I decided that displaying only "expected expression, got ')'"
is the correct choice for invalid syntax like |(1,)+2|.

	André Bargull [:anba] Assignee
	Comment 6 • 8 years ago

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb3438b5f8a07a54364032fcb878a1f15690743

	Tooru Fujisawa [:arai]
	Comment 7 • 8 years ago

forgot to note about test directory name, please rename depending on the poll result :)

	André Bargull [:anba] Assignee
	Comment 8 • 8 years ago

(In reply to Tooru Fujisawa [:arai] from comment #7)
> forgot to note about test directory name, please rename depending on the
> poll result :)

http://logs.glob.uno/?c=mozilla%23jsapi#c799947
http://logs.glob.uno/?c=mozilla%23jsapi#c799948
http://logs.glob.uno/?c=mozilla%23jsapi#c799949

3 votes for ecma_2017, 0 votes for ecma_8. I'll update the patch to use ecma_2017.

	André Bargull [:anba] Assignee
	Comment 9 • 8 years ago

Renamed test directory from "ecma_8" to "ecma_2017". Carrying r+ from arai.

	Pulsebot
	Comment 10 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6438f5d89f
Add support for trailing comma in argument and parameter lists (ES2017). r=arai

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6e6438f5d89f

	Florian Scholz (Open Web Docs)
	Comment 12 • 7 years ago

Mentioned on:
https://developer.mozilla.org/en-US/Firefox/Releases/52#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_Next_support_in_Mozilla

Created a new page with information about trailing commas in JS:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Trailing_commas

As always, feel free to improve the MDN wiki pages!