1303703 - Syntax parse destructuring patterns

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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.

Comment 2

[André Bargull [:anba]]

Attachment: bug1303703-part1-binding-pattern.patch

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!)

Comment 3

[André Bargull [:anba]]

Attachment: bug1303703-part2-indecl-cleanup.patch

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.

Comment 4

[André Bargull [:anba]]

Attachment: 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 ....................^


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: ...........^

Comment 5

[André Bargull [:anba]]

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

Comment 6

[André Bargull [:anba]]

Attachment: bug1303703-part4-constant-folding.patch

This is a pre-existing issue. The array pattern case was already fixed by part 3, so I only needed to change Parser::objectLiteral().

Comment 7

[Shu-yu Guo [: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"?

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8860417 [details] [diff] [review]
bug1303703-part2-indecl-cleanup.patch

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

Nice cleanup.

Comment 9

[Shu-yu Guo [:shu]]

(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 10

[Shu-yu Guo [:shu]]

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.

Comment 11

[Shu-yu Guo [:shu]]

Comment on attachment 8860453 [details] [diff] [review]
bug1303703-part4-constant-folding.patch

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

Nice find.

Comment 12

[André Bargull [:anba]]

(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.

Comment 13

[André Bargull [:anba]]

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

Comment 14

[André Bargull [:anba]]

Attachment: bug1303703-part1-binding-pattern.patch

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.

Comment 15

[André Bargull [:anba]]

Attachment: bug1303703-part2-indecl-cleanup.patch

Updated part 2 to apply on changed part 1, carrying r+.

Comment 16

[André Bargull [:anba]]

Attachment: bug1303703-part3-assignment-pattern.patch

Updated part 3 per review comment, carrying r+.

Comment 17

[André Bargull [:anba]]

Attachment: bug1303703-part4-constant-folding.patch

Fixed small issue in test case, otherwise unchanged. Carrying r+.

Comment 18

[André Bargull [:anba]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ad2e4e4cd72
https://hg.mozilla.org/mozilla-central/rev/d6e29a56658e
https://hg.mozilla.org/mozilla-central/rev/8f2c0ac318b1
https://hg.mozilla.org/mozilla-central/rev/332fa2188d89