Fix parsing of arrow function formal parameters

Fix parsing of arrow function formal parameters

Not all parenthesized AssignmentExpressions whose components are valid
binding patterns are valid arrow function formal parameters.  In
particular (a,b,c)() is not valid, and in general the existing code
wasn't catching the tail productions of ConditionalExpression,
BinaryExpression, PostfixExpression, LeftHandSideExpression,
and MemberExpression.

Thanks to Adrian Perez for the test case.

BUG=v8:4211
LOG=Y
R=rossberg@chromium.org

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

[wingo, 2015-08-20 13:38:41]

PTAL.  This one is pretty serious and probably should be uplifted to beta (45)
before it becomes stable.

[wingo, 2015-08-20 13:38:53]

The CQ bit was checked by wingo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-20 13:39:06]

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

[wingo, 2015-08-20 13:57:09]

On 2015/08/20 13:39:06, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1306583002/20001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1306583002/20001

I had a hard time finding a way to test this one in the ConditionalExpression
case, because the grammar goes like this:

LogicalOrExpression '?' AssignmentExpression : AssignmentExpression

so usually the RHS will eat the arrow.  But I just managed to get, without this
patch:

d8> (a)?b:c=>{}=>e
(a)?b:c=>{}=>e

Buf.  I'll add this to the tests.

[wingo, 2015-08-20 14:03:52]

Patchset #3 (id:40001) has been deleted

[wingo, 2015-08-20 14:04:59]

The CQ bit was checked by wingo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-20 14:05:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306583002/60001

[caitp (gmail), 2015-08-20 15:13:39]

On 2015/08/20 14:05:16, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1306583002/60001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1306583002/60001

the array-native-elements failure doesn't really look related to this, and it's
affecting at least one other build on the v8 waterfall, so maybe should file a
bug about that test case?

[commit-bot: I haz the power, 2015-08-20 15:20:01]

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

[commit-bot: I haz the power, 2015-08-20 15:20:01]

Dry run: This issue passed the CQ dry run.

[caitp (gmail), 2015-08-20 15:22:56]

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

[caitp (gmail), 2015-08-20 15:22:57]

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

https://codereview.chromium.org/1306583002/diff/60001/src/preparser.h#newcode...
src/preparser.h:3604: ArrowFormalParametersUnexpectedToken(classifier);
probably want to make sure destructuring still works properly, `({ x: x =
foo.bar }) => {}` for example. I don't expect `({ x = foo.bar }) => {}` to work
properly because CoverInitializedName parsing is pretty messed up in
maybe-expressions, might be worth marking that as a TODO though in the test case

This also applies to other tokens which can legally occur in binding patterns
within arrow formals, I guess.

If this is covered by other tests, then that's ok

[wingo, 2015-08-20 15:47:57]

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

https://codereview.chromium.org/1306583002/diff/60001/src/preparser.h#newcode...
src/preparser.h:3604: ArrowFormalParametersUnexpectedToken(classifier);
On 2015/08/20 15:22:57, caitp wrote:
> probably want to make sure destructuring still works properly, `({ x: x =
> foo.bar }) => {}` for example. I don't expect `({ x = foo.bar }) => {}` to
work
> properly because CoverInitializedName parsing is pretty messed up in
> maybe-expressions, might be worth marking that as a TODO though in the test
case
> 
> This also applies to other tokens which can legally occur in binding patterns
> within arrow formals, I guess.
> 
> If this is covered by other tests, then that's ok

Thanks for having a look!

I'm pretty sure that destructuring should still work properly, and the reasoning
is a bit convoluted: an assignmentexpression is a valid arrow formal parameter
list if it is a bare identifier, or if it starts with a parenthesis, only
contains binding patterns separated by commas, possibly ending in a rest pattern
(which, oddly, can't contain patterns).  OK.  That is what was already the case
before and is covered by DestructuringPositiveTests and
DestructuringNegativeTests in test-parser.cc.

The way the above classifier works is that when parsing a parenthesized list in
ParsePrimaryExpression, the expression as a whole remains a valid
ArrowFormalParameterList iff the inner productions are valid BindingPatterns. 
Specifically the invalid-ArrowFormalParameterList *does not* accumulate out from
the individual arguments -- instead it's the BindingPattern flag that propagates
out.  See the gnarly special case in ExpressionClassifier::Accumulate.  So I
don't think additional tests are necessary for existing destructuring foo.

What we missed is that the classifier would accept things as valid
ArrowFormalParameterLists that weren't valid.  That's what this patch fixes. 
These additional flags won't cause problems for arrow function formals because
they are explicitly ignored there.

[rossberg, 2015-08-20 16:04:43]

lgtm

[caitp (gmail), 2015-08-20 16:21:04]

On 2015/08/20 15:47:57, wingo wrote:
> https://codereview.chromium.org/1306583002/diff/60001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/1306583002/diff/60001/src/preparser.h#newcode...
> src/preparser.h:3604: ArrowFormalParametersUnexpectedToken(classifier);
> On 2015/08/20 15:22:57, caitp wrote:
> > probably want to make sure destructuring still works properly, `({ x: x =
> > foo.bar }) => {}` for example. I don't expect `({ x = foo.bar }) => {}` to
> work
> > properly because CoverInitializedName parsing is pretty messed up in
> > maybe-expressions, might be worth marking that as a TODO though in the test
> case
> > 
> > This also applies to other tokens which can legally occur in binding
patterns
> > within arrow formals, I guess.
> > 
> > If this is covered by other tests, then that's ok
> 
> Thanks for having a look!
> 
> I'm pretty sure that destructuring should still work properly, and the
reasoning
> is a bit convoluted: an assignmentexpression is a valid arrow formal parameter
> list if it is a bare identifier, or if it starts with a parenthesis, only
> contains binding patterns separated by commas, possibly ending in a rest
pattern
> (which, oddly, can't contain patterns).  OK.  That is what was already the
case
> before and is covered by DestructuringPositiveTests and
> DestructuringNegativeTests in test-parser.cc.
> 
> The way the above classifier works is that when parsing a parenthesized list
in
> ParsePrimaryExpression, the expression as a whole remains a valid
> ArrowFormalParameterList iff the inner productions are valid BindingPatterns. 
> Specifically the invalid-ArrowFormalParameterList *does not* accumulate out
from
> the individual arguments -- instead it's the BindingPattern flag that
propagates
> out.  See the gnarly special case in ExpressionClassifier::Accumulate.  So I
> don't think additional tests are necessary for existing destructuring foo.
> 
> What we missed is that the classifier would accept things as valid
> ArrowFormalParameterLists that weren't valid.  That's what this patch fixes. 
> These additional flags won't cause problems for arrow function formals because
> they are explicitly ignored there.

Cool. Since default parameters are implemented,a similar test case might be
useful there. It mIght end up working out the same way, it's just hard to see if
there's a test case like that in test-parsing by grepping, though.

[wingo, 2015-08-21 09:27:29]

On 2015/08/20 16:21:04, caitp wrote:
> Since default parameters are implemented,a similar test case might be
> useful there. It mIght end up working out the same way, it's just hard to see
if
> there's a test case like that in test-parsing by grepping, though.

Good idea!  Although there are tests for default values for destructuring
patterns in test-parsing I don't see any for default parameter values.  Probably
need to add some, will do in a followup.

[wingo, 2015-08-21 09:27:35]

The CQ bit was checked by wingo@igalia.com

[commit-bot: I haz the power, 2015-08-21 09:27:39]

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

[commit-bot: I haz the power, 2015-08-21 09:28:58]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-08-21 09:29:16]

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