[es6] enable destructuring rest parameters

[es6] enable destructuring rest parameters

Originally, only BindingIdentifiers were a legal operand for the `...` ellipsis
in a function rest parameter. This has since changed, allowing the rest array
to be destructured.

The grammar is now the following:

```
FunctionRestParameter[Yield]:
    BindingRestElement[?Yield]

BindingRestElement[Yield]:
    ... BindingIdentifier[?Yield]
    ... BindingPattern[?Yield]
```

*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

BUG=v8:4627, v8:2159
LOG=N
R=littledan@chromium.org, adamk@chromium.org, wingo@igalia.com, rossberg@chromium.org

Committed: https://crrev.com/dfce900d64f2579607705fd2d94c13f28c2e996f
Cr-Commit-Position: refs/heads/master@{#33192}

[caitp (gmail), 2015-12-17 23:40:52]

Take a quick look when you've got a moment, it's a small change, all deletions
and tests

[caitp (gmail), 2015-12-18 00:03:13]

Patchset #1 (id:1) has been deleted

[caitp (gmail), 2015-12-18 00:06:00]

On 2015/12/17 23:40:52, caitp wrote:
> Take a quick look when you've got a moment, it's a small change, all deletions
> and tests

note: if I'm reading the spec correctly, this will actually do the wrong thing
if someone breaks Array.prototype[Symbol.iterator] --- it looks like it should
still work right regardless, but I'm not sure if that's covered in tests.

The problem will probably solve itself once the fancy compiler team version of
rest parameters is in the tree

[caitp (gmail), 2015-12-18 00:21:25]

Patchset #1 (id:20001) has been deleted

[adamk, 2015-12-18 13:01:05]

The spec change affects not only parameters but binding patterns in general. Is
there a particular reason that this change only aims to tackle parameters?

[caitp (gmail), 2015-12-18 13:02:01]

On 2015/12/18 13:01:05, adamk (ooo until 2016) wrote:
> The spec change affects not only parameters but binding patterns in general.
Is
> there a particular reason that this change only aims to tackle parameters?

rest destructuring was already allowed in BindingPatterns, I assume there are
tests covering it already. If not, I'll add some

[adamk, 2015-12-18 13:11:05]

On 2015/12/18 00:06:00, caitp wrote:
> On 2015/12/17 23:40:52, caitp wrote:
> > Take a quick look when you've got a moment, it's a small change, all
deletions
> > and tests
> 
> note: if I'm reading the spec correctly, this will actually do the wrong thing
> if someone breaks Array.prototype[Symbol.iterator] --- it looks like it should
> still work right regardless, but I'm not sure if that's covered in tests.
> 
> The problem will probably solve itself once the fancy compiler team version of
> rest parameters is in the tree

Can you elaborate here? In general I'd rather not break things :)

[caitp (gmail), 2015-12-18 13:15:54]

On 2015/12/18 13:11:05, adamk (ooo until 2016) wrote:
> On 2015/12/18 00:06:00, caitp wrote:
> > On 2015/12/17 23:40:52, caitp wrote:
> > > Take a quick look when you've got a moment, it's a small change, all
> deletions
> > > and tests
> > 
> > note: if I'm reading the spec correctly, this will actually do the wrong
thing
> > if someone breaks Array.prototype[Symbol.iterator] --- it looks like it
should
> > still work right regardless, but I'm not sure if that's covered in tests.
> > 
> > The problem will probably solve itself once the fancy compiler team version
of
> > rest parameters is in the tree
> 
> Can you elaborate here? In general I'd rather not break things :)

I'm not 100% sure about this, but it looks like:

Array.prototype[Symbol.iterator] = function*() { /* skip every other value or
something */ }

function f(...[a, b, c, etc]) {
  // we build an Array, and then destructure the Array, and don't get all the
correct values
  // because @@iterator has been mutated.
  //
  // I didn't look at it too closely, but it looks like this is supposed to just
use the original Arguments iterator,
  // and not be affected by the mutated A.p[@@iterator]
  //
  // I might just be misreading it though
}

[adamk, 2015-12-18 13:25:46]

On 2015/12/18 13:15:54, caitp wrote:
> On 2015/12/18 13:11:05, adamk (ooo until 2016) wrote:
> > On 2015/12/18 00:06:00, caitp wrote:
> > > On 2015/12/17 23:40:52, caitp wrote:
> > > > Take a quick look when you've got a moment, it's a small change, all
> > deletions
> > > > and tests
> > > 
> > > note: if I'm reading the spec correctly, this will actually do the wrong
> thing
> > > if someone breaks Array.prototype[Symbol.iterator] --- it looks like it
> should
> > > still work right regardless, but I'm not sure if that's covered in tests.
> > > 
> > > The problem will probably solve itself once the fancy compiler team
version
> of
> > > rest parameters is in the tree
> > 
> > Can you elaborate here? In general I'd rather not break things :)
> 
> I'm not 100% sure about this, but it looks like:
> 
> Array.prototype[Symbol.iterator] = function*() { /* skip every other value or
> something */ }
> 
> function f(...[a, b, c, etc]) {
>   // we build an Array, and then destructure the Array, and don't get all the
> correct values
>   // because @@iterator has been mutated.
>   //
>   // I didn't look at it too closely, but it looks like this is supposed to
just
> use the original Arguments iterator,
>   // and not be affected by the mutated A.p[@@iterator]
>   //
>   // I might just be misreading it though
> }

Ah, thanks, that makes sense. I suspect you're right that our current desugaring
gets this wrong. And I would be surprised if the spec required the rest array to
be instantiated. Maybe this change should wait until the new rest implementation
is in? I think there's active work going on for that.

[caitp (gmail), 2015-12-18 13:30:24]

On 2015/12/18 13:25:46, adamk (ooo until 2016) wrote:
> On 2015/12/18 13:15:54, caitp wrote:
> > On 2015/12/18 13:11:05, adamk (ooo until 2016) wrote:
> > > On 2015/12/18 00:06:00, caitp wrote:
> > > > On 2015/12/17 23:40:52, caitp wrote:
> > > > > Take a quick look when you've got a moment, it's a small change, all
> > > deletions
> > > > > and tests
> > > > 
> > > > note: if I'm reading the spec correctly, this will actually do the wrong
> > thing
> > > > if someone breaks Array.prototype[Symbol.iterator] --- it looks like it
> > should
> > > > still work right regardless, but I'm not sure if that's covered in
tests.
> > > > 
> > > > The problem will probably solve itself once the fancy compiler team
> version
> > of
> > > > rest parameters is in the tree
> > > 
> > > Can you elaborate here? In general I'd rather not break things :)
> > 
> > I'm not 100% sure about this, but it looks like:
> > 
> > Array.prototype[Symbol.iterator] = function*() { /* skip every other value
or
> > something */ }
> > 
> > function f(...[a, b, c, etc]) {
> >   // we build an Array, and then destructure the Array, and don't get all
the
> > correct values
> >   // because @@iterator has been mutated.
> >   //
> >   // I didn't look at it too closely, but it looks like this is supposed to
> just
> > use the original Arguments iterator,
> >   // and not be affected by the mutated A.p[@@iterator]
> >   //
> >   // I might just be misreading it though
> > }
> 
> Ah, thanks, that makes sense. I suspect you're right that our current
desugaring
> gets this wrong. And I would be surprised if the spec required the rest array
to
> be instantiated. Maybe this change should wait until the new rest
implementation
> is in? I think there's active work going on for that.

I don't mind waiting if someone wants to clarify it with the editor or other
people, but I think I was reading this wrong.

FunctionRestParameter defers to BindingRestElement's
IteratorBindingInitialization
(http://tc39.github.io/ecma262/#sec-destructuring-binding-patterns-runtime-sem...),
which clearly produces an Array normally, and then performs
IteratorBindingInitialization on the pattern once building the Array is
complete.

So I guess this is working as expected

[caitp (gmail), 2016-01-08 19:09:25]

PTAL --- I've updated this for the recent changes to rest parameters.

There's probably a few codegen-side changes that might ought to be made (such
as, not making the temporary array a lexical variable when destructured), but
those aren't done in this patch

[caitp (gmail), 2016-01-08 19:14:02]

On 2016/01/08 19:09:25, caitp wrote:
> PTAL --- I've updated this for the recent changes to rest parameters.
> 
> There's probably a few codegen-side changes that might ought to be made (such
> as, not making the temporary array a lexical variable when destructured), but
> those aren't done in this patch

eh, apparently they aren't lexical at all right now, so it doesn't matter.

[adamk, 2016-01-08 19:16:34]

lgtm

I like that this is mostly a deletion :)

[caitp (gmail), 2016-01-08 19:49:22]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-01-08 19:49:22]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1532873004/#ps80001
(title: "Update test262 expectations")

[commit-bot: I haz the power, 2016-01-08 19:49:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532873004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532873004/80001

[commit-bot: I haz the power, 2016-01-08 20:22:22]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-01-08 20:22:57]

Description was changed from

==========
[es6] enable destructuring rest parameters

Originally, only BindingIdentifiers were a legal operand for the `...` ellipsis
in a function rest parameter. This has since changed, allowing the rest array
to be destructured.

The grammar is now the following:

```
FunctionRestParameter[Yield]:
    BindingRestElement[?Yield]

BindingRestElement[Yield]:
    ... BindingIdentifier[?Yield]
    ... BindingPattern[?Yield]
```

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

BUG=v8:4627, v8:2159
LOG=N
R=littledan@chromium.org, adamk@chromium.org, wingo@igalia.com,
rossberg@chromium.org
==========

to

==========
[es6] enable destructuring rest parameters

Originally, only BindingIdentifiers were a legal operand for the `...` ellipsis
in a function rest parameter. This has since changed, allowing the rest array
to be destructured.

The grammar is now the following:

```
FunctionRestParameter[Yield]:
    BindingRestElement[?Yield]

BindingRestElement[Yield]:
    ... BindingIdentifier[?Yield]
    ... BindingPattern[?Yield]
```

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

BUG=v8:4627, v8:2159
LOG=N
R=littledan@chromium.org, adamk@chromium.org, wingo@igalia.com,
rossberg@chromium.org

Committed: https://crrev.com/dfce900d64f2579607705fd2d94c13f28c2e996f
Cr-Commit-Position: refs/heads/master@{#33192}
==========

[commit-bot: I haz the power, 2016-01-08 20:22:59]

Patchset 3 (id:??) landed as
https://crrev.com/dfce900d64f2579607705fd2d94c13f28c2e996f
Cr-Commit-Position: refs/heads/master@{#33192}