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

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Swatinem, Assigned: anba)

Tracking

(Blocks: 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla52
x86_64
Linux
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
`[...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.
(Reporter)

Comment 1

5 years ago
There is a similar problem with `[...(rest)]`, but only in declaration contexts. It is valid in assignment contexts.
Summary: make `[...rest,] = []` a SyntaxError → make `[...rest,] = []` and `var [...(rest)] = []` a SyntaxError
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1157419
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.
Summary: make `[...rest,] = []` and `var [...(rest)] = []` a SyntaxError → Make `[...rest,] = []` a SyntaxError
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1204029
Assignee: nobody → jorendorff
(Assignee)

Comment 5

3 years ago
Posted patch bug1041341-part1.patch (obsolete) — Splinter Review
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.
Assignee: jorendorff → andrebargull
Status: NEW → ASSIGNED
Attachment #8799524 - Flags: review?(arai.unmht)
(Assignee)

Comment 6

3 years ago
Create a pending possible destructuring error when we encounter a comma after a spread element in arrayInitializer().
Attachment #8799525 - Flags: review?(arai.unmht)
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.
Attachment #8799524 - Flags: review?(jwalden+bmo)
Attachment #8799524 - Flags: review?(arai.unmht)
Attachment #8799524 - Flags: review+
Attachment #8799525 - Flags: review?(arai.unmht) → review+
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.
Attachment #8799524 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
Updated part 1 per review comments, carrying r+.
Attachment #8799524 - Attachment is obsolete: true
Attachment #8802356 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 11

3 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2821a9fdfd25
https://hg.mozilla.org/mozilla-central/rev/5c23428eec9b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.