ES2017: Implement trailing comma proposal

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

({dev-doc-complete})

Trunk
mozilla52
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 affected, firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Comment 1

3 years ago
Posted 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+
Assignee

Comment 3

3 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 .....^
(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...
Assignee

Comment 5

3 years ago
Posted 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 :)
Assignee

Comment 8

3 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.
Assignee

Comment 9

3 years ago
Renamed test directory from "ecma_8" to "ecma_2017". Carrying r+ from arai.
Attachment #8798470 - Attachment is obsolete: true
Attachment #8798744 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 10

3 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
Keywords: checkin-needed

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e6438f5d89f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1308802
No longer depends on: 1308802
You need to log in before you can comment on or make changes to this bug.