[destructuring] Implement parameter pattern matching.

[destructuring] Implement parameter pattern matching.

Scoping for initializers is yet incorrect. Defaults are not supported.

R=arv@chromium.org,rossberg@chromium.org
BUG=v8:811
LOG=N

Committed: https://crrev.com/42f30f4ded2b1ca0c4caa7639e6206e93c78ee70
Cr-Commit-Position: refs/heads/master@{#29184}

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

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

PTAL

[arv (Not doing code reviews), 2015-06-16 16:56:36]

Now we can add tests for duplicate param names with destructuring.

// sloppy
function f(x, {x}) {  // SyntaxError
}

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:699: var g = {a, b} => { return a - b; };
This should be a SyntaxError. Only when using a single BindingIdentifier is the
parens optional.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defin...

Remove and file bug.

[rossberg, 2015-06-16 17:02:13]

https://codereview.chromium.org/1189743003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1189743003/diff/1/src/parser.cc#newcode4227
src/parser.cc:4227: descriptor.mode = VAR;
Shouldn't this be LET?

https://codereview.chromium.org/1189743003/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1189743003/diff/1/src/scopes.h#newcode377
src/scopes.h:377: Expression* pattern;
Can we not abuse the scope abstraction to pass around parts of the AST? It
should be possible to get this information to the Build function in parser.cc
without piggybacking on the scope. It doesn't belong there.

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:692: (function TestParameters() {
Can we have a few more tests? In particular, including arrow patterns and nested
patterns?
Also, checks that "function({x}) { var x; }' etc is a syntax error.

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:705: var h = {a, b} => a - b;
AFAICT this is not legal ES6 syntax, see the grammar in
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi...

[caitp (gmail), 2015-06-16 19:08:08]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2015-06-16 19:08:09]

https://codereview.chromium.org/1189743003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1189743003/diff/1/src/parser.cc#newcode4227
src/parser.cc:4227: descriptor.mode = VAR;
On 2015/06/16 17:02:12, rossberg wrote:
> Shouldn't this be LET?

I think this is VAR unless computed property names and/or initializers are used,
ie if hasParameterExpressions is true (9.2.12 step 27/28) --- I might be wrong
though, maybe it's always LET for patterns

[Dmitry Lomov (no reviews), 2015-06-17 12:28:48]

Refactored formal parameter parsing to better encapsulate state and separate
concerns.
Will now address other comments.

https://codereview.chromium.org/1189743003/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1189743003/diff/1/src/scopes.h#newcode377
src/scopes.h:377: Expression* pattern;
On 2015/06/16 17:02:13, rossberg wrote:
> Can we not abuse the scope abstraction to pass around parts of the AST? It
> should be possible to get this information to the Build function in parser.cc
> without piggybacking on the scope. It doesn't belong there.

Good point. Addressed.

[Dmitry Lomov (no reviews), 2015-06-17 12:29:07]

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

[commit-bot: I haz the power, 2015-06-17 12:29:16]

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

[commit-bot: I haz the power, 2015-06-17 12:55:24]

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

[commit-bot: I haz the power, 2015-06-17 12:55:25]

Dry run: This issue passed the CQ dry run.

[Dmitry Lomov (no reviews), 2015-06-18 15:00:59]

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

[commit-bot: I haz the power, 2015-06-18 15:01:21]

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

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

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

[commit-bot: I haz the power, 2015-06-18 15:31:18]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)

[Dmitry Lomov (no reviews), 2015-06-19 15:25:14]

Please take another look

https://codereview.chromium.org/1189743003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1189743003/diff/1/src/parser.cc#newcode4227
src/parser.cc:4227: descriptor.mode = VAR;
Changing to LET - it is always LET for patterns since it means
IsSimpleParameterList =false

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
File test/mjsunit/harmony/destructuring.js (right):

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:692: (function TestParameters() {
On 2015/06/16 17:02:13, rossberg wrote:
> Can we have a few more tests? In particular, including arrow patterns and
nested
> patterns?
> Also, checks that "function({x}) { var x; }' etc is a syntax error.

Done.

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:699: var g = {a, b} => { return a - b; };
On 2015/06/16 16:56:36, arv wrote:
> This should be a SyntaxError. Only when using a single BindingIdentifier is
the
> parens optional.
> 
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defin...
> 
> Remove and file bug.

Done.

https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destru...
test/mjsunit/harmony/destructuring.js:705: var h = {a, b} => a - b;
On 2015/06/16 17:02:13, rossberg wrote:
> AFAICT this is not legal ES6 syntax, see the grammar in
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-defi...

Done.

[rossberg, 2015-06-19 16:25:52]

rossberg@chromium.org changed reviewers:
+ wingo@igalia.com

[rossberg, 2015-06-19 16:26:35]

Looks good, but I'm on my way out and only had time for a superficial look. I
think Andy should have closer look as well.

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

https://codereview.chromium.org/1189743003/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6627: "var f = {x,y} => {};",
{x,x} ? But isn't this a plain syntax error anyway?

https://codereview.chromium.org/1189743003/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6653: "var f = {x} => {};",
Same here, this test is unrelated to duplicates.

[arv (Not doing code reviews), 2015-06-19 19:25:26]

LGTM

If you want to handle the property name issue later that is fine with me.

https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc#newcode2057
src/parser.cc:2057: Expression* expression = NewThrowSyntaxError(
Is there a test for this?

https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc#newcode4301
src/parser.cc:4301: 
-1 newline

https://codereview.chromium.org/1189743003/diff/60001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1189743003/diff/60001/src/parser.h#newcode554
src/parser.h:554: 
-1 newline

https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h#newcode...
src/preparser.h:2418:
classifier->RecordDuplicateFormalParameterError(scanner()->location());
The following is not supposed to be an error.

  function f({x: y, x: z}) {}


Can you add a test that this works?

[Dmitry Lomov (no reviews), 2015-06-22 10:13:41]

dslomov@chromium.org changed required reviewers:
+ arv@chromium.org, wingo@igalia.com

[Dmitry Lomov (no reviews), 2015-06-22 10:14:10]

Comments addressed (sorry had to rebase).
PTAL

https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc#newcode4301
src/parser.cc:4301: 
On 2015/06/19 19:25:26, arv wrote:
> -1 newline

Done.

https://codereview.chromium.org/1189743003/diff/60001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1189743003/diff/60001/src/parser.h#newcode554
src/parser.h:554: 
On 2015/06/19 19:25:26, arv wrote:
> -1 newline

Done.

https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h#newcode...
src/preparser.h:2418:
classifier->RecordDuplicateFormalParameterError(scanner()->location());
On 2015/06/19 19:25:26, arv wrote:
> The following is not supposed to be an error.
> 
>   function f({x: y, x: z}) {}
> 
> 
> Can you add a test that this works?
> 

Done.

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

https://codereview.chromium.org/1189743003/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6627: "var f = {x,y} => {};",
On 2015/06/19 16:26:35, rossberg wrote:
> {x,x} ? But isn't this a plain syntax error anyway?

Moved to separate test.

https://codereview.chromium.org/1189743003/diff/60001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6653: "var f = {x} => {};",
On 2015/06/19 16:26:35, rossberg wrote:
> Same here, this test is unrelated to duplicates.

Moved to separate test.

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

Sending half-done review, now to look at rebase.  Overall looks pretty swell but
one question below.

https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h#newcode...
src/preparser.h:3522: //   BindingElement[?Yield, ?GeneratorParameter]
if is_rest is true then we are parsing BindingRestElement, which is only an
identifier.  That is, this is an error AFAIU:

  function foo(...{a,b}) {}

Probably here is the right place to signal an error if is_rest but not
Traits::IsIdentifier(pattern).  We need to do a similar thing in
Parser::ParseArrowFunctionFormalParameters I think.

https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h#newcode...
src/preparser.h:3660: NULL, Token::INIT_VAR, kArrowFunction, CHECK_OK);
Nit: const Variable *name = nullptr, pass by name?  As you like.

[wingo, 2015-06-22 10:28:34]

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

https://codereview.chromium.org/1189743003/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6608: TEST(DestructuringDuplicateParams) {
Need tests that patterns are not allowed in rest parameters

function(...{}) {}
function(...{x}) {}
function(...[x]) {}
(...{}) => {}
(...{x}) => {}
(...[x]) => {}

https://codereview.chromium.org/1189743003/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6630: "function f(x, x, {a}) {}",
Arrow function tests would be nice here:

  ({x, x}) => {}
  (x, {y, y}) => {}
  (x, {x, y}) => {}
  ({x}, ...x) => {}

  ([x, x]) => {}
  (x, [y, y]) => {}
  (x, [x, y]) => {}
  ([x], ...x) => {}

[wingo, 2015-06-22 10:43:18]

LGTM with nit above about rest patterns.

That said :)  Is this expected to work for initializers of eagerly-parsed arrow
functions?  I would expect that the literal_count() or the property_count() of
the initializers would be recorded in the outer scope and not in the arrow
function's scope, and so you'd have to fix up somethingorother.  Where would
that happen?

https://codereview.chromium.org/1189743003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1189743003/diff/80001/src/parser.cc#newcode2057
src/parser.cc:2057: Expression* expression = NewThrowSyntaxError(
nice!

https://codereview.chromium.org/1189743003/diff/80001/src/parser.cc#newcode3848
src/parser.cc:3848: parser_->scope_->RemoveUnresolved(expr->AsVariableProxy());
I see, for patterns this happens in Parser::PatternRewriter::VisitVariableProxy.
 A comment would be appreciated tho :)

[Dmitry Lomov (no reviews), 2015-06-22 11:11:43]

Comments addressed. 
Initializers (+default parameters) have completely wrong scope so far. I'll
address everything relating to initializers in the follow-up

https://codereview.chromium.org/1189743003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1189743003/diff/80001/src/parser.cc#newcode3848
src/parser.cc:3848: parser_->scope_->RemoveUnresolved(expr->AsVariableProxy());
On 2015/06/22 10:43:18, wingo wrote:
> I see, for patterns this happens in
Parser::PatternRewriter::VisitVariableProxy.
>  A comment would be appreciated tho :)

Done.

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

https://codereview.chromium.org/1189743003/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6608: TEST(DestructuringDuplicateParams) {
On 2015/06/22 10:28:33, wingo wrote:
> Need tests that patterns are not allowed in rest parameters
> 
> function(...{}) {}
> function(...{x}) {}
> function(...[x]) {}
> (...{}) => {}
> (...{x}) => {}
> (...[x]) => {}

Done (also fixed)

https://codereview.chromium.org/1189743003/diff/80001/test/cctest/test-parsin...
test/cctest/test-parsing.cc:6630: "function f(x, x, {a}) {}",
On 2015/06/22 10:28:33, wingo wrote:
> Arrow function tests would be nice here:
> 
>   ({x, x}) => {}
>   (x, {y, y}) => {}
>   (x, {x, y}) => {}
>   ({x}, ...x) => {}
> 
>   ([x, x]) => {}
>   (x, [y, y]) => {}
>   (x, [x, y]) => {}
>   ([x], ...x) => {}

For arrow functions duplicate parameter detection does not work in preparser (it
is too expensive to use duplicate finder).
The test are in destructuring.js

[Dmitry Lomov (no reviews), 2015-06-22 11:11:51]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-06-22 11:11:52]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
wingo@igalia.com
Link to the patchset: https://codereview.chromium.org/1189743003/#ps100001
(title: "Feedback + rebased for landing")

[commit-bot: I haz the power, 2015-06-22 11:12:03]

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

[wingo, 2015-06-22 11:14:58]

One nit for a followup perhaps.

https://codereview.chromium.org/1189743003/diff/100001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1189743003/diff/100001/src/preparser.h#newcod...
src/preparser.h:3552: ReportUnexpectedToken(next);
Nit: will this give the correct source location for the bad token?  I'm thinking
it will point after the pattern instead of at the pattern.

[commit-bot: I haz the power, 2015-06-22 11:27:14]

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

[commit-bot: I haz the power, 2015-06-22 11:27:15]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Dmitry Lomov (no reviews), 2015-06-22 11:43:26]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-06-22 11:43:27]

The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com,
arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1189743003/#ps120001
(title: "Updated expectations")

[commit-bot: I haz the power, 2015-06-22 11:43:35]

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

[commit-bot: I haz the power, 2015-06-22 12:07:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-22 12:07:22]

Patchset 7 (id:??) landed as
https://crrev.com/42f30f4ded2b1ca0c4caa7639e6206e93c78ee70
Cr-Commit-Position: refs/heads/master@{#29184}

[Michael Achenbach, 2015-06-22 13:13:53]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1195163007/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks tsan:
http://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/4392.

[Dmitry Lomov (no reviews), 2015-06-22 13:40:26]

dslomov@chromium.org changed reviewers:
+ yangguo@chromium.org

[Dmitry Lomov (no reviews), 2015-06-22 13:40:28]

PS#8 attempts to fix a read-bogus-memory serialization issue.
Yang, ptal at a change in serialize.cc

[Dmitry Lomov (no reviews), 2015-06-22 13:52:32]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-06-22 13:52:33]

The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com,
arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1189743003/#ps140001
(title: "Fix magic number issue")

[commit-bot: I haz the power, 2015-06-22 13:52:45]

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

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

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-22 14:16:11]

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