Parenthesized AssignmentPatterns are not a valid LHS

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: caitp, Assigned: Waldo)

Tracking

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

Trunk
mozilla41
x86
Mac OS X
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
sorry, "at least 2 reasons" is not true, it's more like "at least one reason" :)
Blocks: 694100
(Reporter)

Updated

2 years ago
Summary: Parenthesized AssignmentPatterns should be a syntax error → Parenthesized AssignmentPatterns are not a valid LHS
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 8614339 [details] [diff] [review]
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.
Attachment #8614339 - Flags: review?(efaustbmo)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED

Comment 3

2 years ago
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.
(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.
Created attachment 8614782 [details] [diff] [review]
Updated so [([y])] = [2] and [({z})] = [{z: 3}] are syntax errors
Attachment #8614782 - Flags: review?(efaustbmo)
Attachment #8614339 - Attachment is obsolete: true
Attachment #8614339 - Flags: review?(efaustbmo)

Comment 6

2 years ago
> > > +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.
(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.
Created attachment 8614989 [details] [diff] [review]
Once more, with feeling
Attachment #8614989 - Flags: review?(efaustbmo)
Attachment #8614782 - Attachment is obsolete: true
Attachment #8614782 - Flags: review?(efaustbmo)
Either the spec is harder to read now than in ES3 days, or my brain ossified at some point in the intervening years.
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.
Attachment #8614989 - Flags: review?(efaustbmo) → review+
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
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.
(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.
(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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7386b1f4acfc
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7386b1f4acfc
Depends on: 1176411

Comment 18

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

Comment 19

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

Comment 20

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b1eb8f29c7
Created 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

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.
Attachment #8625031 - Flags: review?(efaustbmo)
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})
Attachment #8625031 - Flags: review?(efaustbmo) → review?(evilpies)
Attachment #8625031 - Flags: review?(evilpies) → review+
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

2 years ago
VariableDeclaration uses destructuring binding pattern (13.3.3), not destructuring assignment (12.14.5).
Oh never mind, this was already fixed in this bug, my build was just old.
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.
Attachment #8625031 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/69a9775dc5c9
https://hg.mozilla.org/mozilla-central/rev/87bc0dcc8318
https://hg.mozilla.org/mozilla-central/rev/b4b1eb8f29c7

Updated

2 years ago
Depends on: 1176702
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.
Keywords: dev-doc-needed
Target Milestone: --- → Future
Depends on: 1177304
I'll document this on the Firefox 41 site compat doc.
Keywords: site-compat
https://developer.mozilla.org/en-US/Firefox/Releases/41/Site_Compatibility#JavaScript

Updated

2 years ago
Keywords: addon-compat

Comment 31

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/04b0cd43c3b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b68947376b58
https://hg.mozilla.org/releases/mozilla-aurora/rev/dc9b15c377e9
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/04b0cd43c3b0
https://hg.mozilla.org/mozilla-central/rev/b68947376b58
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
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
Keywords: dev-doc-needed → dev-doc-complete

Updated

2 years ago
Duplicate of this bug: 1155141
Correcting the version flag.
status-firefox41: --- → fixed
status-firefox42: fixed → ---
Target Milestone: Future → mozilla41
You need to log in before you can comment on or make changes to this bug.