Closed Bug 1146136 Opened 9 years ago Closed 9 years ago

Parenthesized AssignmentPatterns are not a valid LHS

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: caitpotter88, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(3 files, 2 obsolete files)

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
sorry, "at least 2 reasons" is not true, it's more like "at least one reason" :)
Blocks: es6
Summary: Parenthesized AssignmentPatterns should be a syntax error → Parenthesized AssignmentPatterns are not a valid LHS
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch (obsolete) — Splinter Review
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 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.
Attachment #8614339 - Attachment is obsolete: true
Attachment #8614339 - Flags: review?(efaustbmo)
> > > +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.
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+
Attached patch Final patchSplinter Review
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
(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.
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
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.
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+
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
Keywords: addon-compat
Keywords: leave-open
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
Correcting the version flag.
Target Milestone: Future → mozilla41
You need to log in before you can comment on or make changes to this bug.