Closed Bug 1303788 Opened 4 years ago Closed 4 years ago

ES2017: Implement trailing comma proposal

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

Attached patch trailing_comma.patch (obsolete) — Splinter Review
Similar to the other bug, let's start with feedback before the actual review.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8792576 - Flags: feedback?(arai.unmht)
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 ')'|
Attachment #8792576 - Flags: feedback?(arai.unmht) → feedback+
(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 .....^
(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...
Attached patch bug1303788.patch (obsolete) — Splinter Review
(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|.
Attachment #8792576 - Attachment is obsolete: true
Attachment #8798470 - Flags: review?(arai.unmht)
Attachment #8798470 - Flags: review?(arai.unmht) → review+
forgot to note about test directory name, please rename depending on the poll result :)
(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.
Attached patch bug1303788.patchSplinter Review
Renamed test directory from "ecma_8" to "ecma_2017". Carrying r+ from arai.
Attachment #8798470 - Attachment is obsolete: true
Attachment #8798744 - Flags: review+
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e6438f5d89f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.