Support rest parameters in arrow functions

Support rest parameters in arrow functions

R=dslomov@chromium.org, rossberg@chromium.org
LOG=Y
BUG=v8:2700
Committed: https://crrev.com/e7a62c24855ac2bb34271e997ffae5ffcf57862d
Cr-Commit-Position: refs/heads/master@{#28908}

[wingo, 2015-06-10 10:18:41]

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

[commit-bot: I haz the power, 2015-06-10 10:18:49]

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

[commit-bot: I haz the power, 2015-06-10 10:43:21]

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

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

Dry run: This issue passed the CQ dry run.

[Dmitry Lomov (no reviews), 2015-06-10 11:04:41]

https://codereview.chromium.org/1178523002/diff/20001/src/expression-classifi...
File src/expression-classifier.h (right):

https://codereview.chromium.org/1178523002/diff/20001/src/expression-classifi...
src/expression-classifier.h:40: ArrowFormalParametersWithRestProduction = 1 <<
7,
I am not too happy about this irregularity. We aro no longer using the
classifier to report errors, but also to propagate information.

How about this: afaiu this bit is only used in parser, and parser can just
analyze the ASt directly to see if the last parameter is a spread operation or
not. WDYT?

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

https://codereview.chromium.org/1178523002/diff/20001/src/parser.cc#newcode3811
src/parser.cc:3811: if (!duplicate_loc->IsValid()) {
Oops, thanks for fixing this! Can we add a test that fails without this fix? (ok
in separate CL)

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

Fixed to use Spread expression to mark rest parameter presence; ptal :)

https://codereview.chromium.org/1178523002/diff/20001/src/expression-classifi...
File src/expression-classifier.h (right):

https://codereview.chromium.org/1178523002/diff/20001/src/expression-classifi...
src/expression-classifier.h:40: ArrowFormalParametersWithRestProduction = 1 <<
7,
On 2015/06/10 11:04:41, Dmitry S. Lomov wrote:
> I am not too happy about this irregularity. We aro no longer using the
> classifier to report errors, but also to propagate information.
> 
> How about this: afaiu this bit is only used in parser, and parser can just
> analyze the ASt directly to see if the last parameter is a spread operation or
> not. WDYT?

Done.

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

https://codereview.chromium.org/1178523002/diff/20001/src/parser.cc#newcode3811
src/parser.cc:3811: if (!duplicate_loc->IsValid()) {
On 2015/06/10 11:04:41, Dmitry S. Lomov wrote:
> Oops, thanks for fixing this! Can we add a test that fails without this fix?
(ok
> in separate CL)

Sure.  I'll see if I can cook one up for this CL actually.

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

On 2015/06/10 13:21:10, wingo wrote:
> On 2015/06/10 11:04:41, Dmitry S. Lomov wrote:
>
https://codereview.chromium.org/1178523002/diff/20001/src/parser.cc#newcode3811
> src/parser.cc:3811: if (!duplicate_loc->IsValid()) {
> On 2015/06/10 11:04:41, Dmitry S. Lomov wrote:
> > Oops, thanks for fixing this! Can we add a test that fails without this fix?
> (ok
> > in separate CL)
> 
> Sure.  I'll see if I can cook one up for this CL actually.

done

[rossberg, 2015-06-10 13:43:53]

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

https://codereview.chromium.org/1178523002/diff/60001/src/preparser.h#newcode...
src/preparser.h:2266: ReportMessageAt(scanner()->peek_location(),
Nit: Should this really be an eager error here? It seems like it would report
something like

  (...x,y);

as a malformed rest parameter, although it's not even a parameter list.

https://codereview.chromium.org/1178523002/diff/60001/test/mjsunit/harmony/ar...
File test/mjsunit/harmony/arrow-rest-params.js (right):

https://codereview.chromium.org/1178523002/diff/60001/test/mjsunit/harmony/ar...
test/mjsunit/harmony/arrow-rest-params.js:6: 
Maybe add a few representative tests of syntax errors around arrows & rest.

[caitp (gmail), 2015-06-10 14:30:48]

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

[caitp (gmail), 2015-06-10 14:30:49]

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

https://codereview.chromium.org/1178523002/diff/60001/src/preparser.h#newcode...
src/preparser.h:2266: ReportMessageAt(scanner()->peek_location(),
On 2015/06/10 13:43:53, rossberg wrote:
> Nit: Should this really be an eager error here? It seems like it would report
> something like
> 
>   (...x,y);
> 
> as a malformed rest parameter, although it's not even a parameter list.

We should mark it in the expression classifier instead, and report it later on
in ParseAssignmentExpression when the arrow is peeked

[wingo, 2015-06-10 14:54:27]

On 2015/06/10 13:43:53, rossberg wrote:
> https://codereview.chromium.org/1178523002/diff/60001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/1178523002/diff/60001/src/preparser.h#newcode...
> src/preparser.h:2266: ReportMessageAt(scanner()->peek_location(),
> Nit: Should this really be an eager error here? It seems like it would report
> something like
> 
>   (...x,y);
> 
> as a malformed rest parameter, although it's not even a parameter list.

It wouldn't, actually, because (... is parsed with a different case.  But (x,
...y, z) would trigger this case.

But I don't even know.  At that point the production cannot be valid but we
don't know what error to signal.  Do I set *ok=false?  If I do so, then a whole
chain of CHECK_OK's will trigger, but without having signalled an error.  If I
don't I guess I can continue to pack expressions into the Binary(COMMA) result I
guess, which is bogus but probably not dangerous at least.  I guess I'll do
that.

>
https://codereview.chromium.org/1178523002/diff/60001/test/mjsunit/harmony/ar...
> test/mjsunit/harmony/arrow-rest-params.js:6: 
> Maybe add a few representative tests of syntax errors around arrows & rest.

Ack.

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

Fixed nits, PTAL :)

[wingo, 2015-06-10 15:24:48]

On 2015/06/10 15:22:34, wingo wrote:
> Fixed nits, PTAL :)

Gaah, it looks like some of the must-be-last error locations are a token off. 
Will fix.

[rossberg, 2015-06-10 15:26:43]

lgtm

[wingo, 2015-06-10 15:31:38]

The CQ bit was checked by wingo@igalia.com

[wingo, 2015-06-10 15:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1178523002/#ps100001
(title: "Fix error locations for param-after-rest errors")

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

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

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

On 2015/06/10 15:24:48, wingo wrote:
> On 2015/06/10 15:22:34, wingo wrote:
> > Fixed nits, PTAL :)
> 
> Gaah, it looks like some of the must-be-last error locations are a token off. 
> Will fix.

Do you want to fix this in this CL or in a follow up?

[wingo, 2015-06-10 15:35:34]

On 2015/06/10 15:34:14, arv wrote:
> On 2015/06/10 15:24:48, wingo wrote:
> > On 2015/06/10 15:22:34, wingo wrote:
> > > Fixed nits, PTAL :)
> > 
> > Gaah, it looks like some of the must-be-last error locations are a token
off. 
> > Will fix.
> 
> Do you want to fix this in this CL or in a follow up?

Fixed in patchset 6 I think.

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

arv@chromium.org changed reviewers:
+ arv@chromium.org

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

LGTM

https://codereview.chromium.org/1178523002/diff/100001/test/message/arrow-par...
File test/message/arrow-param-after-rest.out (right):

https://codereview.chromium.org/1178523002/diff/100001/test/message/arrow-par...
test/message/arrow-param-after-rest.out:3: ^
I Thought you meant that this should have been pointing at ...

But I guess this is OK.

[wingo, 2015-06-10 15:50:51]

On 2015/06/10 15:40:01, arv wrote:
> LGTM
> 
>
https://codereview.chromium.org/1178523002/diff/100001/test/message/arrow-par...
> File test/message/arrow-param-after-rest.out (right):
> 
>
https://codereview.chromium.org/1178523002/diff/100001/test/message/arrow-par...
> test/message/arrow-param-after-rest.out:3: ^
> I Thought you meant that this should have been pointing at ...
> 
> But I guess this is OK.

Hadn't thought of that.  The error was that it pointed to the token after the
comma after the rest param.  I'm inclined to punt but if someone feels strongly
about this I'm happy to follow up :)

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

On 2015/06/10 15:50:51, wingo wrote:
> On 2015/06/10 15:40:01, arv wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1178523002/diff/100001/test/message/arrow-par...
> > File test/message/arrow-param-after-rest.out (right):
> > 
> >
>
https://codereview.chromium.org/1178523002/diff/100001/test/message/arrow-par...
> > test/message/arrow-param-after-rest.out:3: ^
> > I Thought you meant that this should have been pointing at ...
> > 
> > But I guess this is OK.
> 
> Hadn't thought of that.  The error was that it pointed to the token after the
> comma after the rest param.  I'm inclined to punt but if someone feels
strongly
> about this I'm happy to follow up :)

We get the same error for non arrows. I think it is fine as then.

[commit-bot: I haz the power, 2015-06-10 16:11:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-10 16:11:39]

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