1146136 - Parenthesized AssignmentPatterns are not a valid LHS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Caitlin Potter [:caitp]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2340.0 Safari/537.36

Steps to reproduce:

var a, b;
([a, b]) = [1, 2];
({a, b}) = {a: 3, b: 4};



Actual results:

LHS is parsed as an AssignmentPattern, `a` and `b` are each assigned to corresponding values from the RHS


Expected results:

There are at least 2 reasons why this appears to not be legal:

- 12.14.5.1 (https://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-assignment-static-semantics-early-errors) says to only parse as an AssignmentPattern if the LHS is an ObjectLiteral or ArrayLiteral (n'either applies as the LHS is a ParenthesizedExpression) --- These parenthesized expressions are not valid simple assignment targets, per 12.2.9.3 and 12.2.0.4

Possibly related to bug 933809

Comment 1

[Caitlin Potter [:caitp]]

sorry, "at least 2 reasons" is not true, it's more like "at least one reason" :)

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch

For once, I actually *would* like the test carefully looked at to ensure I'm not accidentally misreading any of the spec and consequently misimplementing anything as *not* an error when it should be, or vice versa.

Comment 3

[André Bargull [:anba]]

Comment on attachment 8614339 [details] [diff] [review]
Patch

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> For once, I actually *would* like the test carefully looked at to ensure I'm
> not accidentally misreading any of the spec and consequently misimplementing
> anything as *not* an error when it should be, or vice versa.

Done! :-)

::: js/src/tests/ecma_6/Expressions/destructuring-pattern-parenthesized.js
@@ +55,5 @@
> +  helper(savedEval, "", nonstrictErr);
> +  helper(savedEval, "'use strict'; ", strictErr);
> +}
> +
> +checkError("var a, b; ([a, b]) = [1, 2];", SyntaxError, SyntaxError);

ReferenceError in both modes - ES2015, 12.14.1:
  It is an early Reference Error if LeftHandSideExpression is neither an ObjectLiteral nor an ArrayLiteral
  and IsValidSimpleAssignmentTarget of LeftHandSideExpression is false.

@@ +56,5 @@
> +  helper(savedEval, "'use strict'; ", strictErr);
> +}
> +
> +checkError("var a, b; ([a, b]) = [1, 2];", SyntaxError, SyntaxError);
> +checkError("var a, b; ({a, b}) = { a: 1, b: 2 };", SyntaxError, SyntaxError);

Same here (ReferenceError instead of SyntaxError).

@@ +83,5 @@
> +checkError("var a, b; [(f()) = 'kthxbai', b] = [1, 2];", SyntaxError, ReferenceError);
> +
> +Function("var a, b; ({ a: (a), b} = { a: 1, b: 2 });")();
> +Function("var a, b; ({ a: (a) = 5, b} = { a: 1, b: 2 });")();
> +Function("var a, b; ({ a: ({ b: b }) } = { a: { b: 42 } });")();

The last test should throw a SyntaxError per 12.14.5.1:
  It is a Syntax Error if LeftHandSideExpression is neither an ObjectLiteral nor an ArrayLiteral and
  IsValidSimpleAssignmentTarget(LeftHandSideExpression) is false.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to André Bargull from comment #3)
> Done! :-)

\o/

> ::: js/src/tests/ecma_6/Expressions/destructuring-pattern-parenthesized.js
> @@ +55,5 @@
> > +  helper(savedEval, "", nonstrictErr);
> > +  helper(savedEval, "'use strict'; ", strictErr);
> > +}
> > +
> > +checkError("var a, b; ([a, b]) = [1, 2];", SyntaxError, SyntaxError);
> 
> ReferenceError in both modes - ES2015, 12.14.1:
>   It is an early Reference Error if LeftHandSideExpression is neither an
> ObjectLiteral nor an ArrayLiteral
>   and IsValidSimpleAssignmentTarget of LeftHandSideExpression is false.

I don't think I buy this.  The left of an = is a LeftHandSideExpression, but the AssignmentPattern grammar *refines* that, restricting to only what's allowed by AssignmentPattern.  And that refined grammar does *not* include this parenthesized form.  So we don't even get to the static-semantics early-error rules -- those would only apply if the refinement grammar admitted parenthesized patterns.

Sure, this is nitpicky, but that's what we do.  :-)  Personally, I'm still waiting for Allen to fix the spec to just require a SyntaxError here, this is all just stupid.

> @@ +56,5 @@
> > +  helper(savedEval, "'use strict'; ", strictErr);
> > +}
> > +
> > +checkError("var a, b; ([a, b]) = [1, 2];", SyntaxError, SyntaxError);
> > +checkError("var a, b; ({a, b}) = { a: 1, b: 2 };", SyntaxError, SyntaxError);
> 
> Same here (ReferenceError instead of SyntaxError).

Nuh-uh!

> > +Function("var a, b; ({ a: ({ b: b }) } = { a: { b: 42 } });")();
> 
> The last test should throw a SyntaxError per 12.14.5.1:
>   It is a Syntax Error if LeftHandSideExpression is neither an ObjectLiteral
> nor an ArrayLiteral and
>   IsValidSimpleAssignmentTarget(LeftHandSideExpression) is false.

Okay, I think you're right about this one.  Easy fix.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Updated so [([y])] = [2] and [({z})] = [{z: 3}] are syntax errors

Comment 6

[André Bargull [:anba]]

> > > +checkError("var a, b; ([a, b]) = [1, 2];", SyntaxError, SyntaxError);
> > 
> > ReferenceError in both modes - ES2015, 12.14.1:
> >   It is an early Reference Error if LeftHandSideExpression is neither an
> > ObjectLiteral nor an ArrayLiteral
> >   and IsValidSimpleAssignmentTarget of LeftHandSideExpression is false.
> 
> I don't think I buy this.  The left of an = is a LeftHandSideExpression, but
> the AssignmentPattern grammar *refines* that, restricting to only what's
> allowed by AssignmentPattern.  And that refined grammar does *not* include
> this parenthesized form.  So we don't even get to the static-semantics
> early-error rules -- those would only apply if the refinement grammar
> admitted parenthesized patterns.

The LeftHandSideExpression is only refined using AssignmentPattern if the parsed production is either an ObjectLiteral or an ArrayLiteral. And `([a, b])` is neither an ObjectLiteral nor an ArrayLiteral.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to André Bargull from comment #6)
> The LeftHandSideExpression is only refined using AssignmentPattern if the
> parsed production is either an ObjectLiteral or an ArrayLiteral. And `([a,
> b])` is neither an ObjectLiteral nor an ArrayLiteral.

Blah.  STAB ALL THIS SPEC LANGUAGE.  STAB ALL EARLY REFERENCE ERRORS.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Once more, with feeling

Comment 9

[Jason Orendorff [:jorendorff]]

Either the spec is harder to read now than in ES3 days, or my brain ossified at some point in the intervening years.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8614989 [details] [diff] [review]
Once more, with feeling

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

Looks OK to me. As the syntax parser gains complexity, I worry about what role we expect it to play, and how that complexity figures into our performance evaluations of the tradeoffs involved.

::: js/src/frontend/Parser.cpp
@@ +3367,5 @@
> +{
> +    MOZ_ASSERT(!handler.isUnparenthesizedDestructuringPattern(expr));
> +
> +    // Parentheses are forbidden around destructuring *patterns* (but allowed
> +    // around names).  Use our nicer error message for parenthesized, nested

micro nit: not sure about this comma after parenthesized

@@ +3391,5 @@
> +    // Otherwise this is an expression in destructuring outside a declaration.
> +    if (!reportIfNotValidSimpleAssignmentTarget(expr, KeyedDestructuringAssignment))
> +        return false;
> +
> +    MOZ_ASSERT(!handler.isFunctionCall(expr),

why assert this in particular? It's not bad, just curious why we single out function calls. Setcall?

@@ +3406,5 @@
> +        // specialized, in the very weird "for (var [x] = i in o) ..."
> +        // case. See bug 558633.
> +        //
> +        // XXX Is this necessary with the changes in bug 1164741?  This is
> +        //     likely removable now.

file a followup to evaluate?

@@ +6614,5 @@
>  
>      if (handler.isPropertyAccess(node))
>          return true;
> +
> +    if (behavior == PermitAssignmentToFunctionCalls) {

:)

@@ +6657,5 @@
>  
> +    if (handler.maybeNameAnyParentheses(target)) {
> +        // Use a special error if the target is arguments/eval.  This ensures
> +        // targeting these names is consistently a SyntaxError (which error numbers
> +        // below don't guarantee) while giving us a nicer error message.

Stab early ReferenceError. Stab.

::: js/src/frontend/SyntaxParseHandler.h
@@ +96,5 @@
> +        NodeUnparenthesizedName,
> +
> +        // Valuable for recognizing potential destructuring patterns.
> +        NodeUnparenthesizedArray,
> +        NodeUnparenthesizedObject,

This exponential blowup is painful. We should talk, more generally, about what the plans are for the SyntaxParser.

::: js/src/tests/ecma_6/Expressions/destructuring-pattern-parenthesized.js
@@ +101,5 @@
> +
> +if (typeof reportCompare === "function")
> +  reportCompare(true, true);
> +
> +print("Tests complete");

If Andre is happy, I can be, also.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Final patch

Okay, this is ready to go, modulo three fixes to addon-sdk code.  Pull request submitted upstream, waiting on a response to that before I go ahead with this:

https://github.com/mozilla/addon-sdk/pull/2018

Comment 12

[Jeff Walden [:Waldo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b38b4a1204d4 and https://treeherder.mozilla.org/#/jobs?repo=try&revision=08108e03cbf4 in case you wanted to see tree greenness for that final patch, incidentally.

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> Created attachment 8621253 [details] [diff] [review]
> Final patch
> 
> Okay, this is ready to go, modulo three fixes to addon-sdk code.  Pull
> request submitted upstream, waiting on a response to that before I go ahead
> with this:
> 
> https://github.com/mozilla/addon-sdk/pull/2018

This may take a little more time than it otherwise should have - the PR looks straightforward but we're in a transition phase wrt getting changes back into m-c.

Comment 14

[Joe Walker [:jwalker]]

(In reply to Jeff Griffiths (:canuckistani) from comment #13)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> > Created attachment 8621253 [details] [diff] [review]
> > Final patch
> > 
> > Okay, this is ready to go, modulo three fixes to addon-sdk code.  Pull
> > request submitted upstream, waiting on a response to that before I go ahead
> > with this:
> > 
> > https://github.com/mozilla/addon-sdk/pull/2018
> 
> This may take a little more time than it otherwise should have - the PR
> looks straightforward but we're in a transition phase wrt getting changes
> back into m-c.

Jeff is on PTO today. We're seriously considering reversing the merge direction, so changes would be merged from m-c to github rather than the other way around. More tomorrow.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7386b1f4acfc

Comment 16

[Jeff Walden [:Waldo]]

Landed basically all the test changes I could land without the actual patch, fixing basically all the parenthesized patterns in the tree right now.  (Except for the addon SDK ones, pending resolution of comment 14.)  I wanted to "lock in" what I had already, and perhaps provide correct examples for anyone doing copypasta tests (and judging from the number of new instances of parenthesized patterns since I updated my tree, it looks like some people might be).

Actual patch to land once comment 14 is ironed out.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7386b1f4acfc

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69a9775dc5c9

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87bc0dcc8318

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b1eb8f29c7

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Followup mop-up to require that in destructuring patterns, an assignment target with a default value must not be parenthesized

So I was writing a blog post announcing the changes here, because some people :-( seem to be daily adding parenthesized destructuring patterns to our tree and it seemed worth explicit note, and I realized I missed an edge case here.  (Wheeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee...)

When a destructuring target has a default, we were doing a simple isKind check, which does *not* enforce the requirement that the node not be parenthesized:

  var [(a = 3)] = []; // BAD, a = 3 must not be parenthesized
  var { x: (p = 7) } = {}; // BAD, p = 7 must not be parenthesized

Do that, add tests, and put the nail in this bug-coffin.

Comment 22

[Tom S [:evilpie]]

Comment on attachment 8625031 [details] [diff] [review]
Followup mop-up to require that in destructuring patterns, an assignment target with a default value must not be parenthesized

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

::: js/src/tests/ecma_6/Expressions/destructuring-pattern-parenthesized.js
@@ +75,2 @@
>  checkError("var a, b; ({ a: ([b]) } = { a: [42] });", SyntaxError, SyntaxError);
> +checkError("var a, b; [(a = 5)] = [1];", SyntaxError, SyntaxError);

b is unused. Add one simpler version:
var a; ({a: (b = 7)} = {b: 1})

Comment 23

[Tom S [:evilpie]]

I think might be yet another problem with spreads: var [...(a)] = [1, 2, 3] is invalid in Andre's es6draft. I am however not sure why. I would think we end up in 12.14.5.1 for DestructuringAssignmentTarget, where we check IsValidSimpleAssignmentTarget, which should be true even with parentheses.

Comment 24

[André Bargull [:anba]]

VariableDeclaration uses destructuring binding pattern (13.3.3), not destructuring assignment (12.14.5).

Comment 25

[Tom S [:evilpie]]

Oh never mind, this was already fixed in this bug, my build was just old.

Comment 26

[Eric Faust [:efaust]]

Comment on attachment 8625031 [details] [diff] [review]
Followup mop-up to require that in destructuring patterns, an assignment target with a default value must not be parenthesized

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

Ya. Looks like review was stolen, but I read it anyway, so whatever.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69a9775dc5c9
https://hg.mozilla.org/mozilla-central/rev/87bc0dcc8318
https://hg.mozilla.org/mozilla-central/rev/b4b1eb8f29c7

Comment 28

[Jeff Walden [:Waldo]]

I embloggenated this change:

http://whereswalden.com/2015/06/20/new-changes-to-make-spidermonkeys-and-firefoxs-parsing-of-destructuring-patterns-more-spec-compliant/

I don't know where in MDN these changes should be noted.  In new-in-41(?) notes for sure.

As for updating destructuring documentation, I'd simply be careful to write them in a way that *only* describes the standard, supported syntax.  If you do that, there's no point in saying "parenthesized versions of these things aren't allowed", because such will just be redundant with the other descriptions (people shouldn't assume a syntax works unless told it does).

Still need to make the mini-adjustments on the last patch to land it.  Maybe today, if I have a few minutes' downtime at some point and haven't decided I'm too dozy.

It won't surprise me if there's a trickle, or somewhat more, of addons that need to un-parenthesize (or differently-parenthesize) stuff to deal with this, a la ABP in bug 1176702.

Comment 29

[Kohei Yoshino [:kohei]]

I'll document this on the Firefox 41 site compat doc.

Comment 30

[Kohei Yoshino [:kohei]]

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

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04b0cd43c3b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b68947376b58

Comment 32

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dc9b15c377e9

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04b0cd43c3b0
https://hg.mozilla.org/mozilla-central/rev/b68947376b58

Comment 34

[Florian Scholz (Open Web Docs)]

Release notes
https://developer.mozilla.org/en-US/Firefox/Releases/41#JavaScript

> As for updating destructuring documentation, I'd simply be careful to write
> them in a way that *only* describes the standard, supported syntax.  If you
> do that, there's no point in saying "parenthesized versions of these things
> aren't allowed", because such will just be redundant with the other
> descriptions (people shouldn't assume a syntax works unless told it does).

I have not touched the main text as it indeed just describes standard behaviour across browsers normally.

Whenever we fix compat or compliance issues, we normally note these things in the compat section as foot notes. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Firefox-specific_notes

Comment 36

[Kohei Yoshino [:kohei]]

Correcting the version flag.