Remove duplicated code from comma-separated Expression parsing

Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.

The only change in behavior is in a few error messages.

Committed: https://crrev.com/9c00c889025f1a69c4f2f9fce793aff377b35b78
Cr-Commit-Position: refs/heads/master@{#39030}

[adamk, 2016-08-26 20:07:58]

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

[commit-bot: I haz the power, 2016-08-26 20:08:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-08-26 20:08:44]

Description was changed from

==========
Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.
==========

to

==========
Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.

The only change in behavior is in a few error messages.
==========

[adamk, 2016-08-26 20:10:05]

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

[commit-bot: I haz the power, 2016-08-26 20:10:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-08-26 20:13:03]

adamk@chromium.org changed reviewers:
+ littledan@chromium.org, nikolaos@chromium.org

[adamk, 2016-08-26 20:13:04]

https://codereview.chromium.org/2279363002/diff/20001/src/messages.h
File src/messages.h (left):

https://codereview.chromium.org/2279363002/diff/20001/src/messages.h#oldcode458
src/messages.h:458: "Rest parameter must be an identifier or destructuring
pattern")           \
I don't think there's much lost for users in losing this message. We didn't emit
it in non-arrow functions anyway.

https://codereview.chromium.org/2279363002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2279363002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1692: while (true) {
The other loop construct that might make sense here would be a do/while. Let me
know if you'd prefer that.

https://codereview.chromium.org/2279363002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1722: if (impl()->IsEmptyExpression(result)) {
This IsEmptyExpression() thing could be replaced by a separate bool, I'd be fine
with switching to that which is less code (since it doesn't need any Impl
support) but is IMHO slightly less clear.

[commit-bot: I haz the power, 2016-08-26 20:34:59]

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

[commit-bot: I haz the power, 2016-08-26 20:35:00]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-08-26 22:48:53]

https://codereview.chromium.org/2279363002/diff/20001/src/messages.h
File src/messages.h (left):

https://codereview.chromium.org/2279363002/diff/20001/src/messages.h#oldcode458
src/messages.h:458: "Rest parameter must be an identifier or destructuring
pattern")           \
On 2016/08/26 at 20:13:03, adamk wrote:
> I don't think there's much lost for users in losing this message. We didn't
emit it in non-arrow functions anyway.

This doesn't make me all that happy. The old message was better. Could we extend
the better message to the non-arrow function case?

https://codereview.chromium.org/2279363002/diff/20001/test/mjsunit/es8/syntac...
File test/mjsunit/es8/syntactic-tail-call-parsing.js (right):

https://codereview.chromium.org/2279363002/diff/20001/test/mjsunit/es8/syntac...
test/mjsunit/es8/syntactic-tail-call-parsing.js:291: { src: `()=>{ (a, b, c, ...
continue f() ) => {}  }`,
This is also a worse message. It's only acceptable to me because the feature is
unlikely to ever ship.

[nickie, 2016-08-29 08:42:59]

I'm just adding a few comments regarding the error messages.  Not done reviewing
yet.

https://codereview.chromium.org/2279363002/diff/20001/src/messages.h
File src/messages.h (left):

https://codereview.chromium.org/2279363002/diff/20001/src/messages.h#oldcode458
src/messages.h:458: "Rest parameter must be an identifier or destructuring
pattern")           \
On 2016/08/26 22:48:53, Dan Ehrenberg wrote:
> On 2016/08/26 at 20:13:03, adamk wrote:
> > I don't think there's much lost for users in losing this message. We didn't
> emit it in non-arrow functions anyway.
> 
> This doesn't make me all that happy. The old message was better. Could we
extend
> the better message to the non-arrow function case?

In principle I would agree with Dan.  But this error is only reported very
rarely and even in those cases, the message is misleading.  All the following
wrong arrow functions produce different error messages (unexpected something or
other):

d8> f = (...42) => 42;
d8> f = (x, ...42) => 42;
d8> f = (...[42]) => 42;
d8> f = (x, ...[42]) => 42;
d8> f = (...[x+1]) => 42;
d8> f = (a, ...[x+1]) => 42;

I'm not saying that these error messages are good, only that Adam's change does
not affect these cases.

Only things like this produce InvalidRestParameter:

d8> f = (...x = []) => 42;
(d8):1: SyntaxError: Rest parameter must be an identifier or destructuring
pattern
f = (...x = []) => 42;
     ^^^^^^^^^
SyntaxError: Rest parameter must be an identifier or destructuring pattern

As I see it, the rest parameter is "x" and it is indeed an identifier.  The
problem is that it carries an initializer and that is forbidden.  I think the
error message is not good anyway.

https://codereview.chromium.org/2279363002/diff/20001/test/mjsunit/es8/syntac...
File test/mjsunit/es8/syntactic-tail-call-parsing.js (right):

https://codereview.chromium.org/2279363002/diff/20001/test/mjsunit/es8/syntac...
test/mjsunit/es8/syntactic-tail-call-parsing.js:291: { src: `()=>{ (a, b, c, ...
continue f() ) => {}  }`,
On 2016/08/26 22:48:53, Dan Ehrenberg wrote:
> This is also a worse message. It's only acceptable to me because the feature
is
> unlikely to ever ship.

I can live with it; I think it is a bad error message, but the previous one was
even worse.  Unexpected expression inside tail call?  It's the tail call
("continue" expression) that is in the wrong place, not what's inside it.

[nickie, 2016-08-29 17:21:27]

Modulo my comments, LGTM.

If we want better error messages for spread parameters, I suggest to do it in a
separate CL.

https://codereview.chromium.org/2279363002/diff/20001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2279363002/diff/20001/src/parsing/parser-base...
src/parsing/parser-base.h:1722: if (impl()->IsEmptyExpression(result)) {
On 2016/08/26 20:13:04, adamk wrote:
> This IsEmptyExpression() thing could be replaced by a separate bool, I'd be
fine
> with switching to that which is less code (since it doesn't need any Impl
> support) but is IMHO slightly less clear.

I'm in favor of the IsEmptyExpression, which I already have in the types branch.
 :-)

[adamk, 2016-08-30 23:38:58]

I've filed https://bugs.chromium.org/p/v8/issues/detail?id=5325 to track the
general badness of these errors.

Dan, would you be OK with me landing this and leaving the error message
improvement for a followup?

[Dan Ehrenberg, 2016-08-31 00:06:31]

lgtm

[adamk, 2016-08-31 00:07:56]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-08-31 00:08:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 00:46:36]

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

[commit-bot: I haz the power, 2016-08-31 00:46:37]

Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/7913)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[adamk, 2016-08-31 01:07:29]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-08-31 01:07:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 01:24:10]

Description was changed from

==========
Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.

The only change in behavior is in a few error messages.
==========

to

==========
Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.

The only change in behavior is in a few error messages.
==========

[commit-bot: I haz the power, 2016-08-31 01:24:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-31 01:24:29]

Description was changed from

==========
Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.

The only change in behavior is in a few error messages.
==========

to

==========
Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.

The only change in behavior is in a few error messages.

Committed: https://crrev.com/9c00c889025f1a69c4f2f9fce793aff377b35b78
Cr-Commit-Position: refs/heads/master@{#39030}
==========

[commit-bot: I haz the power, 2016-08-31 01:24:31]

Patchset 2 (id:??) landed as
https://crrev.com/9c00c889025f1a69c4f2f9fce793aff377b35b78
Cr-Commit-Position: refs/heads/master@{#39030}