ziyunfei Reporter
	Description • 8 years ago

js> function f(...[a, b, c]) { return a + b + c; }
js> f(1, 2, 3) // 6

Spec change: https://github.com/tc39/ecma262/commit/d322357e6be95bc4bd3e03f5944a736aac55fa50

TC39 Discussion: https://github.com/tc39/tc39-notes/blob/master/es7/2015-07/july-28.md#66-bindingrestelement-should-allow-a-bindingpattern-ala-assignmentrestelement

	André Bargull [:anba] Assignee
	Comment 1 • 8 years ago

I noticed this issue when I was adding the new tests.

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

Applies on top of bug 1204024.

It's interesting that this feature only seem to require frontend changes, or did I miss something here?


frontend/NameAnalysisTypes.h
- Added |CoverArrowParameter| as a placeholder declaration kind. This enables me to parse the arrow rest parameter using Parser#destructuringDeclaration(), with matches the spec which uses BindingPattern, and not AssignmentPattern, in CoverParenthesizedExpressionAndArrowParameterList.

frontend/Parser.cpp
- Moved triple-dot code before switch-statement in functionArguments().
- Allow destructuring binding patterns after TOK_TRIPLEDOT in primaryExpr().
- Removed stale comments about BindData in checkDestructuring().

frontend/Parser.h
- Added an explicit #include for DeclarationKind, instead of using the implicit #include through frontend/NameCollections.h.

frontend/BytecodeEmitter.cpp:
- This change isn't strictly required, but it may help future readers to know that the parameter name can be nullptr. But I can remove this if you think it's not necessary to document it here.

	Tooru Fujisawa [:arai]
	Comment 3 • 8 years ago

Comment on attachment 8797123 [details] [diff] [review]
bug1243717.part0.patch

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

Nice :D

::: js/src/tests/ecma_6/Function/length-with-destructuring-and-parameter-expression.js
@@ +4,5 @@
> +
> +assertEq(function([a = 0]){}.length, 1);
> +assertEq(function({p: a = 0}){}.length, 1);
> +assertEq(function({a = 0}){}.length, 1);
> +assertEq(function({[0]: a}){}.length, 1);

it would be nice to also test the case there's non-destructuring parameter before it,
like (function(a, [b=0]){}).length

	Tooru Fujisawa [:arai]
	Comment 4 • 8 years ago

Comment on attachment 8797124 [details] [diff] [review]
bug1243717.part1.patch

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7722,5 @@
>          if (bindings->nonPositionalFormalStart > 0) {
> +            // |paramName| can be nullptr when the rest destructuring syntax is
> +            // used: `function f(...[]) {}`.
> +            JSAtom* paramName = bindings->names[bindings->nonPositionalFormalStart - 1].name();
> +            *result = name == paramName;

it should be better checking or asserting something here.
maybe, name is always non-nullptr, otherwise set *result to false if paramName is nullptr.
so that clarifies *result doesn't become true when paramName is nullptr.

I cannot believe name can be null tho...

::: js/src/tests/ecma_7/Destructuring/rest-parameter-semantics.js
@@ +11,5 @@
> +function f2(...[a, b = 1]) {
> +    return a + b;
> +}
> +assertEq(f2(3, 7), 10);
> +assertEq(f2(4), 5);

can you add |f2(4, undefined)| case too?

@@ +34,5 @@
> +assertEq(f6(10), 1);
> +assertEq(f6(10, 20), 2);
> +
> +
> +// "arguments" tests.

it would be nice to split this section into separated file.
rest-parameter-semantics-arguments.js or something.

same for other sections.

::: js/src/tests/ecma_7/extensions/parse-rest-destructuring-parameter.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function funArgs(params) {
> +    return Reflect.parse(`function f(${params}) {}`).body[0].rest;

Reflect.parse is not available in jsreftest.
this test needs the following at the first line.
// |reftest| skip-if(!xulRuntime.shell)

	André Bargull [:anba] Assignee
	Comment 5 • 8 years ago

(In reply to Tooru Fujisawa [:arai] from comment #4)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +7722,5 @@
> >          if (bindings->nonPositionalFormalStart > 0) {
> > +            // |paramName| can be nullptr when the rest destructuring syntax is
> > +            // used: `function f(...[]) {}`.
> > +            JSAtom* paramName = bindings->names[bindings->nonPositionalFormalStart - 1].name();
> > +            *result = name == paramName;
> 
> it should be better checking or asserting something here.
> maybe, name is always non-nullptr, otherwise set *result to false if
> paramName is nullptr.
> so that clarifies *result doesn't become true when paramName is nullptr.
> 
> I cannot believe name can be null tho...

Yes, |name| can never be nullptr.

I'll probably just change it back to this version, which I used before I posted the patch:
```
*result = paramName && name == paramName;
```

	André Bargull [:anba] Assignee
	Comment 6 • 8 years ago

Updated patch per review comments, carrying r+ from arai.

	André Bargull [:anba] Assignee
	Comment 7 • 8 years ago

Updated patch per review comments, carrying r+ from arai.

	Pulsebot
	Comment 8 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0c73cdf437
Part 0: Set correct function length when parameter expressions, but no defaults are present. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c98b5e580e
Part 1: Allow destructuring for rest parameter (ES2016). r=arai

	Phil Ringnalda (:philor)
	Comment 9 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ca0c73cdf437
https://hg.mozilla.org/mozilla-central/rev/70c98b5e580e

	Sebastian Zartner [:sebo]
	Comment 10 • 8 years ago

Described this here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters#Destructuring_rest_parameters

And added notes about its support:
https://developer.mozilla.org/en-US/Firefox/Releases/52
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_Next_support_in_Mozilla

André, could you please have a quick look whether the documentation is ok?

Sebastian

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

(In reply to Sebastian Zartner [:sebo] from comment #10)
> Described this here:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/
> rest_parameters#Destructuring_rest_parameters

Looks okay. (I consider it to be difficult to find an example where destructuring rest parameters are actually useful, but see below.)


> And added notes about its support:
> https://developer.mozilla.org/en-US/Firefox/Releases/52
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/
> ECMAScript_Next_support_in_Mozilla

Destructuring rest parameters are only one (and probably the least useful) part of the extended ES2016 support for destructuring rest patterns. ES2015 only allowed to destructure a rest-element in a destructuring assignment pattern [1] (e.g. `[...[]] = someIterable`), but forbid to destructure a rest-element in a destructuring binding pattern [2] (e.g. `var [...[]] = []`). ES2016 lifted this restriction and allows to write `var [...[]] = []` [3]. And only as a side-effect of this lifted restriction, it's now also possible to write `function f(...[]) {}`. 
Firefox always (?) supported to destructure a rest-element in a binding pattern when it appears in var/let/const declaration. The only missing feature were destructuring rest parameters.

[1] http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-assignment
[2] http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-binding-patterns
[3] https://tc39.github.io/ecma262/2016/#sec-destructuring-binding-patterns

	Sebastian Zartner [:sebo]
	Comment 12 • 8 years ago

Sorry for the long delay and thank you for the verification and the informative response, André!

Destructuring assigments are already described at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment. It doesn't explicitly mention the destructuring binding pattern, though I've now added an example for the rest-element binding pattern at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Assigning_the_rest_of_an_array_to_a_variable.

Please let me know if there's anything else that should be described.

Sebastian

	André Bargull [:anba] Assignee
	Comment 13 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #12)
> It doesn't explicitly mention the destructuring
> binding pattern, though I've now added an example for the rest-element
> binding pattern at
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/
> Destructuring_assignment#Assigning_the_rest_of_an_array_to_a_variable.

Hmm, I'm a bit confused, because the example (`var [a, ...b] = [1, 2, 3];`) doesn't actually use a binding pattern for the rest element. Binding patterns are either Array or Object destructuring patterns. For example `[...[]] = iterable;` is a simple example for a binding pattern in the rest element. (`[...[]] = iterable;` can be used to iterate over all elements of an iterable without having to store the iterated values in a dummy variable. I'm mostly using this trick on the shell for testing.)