Syntax parse destructuring patterns

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jandem, Assigned: anba)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Since bug 1297706 we can syntax parse most JS features, but destructuring patterns still abort syntax parsing. The tricky case is when we have something like this:

  {x, y} = z;

When we start parsing this, we don't know if we're parsing an object literal or a destructuring pattern. So the full parser parses it as object literal and when it sees the "=" it looks at the object literal's properties and does some extra checks (checkDestructuringPattern). The syntax parser doesn't record this information so we need a different approach.

One option is to save the TokenStream position. Then when we know we have a destructuring pattern, checkDestructuringPattern can rewind and reparse it as a pattern. This will duplicate parts of Parser::objectLiteral and Parser::arrayInitializer so I'm not super excited about it.
Depends on: 1358264
No longer depends on: 1358264
Duplicate of this bug: 1358264
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Adds objectBindingPattern() and arrayBindingPattern() methods to parse destructuring binding patterns. The code is based on the existing objectLiteral() resp. arrayInitializer() methods and was then modified to remove anything not related to binding patterns. 

The checkDestructuringXXX() methods are now only used for validating assignment destructuring patterns and therefore they have been renamed to checkDestructuringAssignmentXXX(). (They will be removed in part 3!)
Attachment #8860412 - Flags: review?(shu)
Now that binding and assignment destructuring patterns are parsed in different methods, we actually no longer need the |inDestructuringDecl| field in ParseContext and can remove it and its helper class AutoClearInDestructuringDecl. In addition to clear and restore |inDestructuringDecl|, AutoClearInDestructuringDecl was also responsible for tracking expressions in formal parameters. This is now done inline in Parser::bindingInitializer() and Parser::computedPropertyName().

And I've replaced a few lines for parsing simple catch-parameter with the new Parser::bindingIdentifier() method to reduce code duplication.
Attachment #8860417 - Flags: review?(shu)
Removes all checkDestructuringAssignmentXXX() methods and replaces their functionality by using the PossibleError class to track destructuring syntax errors. 

The extra-warnings mode made this patch slightly more complicated, because now we also need to track extra-warnings in possible destructuring contexts in PossibleError. Except that this patch doesn't actually support 'extra-warnings', but only a single 'extra-warning'. If we want to keep the functionality to report multiple extra-warnings in a single destructuring assignment pattern, we'll need to modify PossibleError to store a list of possible warnings instead of a single warning. 

I tried to ensure syntax errors are reported at the same location with this patch applied, but for some cases we now report an error at a different location.

For example:
js> function f() { ({a:([])}={}) }
typein:1:19 SyntaxError: destructuring patterns in assignments can't be parenthesized:
typein:1:19 function f() { ({a:([])}={}) }
typein:1:19 ...................^

Was:
typein:1:20 SyntaxError: destructuring patterns in assignments can't be parenthesized:
typein:1:20 function f() { ({a:([])}={}) }
typein:1:20 ....................^


js> function f() { [...a, b] = [] }
typein:2:20 SyntaxError: rest element may not have a trailing comma:
typein:2:20 function f() { [...a, b] = [] }
typein:2:20 ....................^

Was:
typein:2:22 SyntaxError: parameter after rest parameter:
typein:2:22 function f() { [...a, b] = [] }
typein:2:22 ......................^


js> function f() { ({async m(){}}={}) }
typein:3:17 SyntaxError: invalid destructuring target:
typein:3:17 function f() { ({async m(){}}={}) }
typein:3:17 .................^

Was:
typein:3:23 SyntaxError: invalid destructuring target:
typein:3:23 function f() { ({async m(){}}={}) }
typein:3:23 .......................^


js> function f() { "use strict"; ({a:(eval)}={}) }
typein:4:33 SyntaxError: 'eval' can't be defined or assigned to in strict mode code:
typein:4:33 function f() { "use strict"; ({a:(eval)}={}) }
typein:4:33 .................................^

Was:
typein:4:34 SyntaxError: 'eval' can't be defined or assigned to in strict mode code:
typein:4:34 function f() { "use strict"; ({a:(eval)}={}) }
typein:4:34 ..................................^


And with "-w -s" enabled
js> [eval,eval,eval]=[]
typein:1:1 strict warning: 'eval' can't be defined or assigned to in strict mode code:
typein:1:1 strict warning: [eval,eval,eval]=[]
typein:1:1 strict warning: .^

Was:
typein:1:1 strict warning: 'eval' can't be defined or assigned to in strict mode code:
typein:1:1 strict warning: [eval,eval,eval]=[]
typein:1:1 strict warning: .^
typein:1:6 strict warning: 'eval' can't be defined or assigned to in strict mode code:
typein:1:6 strict warning: [eval,eval,eval]=[]
typein:1:6 strict warning: ......^
typein:1:11 strict warning: 'eval' can't be defined or assigned to in strict mode code:
typein:1:11 strict warning: [eval,eval,eval]=[]
typein:1:11 strict warning: ...........^
Attachment #8860437 - Flags: review?(shu)
This is a pre-existing issue. The array pattern case was already fixed by part 3, so I only needed to change Parser::objectLiteral().
Attachment #8860453 - Flags: review?(shu)
Comment on attachment 8860412 [details] [diff] [review]
bug1303703-part1-binding-pattern.patch

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

This is really nice, much simpler than I thought it'd be.

::: js/src/frontend/Parser.cpp
@@ +4581,5 @@
> +                return null();
> +
> +            if (!handler.addPropertyDefinition(literal, propName, bindingExpr))
> +                return null();
> +        } else if (propType == PropertyType::Shorthand) {

Giving a short example of what shorthand is, like in objectLiteral, would be helpful.

@@ +4590,5 @@
> +                return null();
> +
> +            if (!handler.addShorthand(literal, propName, binding))
> +                return null();
> +        } else if (propType == PropertyType::CoverInitializedName) {

A short example of what CoverInitializedName is would be nice too.

@@ +4697,5 @@
> +                 return null();
> +             if (!matched) {
> +                 modifier = TokenStream::None;
> +                 break;
> +             }

What's the error reported now for |[...foo,] = []|? Is it more cryptic than the current "rest element may not have a trailing comma"?
Attachment #8860412 - Flags: review?(shu) → review+
Comment on attachment 8860417 [details] [diff] [review]
bug1303703-part2-indecl-cleanup.patch

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

Nice cleanup.
Attachment #8860417 - Flags: review?(shu) → review+
(In reply to André Bargull from comment #4)
> Created attachment 8860437 [details] [diff] [review]
> bug1303703-part3-assignment-pattern.patch
> 
> Removes all checkDestructuringAssignmentXXX() methods and replaces their
> functionality by using the PossibleError class to track destructuring syntax
> errors. 
> 
> The extra-warnings mode made this patch slightly more complicated, because
> now we also need to track extra-warnings in possible destructuring contexts
> in PossibleError. Except that this patch doesn't actually support
> 'extra-warnings', but only a single 'extra-warning'. If we want to keep the
> functionality to report multiple extra-warnings in a single destructuring
> assignment pattern, we'll need to modify PossibleError to store a list of
> possible warnings instead of a single warning. 
> 
> I tried to ensure syntax errors are reported at the same location with this
> patch applied, but for some cases we now report an error at a different
> location.
> 
> For example:
> js> function f() { ({a:([])}={}) }
> typein:1:19 SyntaxError: destructuring patterns in assignments can't be
> parenthesized:
> typein:1:19 function f() { ({a:([])}={}) }
> typein:1:19 ...................^
> 
> Was:
> typein:1:20 SyntaxError: destructuring patterns in assignments can't be
> parenthesized:
> typein:1:20 function f() { ({a:([])}={}) }
> typein:1:20 ....................^
>  

Given the wording of the error message, the fact that ^ now points to the first left paren is just fine.

>
> js> function f() { [...a, b] = [] }
> typein:2:20 SyntaxError: rest element may not have a trailing comma:
> typein:2:20 function f() { [...a, b] = [] }
> typein:2:20 ....................^
> 
> Was:
> typein:2:22 SyntaxError: parameter after rest parameter:
> typein:2:22 function f() { [...a, b] = [] }
> typein:2:22 ......................^
> 

Ah, this answers one of my questions from a previous review. I'm also fine with this error change. The new error is still clear and actionable.

> 
> js> function f() { ({async m(){}}={}) }
> typein:3:17 SyntaxError: invalid destructuring target:
> typein:3:17 function f() { ({async m(){}}={}) }
> typein:3:17 .................^
> 
> Was:
> typein:3:23 SyntaxError: invalid destructuring target:
> typein:3:23 function f() { ({async m(){}}={}) }
> typein:3:23 .......................^
> 

The new error is better, actually.

> 
> js> function f() { "use strict"; ({a:(eval)}={}) }
> typein:4:33 SyntaxError: 'eval' can't be defined or assigned to in strict
> mode code:
> typein:4:33 function f() { "use strict"; ({a:(eval)}={}) }
> typein:4:33 .................................^
> 
> Was:
> typein:4:34 SyntaxError: 'eval' can't be defined or assigned to in strict
> mode code:
> typein:4:34 function f() { "use strict"; ({a:(eval)}={}) }
> typein:4:34 ..................................^
> 

This, unlike the first error, is made worse with because of we can't skip the parens. In practice nobody's gonna be parenthesizing |eval| with so many parens that the message becomes obscure, so I think we're fine.

> 
> And with "-w -s" enabled
> js> [eval,eval,eval]=[]
> typein:1:1 strict warning: 'eval' can't be defined or assigned to in strict
> mode code:
> typein:1:1 strict warning: [eval,eval,eval]=[]
> typein:1:1 strict warning: .^
> 
> Was:
> typein:1:1 strict warning: 'eval' can't be defined or assigned to in strict
> mode code:
> typein:1:1 strict warning: [eval,eval,eval]=[]
> typein:1:1 strict warning: .^
> typein:1:6 strict warning: 'eval' can't be defined or assigned to in strict
> mode code:
> typein:1:6 strict warning: [eval,eval,eval]=[]
> typein:1:6 strict warning: ......^
> typein:1:11 strict warning: 'eval' can't be defined or assigned to in strict
> mode code:
> typein:1:11 strict warning: [eval,eval,eval]=[]
> typein:1:11 strict warning: ...........^

lol
Comment on attachment 8860437 [details] [diff] [review]
bug1303703-part3-assignment-pattern.patch

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

This is also pleasantly simpler than I thought it'd be. Thank you for being diligent with the error messages.

::: js/src/frontend/Parser.cpp
@@ +9348,5 @@
>  
>  template <template <typename CharT> class ParseHandler, typename CharT>
> +void
> +Parser<ParseHandler, CharT>::checkDestructuringAssignmentTarget(Node expr, TokenPos exprPos,
> +                                                                PossibleError* possibleError)

I think that in practice, many object literals end up setting pending errors here. String literals as keys is probably pretty common. We should early return here if possiblError already has a pending destructuring error.
Attachment #8860437 - Flags: review?(shu) → review+
Comment on attachment 8860453 [details] [diff] [review]
bug1303703-part4-constant-folding.patch

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

Nice find.
Attachment #8860453 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #7)
> @@ +4697,5 @@
> > +                 return null();
> > +             if (!matched) {
> > +                 modifier = TokenStream::None;
> > +                 break;
> > +             }
> 
> What's the error reported now for |[...foo,] = []|? Is it more cryptic than
> the current "rest element may not have a trailing comma"?

It depends on the destructuring kind:

js> var [...foo,] = []
typein:1:11 SyntaxError: missing ] after element list:
typein:1:11 var [...foo,] = []
typein:1:11 ...........^
typein:1:4 note: [ opened at line 1, column 4

js> [...foo,] = []     
typein:2:7 SyntaxError: rest element may not have a trailing comma:
typein:2:7 [...foo,] = []
typein:2:7 .......^


I think it makes sense to change the error for |var [...foo,]| to have the same error message as the assignment destructuring case.
(In reply to Shu-yu Guo [:shu] from comment #9)
> > 
> > js> function f() { "use strict"; ({a:(eval)}={}) }
> > typein:4:33 SyntaxError: 'eval' can't be defined or assigned to in strict
> > mode code:
> > typein:4:33 function f() { "use strict"; ({a:(eval)}={}) }
> > typein:4:33 .................................^
> > 
> > Was:
> > typein:4:34 SyntaxError: 'eval' can't be defined or assigned to in strict
> > mode code:
> > typein:4:34 function f() { "use strict"; ({a:(eval)}={}) }
> > typein:4:34 ..................................^
> > 
> 
> This, unlike the first error, is made worse with because of we can't skip
> the parens. In practice nobody's gonna be parenthesizing |eval| with so many
> parens that the message becomes obscure, so I think we're fine.
> 

Agreed. I didn't hear anyone complain about the placement of the error marker at the opening parenthesis for the normal assignment case, either.

js> "use strict"; (eval) = true
typein:5:14 SyntaxError: 'eval' can't be defined or assigned to in strict mode code:
typein:5:14 "use strict"; (eval) = true
typein:5:14 ..............^
Updated part 1 per review comments and changed error message for trailing comma after rest-element in array binding pattern to match error for array assignment pattern.
Attachment #8860412 - Attachment is obsolete: true
Attachment #8860620 - Flags: review+
Updated part 2 to apply on changed part 1, carrying r+.
Attachment #8860417 - Attachment is obsolete: true
Attachment #8860621 - Flags: review+
Updated part 3 per review comment, carrying r+.
Attachment #8860437 - Attachment is obsolete: true
Attachment #8860622 - Flags: review+
Fixed small issue in test case, otherwise unchanged. Carrying r+.
Attachment #8860453 - Attachment is obsolete: true
Attachment #8860623 - Flags: review+
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad2e4e4cd72
Part 1: Separate binding pattern parsing from object/array literal parsing. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e29a56658e
Part 2: Clean-up bits of destructuring parsing which are no longer needed. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f2c0ac318b1
Part 3: Syntax parse destructuring assignment patterns. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/332fa2188d89
Part 4: Check assignment destructuring syntax before constant-folding. r=shu
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.