Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Description • 11 years ago

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)

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 1 • 11 years ago

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.

	André Bargull [:anba]
	Comment 2 • 11 years ago

(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?

	Jason Orendorff [:jorendorff]
	Comment 3 • 11 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Reporter
	Comment 4 • 11 years ago

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

	Anthony Ricaud (:rik)
	Comment 5 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 6 • 10 years ago

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)

	Arpad Borsos [:Swatinem]
	Comment 7 • 10 years ago

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.

	Arpad Borsos [:Swatinem]
	Comment 8 • 10 years ago

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?

	Arpad Borsos [:Swatinem]
	Comment 9 • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 10 • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 11 • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 12 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 13 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 14 • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 15 • 10 years ago

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.

	Arpad Borsos [:Swatinem]
	Comment 16 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 17 • 10 years ago

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

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

	Jason Orendorff [:jorendorff]
	Comment 18 • 10 years ago

Does this still need work? How close is it?

	Arpad Borsos [:Swatinem]
	Comment 19 • 10 years ago

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.

	Arpad Borsos [:Swatinem]
	Comment 21 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 22 • 10 years ago

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?

	Arpad Borsos [:Swatinem]
	Comment 23 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 24 • 10 years ago

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

	Marijn Haverbeke
	Comment 25 • 10 years ago

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.

	Ingvar Stepanyan (RReverser)
	Comment 26 • 10 years ago

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?

	Arpad Borsos [:Swatinem]
	Comment 27 • 10 years ago

(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"}}
```

	Ingvar Stepanyan (RReverser)
	Comment 28 • 10 years ago

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.

	Arpad Borsos [:Swatinem]
	Comment 29 • 10 years ago

(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?

	Marijn Haverbeke
	Comment 30 • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 31 • 10 years ago

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.

	Arpad Borsos [:Swatinem]
	Comment 32 • 10 years ago

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)

	Ingvar Stepanyan (RReverser)
	Comment 33 • 10 years ago

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;
}
```

	Arpad Borsos [:Swatinem]
	Comment 34 • 10 years ago

(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?

	Ingvar Stepanyan (RReverser)
	Comment 35 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 36 • 10 years ago

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

	Ingvar Stepanyan (RReverser)
	Comment 37 • 10 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 38 • 10 years ago

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"!

	Jason Orendorff [:jorendorff]
	Comment 39 • 10 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 40 • 10 years ago

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.

	Arpad Borsos [:Swatinem]
	Comment 41 • 10 years ago

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

	Ingvar Stepanyan (RReverser)
	Comment 42 • 10 years ago

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`?

	Arpad Borsos [:Swatinem]
	Comment 43 • 10 years ago

I filed bug 1093504 about that some time ago.

	Jason Orendorff [:jorendorff]
	Comment 44 • 10 years ago

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.

	Arpad Borsos [:Swatinem]
	Comment 45 • 10 years ago

(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?

	Ingvar Stepanyan (RReverser)
	Comment 46 • 10 years ago

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?

	Arpad Borsos [:Swatinem]
	Comment 47 • 10 years ago

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

	Ingvar Stepanyan (RReverser)
	Comment 48 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 49 • 10 years ago

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

	Ingvar Stepanyan (RReverser)
	Comment 50 • 10 years ago

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.

	Ingvar Stepanyan (RReverser)
	Comment 51 • 10 years ago

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?

	Arpad Borsos [:Swatinem]
	Comment 52 • 10 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 53 • 10 years ago

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.

	Ingvar Stepanyan (RReverser)
	Comment 54 • 10 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 55 • 9 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 56 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 57 • 9 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 58 • 9 years ago

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

	Arpad Borsos [:Swatinem]
	Comment 59 • 9 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 61 • 8 years ago

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

	Morgan Phillips [:mrrrgn] Assignee
	Comment 62 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 63 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 65 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 66 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 67 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 68 • 8 years ago

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

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

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

	Morgan Phillips [:mrrrgn] Assignee
	Comment 69 • 8 years ago

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

	Morgan Phillips [:mrrrgn] Assignee
	Comment 70 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 71 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 72 • 8 years ago

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

	Morgan Phillips [:mrrrgn] Assignee
	Comment 73 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 74 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 75 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 76 • 8 years ago

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

	Morgan Phillips [:mrrrgn] Assignee
	Comment 78 • 8 years ago

`hg addremove`

	Jason Orendorff [:jorendorff]
	Comment 79 • 8 years ago

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)

	Jason Orendorff [:jorendorff]
	Comment 80 • 8 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 81 • 8 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 85 • 8 years ago

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?

	Morgan Phillips [:mrrrgn] Assignee
	Comment 86 • 8 years ago

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.

	Morgan Phillips [:mrrrgn] Assignee
	Comment 87 • 8 years ago

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

	Morgan Phillips [:mrrrgn] Assignee
	Comment 88 • 8 years ago

s/leave it off/leaving it out/

	Pulsebot
	Comment 89 • 8 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 90 • 8 years ago
bugherder

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

	Jason Orendorff [:jorendorff]
	Comment 91 • 8 years ago

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

	Shu-yu Guo [:shu]
	Comment 92 • 8 years ago

What's the status of this?

	Morgan Phillips [:mrrrgn] Assignee
	Comment 93 • 8 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 94 • 8 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 95 • 8 years ago

Yeah, this landed already.

	Florian Scholz (Open Web Docs)
	Comment 96 • 8 years ago

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