1018628 - Destructured parameters incorrectly disallow default value in function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[nicholas]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

Wrote this function in Firefox 29:

function setCookie(name, value, { secure, path, domain, expires } = {}) {
    // ...
}


Actual results:

Got an error: 
SyntaxError: missing ) after formal parameters


Expected results:

There should be no error. Per ES6, the destructured parameter should allow default value assignment.

Comment 1

[Till Schneidereit [:till]]

Tooru, you wouldn't be interested in working on this, by any chance? It requires work in areas of the engine that you touched in your JSOP_SPREAD-related patches, so might be a good next task to tackle.

Comment 2

[Tooru Fujisawa [:arai]]

Sounds interesting, I'd like to work on it :)

I think we should change initialization order of destrucuting parameter, function declaration and default parameter.
Currently bytecode is emitted in following order:
  1. Destructuring
  2. Functions
  3. Defaults

It should be following:
  1. Functions
  2. Default and Destructuring, for each argument
so we can support nested dependencies of destructuring and default parameter,
such as |function f(a=1, [b]=[a], {c}={c: b}, d=c) {}|.
(If this type of declaration is not valid in ES6 spec, please tell me)

I'll look into this soon.

Comment 3

[Till Schneidereit [:till]]

Great! :)

The order in which all of this has to happen is very precisely specified in [1], [2], and [3]. Specifically, steps 24-25 of [1] say that [2] is used to create a List of values to use as the actual arguments. [2] only specifies how [3] is used. And [3] you should already be acquainted with from your work on ...rest. :)


[1]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-functiondeclarationinstantiation
[2]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions-runtime-semantics-iteratorbindinginitialization
[3]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-binding-patterns-runtime-semantics-iteratorbindinginitialization

Comment 4

[allenwb]

(In reply to Till Schneidereit [:till] from comment #3)
> Great! :)
> 

Yes, the ordering and other details given in [1] are very happy important for a correct implementation.  In particular declarations in the function body are not supposed to be visible to parameter default value expressions and TDZs apply left-to-right while processing the parameters.

Feel free to ask me if anything is unclear in these specs.

> 
> [1]:
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-
> functiondeclarationinstantiation

Comment 5

[Tooru Fujisawa [:arai]]

Thanks, I'm reading those specs now.

(In reply to allenwb from comment #4)
> In particular declarations in the function
> body are not supposed to be visible to parameter default value expressions

That's a good news :)

I was having a trouble in following code:
> function g([a, b]=[1, 2], c=a) {
>   function a() {}
>   return [a, b, c];
> }
To follow current implementation, |g()| should return |[function a() {}, 2, function a() {}]|,
so we should emit bytecode for destructuring and defaults for |b|, but not for |a|,
this is very complicated.

For me, it's more natural that |g()| returns |[function a() {}, 2, 1]|,
and this may be the correct behavior in the current spec.

This can be obtained by simply moving function declarations after parameters in:
  http://dxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#6187
>  1. Default and Destructuring, for each argument
>  2. Functions
we should also emit all default parameter code for non-destructuring,
even if it is turned into use, for consistency with destructuring:
  http://dxr.mozilla.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#6134
of course those changes will break some tests, which suppose old behavior, especially:
  http://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/arguments/defaults-bound-to-function.js

So, the patch for this bug will consist of two parts:
  1. make the order and implementation of default/destructuring/function declarations compatible with current spec
  2. support default for destructuring

Draft patch is almost ready.
I'll post it shortly.

Comment 6

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> I was having a trouble in following code:
> > function g([a, b]=[1, 2], c=a) {
> >   function a() {}
> >   return [a, b, c];
> > }
> To follow current implementation, |g()| should return |[function a() {}, 2,
> function a() {}]|,
> so we should emit bytecode for destructuring and defaults for |b|, but not
> for |a|,
> this is very complicated.
> 
> For me, it's more natural that |g()| returns |[function a() {}, 2, 1]|,
> and this may be the correct behavior in the current spec.

Yes, that is my reading of the spec, too.

> of course those changes will break some tests, which suppose old behavior

That, sadly, happens when working on features that are in development spec-wise, yes. Make also sure that you run a full try build on one platform (Linux, 64 or 32 bit, is usually best in terms of test runtime and resource availability). There might well be incompatible usage in browser JS already.

> 
> So, the patch for this bug will consist of two parts:
>   1. make the order and implementation of default/destructuring/function
> declarations compatible with current spec
>   2. support default for destructuring
> 
> Draft patch is almost ready.
> I'll post it shortly.

Great!

Comment 7

[Tooru Fujisawa [:arai]]

Does step 28.a of [1] mean that |f()| should return |10| instead of |20| in following code?
> function f(a=10, b=(() => a)) {
>   var a = 20;
>   return b();
> }

Current implementation returns |20|, and this needs some more fix if it should return |10|,
but I guess default for destructuring could be implemented without it,
so I'd like to leave it to bug 884372.
Patch Part1 will fix only initialization order (and evaluate all default values), not implementation about scope/env.

Comment 8

[Jason Orendorff [:jorendorff]]

Can we break this into two bugs, one for the destructuring work and one for the function argument work?

Comment 9

[allenwb]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> Does step 28.a of [1] mean that |f()| should return |10| instead of |20| in
> following code?
> > function f(a=10, b=(() => a)) {
> >   var a = 20;
> >   return b();
> > }
> 

Generally, but not in this specific case because the var declaration has the same name as a formal parameter.  See the note in step 29.v (which is what the rest of 29 specifies). it would be different if the name was different or it a let declaration was used.

In this case, there is only one binding of f and function b captures it, so the call to b in the example should return 20.

However, if the example had been:

var z = 10;
function f(b=(() => z) {
  var z = 20;
  return b();
}

calling b should return 10, because it captures the outer binding of z rather than in inner binding.

See https://github.com/rwaldron/tc39-notes/blob/master/es6/2013-09/default-arguments.pdf for more examples and the motivation for the current semantics.  However, follow what the spec. says not the exact details from the presentation as various details have changed since the time of the presentation.

Comment 10

[Tooru Fujisawa [:arai]]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> Can we break this into two bugs, one for the destructuring work and one for
> the function argument work?

I guess required steps and dependencies of them are following:

A. swap "2. Function" and "3. Defaults" in current sequence:
>  1. Destructuring
>  2. Functions
>  3. Defaults
B. evaluate all default parameters in "3. Defaults" (depends on A)
C. merge "1. Destructuring" and "3. Defaults" to fix evaluation order between each argument (depends on A, and maybe B)
D. support default parameter for destructuring (depends on B and C)
E. separate environment for default parameter and function body (depends on A, and maybe D)

A, B and E are the function argument work, so could be separated bug(s) (new bug(s) or bug 884372).
D is the destructuring work and should be fixed here.
I'm not sure which one C belong to... might be the destructuring work?

How do you think?

Comment 11

[Jason Orendorff [:jorendorff]]

You're closer to the issues than I am, so organize the work however you like.

To me, each step here looks challenging on its own. I would make separate bugs for C, D, and E.

I guess I would keep A and B together -- I'm not sure what B means separate from A.

Comment 12

[Tooru Fujisawa [:arai]]

Okay, thanks.
I filed following 2 bugs.
  A+B: Bug 1022962 - Default parameters should be evaluated before function declarations
  E: Bug 1022967 - Separate environment for default parameter and function body if default parameter contains closure

and Bug 884372 will be something like a meta bug.

>I'm not sure what B means separate from A.
Oh, I misunderstood about dependencies of problem.
Yes, those should be merged, they are different aspect of same problem.

Then, I leave C and D together here because C seems has no effect without D,
because destructuring parameter cannot appear after default parameter for now
(I should notice this before...).

This bug will be addressed soon after Bug 1022962 is fixed :)

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Support default parameter for destructuring parameter.

This patch adds support for default parameter for destructuring parameter,
and changes parse tree in following way:
  * create separated PNK_VAR node for each destructuring parameter, and store them in PNK_SEQ node, instead of appending all ASSIGN nodes in single PNK_VAR node, to make it easy to emit bytecode separately
  * create formal parameter node for destructuring parameter to store default parameter.
    It is created even if the parameter does not have default parameter, to simplify code, so the number of formal parameter nodes always equals to the number of parameters

For example "function f([a], [b]) {}" will be parsed into following tree:

(STATEMENTLIST [(FUNCTION (ARGSBODY [#<zero-length name>
                                     #<zero-length name>
                                     (STATEMENTLIST [(SEMI (SEQ [(VAR [(ASSIGN (ARRAY [a])
                                                                               #<zero-length name>)])
                                                                 (VAR [(ASSIGN (ARRAY [b])
                                                                               #<zero-length name>)])]))])]))])

It was following without this patch:

(STATEMENTLIST [(FUNCTION (ARGSBODY [(STATEMENTLIST [(SEMI (VAR [(ASSIGN (ARRAY [a])
                                                                         #<zero-length name>)
                                                                 (ASSIGN (ARRAY [b])
                                                                         #<zero-length name>)]))])]))])

Then, currently all PNK_VAR nodes for destructuring parameter are placed after all of parameter nodes, so the order of them in the parse tree is not equal to evaluation order. Is it okay? or should I place each VAR node under (or after) each parameter node?


Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=46ee05d81a91

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8458553 [details] [diff] [review]
Support default parameter for destructuring parameter.

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

This looks great. I'm sorry for the slow review.

r=me with the following comments addressed. No action is required for the first three, but a follow-up patch would be much appreciated.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6276,5 @@
> +            if (!EmitTree(cx, bce, destruct))
> +                return false;
> +            if (Emit1(cx, bce, JSOP_POP) < 0)
> +                return false;
> +            destruct = destruct->pn_next;

Wow. It would be great if the Parser could generate a saner AST. The AST we have is very ad hoc and ugly. As a result, here we have to traverse pn and pndestruct in sync, and test frameSlot(), which are both fairly weird hacks.

Ideally the emitter should just be able to look at the ParseNode for each formal parameter, see if it has a destructuring BindingPattern, and if so, call the appropriate EmitDestructuringWhatever function.

No action is required -- it's just a thought. But if you'd like to clean this up (in a separate changeset), *please* feel free.

::: js/src/frontend/Parser.cpp
@@ +1606,5 @@
>                   * anonymous positional parameter into the destructuring
>                   * left-hand-side expression and accumulate it in list.
>                   */
>                  HandlePropertyName name = context->names().empty;
>                  Node rhs = newName(name);

Continuing my previous comment: the pre-existing code here "synthesizes a destructuring assignment" for no good reason.

Ideally the parser should generate an AST that looks just like the syntax. No hacky PNK_SEQ nodes, no drama. Boring code. It would make jsreflect.cpp less hackish as a side benefit.

@@ +1620,5 @@
>                      return false;
> +
> +                Node vars = handler.newList(PNK_VAR, item);
> +                if (!vars)
> +                    return false;

This in particular bothers me. Most of the weirdness here is pre-existing ... but it seems like we're continuing to make the AST weirder because it's the fastest way to get something working. It's hard for me to convince myself this code is actually correct.

@@ +1679,1 @@
>                  if (tokenStream.matchToken(TOK_ASSIGN)) {

Isn't allowDefault always true here? If so, go ahead and remove it and unindent.

The |if (*hasRest)| checks for the one case that's not permitted, I think.

::: js/src/jit-test/tests/arguments/defaults-destructuring-mixed.js
@@ +1,5 @@
> +function f1(a, bIs, cIs, dIs, b=3, {c}={c: 4}, [d]=[5]) {
> +    assertEq(a, 1);
> +    assertEq(b, bIs);
> +    assertEq(c, cIs);
> +}

Did you want assertEq(d, dIs) here?

This definitely should test interactions with bug 932080, which I think will land first.

@@ +3,5 @@
> +    assertEq(b, bIs);
> +    assertEq(c, cIs);
> +}
> +assertEq(f1.length, 4);
> +f1(1, 3, 4, 4);

f1(1, 3, 4, 5); right?

::: js/src/jit-test/tests/arguments/defaults-destructuring-object-function-expression.js
@@ +1,1 @@
> +function f1(a, bIs, cIs, dIs, {b}={b: 3}, c=4, [d]=[5]) (

Perhaps it's also worth testing a standard FunctionExpression:

var f1 = function (a, bIs, ... ) { ... };

::: js/src/jit-test/tests/arguments/destructuring-after-defaults.js
@@ +1,3 @@
> +load(libdir + "asserts.js");
> +
> +function f(a, bIs, cIs, dIs, b=1, [c], {d}) {

This could use an extra test to confirm that the defaults and destructuring happen in the right order (for example, the Initializer for b could look at the values of c and d).

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +236,5 @@
> +assertDecl("function f(a=1, [x,y]=[2,3]) { }",
> +           funDecl(ident("f"),
> +                   [ident("a"), arrPatt([ident("x"), ident("y")])],
> +                   blockStmt([]),
> +                   [lit(1), arrExpr([lit(2), lit(3)])]));

Good test.

On the other hand, I'd like to revamp how Reflect.parse treats parameter default-expressions ...but that's a separate bug. (I like how bug 932080 attachment 8457008 [details] [diff] [review] deals with defaults in AssignmentPatterns; maybe something like that.)

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Support default parameter for destructuring parameter.

Thank you!

About the AST, I've created a patch for it, I'll post it in next comment.

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> @@ +1679,1 @@
> >                  if (tokenStream.matchToken(TOK_ASSIGN)) {
> 
> Isn't allowDefault always true here? If so, go ahead and remove it and
> unindent.
> 
> The |if (*hasRest)| checks for the one case that's not permitted, I think.

Yeah, that's true. I fixed it.

> ::: js/src/jit-test/tests/arguments/defaults-destructuring-mixed.js
> @@ +1,5 @@
> > +function f1(a, bIs, cIs, dIs, b=3, {c}={c: 4}, [d]=[5]) {
> > +    assertEq(a, 1);
> > +    assertEq(b, bIs);
> > +    assertEq(c, cIs);
> > +}
> 
> Did you want assertEq(d, dIs) here?

Yes, d should be tested.

> This definitely should test interactions with bug 932080, which I think will
> land first.

I'll add tests for default value after the patch for bug 932080 is landed.
In particular, it should be tested that bytecodes for destructuring and rest parameter are emitted in right order in all cases.

> @@ +3,5 @@
> > +    assertEq(b, bIs);
> > +    assertEq(c, cIs);
> > +}
> > +assertEq(f1.length, 4);
> > +f1(1, 3, 4, 4);
> 
> f1(1, 3, 4, 5); right?

Yes, fixed.

> :::
> js/src/jit-test/tests/arguments/defaults-destructuring-object-function-
> expression.js
> @@ +1,1 @@
> > +function f1(a, bIs, cIs, dIs, {b}={b: 3}, c=4, [d]=[5]) (
> 
> Perhaps it's also worth testing a standard FunctionExpression:
> 
> var f1 = function (a, bIs, ... ) { ... };

Sorry, the filename was wrong, it should be defaults-destructuring-expression-closure.js.
So the test for function with PNK_SEQ node as its body instead of PNK_STATEMENTLIST or PNK_RETURN.

But I should test FunctionExpression too, I added it as defaults-destructuring-function-expression.js.

> ::: js/src/jit-test/tests/arguments/destructuring-after-defaults.js
> @@ +1,3 @@
> > +load(libdir + "asserts.js");
> > +
> > +function f(a, bIs, cIs, dIs, b=1, [c], {d}) {
> 
> This could use an extra test to confirm that the defaults and destructuring
> happen in the right order (for example, the Initializer for b could look at
> the values of c and d).

Sorry if I misread your comment, when Initializer for b refers the value of c and d, they are undefined because b=1 should be evaluated before [c] and {d}, right?

Added the test for it, but please tell me if I'm wrong.

> ::: js/src/tests/js1_8_5/extensions/reflect-parse.js
> @@ +236,5 @@
> > +assertDecl("function f(a=1, [x,y]=[2,3]) { }",
> > +           funDecl(ident("f"),
> > +                   [ident("a"), arrPatt([ident("x"), ident("y")])],
> > +                   blockStmt([]),
> > +                   [lit(1), arrExpr([lit(2), lit(3)])]));
> 
> Good test.

Thanks! :)

> On the other hand, I'd like to revamp how Reflect.parse treats parameter
> default-expressions ...but that's a separate bug. (I like how bug 932080
> attachment 8457008 [details] [diff] [review] deals with defaults in
> AssignmentPatterns; maybe something like that.)

Yeah, I agree, it will be better if we can get simpler tree for default parameter from Reflect.parse :)


Then, I found some mistakes in previous patch.
I noticed them while making patch part 2 for changing AST, thanks :)

::: js/src/frontend/BytecodeEmitter.cpp
>         bool hasDefaults = bce->sc->asFunctionBox()->hasDefaults();
>-        if (hasDefaults) {
>+        if (hasDefaults || hasDestructuring) {
>             ParseNode *rest = nullptr;
>             bool restIsDefn = false;

With this condition, rest parameter will be pushed onto the stack and set to undefined even if the function have no default parameter, but it's better to avoid this for performance.
Fixed in updated patch.
(will be fixed again after the patch for bug 932080 is landed. Updated rule is not correct for default value for destructuring)

::: js/src/jsreflect.cpp
>+        JS_ASSERT(arg->isKind(PNK_NAME) || arg->isKind(PNK_ASSIGN));
>+        ParseNode *argName = arg->isKind(PNK_NAME) ? arg : arg->pn_left;

I forgot to remove PNK_ASSIGN case.
Fixed in updated patch.


I'll ask review again after I updated this patch for default value for destructuring.


Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=1c415c89475a

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Generate AST just like syntax for default and destructuring parameter.

Changed AST as following:
  * Always generate PNK_ASSIGN node for default parameter, instead of storing default value into pn_expr
  * Store destructuring parameter into pn_expr of each anonymous positional parameter PNK_NAME node
  * Remove PND_DEFAULT flag (can be checked with pn_type, PNK_NAME or PNK_ASSIGN)
  * Remove PNX_DESTRUCT flag (prelude in function no longer exists)
  * PNK_ARGSBODY will no longer have PNK_SEQ node as its body (because of no prelude)

I don't modified jsreflect's tree with this patch, it generates same tree as before.


Parser generates following tree for "function f(a, b=1, [c], [d]=[1]) {}":

(STATEMENTLIST [(FUNCTION (ARGSBODY [a
                                     (ASSIGN b
                                             1)
                                     (#<zero-length name> (ARRAY [c]))
                                     (#<zero-length name> (ASSIGN (ARRAY [d])
                                                                  (ARRAY [1])))
                                     (STATEMENTLIST [])]))])

To get above output, I've modified NameNode::dump to dump pn_expr for destructuring parameter, so in above output,
  (#<zero-length name> (ARRAY [c]))
means that it's anonymous PNK_NAME node with '[c]' as a destructuring pattern stored in pn_expr.


Actually, it may be better to generate following tree, to make it just like syntax, but we have to store anonymouns PNK_NAME node for destructuring in somewhere in the tree, to emit GETARG and SETARG. Do you have any idea?

(STATEMENTLIST [(FUNCTION (ARGSBODY [a
                                     (ASSIGN b
                                             1)
                                     (ARRAY [c])
                                     (ASSIGN (ARRAY [d])
                                             (ARRAY [1]))
                                     (STATEMENTLIST [])]))])


(In reply to Jason Orendorff [:jorendorff] from comment #14)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +6276,5 @@
> > +            if (!EmitTree(cx, bce, destruct))
> > +                return false;
> > +            if (Emit1(cx, bce, JSOP_POP) < 0)
> > +                return false;
> > +            destruct = destruct->pn_next;
> 
> Wow. It would be great if the Parser could generate a saner AST. The AST we
> have is very ad hoc and ugly. As a result, here we have to traverse pn and
> pndestruct in sync, and test frameSlot(), which are both fairly weird hacks.
> 
> Ideally the emitter should just be able to look at the ParseNode for each
> formal parameter, see if it has a destructuring BindingPattern, and if so,
> call the appropriate EmitDestructuringWhatever function.
> 
> No action is required -- it's just a thought. But if you'd like to clean
> this up (in a separate changeset), *please* feel free.

With this patch, emitter can emit bytecode for default and destructuring only from the ParseNode for each formal parameter.
But it checks destructuring by comparing parameter's name with names().empty, because of the tree structure I noted above.

> ::: js/src/frontend/Parser.cpp
> @@ +1606,5 @@
> >                   * anonymous positional parameter into the destructuring
> >                   * left-hand-side expression and accumulate it in list.
> >                   */
> >                  HandlePropertyName name = context->names().empty;
> >                  Node rhs = newName(name);
> 
> Continuing my previous comment: the pre-existing code here "synthesizes a
> destructuring assignment" for no good reason.
> 
> Ideally the parser should generate an AST that looks just like the syntax.
> No hacky PNK_SEQ nodes, no drama. Boring code. It would make jsreflect.cpp
> less hackish as a side benefit.
> 
> @@ +1620,5 @@
> >                      return false;
> > +
> > +                Node vars = handler.newList(PNK_VAR, item);
> > +                if (!vars)
> > +                    return false;
> 
> This in particular bothers me. Most of the weirdness here is pre-existing
> ... but it seems like we're continuing to make the AST weirder because it's
> the fastest way to get something working. It's hard for me to convince
> myself this code is actually correct.

PNK_VAR and PNK_ASSIGN nodes are removed, and destructuring pattern is stored into anonymous positional parameter's pn_expr.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=aedbac935fd4

Comment 17

[Jason Orendorff [:jorendorff]]

(In reply to Tooru Fujisawa [:arai] from comment #15)
> Sorry if I misread your comment, when Initializer for b refers the value of
> c and d, they are undefined because b=1 should be evaluated before [c] and
> {d}, right?
> 
> Added the test for it, but please tell me if I'm wrong.

You're right, that is what I meant.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 8462969 [details] [diff] [review]
Part 2: Generate AST just like syntax for default and destructuring parameter.

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

I only skimmed it, but it looks great. A nice simplification throughout.

::: js/src/frontend/ParseNode.h
@@ -231,5 @@
>   *                              PNK_STATEMENTLIST node for function body
>   *                                statements,
> - *                              PNK_RETURN for expression closure, or
> - *                              PNK_SEQ for expression closure with
> - *                                destructured formal parameters

This is the last mention of PNK_SEQ in this comment ... but it's still in use, which means whoever wrote the bit in Parser::forStatement() that uses PNK_SEQ didn't document what they were doing. Could you add a row in this table for PNK_SEQ please?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b0631da5300

Comment 20

[Tooru Fujisawa [:arai]]

Sorry, the bug number in the commit message was wrong.
that patch was for bug 1126361.

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Support default parameter for destructuring. r=jorendorff

Just rebased Part1.

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Generate AST just like syntax for default and destructuring parameter.

Rebased Part2, and added comment for PNK_SEQ.

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add test for default parameter with destructuring default value.

Added testcase for default parameter and default value.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38d72cf67193

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Support default parameter for destructuring. r=jorendorff

rebased.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad99f388a06a

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Generate AST just like syntax for default and destructuring parameter.

also rebased Part 2.
no changes in Part 3.

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Support default parameter for destructuring. r=jorendorff

Just rebased.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05564a140ac5

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Generate AST just like syntax for default and destructuring parameter.

also rebased Part 2.
no changes in Part 3 again.

Comment 28

[Jason Orendorff [:jorendorff]]

Comment on attachment 8610947 [details] [diff] [review]
Part 2: Generate AST just like syntax for default and destructuring parameter.

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

Offhand, I think I would have tried to make the destructuring argument with a default look like this:

    {
      type: PNK_ASSIGN,
      lhs: {
        argument definition node,
        pn_atom: "",
        pn_expr: destructuring pattern
      },
      rhs: default expression node
    }

I think this might require less weirdness in FullParseHandler.h, BytecodeEmitter.cpp, and jsreflect.cpp.

But this is such a vast improvement and has been waiting on me for too long already. r=me with the comments below addressed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6061,1 @@
>      for (ParseNode* pn2 = pnchild; pn2; pn2 = pn2->pn_next) {

Please inline 'pn->pn_head' here and delete the 'pnchild' variable.

@@ +6644,1 @@
>  BytecodeEmitter::emitSyntheticStatements(ParseNode* pn, ptrdiff_t top)

The whole method can go!

::: js/src/frontend/FullParseHandler.h
@@ +568,5 @@
>      inline bool addCatchBlock(ParseNode* catchList, ParseNode* letBlock,
>                                ParseNode* catchName, ParseNode* catchGuard, ParseNode* catchBody);
>  
> +    inline ParseNode* makeAssignmentFromArg(ParseNode* arg, ParseNode* lhs, ParseNode* rhs);
> +    inline void replaceLastFunctionArgument(ParseNode* funcpn, ParseNode* pn);

Make these `private:`, please.

@@ +821,5 @@
>      return true;
>  }
>  
> +inline ParseNode*
> +FullParseHandler::makeAssignmentFromArg(ParseNode* arg, ParseNode* lhs, ParseNode* rhs)

Please try changing this whole method to just:

    return newBinary(PNK_ASSIGN, lhs, rhs, JSOP_NOP);

Changing a node from one type to another should be avoided if possible (especially if you can also delete 10 lines of code)...

@@ +870,5 @@
> +    if (!pn)
> +        return false;
> +
> +    if (arg->pn_expr)
> +        arg->pn_expr = pn;

This arrangement is definitely a little weird, and the code that walks the arguments (in BytecodeEmitter.cpp and jsreflect.cpp) has to be weird to deal with it. Please make sure this is documented in the huge comment in ParseNode.h.

::: js/src/frontend/ParseNode.h
@@ +273,5 @@
>   * PNK_FOR      binary      pn_left: either PNK_FORIN (for-in statement),
>   *                            PNK_FOROF (for-of) or PNK_FORHEAD (for(;;))
>   *                          pn_right: body
> + * PNK_SEQ      list        pn_head: list of PNK_VAR for hoisted variable
> + *                            and PNK_FOR for for-in statement

Looks like this is dead!

@@ -688,5 @@
>  #define PND_DEOPTIMIZED         0x20    /* former pn_used name node, pn_lexdef
>                                             still valid, but this use no longer
>                                             optimizable via an upvar opcode */
>  #define PND_CLOSED              0x40    /* variable is closed over */
> -#define PND_DEFAULT             0x80    /* definition is an arg with a default */

I'm so glad to see this go.

@@ -706,5 @@
>                                             needs popping */
>  #define PNX_FUNCDEFS    0x02            /* contains top-level function statements */
>  #define PNX_SETCALL     0x04            /* call expression in lvalue context */
> -#define PNX_DESTRUCT    0x08            /* code evaluating destructuring
> -                                           arguments occurs before function body */

And this.

::: js/src/jsreflect.cpp
@@ +3490,2 @@
>                  return false;
>              destruct = destruct->pn_next;

This last line looks like it should be deleted.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f8f512fa21
https://hg.mozilla.org/integration/mozilla-inbound/rev/894633bf5002
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed36d04f4ea0

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0f8f512fa21
https://hg.mozilla.org/mozilla-central/rev/894633bf5002
https://hg.mozilla.org/mozilla-central/rev/ed36d04f4ea0

Comment 31

[Tooru Fujisawa [:arai]]

Thank you! :D

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/41
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

Comment 32

[Florian Scholz (Open Web Docs)]

Reviewed. Thanks, arai! :-)