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" :)

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

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

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.

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.

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.

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

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.

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

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.

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

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

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

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.