Support default values in destructuring

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: mrrrgn)

Tracking

(Blocks 1 bug, {dev-doc-complete})

25 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(4 attachments, 14 obsolete attachments)

4.19 KB, patch
jorendorff
: feedback+
Details | Diff | Splinter Review
24.39 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
13.65 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
52.62 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
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)
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.
Flags: needinfo?(jorendorff)
(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?
Let's not do this piecemeal. Filed bug 933296 and I'll give it some thought tomorrow.
Depends on: 933296
Flags: needinfo?(jorendorff)
I'm unassigning myself because I won't be working on this anytime soon.
Assignee: nfitzgerald → nobody
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
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});
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)
Assignee: nobody → arpad.borsos
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.
Attachment #8456277 - Flags: review?(jorendorff)
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?
Attachment #8456940 - Flags: feedback?(jorendorff)
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
Attachment #8457008 - Flags: feedback?(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
Attachment #8456277 - Flags: review?(jorendorff) → review+
(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.
Attachment #8456940 - Flags: feedback?(jorendorff) → feedback+
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. :)
Attachment #8457008 - Flags: feedback?(jorendorff) → feedback+
There was a bug exposed by "let"
Oh and this is on top of my destructuring-rest patch, in case you didnt notice.
Attachment #8459149 - Flags: review?(jorendorff)
Attachment #8456277 - Attachment is obsolete: true
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.
Attachment #8459149 - Flags: review?(jorendorff) → review+
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
Another case for bug 933296:
`[(a = 10)] = []` and `{a: (a = 10)} = {}` should all be syntax errors!

`{(a = 10)} = {}` <- that case is already caught
Blocks: 1018628
Does this still need work? How close is it?
Flags: needinfo?(arpad.borsos)
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.
Flags: needinfo?(arpad.borsos)
There was a lot that changed, so I would feel more comfortable with another r+ :-)
Attachment #8493119 - Flags: review?(jorendorff)
Attachment #8459149 - Attachment is obsolete: true
Attachment #8457008 - Attachment is obsolete: true
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?
Attachment #8493118 - Flags: review?(marijnh)
Attachment #8493118 - Flags: review?(ariya.hidayat)
Attachment #8493118 - Flags: review?(marijnh)
Attachment #8493118 - Flags: review?(ariya.hidayat)
Attachment #8493118 - Flags: feedback?(marijnh)
Attachment #8493118 - Flags: feedback?(ariya.hidayat)
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 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

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

5 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).
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.
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)
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;
}
```
Blocks: 1093504
(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?
> 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?
(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".
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.
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 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".
Attachment #8493119 - Flags: review?(jorendorff) → review+
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.
Attachment #8493118 - Flags: review?(jorendorff)
Attachment #8493118 - Flags: feedback?(marijnh)
Attachment #8493118 - Flags: feedback?(ariya.hidayat)
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 '='.
Attachment #8533631 - Flags: review?(jorendorff)
Attachment #8493118 - Attachment is obsolete: true
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`?
I filed bug 1093504 about that some time 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.
Attachment #8533631 - Flags: review?(jorendorff) → review+
(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?
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?
Flags: needinfo?(arpad.borsos)
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}]})
Flags: needinfo?(arpad.borsos)
Hm… Why does it produce AssignmentExpression and not an AssignmentPattern on the left?
(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.
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.
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?
Flags: needinfo?(arpad.borsos)
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).
Flags: needinfo?(arpad.borsos)
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.
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.

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(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.
Depends on: 1124480
Depends on: 1125658
Depends on: 1126361
Depends on: 1126518
Depends on: 1126555
Depends on: 1130604
Depends on: 1131267
Depends on: 1133354
Depends on: 1140196
Swatinem, are you interested in working on this some more?
Flags: needinfo?(arpad.borsos)
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.
Flags: needinfo?(arpad.borsos)
Assignee: arpad.borsos → nobody
Assignee

Updated

3 years ago
Assignee: nobody → winter2718
Assignee

Comment 60

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
Assignee

Comment 61

3 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).
Attachment #8716165 - Flags: feedback?(jorendorff)
Assignee

Comment 62

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

Updated

3 years ago
Attachment #8716165 - Flags: feedback?(jorendorff)
Assignee

Comment 63

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
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.
Attachment #8716165 - Attachment is obsolete: true
Assignee

Comment 64

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
Attachment #8717717 - Attachment is obsolete: true
Assignee

Comment 65

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
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.
Attachment #8718673 - Attachment is obsolete: true
Assignee

Comment 66

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

Comment 67

3 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.
Attachment #8718929 - Flags: feedback?(jorendorff)
Assignee

Comment 68

3 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....
Attachment #8718929 - Flags: feedback?(jorendorff)
Assignee

Comment 69

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
https://www.youtube.com/watch?v=lydBPm2KRaU#t=1m But seriously, yell at me about holes you notice in the test cases.
Attachment #8718929 - Attachment is obsolete: true
Attachment #8719060 - Flags: feedback?(jorendorff)
Assignee

Comment 70

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

Comment 71

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

Comment 72

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

Comment 73

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

Comment 74

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

Comment 75

3 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.
Attachment #8719060 - Flags: feedback?(jorendorff)
Assignee

Comment 76

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
Hmm, I think this is the wippiest WIP I've posted. Me IRL https://www.youtube.com/watch?v=O_05qJTeNNI
Attachment #8720668 - Flags: feedback?(jorendorff)
Assignee

Updated

3 years ago
Attachment #8719060 - Attachment is obsolete: true
Assignee

Comment 77

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
Attachment #8720668 - Attachment is obsolete: true
Attachment #8720668 - Flags: feedback?(jorendorff)
Attachment #8720869 - Flags: feedback?(jorendorff)
Assignee

Comment 78

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
`hg addremove`
Attachment #8720869 - Attachment is obsolete: true
Attachment #8720869 - Flags: feedback?(jorendorff)
Attachment #8720874 - Flags: feedback?(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 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).
Attachment #8720874 - Flags: feedback?(jorendorff) → feedback+
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.
Assignee

Comment 82

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
Attachment #8720874 - Attachment is obsolete: true
Assignee

Comment 83

3 years ago
Posted patch destructuring.diff (obsolete) — Splinter Review
Attachment #8723668 - Attachment is obsolete: true
Assignee

Comment 84

3 years ago
Attachment #8723728 - Attachment is obsolete: true
Attachment #8723732 - Flags: review?(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?
Attachment #8723732 - Flags: review?(jorendorff) → review+
Assignee

Comment 86

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

Comment 87

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

Comment 88

3 years ago
s/leave it off/leaving it out/
Depends on: 1254335
Depends on: 1255167
Depends on: 1257053
(this still blocks es6, doing bookkeeping...)
No longer blocks: es6
What's the status of this?
Flags: needinfo?(winter2718)
Assignee

Comment 93

3 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.
Flags: needinfo?(winter2718) → needinfo?(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".
Flags: needinfo?(jorendorff) → needinfo?(winter2718)
Yeah, this landed already.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Flags: needinfo?(winter2718)
You need to log in before you can comment on or make changes to this bug.