[es6] support rest parameters in arrow functions

[es6] support rest parameters in arrow functions

BUG=v8:2159
LOG=N
R=marja@chromium.org, arv@chromium.org, dslomov@chromium.org

[caitp (gmail), 2015-03-09 22:27:34]

okay... so, this incorporates a change to the parser to support more lookahead
tokens, and it's kind of a mess.

But how does the rest of it look?

[marja, 2015-03-10 09:11:20]

What do the efficiency results for this say? (You can try with e.g.,
parser-shell and a big .js file, plus benchmarks like mandreel and code load).

The peeking approach lg in general.

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode914
src/preparser.h:914: return TypeField::decode(code_) & kSpreadExpression;
Hmm... so this works because kSpreadExpression happens to be 4 (which is a
single bit), right? But it's fragile and wouldn't work if somebody reorders the
Type enum...

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode964
src/preparser.h:964: kSpreadExpression
So maybe add "= 1 << 3 " here to hint that kSpreadExpression needs to be a
single-bit thing.

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode973
src/preparser.h:973: enum ExpressionType {
(Huh, why do we have Type and ExpressionType? Urgh. Seems to be coming from
https://codereview.chromium.org/422363002 .)

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode1968
src/preparser.h:1968: int x = 5;
?

https://codereview.chromium.org/987083003/diff/20001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/987083003/diff/20001/src/scanner.cc#newcode717
src/scanner.cc:717: // Not supported when there is multiple lookahead tokens
... what prevents this from happening?

[arv (Not doing code reviews), 2015-03-10 13:44:03]

This look ahead looks scary.

I don't think you need it for rest params in arrow functions. The reason is that
we can validate the paren expression when we close the `)`. If we see a `=>` we
go back and check that there are no spread expressions in non tail position. If
we don't see a `=>` we check that there are no spread expressions. We can also
keep track of the first spread location and other data we need so we don't
actually have to go back over the ast.

The only complicated thing about this is the order we report errors (but I think
we might be getting this wrong in other places too):

(...x, &y)

should probably report `...` before `&`.

https://codereview.chromium.org/987083003/diff/20001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/987083003/diff/20001/src/ast.h#newcode95
src/ast.h:95: V(SpreadOperation)            \
Maybe just Spread?

https://codereview.chromium.org/987083003/diff/20001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/987083003/diff/20001/src/full-codegen.cc#newc...
src/full-codegen.cc:292: UNREACHABLE();
This does not seem right. Shouldn't this be:

Visit(expr->expression());

... or maybe I am thinking to far ahead when we have array and call spread.

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

https://codereview.chromium.org/987083003/diff/20001/src/parser.cc#newcode3556
src/parser.cc:3556: bool is_rest = false;
Maybe this should go after the IsVariableProxy check since the comment above is
about that.

Or at least update the comment.

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode1894
src/preparser.h:1894: //   ... Identifier ')' '=>'
Why ')' '=>'?

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode1968
src/preparser.h:1968: int x = 5;
On 2015/03/10 09:11:19, marja wrote:
> ?

Looks like debug code... remove?

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode2019
src/preparser.h:2019: &is_strict_reserved_name, CHECK_OK);
I don't see where is_strict_reserved_name is being checked?
ArrowFormalParameters are always strict.

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode2031
src/preparser.h:2031: ReportUnexpectedTokenAt(scanner_->peek_location(1),
peek(1));
Another option here is to report the invalid `...`

[caitp (gmail), 2015-03-10 14:47:51]

I haven't run benchmarks, but I feel like the peeking approach is probably going
to slow things down a lot. That doesn't mean that multiple lookahead wouldn't be
useful, but it might need some fine-tuning.

The use of ::memmove() when skipping might be expensive (but, this is replacing
an extra Scan() which already happened, so maybe it's not that bad).

I would prefer to not need multiple lookahead tokens, but maybe it's more
worthwhile to just do as Erik suggested and add error checking for Spread
expressions everywhere a primary expression is expected

https://codereview.chromium.org/987083003/diff/20001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/987083003/diff/20001/src/full-codegen.cc#newc...
src/full-codegen.cc:292: UNREACHABLE();
On 2015/03/10 13:44:02, arv wrote:
> This does not seem right. Shouldn't this be:
> 
> Visit(expr->expression());
> 
> ... or maybe I am thinking to far ahead when we have array and call spread.

eventually yeah, but for now it's not really needed since these nodes are
removed from the graph

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode1894
src/preparser.h:1894: //   ... Identifier ')' '=>'
On 2015/03/10 13:44:02, arv wrote:
> Why ')' '=>'?

So, you're right that we can just check for the `)`, but this means there are
potentially a lot more places where we have to look for problems. I liked doing
it this way because it meant there was only one place where a syntax error could
occur

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode1968
src/preparser.h:1968: int x = 5;
On 2015/03/10 13:44:02, arv wrote:
> On 2015/03/10 09:11:19, marja wrote:
> > ?
> 
> Looks like debug code... remove?

Hah, yeah --- I'll get rid of that =)

https://codereview.chromium.org/987083003/diff/20001/src/preparser.h#newcode2019
src/preparser.h:2019: &is_strict_reserved_name, CHECK_OK);
On 2015/03/10 13:44:03, arv wrote:
> I don't see where is_strict_reserved_name is being checked?
> ArrowFormalParameters are always strict.

to match the function signature... pretty much just trying to match what's
happening in CheckAndDeclareArrowFunctionParameters or whatever it's called.

https://codereview.chromium.org/987083003/diff/20001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/987083003/diff/20001/src/scanner.cc#newcode717
src/scanner.cc:717: // Not supported when there is multiple lookahead tokens
On 2015/03/10 09:11:19, marja wrote:
> ... what prevents this from happening?

it just seemed complicated to make it work with multiple lookaheads

[caitp (gmail), 2015-03-10 15:40:37]

speaking of running benchmarks, is there any way to run the Octane suite with
patched v8, without having to build Chromium with it? Having trouble finding
resources for this

[Jakob Kummerow, 2015-03-10 19:00:19]

On 2015/03/10 15:40:37, caitp wrote:
> speaking of running benchmarks, is there any way to run the Octane suite with
> patched v8, without having to build Chromium with it? Having trouble finding
> resources for this

$ svn checkout http://octane-benchmark.googlecode.com/svn/trunk/ octane
$ cd octane
$ .../path/to/d8 run.js

If you want to run it from your V8 workspace, copy run.js and edit the value of
"var base_dir" (first non-comment line in run.js) to be relative to the desired
working directory.

[caitp (gmail), 2015-03-10 19:03:30]

On 2015/03/10 19:00:19, Jakob wrote:
> On 2015/03/10 15:40:37, caitp wrote:
> > speaking of running benchmarks, is there any way to run the Octane suite
with
> > patched v8, without having to build Chromium with it? Having trouble finding
> > resources for this
> 
> $ svn checkout http://octane-benchmark.googlecode.com/svn/trunk/ octane
> $ cd octane
> $ .../path/to/d8 run.js
> 
> If you want to run it from your V8 workspace, copy run.js and edit the value
of
> "var base_dir" (first non-comment line in run.js) to be relative to the
desired
> working directory.

Ah, thank you for clarifying =)

[caitp (gmail), 2015-03-10 19:44:24]

https://docs.google.com/spreadsheets/d/1p2Q6kIVRTeUCa4iJJoV6MjO2s5NvBFI5YmVXT...

is a spreadsheet with 2 single runs (statistically not very significant), but
they look comparable.