Closed Bug 1243717 Opened 4 years ago Closed 3 years ago

Support destructuring for rest parameters

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: 446240525, Assigned: anba)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(2 files, 2 obsolete files)

Depends on: 1204024
Attached patch bug1243717.part0.patch (obsolete) — Splinter Review
I noticed this issue when I was adding the new tests.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8797123 - Flags: review?(arai.unmht)
Attached patch bug1243717.part1.patch (obsolete) — Splinter Review
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.
Attachment #8797124 - Flags: review?(arai.unmht)
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
Attachment #8797123 - Flags: review?(arai.unmht) → review+
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)
Attachment #8797124 - Flags: review?(arai.unmht) → review+
(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;
```
Updated patch per review comments, carrying r+ from arai.
Attachment #8797123 - Attachment is obsolete: true
Attachment #8799512 - Flags: review+
Updated patch per review comments, carrying r+ from arai.
Attachment #8797124 - Attachment is obsolete: true
Attachment #8799514 - Flags: review+
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca0c73cdf437
https://hg.mozilla.org/mozilla-central/rev/70c98b5e580e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(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
Flags: needinfo?(andrebargull)
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
(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.)
You need to log in before you can comment on or make changes to this bug.