932080 - Support default values in destructuring

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

For example:

> let { foo = 10, bar = 5 } = { foo: 12 };
> console.log(foo); // 12
> console.log(bar); // 5

Supporting Grammar:

> BindingPattern :
>   ObjectBindingPattern

> ObjectBindingPattern :
>   { BindingPropertyList }

> BindingPropertyList :
>   BindingProperty
>   BindingPropertyList , BindingProperty

> BindingProperty :
>   SingleNameBinding

> SingleNameBinding :
>   BindingIdentifier Initialiser(opt)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jason, can you give me pointers to the locations in the code I will have to change? I feel like the parsing will be pretty easy, but I don't have much experience elsewhere in spider monkey.

Comment 2

[André Bargull [:anba]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> I feel like the parsing will be pretty easy, but I don't have much experience elsewhere in spider monkey.

I may turn out to be less easy than expected due to the "CoverInitialisedName" production in "12.1.5  Object Initialiser". For example `({a = 0} = {}) => {}` which is first an invalid ObjectInitialiser `{a = 0}`, then a destructuring assignment `{a = 0} = {}` and only finally an ArrowFunction. Or do you mean to only add support for default values in BindingPattern instead of BindingPattern + AssignmentPattern?

Comment 3

[Jason Orendorff [:jorendorff]]

Let's not do this piecemeal. Filed bug 933296 and I'll give it some thought tomorrow.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'm unassigning myself because I won't be working on this anytime soon.

Comment 5

[Anthony Ricaud (:rik)]

This would allow some really nice constructions for named, optional parameters with default values. This would make boolean parameters way more readable at call sites.

function foo({bar = false, baz = true}) {}

foo({bar: true});

Comment 6

[Arpad Borsos [:Swatinem]]

Taking this.

The way this is implemented, it first parses the AssignmentPattern using the normal object/array path, and then checks via checkDestructuring that it is in fact a valid AssignmentPattern.
So supporting things like `[a = 10] = []` and `({a: b = 10} = {})` is trivial. However the pattern `({a = 10} = {})` fails in the normal object parser already.
How about I first get the "easy" case going, and then tackle the other case as a followup?

My second question is concerning the Parser API:

* for `[a,b] = []`, we get an ArrayPattern, with a bunch of Identifiers in "elements".
* for `({a, b: c} = {})`, we get an ObjectPattern with a bunch of Propery nodes in "properties" that have a "key" and a "value".

With no further specialization, I would get an AssignmentExpression as the "elements" of the ArrayPattern or as the "value" of the ObjectPatterns "properties".

This seems kind of meh. Would be nicer if we had an "AssignmentElement" with "id" and "init" (may be null) and a "AssignmentProperty" with "property", "id", "shorthand" and "init" (may be null).

Any ideas? How about backwards compatibility? How about other implementations? (I haven’t looked at what esprima does yet)

Comment 7

[Arpad Borsos [:Swatinem]]

Attachment: part1: support default values in destructuring of array and full objects

So far for the easy part.

part2 will be also supporting the more complex case with shorthands, such as ({a = 10} = {})
part3 would be updating Reflect.parse like I suggested in comment 6.

I will jump on IRC to discuss those further once I have some time.

Comment 8

[Arpad Borsos [:Swatinem]]

Attachment: part2: support destructuring default for shorthand objects

Ok, now you can do destructuring defaults with shorthand objects such as |({a = 10} = {})|.
But as expected, I broke normal ObjectLiterals, as you can now also do |({a = 10})| as an object literal.
How would I best fix that?

Comment 9

[Arpad Borsos [:Swatinem]]

Attachment: part3: change Reflect.parse to have an Assignment{Property,Element} type

I’m using "pattern" instead of "id", since they can be recursive.
Also, I will add reflect-parse tests later, just attaching this for some early feedback

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8456277 [details] [diff] [review]
part1: support default values in destructuring of array and full objects

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

Well, I suppose we can take the easy bits first, since the patch is already done. Looks fantastic. Just needs some tests.

r=me with the added tests.

::: js/src/jit-test/tests/basic/destructuring-default.js
@@ +19,5 @@
> +
> +assertEqArray(obj([]), [1, 2, 3, 4, 5, 6]);
> +assertEqArray(obj([2, 3, 4, 5, 6, 7, 8, 9]), [2, 3, 4, 5, 6, 7]);
> +assertEqArray(obj([undefined, 0, false, null, "", undefined]), [1, 0, false, null, "", 6]);
> +assertEqArray(obj([0, false]), [0, false, 3, 4, 5, 6]);

Order is observable here, I think. (?) Add something like:

    var {a: a = 1, b: b = 2} = {
        get a() {
            assertEq(a, undefined);
            assertEq(b, undefined);
            return undefined;
        },
        get b() {
            assertEq(a, 1);
            assertEq(b, undefined);
        }
    };

(and please confirm that that's how the spec actually says it's supposed to work).

Add tests where the destructuring is not in a var but in a plain old ExpressionStatement, the head of a catch block, or a function argument.

Add tests where the target of the default value Initializer is itself an ObjectAssignmentPattern or ArrayAssignmentPattern:
    [{a} = {}] = []
    [[a, b] = defaults] = []

including the case where there's nesting:
    {options: {glitter: glitter = false, rainbows: rainbows = false} = {}} = arg

Test that in such cases, it's a TypeError if the Initializer doesn't produce an object/iterable.

Test these weird cases:
    [a=1, a=2] = [];
    [a=someobj, a.x=0] = [];

Test with destructuring const, both in global scope and inside a function:
    const [a=1, b=2] = whatever;

Test that even when there's nothing to deconstruct, the Initializer is evaluated if the value to assign would otherwise be undefined:
    ({x: {} = f()}) = {}   // f should be called
In that case, also test that it is *not* a TypeError if f() returns null or undefined.

However in array destructuring, it is.
    [[]=f()] = [];  // TypeError if f returns anything that's not iterable

Also please write tests confirming that various syntax errors are still syntax errors and don't crash us.
For example, test that an Initializer is not allowed on an AssignmentRestElement:

    [...others = null] = [] //SyntaxError

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Arpad Borsos (Swatinem) from comment #8)
> Ok, now you can do destructuring defaults with shorthand objects such as
> |({a = 10} = {})|.
> But as expected, I broke normal ObjectLiterals, as you can now also do |({a
> = 10})| as an object literal.
> How would I best fix that?

The patch looks fantastic.

I just wrote bug 933296 comment 3 which explains my preferred approach. Thanks to André Bargull for the pointer.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8457008 [details] [diff] [review]
part3: change Reflect.parse to have an Assignment{Property,Element} type

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

This looks great.

::: js/src/jit-test/tests/basic/destructuring-default.js
@@ +37,5 @@
>  
> +function objnested(obj) {
> +  var {a = 1, b: [b = 2] = [], c: {c: [c]} = {c: [3]}} = obj;
> +  return [a,b,c];
> +}

Oh hi, it looks like these are some of the tests I asked for in my review of part 1. They just ended up in the wrong patch, that's all. :)

Comment 13

[Arpad Borsos [:Swatinem]]

Attachment: part1: support default values in destructuring of array and full objects

There was a bug exposed by "let"
Oh and this is on top of my destructuring-rest patch, in case you didnt notice.

Comment 14

[Arpad Borsos [:Swatinem]]

https://tbpl.mozilla.org/?tree=Try&rev=72cc2129b209
part1 + all the prerequisite patches.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 8459149 [details] [diff] [review]
part1: support default values in destructuring of array and full objects

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

Great work. r=me.

My only comments involve giving meaningful names to ParseNode variables/arguments. It would be OK to fix those in a follow-up patch, except the new ones you're adding in this patch (in EmitDefault and Parser<>::checkDestructuring).

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3081,5 @@
>      // by an assignment op.
>      if (pn->isKind(PNK_SPREAD))
>          pn = pn->pn_kid;
> +    else if (pn->isKind(PNK_ASSIGN))
> +        pn = pn->pn_left;

Feel free to rename the "pn" argument to "target" (see comment below).

@@ +3226,5 @@
> + * EmitDefault will check if the value on top of the stack is "undefined".
> + * If so, it will replace that value on the stack with the value defined by the expression in |pn|.
> + */
> +static bool
> +EmitDefault(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)

Please rename "pn" to "expr" or "defaultExpr".

::: js/src/frontend/Parser.cpp
@@ +3169,5 @@
>  
>      if (left->isKind(PNK_ARRAY)) {
>          for (ParseNode *pn = left->pn_head; pn; pn = pn->pn_next) {
>              if (!pn->isKind(PNK_ELISION)) {
> +                ParseNode *pn2 = pn;

Good change. Thank you.

There's a lot of old code in js/src/frontend that uses names like "pn2", but we all now prefer meaningful variable names.

So please rename "pn" to "element" (it's a BindingElement or AssignmentElement, except when it's an Elision).
And rename "pn2" to "target" (the ES6 spec doesn't really have a nice name for this; it's either a LeftHandSideExpression, a SingleNameBinding, or a BindingPattern).

A separate changeset would be ok for the renaming. r=me.

@@ +3203,5 @@
>              MOZ_ASSERT(member->isKind(PNK_COLON) || member->isKind(PNK_SHORTHAND));
>              ParseNode *expr = member->pn_right;
>  
> +            if (expr->isKind(PNK_ASSIGN))
> +                expr = expr->pn_left;

"binding" would probably be better than "expr" here, too. Your call.

Comment 16

[Arpad Borsos [:Swatinem]]

Comment on attachment 8459149 [details] [diff] [review]
part1: support default values in destructuring of array and full objects

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

::: js/src/frontend/Parser.cpp
@@ +3203,5 @@
>              MOZ_ASSERT(member->isKind(PNK_COLON) || member->isKind(PNK_SHORTHAND));
>              ParseNode *expr = member->pn_right;
>  
> +            if (expr->isKind(PNK_ASSIGN))
> +                expr = expr->pn_left;

I made that "target" to match the naming in the ARRAY case above, and the case in BCE.cpp

Comment 17

[Arpad Borsos [:Swatinem]]

Another case for bug 933296:
`[(a = 10)] = []` and `{a: (a = 10)} = {}` should all be syntax errors!

`{(a = 10)} = {}` <- that case is already caught

Comment 18

[Jason Orendorff [:jorendorff]]

Does this still need work? How close is it?

Comment 19

[Arpad Borsos [:Swatinem]]

As a matter of fact, I’m on vacation starting this weekend, so I will have time to work on this again.
I have rebased patch 1, should be ready after I verify that I didnt break any tests.
I will then probably move the Reflect.parse patch in front, finishing it up with tests.
The more complex part 2 that depends on the better parser will need work.

Comment 20

[Arpad Borsos [:Swatinem]]

Attachment: part0: change Reflect.parse to have an Assignment{Property,Element} type

Comment 21

[Arpad Borsos [:Swatinem]]

Attachment: part1: support default values in destructuring of array and full objects

There was a lot that changed, so I would feel more comfortable with another r+ :-)

Comment 22

[Arpad Borsos [:Swatinem]]

Comment on attachment 8493118 [details] [diff] [review]
part0: change Reflect.parse to have an Assignment{Property,Element} type

@ariya @marijnh:
Requesting feedback from you guys whether you are fine with the new AST syntax here, or do you propose something else?

Comment 23

[Arpad Borsos [:Swatinem]]

Btw, try build is here: https://tbpl.mozilla.org/?tree=Try&rev=075751a95f04

Also regarding the new Reflect.parse AST:

For ArrayPatterns, elements now have a new type:
```
{
  type: "AssignmentElement",
  target: any assignment pattern,
  default: an arbitrary expression OR null
}
```

For ObjectPatterns, the properties are:
```
{
  type: "AssignmentProperty",
  property: Identifier,
  target: any assignment pattern,
  default: arbitrary expression OR null,
  shorthand: Boolean (if true, you should ignore "target" [in this implementation its the same as "property", but it might as well be null])
}
```

Comment 24

[Arpad Borsos [:Swatinem]]

Comment on attachment 8493118 [details] [diff] [review]
part0: change Reflect.parse to have an Assignment{Property,Element} type

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

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +683,5 @@
>          // variable declarations in blocks
>          assertDecl("var " + pattSrcs[i].join(",") + ";", varDecl(pattPatts[i]));
>  
>          assertGlobalDecl("let " + pattSrcs[i].join(",") + ";", varDecl(pattPatts[i]));
> +        assertLocalDecl("let " + pattSrcs[i].join(",") + ";", letDecl(pattPatts[i]));

See also the case above, I had to change "var" to "let". I think thats fallout from a different patch before mine. Too bad this test does not actually run on tbpl. :-(

Comment 25

[Marijn Haverbeke]

The changes to the AST format seem reasonable, at a glance, but Acorn doesn't implement default values yet, so I have never really thought deeply about the issue. I'd love to hear @ariya chime in as well.

Comment 26

[Ingvar Stepanyan (RReverser)]

Hm. So now we put information about assignment `target` directly into AssignmentElement/AssignmentProperty instead of AssignmentExpression?

How should AST look like for cases like `[a,b]=[0,1,2]` - Should we both store array on right side and duplicate Literal(0) and Literal(1) in AssignmentElement.target? Or are we dropping array on the right side completely?

Comment 27

[Arpad Borsos [:Swatinem]]

(In reply to me from comment #26)
> How should AST look like for cases like `[a,b]=[0,1,2]` - Should we both
> store array on right side and duplicate Literal(0) and Literal(1) in
> AssignmentElement.target? Or are we dropping array on the right side
> completely?

The "target" is the identifier (or sub-pattern, or lhs expression) the values will be bound to.
The changes just concern the left hand side, they have nothing to do with the right hand side.
So your simple example would become:
```
{type:"AssignmentExpression", operator:"=", left:
{type:"ArrayPattern", elements:[
{type:"AssignmentElement", target:{type:"Identifier", name:"a"}, default:null},
{type:"AssignmentElement", target:{type:"Identifier", name:"b"}, default:null}
]}, right:
{type:"ArrayExpression", elements:[{type:"Literal", value:1}, {type:"Literal", value:2}, {type:"Literal", value:3}]}}
```

With a more complex target and a default, for `[x.call().a = 10] = a`:
```
{type:"AssignmentExpression", operator:"=", left:
{type:"ArrayPattern", elements:[
{type:"AssignmentElement", target:
{type:"MemberExpression", object:{type:"CallExpression", callee:{type:"MemberExpression", object:{type:"Identifier", name:"x"}, property:{type:"Identifier", name:"call"}, computed:false}, arguments:[]}, property:{type:"Identifier", name:"a"}, computed:false}, default:{type:"Literal", value:10}}
]}, right:
{type:"Identifier", name:"a"}}
```

Comment 28

[Ingvar Stepanyan (RReverser)]

Makes sense. Still thinking... Shouldn't/Can't we leave as-is and preserve backward compatibility by leaving regular Identifier nodes in place of elements that don't have default values and use AssignmentElement only when it has the default value? Then AssignmentElement would have a same sense as AssignmentExpression; in that case it can even have `left` and `right` props to preserve similarity as ArrayPattern does to ArrayExpression and ObjectPattern does to ObjectExpression.

Comment 29

[Arpad Borsos [:Swatinem]]

(In reply to me from comment #28)
> Shouldn't/Can't we leave as-is and preserve
> backward compatibility by leaving regular Identifier nodes in place of
> elements that don't have default values and use AssignmentElement only when
> it has the default value?

I think that would make things more complicated.

The only way I think would retain backwards compat is if we added a new array "defaults" to ArrayPattern/ObjectPattern which had the corresponding defaults for the elements/properties.
That also seems kind of strange.

Problem is that ES6 (or at least parts of it) is still subject to change and we should be able to break these things as we see fit.
But there are a lot of transpilers already out there that work with the current syntax/ast.

Maybe we should move this bikeshedding to a mailing list instead of this bug?

Comment 30

[Marijn Haverbeke]

>> and use AssignmentElement only when it has the default value?

> I think that would make things more complicated.

Why do you think so? This looks like a simple, clean solution with an existing precedent (regular expressions are only wrapped when needed, as Ingvar pointed out, and this really seems a similar situation).

Comment 31

[Jason Orendorff [:jorendorff]]

Did the discussion on the Reflect.parse() output bike shed color ever actually happen somewhere?

I'll review this in more detail tomorrow, but in general, nodes that cover the same range of source characters as their sole child node should not be reflected.

I think it makes sense to treat the production

    AssignmentElement :
        DestructuringAssignmentTarget Initializer_opt

as two productions:

    AssignmentElement :
        DestructuringAssignmentTarget
        DestructuringAssignmentTarget Initializer

and in the first case, at least, elide the AssignmentElement node.

Comment 32

[Arpad Borsos [:Swatinem]]

Well, it didn’t result in as lively a discussion I hoped :-(

The last proposal I did was this, also unifying with formal parameters to functions:

```
PropertyPattern:
* property: Identifier OR Literal (it might also be ok to use "key" here, just like with Property)
* target: LHSExpression
* default: Expression OR null

ElementPattern:
* target: LHSExpression
* default: Expression OR null

RestElement:
* target: LHSExpression (but *not* Pattern)

(note that these three are *not* Patterns. you can’t have them on the LHS of expressions; just the parent Array/ObjectPatterns)

ArrayPattern: Pattern:
* elements: Array<ElementPattern OR RestElement>

ObjectPattern: Pattern:
* properties: Array<PropertyPattern>

FunctionExpression/Declaration:
* params: Array<ElementPattern OR RestElement>

Pattern: LHSExpression
(a Pattern is a valid LHSExpression)
(also, a LHSExpression is a valid Pattern inside Assignments, but not inside formal parameters or bindings)
```

So you propose to just "unwrap" the ElementPattern if there is no default, so it will just be the value of "target", which is most cases is just an Identifier, and sometimes a sub-Pattern.

That sure has the least changes compared to ES5. Should I just go ahead with that? (minus the changes to Functions formal parameters; which should be done in a separate bug maybe)

Comment 33

[Ingvar Stepanyan (RReverser)]

In any case, I like that we agree that `defaults` and `rest` Function properties should be deprecated in favor of patterns and RestElement. Is there already separate bug for this?

Regarding Pattern nodes - I'd propose to reuse as much of existing interfaces as possible for easier subclassing (DRY FTW).

This can be described in the following way:

```
interface Pattern <: Expression {}

interface AssignmentPattern <: Pattern, AssignmentExpression {
  // operator: "=";
  left: Pattern;
}

interface ObjectPattern <: Pattern, ObjectExpression {
  properties: [ PropertyPattern ];
}

interface PropertyPattern <: Pattern, Property {
  // kind: "init";
  value: Pattern;
}

/*
remove:

 - interface SpreadElement {
 -   argument: Expression;
 - }

in favor of:
*/

enum UnaryOperator {
  "-" | "+" | "!" | "~" | "typeof" | "void" | "delete" | "..."
}

// special sub-case of unary expression
interface SpreadExpression <: UnaryExpression {
  // operator: "...";
  // prefix: true;
}

interface SpreadPattern <: Pattern, SpreadExpression {
  argument: Pattern;
}

interface Function <: Node {
    id: Identifier | null;
    params: [ Pattern ];
/*
remove:

  - defaults: [ Expression ];
  - rest: Identifier | null;
*/
    body: BlockStatement | Expression;
    generator: boolean;
    expression: boolean;
}
```

Comment 34

[Arpad Borsos [:Swatinem]]

(In reply to Ingvar Stepanyan (RReverser) from comment #33)
> In any case, I like that we agree that `defaults` and `rest` Function
> properties should be deprecated in favor of patterns and RestElement. Is
> there already separate bug for this?

I filed bug 1093504 for that.

We still need to find a consensus about how the AST should look like.

> interface AssignmentPattern <: Pattern, AssignmentExpression {
>   // operator: "=";
>   left: Pattern;
> }

So the AssignmentPattern would be used in case of a default?
So that an ArrayPattern might have either Identifier (/LHSExpression) children or AssignmentPattern children where "right" is the default?

Well that would be similar to the nodes we have right now, but I would still like better naming than "left" "right" for this case.

Who would decide on which way to go forward? jorendorff?

Comment 35

[Ingvar Stepanyan (RReverser)]

> So that an ArrayPattern might have either Identifier (/LHSExpression) children or AssignmentPattern children where "right" is the default?

Yep, exactly.

> I would still like better naming than "left" "right" for this case.

I would like this naming to be different even for AssignmentExpression, but such thing is hard to change. Not sure that we need to create totally different node just for better naming.

Can you please let me know your thoughts on proposed Spread/Rest changes?

Comment 36

[Arpad Borsos [:Swatinem]]

(In reply to Ingvar Stepanyan (RReverser) from comment #35)
> Can you please let me know your thoughts on proposed Spread/Rest changes?

Changing SpreadExpression to a UnaryExpression with operator: "..." seems good considering JS is "stringly typed".

For the case of Rest, I would go with a different type though, since it is not an expression, but rather the opposite. So I would refer to my suggestion in comment 32: make it a "RestElement" (/"RestPattern") node with a "target".

Comment 37

[Ingvar Stepanyan (RReverser)]

Makes sense.

On other hand, any of Pattern node types is similar to it's Expression analogue while "it is not an expression, but rather the opposite".

So I thought it might be better to reflect similarity for Spread/Rest case, too, just like we have with Object/Array/etc. patterns<->expressions.

Comment 38

[Jason Orendorff [:jorendorff]]

Designing Reflect.parse output is an ongoing problem. There are several design principles that are constantly at odds. It's kind of unfortunate how many degrees of freedom there are. :-P

1. "AssignmentPattern" sounds good. It's kind of weird in that it sort of suggests that they can nest, which of course they can't. But OK.

2. "RestPattern" is all right but I would prefer to use "SpreadElement" and "RestElement" since (a) that's closer to what the spec says, and (b) by itself, a SpreadElement is not an expression and a RestElement is not a pattern.

3. I definitely don't think SpreadElements should be "UnaryExpressions"!

Comment 39

[Jason Orendorff [:jorendorff]]

Comment on attachment 8493119 [details] [diff] [review]
part1: support default values in destructuring of array and full objects

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

I'm sorry for the slow review here.

This should have a test for a destructuring pattern followed by an initializer, as in
    [{x, y} = {x: 1, y: 2}] = [undefined];

Apart from that and the Reflect.parse format (which we should change as discussed in this bug), this looks good.

::: js/src/frontend/Parser.cpp
@@ +3205,5 @@
>              expr = member->pn_right;
>          }
> +        if (expr->isKind(PNK_ASSIGN)) {
> +            expr = expr->pn_left;
> +        }

Style nit: No curly braces here since the condition and the consequent each fit on a single line.

::: js/src/jit-test/tests/basic/destructuring-default.js
@@ +92,5 @@
> +testAll(testThrow);
> +
> +// XXX: Support for let blocks and expressions will be removed in bug 1023609.
> +// However, they test a special code path in destructuring assignment so having
> +// these tests here for now seems like a good idea.

+1, and another +1 for the nice comment. You can remove the "XXX".

Comment 40

[Jason Orendorff [:jorendorff]]

Comment on attachment 8493118 [details] [diff] [review]
part0: change Reflect.parse to have an Assignment{Property,Element} type

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

Clearing reviews on this one. I *think* we all have consensus on a different approach.

Comment 41

[Arpad Borsos [:Swatinem]]

Attachment: part0: Reflect.parse changes for destructuring

So I removed the changes to the AST format, now it supports the new destructuring things 
without any code change, yay.
The changes in the reflect-parse test however should make it easier to change the things 
afterwards. It was also less painful for me to rebase.

So without those changes, the AST for properties becomes a normal "Property" node, with 
key, value and shorthand. In case of a default, there will be a AssignmentExpression 
with left, right and operator '='.

Comment 42

[Ingvar Stepanyan (RReverser)]

Woohoo! But what about function parameters? Are we going to deprecate those extra `defaults` and `rest` properties in favor of this "common" way of handling defaults with `AssignmentExpression` and `RestElement`?

Comment 43

[Arpad Borsos [:Swatinem]]

I filed bug 1093504 about that some time ago.

Comment 44

[Jason Orendorff [:jorendorff]]

Comment on attachment 8533631 [details] [diff] [review]
part0: Reflect.parse changes for destructuring

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

Thanks!

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +239,5 @@
>  assertDecl("function f(a,b,c) { function b() { } }",
>             funDecl(ident("f"), [ident("a"),ident("b"),ident("c")], blockStmt([funDecl(ident("b"), [], blockStmt([]))])));
>  assertDecl("function f(a,[x,y]) { function a() { } }",
>             funDecl(ident("f"),
> +                   [ident("a"), arrPatt([assignElem(ident("x")), assignElem(ident("y"))])],

Please revert the places like this where you changed ident(_) to assignElem(ident(_)). The original should still work, and it's clearer. The assignProp() stuff looks good though.

Comment 45

[Arpad Borsos [:Swatinem]]

(In reply to Jason Orendorff [:jorendorff] from comment #44)
> Please revert the places like this where you changed ident(_) to
> assignElem(ident(_)). The original should still work, and it's clearer. The
> assignProp() stuff looks good though.

Would you mind if I just used the same hack as in assignProp to avoid the redundant ident()? (So it would just become assignElem("foo"))
That way it would lead to less churn if we decide to change the ast representation?

Comment 46

[Ingvar Stepanyan (RReverser)]

Just to clarify - so we agreed that old representations are left as-is and for default case

> interface AssignmentPattern <: Pattern, AssignmentExpression {
>   // operator: "=";
>   left: Pattern;
> }

is used as i.e. `Property.value`, right?

Comment 47

[Arpad Borsos [:Swatinem]]

Sorry for the delay.
With my patches, I get these results (these are with *no code change* whatsoever [patch 0])

js> Reflect.parse('[a] = o', {loc: false}).body[0].expression.left;
({loc:null, type:"ArrayPattern", elements:[{loc:null, type:"Identifier", name:"a"}]})
js> Reflect.parse('[a = 10] = o', {loc: false}).body[0].expression.left;
({loc:null, type:"ArrayPattern", elements:[{loc:null, type:"AssignmentExpression", operator:"=", left:{loc:null, type:"Identifier", name:"a"}, right:{loc:null, type:"Literal", value:10}}]})
js> Reflect.parse('({a}) = o', {loc: false}).body[0].expression.left;
({loc:null, type:"ObjectPattern", properties:[{loc:null, type:"Property", key:{loc:null, type:"Identifier", name:"a"}, value:{loc:null, type:"Identifier", name:"a"}, kind:"init", shorthand:true}]})
js> Reflect.parse('({a: b}) = o', {loc: false}).body[0].expression.left;
({loc:null, type:"ObjectPattern", properties:[{loc:null, type:"Property", key:{loc:null, type:"Identifier", name:"a"}, value:{loc:null, type:"Identifier", name:"b"}, kind:"init", shorthand:false}]})
js> Reflect.parse('({a: b = 10}) = o', {loc: false}).body[0].expression.left;
({loc:null, type:"ObjectPattern", properties:[{loc:null, type:"Property", key:{loc:null, type:"Identifier", name:"a"}, value:{loc:null, type:"AssignmentExpression", operator:"=", left:{loc:null, type:"Identifier", name:"b"}, right:{loc:null, type:"Literal", value:10}}, kind:"init", shorthand:false}]})
js> Reflect.parse('({a = 10}) = o', {loc: false}).body[0].expression.left;    
({loc:null, type:"ObjectPattern", properties:[{loc:null, type:"Property", key:{loc:null, type:"Identifier", name:"a"}, value:{loc:null, type:"AssignmentExpression", operator:"=", left:{loc:null, type:"Identifier", name:"a"}, right:{loc:null, type:"Literal", value:10}}, kind:"init", shorthand:true}]})

Comment 48

[Ingvar Stepanyan (RReverser)]

Hm… Why does it produce AssignmentExpression and not an AssignmentPattern on the left?

Comment 49

[Arpad Borsos [:Swatinem]]

(In reply to Ingvar Stepanyan (RReverser) from comment #48)
> Hm… Why does it produce AssignmentExpression and not an AssignmentPattern on
> the left?

Because that’s what spidermonkey uses internally and it just gets serialized, independently of where it is embedded.

Lets maybe move the discussion/bikeshedding to a different followup bug. I would like to get this feature landed and worry about the AST later.

Comment 50

[Ingvar Stepanyan (RReverser)]

Well, AST is direct part of this feature, and I'm interested in it in order to implement same behavior in Acorn. That's why I asked to confirm if we agreed on AssignmentPattern in Property.value / ArrayPattern.elements[] - if we do, then I have no more questions and just going to implement it.

Comment 51

[Ingvar Stepanyan (RReverser)]

Sorry, one more question (because I didn't see it in Reflect.parse logs above) - how do we handle case like following?

var {[computedPropName()]: localVarName = defaultValue} = obj;

This is allowed by spec, so wondering whether we are going to set `Property.computed = true` and just put `CallExpression("computedPropName")` in `AssignmentPattern.left` or if we are going to mark that left side has special meaning (is computed) in `AssignmentPattern` node as well?

Comment 52

[Arpad Borsos [:Swatinem]]

AssignmentProperty[Yield] :
IdentifierReference[?Yield] Initializer[In,?Yield]opt
PropertyName : AssignmentElement[?Yield]

Indeed! PropertyName can be a ComputedPropertyName as well.
I didn’t know, I’m not even sure we support it at all (yet).

Comment 53

[Arpad Borsos [:Swatinem]]

js> Reflect.parse("({[foo()]: a = 10} = {})", {loc: false}).body[0].expression;
({loc:null, type:"AssignmentExpression", operator:"=", left:{loc:null, type:"ObjectPattern", properties:[{loc:null, type:"Property", key:{loc:null, type:"ComputedName", name:{loc:null, type:"CallExpression", callee:{loc:null, type:"Identifier", name:"foo"}, arguments:[]}}, value:{loc:null, type:"AssignmentExpression", operator:"=", left:{loc:null, type:"Identifier", name:"a"}, right:{loc:null, type:"Literal", value:10}}, kind:"init", shorthand:false}]}, right:{loc:null, type:"ObjectExpression", properties:[]}})

This is how it looks now.

Comment 54

[Ingvar Stepanyan (RReverser)]

Ah, cool, thanks! The only deviation I see here is separate `ComputedName` type for computed property key - both Esprima and Acorn use `computed` boolean flag on property instead - but it's not related to this issue anyway.

Comment 55

[Jason Orendorff [:jorendorff]]

(In reply to Arpad Borsos (Swatinem) from comment #45)
> Would you mind if I just used the same hack as in assignProp to avoid the
> redundant ident()? (So it would just become assignElem("foo"))

Sure, that's fine.

Comment 56

[Arpad Borsos [:Swatinem]]

Parts 0+1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9df8c79bc4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8832848bf234

Comment 57

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

https://hg.mozilla.org/mozilla-central/rev/e9df8c79bc4b
https://hg.mozilla.org/mozilla-central/rev/8832848bf234

Comment 58

[Jason Orendorff [:jorendorff]]

Swatinem, are you interested in working on this some more?

Comment 59

[Arpad Borsos [:Swatinem]]

I did take a look at the parser for bug 933296 which is needed for this (to allow the destructuring syntax but make it a parse error when parsing object literals) but it was a bit over my head. Feel free to assign this to someone else.

Comment 60

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Comment 61

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8716165 [details] [diff] [review]
destructuring.diff

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

A sloppy WIP. Assuming that this approach turns out to be alright, I'm interested in lazily initializing the pendingCoverInitializedName table and making it into an ordered map (right now I'm not necessarily showing the first occurrence of a syntax error but it should be easy to fix in a finalized patch).

Comment 62

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8716165 [details] [diff] [review]
destructuring.diff

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

::: js/src/frontend/Parser.cpp
@@ +9077,5 @@
> +            if (!seenShorthandDefault)
> +                if (!pc->pendingCoverInitializedName.putNew(literal,
> +                    tokenStream.currentToken().pos.begin))
> +                    return null();
> +            seenShorthandDefault = true;

This should be moved over and put in a block.

Comment 63

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Posting as a progress update. This approach seems to cover all cases and catches errors nicely enough, however, it's not elegant. I'm going to try using an RAII pattern where checks happen in the destructor of a CoverInitializedName object instead of checking with if statements after every call of assignExpr.

Comment 64

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Comment 65

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Perhaps naively, I believe this is about where the patch should be. There is an RAII style class for deferring parse errors, enforcing that they are always handled, which should catch all possible misuses of "CoverInitializedName" or default shorthand destruction.

Comment 66

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8718929 [details] [diff] [review]
destructuring.diff

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

::: js/src/frontend/Parser.h
@@ +372,5 @@
>  enum PropListType { ObjectLiteral, ClassBody, DerivedClassBody };
>  enum class PropertyType {
>      Normal,
>      Shorthand,
> +    ShorthandDefault,

I should probably change this to "CoverInitializedName" to match the non-terminal's name as it appears in the spec.

Comment 67

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8718929 [details] [diff] [review]
destructuring.diff

Still adding more test cases, but I feel okay about the approach. Interested to hear your thoughts.

Comment 68

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8718929 [details] [diff] [review]
destructuring.diff

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

Need to tighten up a few more things....

Comment 69

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

https://www.youtube.com/watch?v=lydBPm2KRaU#t=1m But seriously, yell at me about holes you notice in the test cases.

Comment 70

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8719060 [details] [diff] [review]
destructuring.diff

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

::: js/src/frontend/Parser.cpp
@@ +5988,5 @@
>      // point of view).  No block-related work complicates matters, so delegate
>      // to Parser::declaration.
>      if (tt == TOK_VAR) {
>          tokenStream.consumeKnownToken(tt, TokenStream::Operand);
> +        // TODO: foopie

lol, wat? I think I'd meant to take a closer look at this section of code earlier in the patch.

@@ +6361,5 @@
>              caseExpr = null();  // The default case has pn_left == nullptr.
>              break;
>  
>            case TOK_CASE:
> +            caseExpr = expr(InAllowed, yieldHandling, TripledotProhibited, &possibleError);

Looks like I forgot to handle this! Fixing on my end.

Comment 71

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8719060 [details] [diff] [review]
destructuring.diff

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

::: js/src/frontend/Parser.cpp
@@ +3979,5 @@
> +}
> +
> +template <typename ParseHandler>
> +void
> +Parser<ParseHandler>::PossibleError::setResolved() {

Arg, style.

Comment 72

[Morgan Phillips [:mrrrgn]]

I forgot to support forOf completely:

var users = [
  { user: "Name1" },
  { user: "Name2", age: 2 },
  { user: "Name2" },
  { user: "Name3", age: 4 }
];

[for ({user, age = "DEFAULT AGE" } of users) console.log(user, age)];

Adding now....

Comment 73

[Morgan Phillips [:mrrrgn]]

Well, scratch that. While I was adding it I found a comment that led me to bug 980828. This falls under the scope of that, so I'll see about stealing (at least this section) of that bug after this one is squished.

Comment 74

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8719060 [details] [diff] [review]
destructuring.diff

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

::: js/src/frontend/Parser.cpp
@@ +3968,5 @@
> +template <typename ParseHandler>
> +void
> +Parser<ParseHandler>::PossibleError::setPending(ParseReportKind kind, unsigned errorNumber,
> +                                                bool strict) {
> +    // If we report an error later, we'll do it from the position where we set

It's probably wise to make it impossible call setPending twice on the same PossibleError.

@@ +6224,5 @@
>              MOZ_ASSERT(!letStmt);
>              pn1 = null();
>              pn2 = target;
>  
> +            if (!checkAndMarkAsAssignmentLhs(pn2, PlainAssignment, nullptr))

Needs a /* possibleError */ comment for the nullptr.

@@ +8396,4 @@
>      if (!rhs)
>          return null();
>  
> +    if (possibleError.maybeReport())

!rhs should come after here. Otherwise we could needlessly crash a debug build.

Comment 75

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8719060 [details] [diff] [review]
destructuring.diff

Good better best, never let it rest, until your good is your better, and your better is your best.

Comment 76

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Hmm, I think this is the wippiest WIP I've posted. Me IRL https://www.youtube.com/watch?v=O_05qJTeNNI

Comment 77

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Comment 78

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

`hg addremove`

Comment 79

[Jason Orendorff [:jorendorff]]

Comment on attachment 8720874 [details] [diff] [review]
destructuring.diff

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

I will really look at this tomorrow, but for now, a few nit-picky comments:

::: js/src/frontend/Parser.h
@@ +283,5 @@
>      {
>          prs->pc = this;
>          if (sc->isFunctionBox())
>              parseUsingFunctionBox.emplace(prs->context, sc->asFunctionBox());
> +

Looks like you accidentally added a blank line here.

@@ +418,5 @@
>      };
>  
> +    // A class for temporarily stashing errors while parsing continues. Possible
> +    // errors _must_ be resolved before going out of scope, otherwise
> +    // an assertion will be triggered.

The comment also should say

- why you'd want to temporarily stash an error while parsing continues, because
  that's going to sound crazy to people at first
- what sort of errors we're really talking about here
- when and how to use this class

@@ +432,5 @@
> +        Parser<ParseHandler>& parser_;
> +        bool checked_, strict_;
> +
> +        public:
> +          PossibleError(Parser<ParseHandler>& parser);

In C++, single-argument constructors are super weird. C++ can call them to autoconvert values. Like, if you tried to pass a `Parser<ParseHandler>&` to a function, but the type of the argument is `PossibleError`, then C++ would auto-call the constructor to create a temporary `PossibleError` object for that function call.

Sometimes that's nice, but in this case we don't want C++ to autoconvert Parser& values to PossibleError objects.

Adding `explicit` before this constructor's declaration will make it just a regular old constructor, and C++ will not call it implicitly. Of course it'll still use it when you actually say something like:

    PossibleError pe(parser);

There's a static analysis that complains if you have a non-explicit single-argument constructor (unless you mark it with MOZ_IMPLICIT). So I think if you actually landed this, something would turn red.

@@ +444,5 @@
> +          // false.
> +          bool maybeReport();
> +
> +          PossibleError& operator=(PossibleError& other) {
> +              if (this != &other) {

I see what you did there!! But in this case we should probably just ASSERT that the user isn't doing this, because it would be really weird.

@@ +447,5 @@
> +          PossibleError& operator=(PossibleError& other) {
> +              if (this != &other) {
> +                  // We should never allow fields to be copied between instances
> +                  // pointing to differnt underlying parsers.
> +                  MOZ_ASSERT(&parser_ == &other.parser_);

Nice assertion. Typo in the comment: "differnt". Or maybe that's just how it's spelled in Kentucky? (OH BURN - tennessee-on-kentucky burn)

Comment 80

[Jason Orendorff [:jorendorff]]

Comment on attachment 8720874 [details] [diff] [review]
destructuring.diff

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

We already talked about weakening the assertion so that .ignore() is not required in so many places. In fact I think you only need one of .ignore() / .setResolved().

The convention that we have an error on *false* returns is very strong throughout SpiderMonkey, so please change the name of .maybeReport() to .checkForExprErrors() or something and flip it to return false if it reported an error.

Instead of changing every expr() and assignExpr() call site, please add a second method, also called assignExpr(), without the possibleError argument, that just does

    PossibleError possibleError(*this);
    Node e = assignExpr(inHandling, yieldHandling, TripledotProhibited, &possibleError);
    if (!e || !possibleError.checkForExprErrors())
        return null();
    return e;

and a similar method for expr(). This allows most call sites to remain unchanged. The exceptions are the places where expr() calls assignExpr() and primaryExpr() calls exprInParens().

destructuringExpr() and friends shouldn't take a possibleError argument. Instead they should create their own local PossibleError and should always ignore, because if you're in destructuringExpr(), shorthand defaults are definitely OK.

checkDestructuringPattern() shouldn't take a possibleError argument. Instead, I think there are only three places where a possibleError is ignored: destructuringExpr(), assignExpr() for TOK_ARROW, and assignExpr() for assignment.

That'll do for now. This is actually in quite good shape - I think the approach is solid, and victory is closer than it seems.

::: js/src/frontend/Parser.cpp
@@ +1368,5 @@
>          MOZ_ASSERT(type == ExpressionBody);
> +        PossibleError possibleError(*this);
> +        Node kid = assignExpr(inHandling, yieldHandling, TripledotProhibited,
> +                              &possibleError);
> +        if (possibleError.maybeReport() || !kid)

The order of the two tests bothers me because if we ever forget to clear possibleError, so that we have both a possibleError and an actual parse error, we can end up reporting the possiblError even though it isn't really an error. The error message could be wrong, maybe even the wrong type. This is better:

    Node kid = assignExpr(...);
    if (!kid || !possibleError.checkForExprErrors())
        return null();

@@ +2174,5 @@
> +                Node destruct = destructuringExprWithoutYield(yieldHandling, &data,
> +                                                              &possibleError,
> +                                                              tt, JSMSG_YIELD_IN_DEFAULT);
> +
> +                if (possibleError.maybeReport() || ! destruct)

Style nit: no space after `!`.

@@ +4696,2 @@
>  
>      if (initialDeclaration && forHeadKind) {

The first thing we should do if we get into this if-block is possibleError.ignore(), because in this case, destructuring is definitely OK. The other changes in this block then become unnecessary.

::: js/src/tests/ecma_6/Object/destructuring-shorthand-defaults.js
@@ +6,5 @@
> +// e.g. |{x=1, y=2} = {}| properly raises a syntax error within an object
> +// literal. As per ES6 12.2.6 Object Initializer, "NOTE 3."
> +
> +const ERROR_STATEMENTS = [
> +    // expressions

Please add a test for compound assignment:  `({x} += {})` should be a ReferenceError (not a SyntaxError -- don't ask me why).

Comment 81

[Jason Orendorff [:jorendorff]]

Comment on attachment 8720874 [details] [diff] [review]
destructuring.diff

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

More review comments! Enjoy!

::: js/src/frontend/Parser.cpp
@@ +8005,5 @@
>          MOZ_ASSERT(!tokenStream.isCurrentTokenAssignment());
> +        if (possibleError)
> +            *possibleError = possibleErrorInner;
> +        else
> +            possibleErrorInner.ignore();

Code that explicitly handles cases that can never occur is bad. It's a "head fake". So let's either (a) actually pass a null possibleError pointer sometimes so that this `if` is alive, or (b) accept that it's never null, kill this if-statement, and always assign.

Separately: instead of overloading PossibleError::operator=(), consider using a plain old method. (EIBTI. It is much easier for hackers to grep the code for a method name, when they see something funny and say "I wonder where this is used...")

(You could consider changing the method to ignore new possible errors when there's already an earlier possible error. That way, we always report the first error in the source code if there are two. But this is kind of a waste of time so I probably shouldn't mention it.....)

::: js/src/tests/ecma_6/Object/destructuring-shorthand-defaults.js
@@ +15,5 @@
> +    "({x=1} = {y=1});",
> +    "({x: y={z=1}}={})",
> +    "({x=1}),",
> +    "({z=1}={}, {w=2}, {e=3})=>{};",
> +    "({z={x=1}})=>{};",

To these SyntaxError tests, please add an arrow nested in a bad object literal:   "({x = ({y=1}) => y})"

Also an arrow that's busted because of extra parentheses:  "(({x=1})) => x"

And an assignment that's busted because of extra parentheses: "({x = 1}) = {x: 2};" but this one has to be a ReferenceError per the standard.

Comment 82

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Comment 83

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Comment 84

[Morgan Phillips [:mrrrgn]]

Attachment: destructuring.diff

Comment 85

[Jason Orendorff [:jorendorff]]

Comment on attachment 8723732 [details] [diff] [review]
destructuring.diff

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

r=me ...if I understand correctly.

::: js/src/frontend/Parser.cpp
@@ +4006,5 @@
> +        other->offset_        = offset_;
> +        other->reportKind_    = reportKind_;
> +        other->errorNumber_   = errorNumber_;
> +        other->strict_        = strict_;
> +        other->state_         = state_;

Hmm. Is this correct? What if `other` already has an earlier possible error, and `this` doesn't? Why doesn't this clobber the pending error?

If this isn't a bug, then I'm really confused, so let's talk it through...

@@ +4686,5 @@
>      Node pattern;
>      {
>          pc->inDeclDestructuring = true;
> +        // No possible error is required because we already know we're
> +        // destructuring.

Style nit: Blank line before comment.

@@ +6918,5 @@
>                  catchName = destructuringExpr(yieldHandling, &data, tt);
>                  if (!catchName)
>                      return null();
>                  break;
> +              }

These curly braces are unnecessary.

@@ +7579,5 @@
>          }
> +        // Additional calls to assignExpr should not reuse the possibleError
> +        // which had been passed into the function. Otherwise we would lose
> +        // information needed to determine whether or not we're dealing with
> +        // a non-recoverable situation.

Nit: Blank line before this comment.

@@ +7815,5 @@
>          }
>  
> +        bool isDestructuring = checkDestructuringPattern(nullptr, target);
> +        // Here we've successfully distinguished between destructuring and
> +        // an object literal. In the case where "CoverInitializedName"

Nit: Blank line before comment.

@@ +7904,5 @@
>  
> +    PossibleError possibleErrorInner(*this);
> +    Node lhs = condExpr1(inHandling, yieldHandling, tripledotHandling, &possibleErrorInner, invoked);
> +    if (!lhs) {
> +        // It is possible to return null here without triggering an exception,

I don't remember if we decided this was true or not. (Shouldn't be, but that's beside the point...)

::: js/src/frontend/Parser.h
@@ +455,5 @@
> +     * triggered.
> +     */
> +    class MOZ_STACK_CLASS PossibleError
> +    {
> +        enum ErrorState { None, Pending, Resolved };

Is the `Resolved` state actually any different from `None`?

@@ +463,5 @@
> +        uint32_t offset_;
> +        unsigned errorNumber_;
> +        ParseReportKind reportKind_;
> +        Parser<ParseHandler>& parser_;
> +        bool checked_, strict_;

Please move `checked_` out of here - it's not an error-reporting field so it should be on its own line and probably DEBUG-only.

@@ +471,5 @@
> +          ~PossibleError();
> +
> +          // Set a pending error.
> +          void setPending(ParseReportKind kind, unsigned errorNumber, bool strict);
> +          // Resolve any pending error.

Style nit: blank line before comment.

@@ +518,5 @@
>      /* Perform constant-folding; must be true when interfacing with the emitter. */
>      const bool          foldConstants:1;
> +#if DEBUG
> +    /* Possible errors have destructed without being checked. */
> +    bool hasUncheckedPossibleErrors:1;

Why is this bool necessary?

ISTR already admitting that the PossibleError destructor can't just
    MOZ_ASSERT_IF(!checked_, parser.context->isExceptionPending());
because, unfortunately, there are a couple different reasons we may be unwinding, so isExceptionPending is insufficient.

But then why is the current assertion in Parser::~Parser() OK?

::: js/src/jsapi.cpp
@@ +4187,5 @@
>          cx->clearPendingException();
> +#ifdef DEBUG
> +        // Ignore any unchecked possible errors.
> +        parser.hasUncheckedPossibleErrors = false;
> +#endif

Why is this needed? Shouldn't the parser always check?

Comment 86

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8723732 [details] [diff] [review]
destructuring.diff

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

::: js/src/frontend/Parser.h
@@ +518,5 @@
>      /* Perform constant-folding; must be true when interfacing with the emitter. */
>      const bool          foldConstants:1;
> +#if DEBUG
> +    /* Possible errors have destructed without being checked. */
> +    bool hasUncheckedPossibleErrors:1;

It's too early to know if an error is pending (we haven't yet returned null/false). This flag lets us delay the check until we have all the information we need.

::: js/src/jsapi.cpp
@@ +4186,5 @@
>  
>          cx->clearPendingException();
> +#ifdef DEBUG
> +        // Ignore any unchecked possible errors.
> +        parser.hasUncheckedPossibleErrors = false;

We clear all pending exceptions up above, so our normal assumptions about whether or not it's okay to not check possible errors become invalid. I'll double check this and leave a better comment.

Comment 87

[Morgan Phillips [:mrrrgn]]

(In reply to Morgan Phillips [:mrrrgn] from comment #86)
> Comment on attachment 8723732 [details] [diff] [review]
> destructuring.diff
> 
> Review of attachment 8723732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/Parser.h
> @@ +518,5 @@
> >      /* Perform constant-folding; must be true when interfacing with the emitter. */
> >      const bool          foldConstants:1;
> > +#if DEBUG
> > +    /* Possible errors have destructed without being checked. */
> > +    bool hasUncheckedPossibleErrors:1;
> 
> It's too early to know if an error is pending (we haven't yet returned
> null/false). This flag lets us delay the check until we have all the
> information we need.
> 
> ::: js/src/jsapi.cpp
> @@ +4186,5 @@
> >  
> >          cx->clearPendingException();
> > +#ifdef DEBUG
> > +        // Ignore any unchecked possible errors.
> > +        parser.hasUncheckedPossibleErrors = false;
> 
> We clear all pending exceptions up above, so our normal assumptions about
> whether or not it's okay to not check possible errors become invalid. I'll
> double check this and leave a better comment.

Looking at this again, the whole checked_ field seems broken. There are more cases where the assertion fails. I think we're better off leave it off and investigating why on earth it's possible to return null from parser methods without an exception being thrown.

Comment 88

[Morgan Phillips [:mrrrgn]]

s/leave it off/leaving it out/

Comment 89

[Pulsebot]

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

Comment 90

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbab21ac3b8c

Comment 91

[Jason Orendorff [:jorendorff]]

(this still blocks es6, doing bookkeeping...)

Comment 92

[Shu-yu Guo [:shu]]

What's the status of this?

Comment 93

[Morgan Phillips [:mrrrgn]]

(In reply to Shu-yu Guo [:shu] from comment #92)
> What's the status of this?

I'm don't recollect the bookkeeping that Jason mentioned in comment 91, forwarding to request to him.

Comment 94

[Jason Orendorff [:jorendorff]]

This should land! Comment 91 was supposed to mean "pay no attention to this little change I am making; we still want this bug; I am just doing stupid bookkeeping on the ES6 dependency tree".

Comment 95

[Jason Orendorff [:jorendorff]]

Yeah, this landed already.

Comment 96

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment