1041341 - Make `[...rest,] = []` a SyntaxError

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Arpad Borsos [:Swatinem]]

`[...spread,]` is a valid ArrayLiteral. But it is *not* a valid ArrayAssignmentPattern.

Currently checkDestructuring works by checking a fully constructed ARRAY ParseNode, at which point there is no way to detect a trailing comma.

Hopefully this can be done once bug 933296 is finished.

Comment 1

[Arpad Borsos [:Swatinem]]

There is a similar problem with `[...(rest)]`, but only in declaration contexts. It is valid in assignment contexts.

Comment 3

[Jeff Walden [:Waldo]]

My patch in bug 1146136 fixes the ...(rest) issue.  I think we'll need to do a wholesale rewrite of array/object parsing, that tracks when non-destructuring syntax is first encountered, to fix the trailing-elision problem noted here.

Comment 5

[André Bargull [:anba]]

Attachment: bug1041341-part1.patch

Stealing from jorendorff...

Patches apply on top of bug 1204024 and 1243717.

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


The interface of PossibleError was changed as follows:
Public methods are now restricted to:
- setPendingDestructuringError/setPendingExpressionError to create a new pending error,
- checkForDestructuringError/checkForExpressionError to check for a pending error
- and transferErrorsTo to transfer a pending error to another instance.

The remaining methods are now private and receive an ErrorKind enum parameter to distinguish between destructuring and expression errors.

Comment 6

[André Bargull [:anba]]

Attachment: bug1041341-part2.patch

Create a pending possible destructuring error when we encounter a comma after a spread element in arrayInitializer().

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8799524 [details] [diff] [review]
bug1041341-part1.patch

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

It would be nice to explain what "destructuring error" and "expression error" mean, in PossibleError class comment.

::: js/src/frontend/Parser.cpp
@@ +3731,5 @@
>  
>  template <typename ParseHandler>
>  Parser<ParseHandler>::PossibleError::PossibleError(Parser<ParseHandler>& parser)
> +                                                   : parser_(parser), exprError(),
> +                                                     destructuringError()

(assuming the indentation is fixed in bug 1204024)
exprError() and destructuringError() are not necessary.
please remove them.

::: js/src/frontend/Parser.h
@@ +762,5 @@
> +
> +        enum class ErrorState { None, Pending };
> +
> +        struct Error {
> +            ErrorState state_ = ErrorState::None;

I'd like to forward this part to Waldo.
It's first time seeing default member initializer in tree,
and I'm not sure if it's allowed.

@@ +773,3 @@
>          Parser<ParseHandler>& parser_;
> +        Error exprError;
> +        Error destructuringError;

can you append "_" to them?

@@ +776,3 @@
>  
> +        // Returns the error report.
> +        Error* error(ErrorKind errKind);

ErrorKind kind

to make it match to definition and other methods.

also, can this be made return reference instead of pointer?
as it won't return nullptr.

@@ +796,5 @@
>  
>        public:
>          explicit PossibleError(Parser<ParseHandler>& parser);
>  
> +        // Set a pending destructuring error. Only a single error may be set per

please wrap this line to 79 chars.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8799524 [details] [diff] [review]
bug1041341-part1.patch

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

Dunno whether you only wanted a look at the one member initializer thing, but I needed to learn about PossibleError at some point anyway, so I read the whole thing (and part 2 as well).

::: js/src/frontend/Parser.cpp
@@ +3771,5 @@
>      // the state to pending.
> +    Error* err = error(kind);
> +    err->offset_      = (pn ? parser_.handler.getPosition(pn) : parser_.pos()).begin;
> +    err->errorNumber_ = errorNumber;
> +    err->state_       = ErrorState::Pending;

Can we not align these?  This style is just not common for us, and it turns into a mess whenever names get refactored.

@@ +3792,5 @@
>  template <typename ParseHandler>
>  bool
> +Parser<ParseHandler>::PossibleError::checkForError(ErrorKind kind)
> +{
> +    if (hasError(kind)) {

I'd probably invert this, so there's an early-return and less indentation.

::: js/src/frontend/Parser.h
@@ +746,2 @@
>       *   // A JSMSG_BAD_PROP_ID ParseError is reported, returns false.
> +     *   possibleError.checkForExpressionError();

Make this (and the other cases) actually spell out the if that would surround this:

  PossibleError possibleError(*this);
  possibleError.setPendingExpressionError(pn, JSMSG_BAD_PROP_ID);
  // A JSMSG_BAD_PROP_ID ParseError is reported, returns false.
  if (!possibleError.checkForExpressionError())
      return false; // we reach this point with a pending exception

@@ +751,2 @@
>       *   // Returns true, no error is reported.
> +     *   possibleError.checkForDestructuringError();

Then here,

  if (!possibleError.checkForDestructuringError())
      return false; // not reached, no pending exception

@@ +753,4 @@
>       *
>       *   PossibleError possibleError(*this);
>       *   // Returns true, no error is reported.
> +     *   possibleError.checkForExpressionError();

And the same treatment here.

@@ +759,5 @@
>      {
> +      private:
> +        enum class ErrorKind { Expression, Destructuring };
> +
> +        enum class ErrorState { None, Pending };

Might want NoError instead of None -- there's an X11 (?) system header that #defines None.

@@ +762,5 @@
> +
> +        enum class ErrorState { None, Pending };
> +
> +        struct Error {
> +            ErrorState state_ = ErrorState::None;

https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code lists "member initializers", which I'm pretty sure is this feature, as supported.

@@ +797,5 @@
>        public:
>          explicit PossibleError(Parser<ParseHandler>& parser);
>  
> +        // Set a pending destructuring error. Only a single error may be set per
> +        // instance.

What does "Only a single error may be set" mean?  That you *must* only call this once?  Or that if you call it multiple times in succession, only the first call will have any effect (and the others will effectively be ignored)?  Looks like the second -- be clear that this is what happens.

@@ +803,3 @@
>  
> +        // Set a pending expression error. Only a single error may be set per
> +        // instance.

Same comment.

Comment 9

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> @@ +759,5 @@
> >      {
> > +      private:
> > +        enum class ErrorKind { Expression, Destructuring };
> > +
> > +        enum class ErrorState { None, Pending };
> 
> Might want NoError instead of None -- there's an X11 (?) system header that
> #defines None.

Shouldn't be an issue for us any longer -> bug 1288686.

Comment 10

[André Bargull [:anba]]

Attachment: bug1041341-part1.patch

Updated part 1 per review comments, carrying r+.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2821a9fdfd25
Part 1: Add support to store a pending destructuring error in PossibleError. r=arai, r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c23428eec9b
Part 2: Report a SyntaxError for destructuring rest with trailing comma. r=arai

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2821a9fdfd25
https://hg.mozilla.org/mozilla-central/rev/5c23428eec9b

Comment 13

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/array-destructuring-with-rest-element-will-throw-if-trailing-comma-follows/

Comment 14

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/52#JavaScript

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assigning_the_rest_of_an_array_to_a_variable

A new error was introduced: JSMSG_REST_WITH_COMMA "rest element may not have a trailing comma". It could be linked from the console if we want. I don't feel a strong need for it for now. Let me know if you do.