[destructuring] Parse binding patterns in formal parameters.

[destructuring] Parse binding patterns in formal parameters.

R=arv@chromium.org,wingo@igalia.com,caitpotter88@gmail.com
LOG=N
BUG=v8:811
Committed: https://crrev.com/a7fce18647298a2ccef8255cbfbc3acb7046a539
Cr-Commit-Position: refs/heads/master@{#29029}

[Dmitry Lomov (no reviews), 2015-06-10 15:11:26]

WIP - Please take initial look at your leasure. 
I am almost done, but some tests still fail, in particular

var f = ({x = 42, y = 15}) => {};

Not sure yet why.

[wingo, 2015-06-10 15:21:15]

Looks good!

On 2015/06/10 15:11:26, Dmitry S. Lomov wrote:
> WIP - Please take initial look at your leasure. 
> I am almost done, but some tests still fail, in particular
> 
> var f = ({x = 42, y = 15}) => {};
> 
> Not sure yet why.

If I had to guess it would be that literal indexes from the parsed {} pattern
are eagerly assigned in the outer function.  Possible?

[caitp (gmail), 2015-06-10 15:31:51]

One runtime feature we could add now is to mark the function scope as having a
complex parameter list if binding patterns are used, forcing the allocation of
strict arguments, per 9.2.12.22.a

[arv (Not doing code reviews), 2015-06-10 15:49:56]

On 2015/06/10 15:31:51, caitp wrote:
> One runtime feature we could add now is to mark the function scope as having a
> complex parameter list if binding patterns are used, forcing the allocation of
> strict arguments, per 9.2.12.22.a

That seems like a good optimization for a followup.

[Dmitry Lomov (no reviews), 2015-06-15 13:42:34]

The CQ bit was checked by dslomov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-15 13:42:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1167393005/20001

[Dmitry Lomov (no reviews), 2015-06-15 13:42:45]

This is ready for review.

[commit-bot: I haz the power, 2015-06-15 14:06:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-15 14:06:35]

Dry run: This issue passed the CQ dry run.

[arv (Not doing code reviews), 2015-06-15 15:21:59]

LGTM

https://codereview.chromium.org/1167393005/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1167393005/diff/20001/src/parser.cc#newcode1187
src/parser.cc:1187: BlockState block_state(&scope_, scope);
Why was this block state added? It does not seem like it is needed for parsing.

https://codereview.chromium.org/1167393005/diff/20001/src/parser.cc#newcode3813
src/parser.cc:3813: // TODO(wingo): Support rest parameters.
Andy implemented this last week.

https://codereview.chromium.org/1167393005/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1167393005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6617: "{a : a}",
Isn't this case already covered on line 6405?

[Dmitry Lomov (no reviews), 2015-06-15 16:12:51]

comments addressed, landing

https://codereview.chromium.org/1167393005/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1167393005/diff/20001/src/parser.cc#newcode1187
src/parser.cc:1187: BlockState block_state(&scope_, scope);
On 2015/06/15 15:21:59, arv wrote:
> Why was this block state added? It does not seem like it is needed for
parsing.

Ah, this is a tricky bit. Parsing patterns as variable reference expressions
creates NewUnresolved references in current scope. This BlockState makes arrow
function scope the current scope - otherwise we will create NewUnresolved in
outer scope - this is logically wrong, but not only - in case of lazy parsing
the outer scope is immutable, so we assert.

https://codereview.chromium.org/1167393005/diff/20001/src/parser.cc#newcode3813
src/parser.cc:3813: // TODO(wingo): Support rest parameters.
On 2015/06/15 15:21:59, arv wrote:
> Andy implemented this last week.

Removed.

https://codereview.chromium.org/1167393005/diff/20001/test/cctest/test-parsin...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1167393005/diff/20001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6617: "{a : a}",
On 2015/06/15 15:21:59, arv wrote:
> Isn't this case already covered on line 6405?

Removed.

[Dmitry Lomov (no reviews), 2015-06-15 16:12:56]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-06-15 16:12:56]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1167393005/#ps40001
(title: "CR feedback + rebase")

[commit-bot: I haz the power, 2015-06-15 16:13:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1167393005/40001

[commit-bot: I haz the power, 2015-06-15 17:06:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-15 17:06:56]

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