[es6] Re-implement rest parameters via desugaring.

[es6] Re-implement rest parameters via desugaring.

Kills the kRestParameter bailout/disabled optimization, and fixes
lazily parsed arrow functions with rest parameters.

Supercedes https://crrev.com/1235153006/

BUG=chromium:508074, v8:2160, v8:2700
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, wingo@igalia.com

Committed: https://crrev.com/510baeacbab311798d5e8795800ff773d00d062c
Cr-Commit-Position: refs/heads/master@{#30550}

[caitp (gmail), 2015-08-05 20:32:21]

I could use a bit of help on this, I'm having trouble fixing the arrow functions
with rest parameters again in this version. PTAL when time permits

[adamk, 2015-08-05 22:13:02]

Looking into the crash locally, but here's some stylistic comments in the
meantime.

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

https://codereview.chromium.org/1272673003/diff/1/src/parser.cc#newcode4326
src/parser.cc:4326: auto parameter_proxy =
factory()->NewVariableProxy(parameter_var);
I'd move this down to where it's used to avoid accidentally using it twice.

https://codereview.chromium.org/1272673003/diff/1/src/parser.cc#newcode4333
src/parser.cc:4333: // for (i = rest_index; i < %_ArgumentsLength(); ++i) {
This desugaring needs updating to match what's going on below:

- Replace "i" with "arguments_index"
- No more tmp_var
- Use %AppendElement instead of %AddElement

https://codereview.chromium.org/1272673003/diff/1/src/parser.cc#newcode4358
src/parser.cc:4358: auto arguments_length =
Runtime::FunctionForId(Runtime::kArgumentsLength);
I think you want kInlineArgumentsLength here (if I'm properly understanding the
way these runtime/inline intrinsics work these days).

https://codereview.chromium.org/1272673003/diff/1/src/parser.cc#newcode4377
src/parser.cc:4377: auto arguments =
Runtime::FunctionForId(Runtime::kArguments);
kInlineArguments

https://codereview.chromium.org/1272673003/diff/1/src/parser.cc#newcode4382
src/parser.cc:4382: // %AppendElement($rest, $_Arguments(arguments_index))
Should read:

%AppendElement($rest, %_Arguments(arguments_index))

or whatever you end up calling $rest in the desugaring comment above.

[adamk, 2015-08-05 23:36:00]

After hacking on this for awhile, I think at least part of the problem is the
existing rest-handling code in scopes.cc, which I think should probably be
deleted. At the least, deleting the special handling of Scope::rest_parameter_
makes arrow functions stop crashing and start failing.

[adamk, 2015-08-06 00:23:16]

On 2015/08/05 23:36:00, adamk wrote:
> After hacking on this for awhile, I think at least part of the problem is the
> existing rest-handling code in scopes.cc, which I think should probably be
> deleted. At the least, deleting the special handling of Scope::rest_parameter_
> makes arrow functions stop crashing and start failing.

Further investigation suggests I was probably on the wrong track. Not sure where
to look next.

[caitp (gmail), 2015-08-06 02:09:45]

On 2015/08/06 00:23:16, adamk wrote:
> On 2015/08/05 23:36:00, adamk wrote:
> > After hacking on this for awhile, I think at least part of the problem is
the
> > existing rest-handling code in scopes.cc, which I think should probably be
> > deleted. At the least, deleting the special handling of
Scope::rest_parameter_
> > makes arrow functions stop crashing and start failing.
> 
> Further investigation suggests I was probably on the wrong track. Not sure
where
> to look next.

My theory is it's a LoadIC failure --- You get a reference error trying to point
to `f` after declaring `var f = (...a) => a` or similar. So, somewhere,
something is not being stored that should be, and at some point that leads to an
access general protection fault. The closest code to the generated assembly
looks like it lives in `GenerateFastArrayLoad`, to me.

[caitp (gmail), 2015-08-06 02:15:06]

On 2015/08/06 02:09:45, caitp wrote:
> On 2015/08/06 00:23:16, adamk wrote:
> > On 2015/08/05 23:36:00, adamk wrote:
> > > After hacking on this for awhile, I think at least part of the problem is
> the
> > > existing rest-handling code in scopes.cc, which I think should probably be
> > > deleted. At the least, deleting the special handling of
> Scope::rest_parameter_
> > > makes arrow functions stop crashing and start failing.
> > 
> > Further investigation suggests I was probably on the wrong track. Not sure
> where
> > to look next.
> 
> My theory is it's a LoadIC failure --- You get a reference error trying to
point
> to `f` after declaring `var f = (...a) => a` or similar. So, somewhere,
> something is not being stored that should be, and at some point that leads to
an
> access general protection fault. The closest code to the generated assembly
> looks like it lives in `GenerateFastArrayLoad`, to me.

mmm or that repro may have come from a typo :( I think the reference error was
not real.

[rossberg, 2015-08-06 14:43:53]

I also tried digging into it, but without much success. Something must be
pointing into the wild, but I couldn't find out what.

Some cases fail with an invalid access. I'd try to breakpoint on those in the
debugger and try to figure out what there pointing at and where it is coming
from. Maybe that gives a hint.

The next thing you could try is not invoking DeclareParameter on the rest param
at all, and just desugar using a temporary instead. In fact, it should be
possible to make it such that all traces of a rest parameter effectively
disappear after desugaring. (The only thing you probably need to remember is
whether the parameter list was simple, for allocating the right arguments object
later.)

You could also try to temporarily replace the desugaring with a nop. If that
doesn't make the problem go away then maybe something is f-ed up with
variable/temporary allocation?

Maybe there also is some weird problem with doing the desugaring after
processing the body. Could be worth a shot trying to undo that (with the TC39
resolution it doesn't seem necessary anymore anyway).

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

On 2015/08/06 14:43:53, rossberg wrote:
> I also tried digging into it, but without much success. Something must be
> pointing into the wild, but I couldn't find out what.
> 
> Some cases fail with an invalid access. I'd try to breakpoint on those in the
> debugger and try to figure out what there pointing at and where it is coming
> from. Maybe that gives a hint.

The "some cases" clause raises a flag for me, because I wasn't able to find any
cases involving [[Call]]-ing an arrow function with rest parameters which didn't
result in an access violation. Reading all registers makes it look like rbx is
null and being dereferenced somehow, but it could be from dereferencing the [
rax + 0xf ] operand, wherever that is coming from. I was hoping someone more
accustomed to the various parts of the baseline x64 codegen might have an idea
what it's likely these are.

> The next thing you could try is not invoking DeclareParameter on the rest
param
> at all, and just desugar using a temporary instead. In fact, it should be
> possible to make it such that all traces of a rest parameter effectively
> disappear after desugaring. (The only thing you probably need to remember is
> whether the parameter list was simple, for allocating the right arguments
object
> later.)
> 
> You could also try to temporarily replace the desugaring with a nop. If that
> doesn't make the problem go away then maybe something is f-ed up with
> variable/temporary allocation?

Could be, I'll spend some more time on that this week

> Maybe there also is some weird problem with doing the desugaring after
> processing the body. Could be worth a shot trying to undo that (with the TC39
> resolution it doesn't seem necessary anymore anyway).

I'll revert-ish the change from July (thus desugar before the body is parsed),
but I won't add any early errors when the language mode directive is found. I
don't think people are going to appreciate that change very much :( It
definitely does make the language weird, at least until people are reliably able
to use modules.

[caitp (gmail), 2015-08-28 14:09:03]

Hey, can folks take another look at this? I need someone to review the new AST
node added to simplify some stuff.

This now works with lazy parsing for both regular functions and arrows, so
that's pretty cool.

Summary of changes:

  - New AST node for rest parameters, to make literal reindexing work better
without (further) breaking the const-correctness of the existing code
    (Also allows some cleanup to various is_simple_parameter tests which also
check `is_rest` in addition to IsVariableProxy())`
  - Some hacks to ensure materialized literal counts from the formals are
included in the function during creation when lazily parsed (this was not
    happening before, which seems weird since it would have been needed
previously for the old binding pattern code?)
  - Adds some code to enable the preparser to figure out that an arrow function
has a rest parameter (also necessary for lazily parsing arrow functions)

There's probably some other bits, and maybe some cleanup to be done as well. I'd
appreciate a look =)

[adamk, 2015-08-28 15:47:52]

I will take a look at this today (sorry I've been slow to respond this week,
have been swamped with non-engineering tasks)

[adamk, 2015-08-28 23:26:37]

Excited to see this passing tests!

As you suspected, I'm a little bit skeptical of adding a new AST node for
RestParameter. I'm probably missing something, but it seems like a big hammer
just to make reindexing work, as no other AST visitor will ever care about it.

Not sure who else would be an appropriate reviewer, wingo@ is probably the other
right person (besides rossberg, who'll be out for the next while). I'd say
dslomov, as he wrote the reindexer, but yeah :)

https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visi...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visi...
src/ast-expression-visitor.cc:206: void
AstExpressionVisitor::VisitRestParameter(RestParameter* expr) {
Why isn't this one UNREACHABLE()?

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4012
src/parser.cc:4012: for (const auto p : parameters.params) {
So is const-correctness the only thing keeping you from handling rest specially
here, instead of adding a new AST node? The AST node adds a good bit of
cognitive overhead, and a very strange new type of "Expression" (though I guess
ExpressionClassifier already let that boat sail).

If you could put the logic here, it seems like all you'd need is a field in
ParserFormalParameters::Parameter to store the literal index, and a public
method on AstLiteralReindexer to do the updating.

https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4416
src/parser.cc:4416: if (parameter.is_rest) {
Please add a comment here with the full desugaring, it's hard for me to piece
together among all the parser boilerplate.

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.h#newcode1002
src/parser.h:1002: bool is_rest = false;
Does this compile? I think this and the below should be initialized in the
constructor.

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

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode708
src/preparser.h:708: inline void ParseFormalParameter(FormalParametersT*
parameters,
Is it really worth having two versions of this method? I don't think there are
so many callers that you couldn't just do the peek_position() at the callsites
and leave out the overload.

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode927
src/preparser.h:927: BinaryOperationTokenField::encode(op) |
This seems overly general. Can you get away with just:

HasRestField::encode(op == Token::COMMA && right->IsSpreadExpression())

It doesn't look to me like you're using the token for anything else; see below
on HasRestParameter for the rest of this comment.

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode983
src/preparser.h:983: PreParserExpression AsVariableProxy() const { return *this;
}
If you end up needing this, it seems like it should have a
DCHECK(IsIdentifier())

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) == Token::COMMA
&&
I'm not sure that you're getting much from this token field. Wouldn't the
following expression also return true for HasRestParameter?

"b + c, ...d"

Which clearly isn't a valid parameter list. Given the narrow usage of this Token
field, it seems like it might make more sense to directly encode the bit you
care about.

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:2897: this->ParseArrowFunctionFormalParameterList(&parameters,
expression, loc,
So this had to move up because it now affects materialized_literals_count?

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:3866: DCHECK(formal_parameters.materialized_literals_count > 0);
Would this ever be anything other than 1 in this case?

[caitp (gmail), 2015-08-28 23:58:01]

https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visi...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visi...
src/ast-expression-visitor.cc:206: void
AstExpressionVisitor::VisitRestParameter(RestParameter* expr) {
On 2015/08/28 23:26:37, adamk wrote:
> Why isn't this one UNREACHABLE()?

I wasn't sure if AstExpressionVisitor was ever used before compilation or not.
Sounds like it isn't, so I guess it can probably be UNREACHABLE

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4012
src/parser.cc:4012: for (const auto p : parameters.params) {
On 2015/08/28 23:26:37, adamk wrote:
> So is const-correctness the only thing keeping you from handling rest
specially
> here, instead of adding a new AST node? The AST node adds a good bit of
> cognitive overhead, and a very strange new type of "Expression" (though I
guess
> ExpressionClassifier already let that boat sail).
> 
> If you could put the logic here, it seems like all you'd need is a field in
> ParserFormalParameters::Parameter to store the literal index, and a public
> method on AstLiteralReindexer to do the updating.

Yeah, it's true that doing it all here could get rid of the new node --- but it
also means changing a lot of const references to non-const, and maybe mutating
stuff that you normally wouldn't expect to be mutated. It could lead to bugs,
but if the simplicity is worthwhile, I'm all for that

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.h#newcode1002
src/parser.h:1002: bool is_rest = false;
On 2015/08/28 23:26:37, adamk wrote:
> Does this compile? I think this and the below should be initialized in the
> constructor.

Compiles with clang version 3.8.0 (trunk 242792), but will do. This pattern is
used in a few other structs in the tree though

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

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode708
src/preparser.h:708: inline void ParseFormalParameter(FormalParametersT*
parameters,
On 2015/08/28 23:26:37, adamk wrote:
> Is it really worth having two versions of this method? I don't think there are
> so many callers that you couldn't just do the peek_position() at the callsites
> and leave out the overload.

Sure, this way just seemed like less work than hunting down all the callers.
I'll make that change during the next round of cleanup

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode927
src/preparser.h:927: BinaryOperationTokenField::encode(op) |
On 2015/08/28 23:26:37, adamk wrote:
> This seems overly general. Can you get away with just:
> 
> HasRestField::encode(op == Token::COMMA && right->IsSpreadExpression())
> 
> It doesn't look to me like you're using the token for anything else; see below
> on HasRestParameter for the rest of this comment.

Acknowledged

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode983
src/preparser.h:983: PreParserExpression AsVariableProxy() const { return *this;
}
On 2015/08/28 23:26:37, adamk wrote:
> If you end up needing this, it seems like it should have a
> DCHECK(IsIdentifier())

If it only returns false for `this`, that's probably okay. Acknowledged

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) == Token::COMMA
&&
On 2015/08/28 23:26:37, adamk wrote:
> I'm not sure that you're getting much from this token field. Wouldn't the
> following expression also return true for HasRestParameter?
> 

It's important for making lazy parsing work with arrow functions :( The
pre-parser needs to be able to figure out that it needs to increase the
materialized literal count for the new function. Without it, you end up with a
segfault, which isn't ideal :(

> "b + c, ...d"
> 
> Which clearly isn't a valid parameter list. Given the narrow usage of this
Token
> field, it seems like it might make more sense to directly encode the bit you
> care about.

I think (hope) that `b + c` as a parameter would cause the classifier to record
an arrow formal parameters error. The flag is used only after the parameters
have been determined to be valid, so this situation doesn't occur.

I could leave a comment indicating the conditions under which it can be used, if
that helps

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:2897: this->ParseArrowFunctionFormalParameterList(&parameters,
expression, loc,
On 2015/08/28 23:26:37, adamk wrote:
> So this had to move up because it now affects materialized_literals_count?

Yeah :( I tried to find a better way to do it, but storing this info in the
ExpressionClassifier was a bit problematic.

Would a comment help indicate the reasoning for moving it?

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:3866: DCHECK(formal_parameters.materialized_literals_count > 0);
On 2015/08/28 23:26:37, adamk wrote:
> Would this ever be anything other than 1 in this case?

Yes --- I believe initializers with materialized literal values can increase the
value

In fact, it turns out that there's a bug in default parameters with lazy
parsing, so in fact we really need this to happen whenever
`materialized_literals_count > 0` and not just when has_rest is true.

I'll file a bug about that.

[adamk, 2015-08-29 01:01:07]

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4012
src/parser.cc:4012: for (const auto p : parameters.params) {
On 2015/08/28 23:58:01, caitp wrote:
> On 2015/08/28 23:26:37, adamk wrote:
> > So is const-correctness the only thing keeping you from handling rest
> specially
> > here, instead of adding a new AST node? The AST node adds a good bit of
> > cognitive overhead, and a very strange new type of "Expression" (though I
> guess
> > ExpressionClassifier already let that boat sail).
> > 
> > If you could put the logic here, it seems like all you'd need is a field in
> > ParserFormalParameters::Parameter to store the literal index, and a public
> > method on AstLiteralReindexer to do the updating.
> 
> Yeah, it's true that doing it all here could get rid of the new node --- but
it
> also means changing a lot of const references to non-const, and maybe mutating
> stuff that you normally wouldn't expect to be mutated. It could lead to bugs,
> but if the simplicity is worthwhile, I'm all for that

The current thing is cheating quite a bit, const-wise: it only works because the
parameters hold non-const pointers to Expression*. I'm not sure why
FormalParametersT is passed by const-ref everywhere while ExpressionClassifier,
for example, is passed by mutable-pointer. To get a minimal change going you
could even use the 'mutable' keyword on the bit you need to change in a
Parameter.

So, fwiw, my taste would be to avoid a new AST node here.

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.h#newcode1002
src/parser.h:1002: bool is_rest = false;
On 2015/08/28 23:58:01, caitp wrote:
> On 2015/08/28 23:26:37, adamk wrote:
> > Does this compile? I think this and the below should be initialized in the
> > constructor.
> 
> Compiles with clang version 3.8.0 (trunk 242792), but will do. This pattern is
> used in a few other structs in the tree though

Huh, I may just be out of date. Feel free to leave this as-is if we're already
doing this elsewhere.

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

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) == Token::COMMA
&&
On 2015/08/28 23:58:01, caitp wrote:
> On 2015/08/28 23:26:37, adamk wrote:
> > I'm not sure that you're getting much from this token field. Wouldn't the
> > following expression also return true for HasRestParameter?
> > 
> 
> It's important for making lazy parsing work with arrow functions :( The
> pre-parser needs to be able to figure out that it needs to increase the
> materialized literal count for the new function. Without it, you end up with a
> segfault, which isn't ideal :(
> 
> > "b + c, ...d"
> > 
> > Which clearly isn't a valid parameter list. Given the narrow usage of this
> Token
> > field, it seems like it might make more sense to directly encode the bit you
> > care about.
> 
> I think (hope) that `b + c` as a parameter would cause the classifier to
record
> an arrow formal parameters error. The flag is used only after the parameters
> have been determined to be valid, so this situation doesn't occur.
> 
> I could leave a comment indicating the conditions under which it can be used,
if
> that helps

I think I wasn't as clear as I could have been. I understand what you're trying
to detect here (and why it's necessary), I just don't think you need to encode
as much information as you're currently encoding.

Basically, the only binary op whose token you care about is the one whose RHS is
a Spread. So you should be able to handle the COMMA check when you're creating
that binary op.

The advantage of doing it that way is you don't need 8 bits of
PreParserExpression space.

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:2897: this->ParseArrowFunctionFormalParameterList(&parameters,
expression, loc,
On 2015/08/28 23:58:01, caitp wrote:
> On 2015/08/28 23:26:37, adamk wrote:
> > So this had to move up because it now affects materialized_literals_count?
> 
> Yeah :( I tried to find a better way to do it, but storing this info in the
> ExpressionClassifier was a bit problematic.
> 
> Would a comment help indicate the reasoning for moving it?

No need for a comment, as long as a test will fail if these are reordered again.
I was mostly checking that my assumption was correct; after this patch, this
order seems fine too.

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:3866: DCHECK(formal_parameters.materialized_literals_count > 0);
On 2015/08/28 23:58:01, caitp wrote:
> On 2015/08/28 23:26:37, adamk wrote:
> > Would this ever be anything other than 1 in this case?
> 
> Yes --- I believe initializers with materialized literal values can increase
the
> value
> 
> In fact, it turns out that there's a bug in default parameters with lazy
> parsing, so in fact we really need this to happen whenever
> `materialized_literals_count > 0` and not just when has_rest is true.
> 
> I'll file a bug about that.

Ah, I meant to say "can this ever happen without default params", but I guess
we're pretty much living in that world already. Not surprised to hear there are
more bugs in that stuff, look forward to your report.

[caitp (gmail), 2015-08-29 15:52:00]

I think this addresses all of the comments

https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visi...
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1272673003/diff/60001/src/ast-expression-visi...
src/ast-expression-visitor.cc:206: void
AstExpressionVisitor::VisitRestParameter(RestParameter* expr) {
On 2015/08/28 23:26:37, adamk wrote:
> Why isn't this one UNREACHABLE()?

All the AST node stuff is now gone, replaced with the use of the mutable
qualifier

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4012
src/parser.cc:4012: for (const auto p : parameters.params) {
On 2015/08/29 01:01:07, adamk wrote:
> On 2015/08/28 23:58:01, caitp wrote:
> > On 2015/08/28 23:26:37, adamk wrote:
> > > So is const-correctness the only thing keeping you from handling rest
> > specially
> > > here, instead of adding a new AST node? The AST node adds a good bit of
> > > cognitive overhead, and a very strange new type of "Expression" (though I
> > guess
> > > ExpressionClassifier already let that boat sail).
> > > 
> > > If you could put the logic here, it seems like all you'd need is a field
in
> > > ParserFormalParameters::Parameter to store the literal index, and a public
> > > method on AstLiteralReindexer to do the updating.
> > 
> > Yeah, it's true that doing it all here could get rid of the new node --- but
> it
> > also means changing a lot of const references to non-const, and maybe
mutating
> > stuff that you normally wouldn't expect to be mutated. It could lead to
bugs,
> > but if the simplicity is worthwhile, I'm all for that
> 
> The current thing is cheating quite a bit, const-wise: it only works because
the
> parameters hold non-const pointers to Expression*. I'm not sure why
> FormalParametersT is passed by const-ref everywhere while
ExpressionClassifier,
> for example, is passed by mutable-pointer. To get a minimal change going you
> could even use the 'mutable' keyword on the bit you need to change in a
> Parameter.
> 
> So, fwiw, my taste would be to avoid a new AST node here.

Done.

https://codereview.chromium.org/1272673003/diff/60001/src/parser.cc#newcode4416
src/parser.cc:4416: if (parameter.is_rest) {
On 2015/08/28 23:26:37, adamk wrote:
> Please add a comment here with the full desugaring, it's hard for me to piece
> together among all the parser boilerplate.

Done.

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

https://codereview.chromium.org/1272673003/diff/60001/src/parser.h#newcode1002
src/parser.h:1002: bool is_rest = false;
On 2015/08/29 01:01:07, adamk wrote:
> On 2015/08/28 23:58:01, caitp wrote:
> > On 2015/08/28 23:26:37, adamk wrote:
> > > Does this compile? I think this and the below should be initialized in the
> > > constructor.
> > 
> > Compiles with clang version 3.8.0 (trunk 242792), but will do. This pattern
is
> > used in a few other structs in the tree though
> 
> Huh, I may just be out of date. Feel free to leave this as-is if we're already
> doing this elsewhere.

It turns out these members were unused anyways, so there isn't any reason to
keep them regardless. They're gone

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

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode708
src/preparser.h:708: inline void ParseFormalParameter(FormalParametersT*
parameters,
On 2015/08/28 23:26:37, adamk wrote:
> Is it really worth having two versions of this method? I don't think there are
> so many callers that you couldn't just do the peek_position() at the callsites
> and leave out the overload.

Method is gone, isn't needed without the AST node (might make errors slightly
uglier though)

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode983
src/preparser.h:983: PreParserExpression AsVariableProxy() const { return *this;
}
On 2015/08/28 23:26:37, adamk wrote:
> If you end up needing this, it seems like it should have a
> DCHECK(IsIdentifier())

It's not needed without the new AST node, so it's gone anyways

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:1051: BinaryOperationTokenField::decode(code_) == Token::COMMA
&&
On 2015/08/29 01:01:07, adamk wrote:
> On 2015/08/28 23:58:01, caitp wrote:
> > On 2015/08/28 23:26:37, adamk wrote:
> > > I'm not sure that you're getting much from this token field. Wouldn't the
> > > following expression also return true for HasRestParameter?
> > > 
> > 
> > It's important for making lazy parsing work with arrow functions :( The
> > pre-parser needs to be able to figure out that it needs to increase the
> > materialized literal count for the new function. Without it, you end up with
a
> > segfault, which isn't ideal :(
> > 
> > > "b + c, ...d"
> > > 
> > > Which clearly isn't a valid parameter list. Given the narrow usage of this
> > Token
> > > field, it seems like it might make more sense to directly encode the bit
you
> > > care about.
> > 
> > I think (hope) that `b + c` as a parameter would cause the classifier to
> record
> > an arrow formal parameters error. The flag is used only after the parameters
> > have been determined to be valid, so this situation doesn't occur.
> > 
> > I could leave a comment indicating the conditions under which it can be
used,
> if
> > that helps
> 
> I think I wasn't as clear as I could have been. I understand what you're
trying
> to detect here (and why it's necessary), I just don't think you need to encode
> as much information as you're currently encoding.
> 
> Basically, the only binary op whose token you care about is the one whose RHS
is
> a Spread. So you should be able to handle the COMMA check when you're creating
> that binary op.
> 
> The advantage of doing it that way is you don't need 8 bits of
> PreParserExpression space.

Done.

https://codereview.chromium.org/1272673003/diff/60001/src/preparser.h#newcode...
src/preparser.h:3866: DCHECK(formal_parameters.materialized_literals_count > 0);
On 2015/08/29 01:01:07, adamk wrote:
> On 2015/08/28 23:58:01, caitp wrote:
> > On 2015/08/28 23:26:37, adamk wrote:
> > > Would this ever be anything other than 1 in this case?
> > 
> > Yes --- I believe initializers with materialized literal values can increase
> the
> > value
> > 
> > In fact, it turns out that there's a bug in default parameters with lazy
> > parsing, so in fact we really need this to happen whenever
> > `materialized_literals_count > 0` and not just when has_rest is true.
> > 
> > I'll file a bug about that.
> 
> Ah, I meant to say "can this ever happen without default params", but I guess
> we're pretty much living in that world already. Not surprised to hear there
are
> more bugs in that stuff, look forward to your report.

I've changed it to always add the formals materialized literals whether has_rest
is true or not, but as noted in the bug I filed
(https://code.google.com/p/v8/issues/detail?id=4400) it doesn't actually solve
the problem for default arguments, which is a bit more complicated.

[wingo, 2015-08-31 13:07:31]

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode3947
src/parser.cc:3947: ReportMessageAt(params_loc,
MessageTemplate::kMalformedArrowFunParamList);
Why is this necessary?  ParseArrowFunctionFormalParameters should only be called
on a valid production.

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4187
src/parser.cc:4187: }
Is this bit specific to this patch or is it a general fix?

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4414
src/parser.cc:4414: if (parameter.is_rest) {
This should probably be "else if" and the previous case should
DCHECK(!parameter.is_rest), I think

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4430
src/parser.cc:4430: empty_values, parameters.rest_array_literal_index,
Is it necessary to reserve a literal index in the reindexer?  Since we make it
here and we're in the context of the function, seems to me we might be able to
reserve the index with the normal mechanism.

https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h#newcod...
src/preparser.h:1044: (IsBinaryOperation() && HasRestField::decode(code_));
Need more comments about the preconditions, i.e. the result is only valid on the
result of ParseConditionalExpression when the conditional expression stared with
an LPAREN and the classifier returns true for
is_valid_arrow_formal_parameters().  Actually I think this would be better
inlined into the trait-specific ParseArrowFunctionFormalParameters.

https://codereview.chromium.org/1272673003/diff/110001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/arrow-rest-params-lazy-parsing.js (right):

https://codereview.chromium.org/1272673003/diff/110001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-rest-params-lazy-parsing.js:1: // Copyright 2015 the
V8 project authors. All rights reserved.
IIRC we don't update these lines; Adam?

https://codereview.chromium.org/1272673003/diff/110001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-rest-params-lazy-parsing.js:22: var strictTest =
(function() {
Why this change?

[caitp (gmail), 2015-08-31 13:23:40]

Thanks Andy, few more cleanup points here =)

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode3947
src/parser.cc:3947: ReportMessageAt(params_loc,
MessageTemplate::kMalformedArrowFunParamList);
On 2015/08/31 13:07:31, wingo wrote:
> Why is this necessary?  ParseArrowFunctionFormalParameters should only be
called
> on a valid production.

You're right, I realized that this morning but haven't fixed it yet. Going to
airport soon, but I'll update with a new patchset with a fix when I get back

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4187
src/parser.cc:4187: }
On 2015/08/31 13:07:31, wingo wrote:
> Is this bit specific to this patch or is it a general fix?

Well, it's a mix. See https://code.google.com/p/v8/issues/detail?id=4400 for
other bugs related to this.

I'm hoping the various literal counter strategies can be sort of unified later
on to make things simpler, but this was the simplest way to do it for this CL

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4430
src/parser.cc:4430: empty_values, parameters.rest_array_literal_index,
On 2015/08/31 13:07:31, wingo wrote:
> Is it necessary to reserve a literal index in the reindexer?  Since we make it
> here and we're in the context of the function, seems to me we might be able to
> reserve the index with the normal mechanism.

It seems to be necessary. Without it, an extra slot is allocated in the
context's literals array, which makes things hard for lazy parsing, and is
potentially a bug farm too. I haven't gotten it working without saving the
literal index (either in an AST node or here) and rewriting it correctly

https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h#newcod...
src/preparser.h:1044: (IsBinaryOperation() && HasRestField::decode(code_));
On 2015/08/31 13:07:31, wingo wrote:
> Need more comments about the preconditions, i.e. the result is only valid on
the
> result of ParseConditionalExpression when the conditional expression stared
with
> an LPAREN and the classifier returns true for
> is_valid_arrow_formal_parameters().  Actually I think this would be better
> inlined into the trait-specific ParseArrowFunctionFormalParameters.

I'll inline it

https://codereview.chromium.org/1272673003/diff/110001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/arrow-rest-params-lazy-parsing.js (right):

https://codereview.chromium.org/1272673003/diff/110001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-rest-params-lazy-parsing.js:1: // Copyright 2015 the
V8 project authors. All rights reserved.
On 2015/08/31 13:07:31, wingo wrote:
> IIRC we don't update these lines; Adam?

This isn't really an update, it's a new file

https://codereview.chromium.org/1272673003/diff/110001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-rest-params-lazy-parsing.js:22: var strictTest =
(function() {
On 2015/08/31 13:07:31, wingo wrote:
> Why this change?

It's not legal to have a language mode directive in a function with a rest
parameter

[caitp (gmail), 2015-08-31 13:28:17]

https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h#newcod...
src/preparser.h:1044: (IsBinaryOperation() && HasRestField::decode(code_));
On 2015/08/31 13:23:40, caitp wrote:
> On 2015/08/31 13:07:31, wingo wrote:
> > Need more comments about the preconditions, i.e. the result is only valid on
> the
> > result of ParseConditionalExpression when the conditional expression stared
> with
> > an LPAREN and the classifier returns true for
> > is_valid_arrow_formal_parameters().  Actually I think this would be better
> > inlined into the trait-specific ParseArrowFunctionFormalParameters.
> 
> I'll inline it

Actually, it can't really be inlined since there's no way to get at the
bitfield, and it's really more code to change that and inline it.

How would you feel about the name just indicating it's specific to arrow
function parameters?

[wingo, 2015-08-31 13:39:49]

On 2015/08/31 13:28:17, caitp wrote:
> https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h
> File src/preparser.h (right):
> 
>
https://codereview.chromium.org/1272673003/diff/110001/src/preparser.h#newcod...
> src/preparser.h:1044: (IsBinaryOperation() && HasRestField::decode(code_));
> On 2015/08/31 13:23:40, caitp wrote:
> > On 2015/08/31 13:07:31, wingo wrote:
> > > Need more comments about the preconditions, i.e. the result is only valid
on
> > the
> > > result of ParseConditionalExpression when the conditional expression
stared
> > with
> > > an LPAREN and the classifier returns true for
> > > is_valid_arrow_formal_parameters().  Actually I think this would be better
> > > inlined into the trait-specific ParseArrowFunctionFormalParameters.
> > 
> > I'll inline it
> 
> Actually, it can't really be inlined since there's no way to get at the
> bitfield, and it's really more code to change that and inline it.
> 
> How would you feel about the name just indicating it's specific to arrow
> function parameters?

Sure, or a comment saying it's not for general use would be fine too, dunno. 
BTW I guess the newest patch is rebased?  The interdiffs seem to include a bunch
of other things.

[wingo, 2015-08-31 13:54:09]

Hi :)

On 2015/08/31 13:23:40, caitp wrote:
>
https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4187
> src/parser.cc:4187: }
> On 2015/08/31 13:07:31, wingo wrote:
> > Is this bit specific to this patch or is it a general fix?
> 
> Well, it's a mix. See https://code.google.com/p/v8/issues/detail?id=4400 for
> other bugs related to this.

Does this patch fix v8:4400?  If so, it seems like a good idea to land this one
separately.

[caitp (gmail), 2015-08-31 14:07:33]

On 2015/08/31 13:54:09, wingo wrote:
> Hi :)
> 
> On 2015/08/31 13:23:40, caitp wrote:
> >
>
https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4187
> > src/parser.cc:4187: }
> > On 2015/08/31 13:07:31, wingo wrote:
> > > Is this bit specific to this patch or is it a general fix?
> > 
> > Well, it's a mix. See https://code.google.com/p/v8/issues/detail?id=4400 for
> > other bugs related to this.
> 
> Does this patch fix v8:4400?  If so, it seems like a good idea to land this
one
> separately.

As noted on the bug, it's a bit more complicated to fix the default parameters
case. This cl crashes if we don't do this during lazy parsing, though.

[adamk, 2015-08-31 20:44:09]

adamk@chromium.org changed reviewers:
+ bmeurer@chromium.org

[adamk, 2015-08-31 20:44:10]

This is looking good to me, will look once more after you've handled the rest of
wingo's comments.

I agree that we should do something nicer with the literals count, it's quite
messy at the moment and not at all obvious why the count is valid in all cases.

+bmeurer for full-codegen OWNERs (luckily this is mostly deletion).

[caitp (gmail), 2015-08-31 21:03:50]

On 2015/08/31 20:44:10, adamk wrote:
> This is looking good to me, will look once more after you've handled the rest
of
> wingo's comments.
> 

I believe these have been addressed in the last patchset

> I agree that we should do something nicer with the literals count, it's quite
> messy at the moment and not at all obvious why the count is valid in all
cases.
> 

I don't think this is ever going to be really nice, but it can at least be
unified more, with fewer special cases. Unfortunately, the cover grammars just
make this more complicated than it was for es5.

I have an idea to fix/unify this, which is to separate literal count tracking
from FunctionState, and do it on an expression by expression basis. This should
work, but will be a tiny bit more complicated. I think this should happen in a
followup or separate CL.

> +bmeurer for full-codegen OWNERs (luckily this is mostly deletion).

[adamk, 2015-08-31 21:17:28]

lgtm % one more wingo comment I point to

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4414
src/parser.cc:4414: if (parameter.is_rest) {
On 2015/08/31 13:07:31, wingo wrote:
> This should probably be "else if" and the previous case should
> DCHECK(!parameter.is_rest), I think

Agreed on the else-if suggestion here.

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4422
src/parser.cc:4422: DCHECK_NULL(parameter.initializer);
That'll get rid of this DCHECK (or rather replace it with the DCHECK proposed by
wingo)

[caitp (gmail), 2015-08-31 22:43:15]

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1272673003/diff/110001/src/parser.cc#newcode4422
src/parser.cc:4422: DCHECK_NULL(parameter.initializer);
On 2015/08/31 21:17:28, adamk wrote:
> That'll get rid of this DCHECK (or rather replace it with the DCHECK proposed
by
> wingo)

Right, forgot about this one. Done in the last patchset (also includes a rebase
though, so the interdiff will be bad)

[Benedikt Meurer, 2015-09-01 03:09:10]

LGTM with question and nit.

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4415
src/parser.cc:4415: // for (var $argument_index = $rest_index;
I guess there was some discussion about this desugaring; so do we have any plans
how to make this work with inlining? esp. inlining in TurboFan?

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4425
src/parser.cc:4425: auto var = parameters.scope->parameter(i);
Nit: Wow, this heavy autofication. I'm not sure what the convention for the
parser is, but I'd prefer to reduce the use of auto, esp. when it's used in
place of a pointer type.

[caitp (gmail), 2015-09-01 03:55:02]

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4415
src/parser.cc:4415: // for (var $argument_index = $rest_index;
On 2015/09/01 03:09:10, Benedikt Meurer wrote:
> I guess there was some discussion about this desugaring; so do we have any
plans
> how to make this work with inlining? esp. inlining in TurboFan?

I don't think there are plans per se (but you might have seen there was some TF
code previously for this, not sure how complete it was in terms of inlining).

There are at least some ideas about how the runtime call could be changed into
something relatively quick. The spec'd algorithm uses CreateDataProperty(O, P,
V) to add the element to the array. For rest parameters, we can make a lot of
assumptions which lend themselves well to inlining:

- the rest array's own "length" property is always writable, never configurable
- the rest array is always extensible
- the rest array's own indexed properties are always writable and configurable
(they aren't present until the rest parameter algorithm sets them up)

Based on this, yes, the current implementation probably isn't doing the right
thing, but it's pretty close.

I guess there are also statistical guesses about the size of the array to
allocate, and whether or not to perform the loop at all, after a bit of
profiling. But maybe compiler-folks are better for talking about that stuff.

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4425
src/parser.cc:4425: auto var = parameters.scope->parameter(i);
On 2015/09/01 03:09:10, Benedikt Meurer wrote:
> Nit: Wow, this heavy autofication. I'm not sure what the convention for the
> parser is, but I'd prefer to reduce the use of auto, esp. when it's used in
> place of a pointer type.

My feeling is that it's alright for AST nodes (because of the `New<NodeType>`
methods providing all the missing info), which is the bulk of the auto use. If
folks want explicit typenames used everywhere, we can do that too.

How important is this, given that the majority of this is AST nodes?

[Benedikt Meurer, 2015-09-01 04:02:15]

On 2015/09/01 03:55:02, caitp wrote:
> https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc
> File src/parser.cc (right):
> 
>
https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4415
> src/parser.cc:4415: // for (var $argument_index = $rest_index;
> On 2015/09/01 03:09:10, Benedikt Meurer wrote:
> > I guess there was some discussion about this desugaring; so do we have any
> plans
> > how to make this work with inlining? esp. inlining in TurboFan?
> 
> I don't think there are plans per se (but you might have seen there was some
TF
> code previously for this, not sure how complete it was in terms of inlining).
> 
> There are at least some ideas about how the runtime call could be changed into
> something relatively quick. The spec'd algorithm uses CreateDataProperty(O, P,
> V) to add the element to the array. For rest parameters, we can make a lot of
> assumptions which lend themselves well to inlining:
> 
> - the rest array's own "length" property is always writable, never
configurable
> - the rest array is always extensible
> - the rest array's own indexed properties are always writable and configurable
> (they aren't present until the rest parameter algorithm sets them up)
> 
> Based on this, yes, the current implementation probably isn't doing the right
> thing, but it's pretty close.
> 
> I guess there are also statistical guesses about the size of the array to
> allocate, and whether or not to perform the loop at all, after a bit of
> profiling. But maybe compiler-folks are better for talking about that stuff.

I'm more worried about the %_ArgumentsLength and %_Argument intrinsics. Not sure
if
and how we can support that with inlining, esp. %_Argument(i) with non-constant
i will
probably need to be turned into some kind of switch during inlining. From a
compiler
perspective (and without deep understanding of rest parameters, yet), I'd think
that a
dedicated RestParameter AST node, which is then turned into a
JSCreateRestParameters
graph node is easiest to optimize wrt. inlining; as soon as the inliner
discovers a
JSCreateRestParameters (which should hang off the Start node), it will replace
it with a
JSArray containing all the (then statically) known rest parameters. Otherwise
the rest
parameters are lowered to a runtime call or a stub call, which is responsible to
allocate
the JSArray.

[Benedikt Meurer, 2015-09-01 04:42:33]

On 2015/09/01 04:02:15, Benedikt Meurer wrote:
> On 2015/09/01 03:55:02, caitp wrote:
> > https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc
> > File src/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4415
> > src/parser.cc:4415: // for (var $argument_index = $rest_index;
> > On 2015/09/01 03:09:10, Benedikt Meurer wrote:
> > > I guess there was some discussion about this desugaring; so do we have any
> > plans
> > > how to make this work with inlining? esp. inlining in TurboFan?
> > 
> > I don't think there are plans per se (but you might have seen there was some
> TF
> > code previously for this, not sure how complete it was in terms of
inlining).
> > 
> > There are at least some ideas about how the runtime call could be changed
into
> > something relatively quick. The spec'd algorithm uses CreateDataProperty(O,
P,
> > V) to add the element to the array. For rest parameters, we can make a lot
of
> > assumptions which lend themselves well to inlining:
> > 
> > - the rest array's own "length" property is always writable, never
> configurable
> > - the rest array is always extensible
> > - the rest array's own indexed properties are always writable and
configurable
> > (they aren't present until the rest parameter algorithm sets them up)
> > 
> > Based on this, yes, the current implementation probably isn't doing the
right
> > thing, but it's pretty close.
> > 
> > I guess there are also statistical guesses about the size of the array to
> > allocate, and whether or not to perform the loop at all, after a bit of
> > profiling. But maybe compiler-folks are better for talking about that stuff.
> 
> I'm more worried about the %_ArgumentsLength and %_Argument intrinsics. Not
sure
> if
> and how we can support that with inlining, esp. %_Argument(i) with
non-constant
> i will
> probably need to be turned into some kind of switch during inlining. From a
> compiler
> perspective (and without deep understanding of rest parameters, yet), I'd
think
> that a
> dedicated RestParameter AST node, which is then turned into a
> JSCreateRestParameters
> graph node is easiest to optimize wrt. inlining; as soon as the inliner
> discovers a
> JSCreateRestParameters (which should hang off the Start node), it will replace
> it with a
> JSArray containing all the (then statically) known rest parameters. Otherwise
> the rest
> parameters are lowered to a runtime call or a stub call, which is responsible
to
> allocate
> the JSArray.

Thinking more about this, desugaring to a loop will probably generate
quite slow code in Crankshaft and fullcodegen as well, compared to
just a single runtime/stub call that collects all arguments into a JSArray.

If we'd have something like JSCreateArgumentsArray(start_index), backed
by a runtime call and stub, then we could use that for both desugaring
rest parameters and arguments.

I don't want to block you on this, but I have the feeling that we might
need to think harder about the arguments/rest parameter story.

[wingo, 2015-09-01 14:41:20]

Some more nits.  I defer to Adam's review of course.

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4187
src/parser.cc:4187: }
No need for the if, just execute the body unconditionally, no?

Aside: it's pretty confusing that one field is "materialized_literal_count" and
the other is "materialized_literals_count".  Probably need to rename the latter
to the former, because the former has been around for longer.

https://codereview.chromium.org/1272673003/diff/150001/src/parser.cc#newcode4425
src/parser.cc:4425: auto var = parameters.scope->parameter(i);
On 2015/09/01 03:55:02, caitp wrote:
> On 2015/09/01 03:09:10, Benedikt Meurer wrote:
> > Nit: Wow, this heavy autofication. I'm not sure what the convention for the
> > parser is, but I'd prefer to reduce the use of auto, esp. when it's used in
> > place of a pointer type.
> 
> My feeling is that it's alright for AST nodes (because of the `New<NodeType>`
> methods providing all the missing info), which is the bulk of the auto use. If
> folks want explicit typenames used everywhere, we can do that too.
> 
> How important is this, given that the majority of this is AST nodes?

I had this same thought when I read this FWIW; but it was only on this one in
particular (parameters.scope->parameter(i)).  I don't know what the type of var
is; is it a proxy or a variable?  I guess the latter but since it's not a
constructor a type would be appreciated.

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode34
src/preparser.h:34: mutable int rest_array_literal_index = -1;
Is mutable needed?  I think not, according to uses in this file.

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:1067: // Depends on
`ExpressionClassifier::is_valid_arrow_formal_parameters()`
I apologize for nitpicking but can we have sentences with full stops please.  I
also find "depends on" to be a bit vague in this context.  A possibility:

"If (and only if) the expression classifier has determined that this expression
is a valid arrow formal parameter list, return true if the formal parameter list
has a rest parameter."

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:1898: }
I am OK with this I guess.  Is it possible, possibly in a followup, to separate
out a desurgaring phase which runs before parsing the function body, in all
cases?  "Parse" should produce a FormalParametersT, and ideally the desugaring
could be in ParserBase.  If you have to make an array literal node for the rest
array, the side effect on the materialized literal count comes out naturally. 
Dunno, I recognize that the parser is a gnarly thing onto which we keep
hammering new things and we can't always get it right, but fudging about with
the materialized literal count in four places smells wrong to me.

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:2463: ReportUnexpectedTokenAt(next_location, next);
Why is this here?  I can't reproduce this bug but I also don't see where this
error case would be caught.

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:2920: this->ParseArrowFunctionFormalParameterList(&parameters,
expression, loc,
Yeah moving this before the checkpoint just sounds like a bad idea to me.  I
would prefer that the reservation of the literals index for the array happened
while parsing the function, within its scope and literals count, and before
parsing the function's body.  Is that not possible?

[caitp (gmail), 2015-09-01 15:17:25]

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode34
src/preparser.h:34: mutable int rest_array_literal_index = -1;
On 2015/09/01 14:41:20, wingo wrote:
> Is mutable needed?  I think not, according to uses in this file.

It is needed by the pattern rewriter, because I was asked to remove the AST node
(although bmeurer seems to prefer the AST node --- I'm getting confused by
conflicting requests D:).

PatternRewriter touches mutable AST nodes, but because this (rest parameters)
isn't (currently) represented as an AST node, the Parser's ReindexLiterals
method needs to be able to update the literal index appropriately.

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:1067: // Depends on
`ExpressionClassifier::is_valid_arrow_formal_parameters()`
On 2015/09/01 14:41:20, wingo wrote:
> I apologize for nitpicking but can we have sentences with full stops please. 
I
> also find "depends on" to be a bit vague in this context.  A possibility:
> 
> "If (and only if) the expression classifier has determined that this
expression
> is a valid arrow formal parameter list, return true if the formal parameter
list
> has a rest parameter."

Done.

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:1898: }
On 2015/09/01 14:41:20, wingo wrote:
> I am OK with this I guess.  Is it possible, possibly in a followup, to
separate
> out a desurgaring phase which runs before parsing the function body, in all
> cases?  "Parse" should produce a FormalParametersT, and ideally the desugaring
> could be in ParserBase.  If you have to make an array literal node for the
rest
> array, the side effect on the materialized literal count comes out naturally. 
> Dunno, I recognize that the parser is a gnarly thing onto which we keep
> hammering new things and we can't always get it right, but fudging about with
> the materialized literal count in four places smells wrong to me.

I think we'd be better off getting rid of FormalParametersBase's
materialized_literal_count entirely, and solving this problem properly. This
will hopefully be cleaned up as a part of a fix for v8:4400, which has been
discussed on IRC a bit

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:2463: ReportUnexpectedTokenAt(next_location, next);
On 2015/09/01 14:41:20, wingo wrote:
> Why is this here?  I can't reproduce this bug but I also don't see where this
> error case would be caught.

It produces a better error message --- it was moved as part of the AST node
patch set to produce the same message as the full parser, but can be removed
now.

Reverted this change

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcod...
src/preparser.h:2920: this->ParseArrowFunctionFormalParameterList(&parameters,
expression, loc,
On 2015/09/01 14:41:20, wingo wrote:
> Yeah moving this before the checkpoint just sounds like a bad idea to me.  I
> would prefer that the reservation of the literals index for the array happened
> while parsing the function, within its scope and literals count, and before
> parsing the function's body.  Is that not possible?

It's not currently possible with the FormalParametersT + FunctionState scheme. I
agree that this would be better, but we need a better way to track these in
order to make that work.

[adamk, 2015-09-01 15:43:13]

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1272673003/diff/150001/src/preparser.h#newcode34
src/preparser.h:34: mutable int rest_array_literal_index = -1;
On 2015/09/01 15:17:24, caitp wrote:
> On 2015/09/01 14:41:20, wingo wrote:
> > Is mutable needed?  I think not, according to uses in this file.
> 
> It is needed by the pattern rewriter, because I was asked to remove the AST
node
> (although bmeurer seems to prefer the AST node --- I'm getting confused by
> conflicting requests D:).

To be clear about the conflicting requests: I only objected to the use of an AST
node given the use of that node. If we go down the road of supporting this in
codegen (again), then clearly an AST node is required.

[wingo, 2015-09-01 16:03:18]

LGTM, thanks for humoring my nitpicking, doing the perfect thing seems
impossible here.

Regarding desugaring: to my limited understanding there is no inlining hazard;
TF will be able to do just as well with this desugaring as with indexed
arguments access.  It seems to me that TF will have to support arguments[n]
anyway, so I don't share Benedikt's concerns.  This desugaring also allows for
unrolling and load/store forwarding.  Haven't poked TF at all yet though so I'm
sure Benedikt knows more there.

[caitp (gmail), 2015-09-01 23:09:12]

On 2015/09/01 16:03:18, wingo wrote:
> LGTM, thanks for humoring my nitpicking, doing the perfect thing seems
> impossible here.
> 
> Regarding desugaring: to my limited understanding there is no inlining hazard;
> TF will be able to do just as well with this desugaring as with indexed
> arguments access.  It seems to me that TF will have to support arguments[n]
> anyway, so I don't share Benedikt's concerns.  This desugaring also allows for
> unrolling and load/store forwarding.  Haven't poked TF at all yet though so
I'm
> sure Benedikt knows more there.

I think everyone is as happy with this CL as they're going to get. I'm happy to
move things further in a follow-up, if anything else is needed. I'm also fairly
confident that this will perform better than the current code which disables
optimization and misses out on clever allocation strategies, too. If there are
any major problems, reverting is an option.

[caitp (gmail), 2015-09-01 23:09:27]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-09-01 23:09:27]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1272673003/#ps190001
(title: "Reduce use of "auto" a bit, remove unneeded changes")

[commit-bot: I haz the power, 2015-09-01 23:09:34]

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

[commit-bot: I haz the power, 2015-09-01 23:10:42]

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

[commit-bot: I haz the power, 2015-09-01 23:10:43]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/7680)
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/8297)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/9246)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/9359)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5468)

[caitp (gmail), 2015-09-01 23:28:26]

On 2015/09/01 23:10:43, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
>   v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
>   v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/7680)
>   v8_linux_dbg on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/8297)
>   v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
>   v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
>   v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
>   v8_linux_rel on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/9246)
>   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/9359)
>   v8_presubmit on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5468)

Well, or maybe this needs another look

[caitp (gmail), 2015-09-01 23:32:34]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-09-01 23:32:35]

The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com,
bmeurer@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1272673003/#ps210001
(title: "Rebase (ARM full-codegen)")

[commit-bot: I haz the power, 2015-09-01 23:32:44]

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

[commit-bot: I haz the power, 2015-09-01 23:37:09]

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

[commit-bot: I haz the power, 2015-09-01 23:37:10]

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

[caitp (gmail), 2015-09-01 23:47:04]

On 2015/09/01 23:37:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

This needs a few more changes for MIPS (when did the bots start running MIPS
tests? I was expecting MIPS folks to do those ports =) --- I think someone
should take a quick second look at it once it's ready

[caitp (gmail), 2015-09-02 00:22:14]

On 2015/09/01 23:47:04, caitp wrote:
> On 2015/09/01 23:37:10, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
> 
> This needs a few more changes for MIPS (when did the bots start running MIPS
> tests? I was expecting MIPS folks to do those ports =) --- I think someone
> should take a quick second look at it once it's ready

Alright, last issue is that lazily parsed functions are not removing the
unresolved rest parameter, and it hits the DCHECK despite passing tests in
release build.

[rossberg, 2015-09-02 00:55:27]

Parser changes LGTM, haven't looked at the rest.

[caitp (gmail), 2015-09-02 20:09:42]

On 2015/09/02 00:55:27, rossberg wrote:
> Parser changes LGTM, haven't looked at the rest.

Hey, I just made one quick adjustment to the parser, in addition to a few
updates for mips to get tests passing on the mips bots.

I'll leave an inline comment to explain it, it would be good to get a quick look
at it, but I think it will be alright

[caitp (gmail), 2015-09-02 20:11:31]

https://codereview.chromium.org/1272673003/diff/230001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1272673003/diff/230001/src/parser.h#newcode1324
src/parser.h:1324: !is_rest && pattern->IsVariableProxy() && initializer ==
nullptr;
This makes sure the empty string is used instead of the variable name, so that
the variable doesn't get declared twice. This seems to fix the bug in the tests.
I'm not sure if this code should be here, or maybe live somewhere else, but it
should be okay either way.

[caitp (gmail), 2015-09-02 20:43:17]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-09-02 20:43:17]

The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com,
bmeurer@chromium.org, adamk@chromium.org, rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1272673003/#ps230001
(title: "Oops.")

[commit-bot: I haz the power, 2015-09-02 20:43:27]

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

[rossberg, 2015-09-02 20:44:10]

https://codereview.chromium.org/1272673003/diff/230001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1272673003/diff/230001/src/parser.h#newcode1324
src/parser.h:1324: !is_rest && pattern->IsVariableProxy() && initializer ==
nullptr;
On 2015/09/02 20:11:31, caitp wrote:
> This makes sure the empty string is used instead of the variable name, so that
> the variable doesn't get declared twice. This seems to fix the bug in the
tests.
> I'm not sure if this code should be here, or maybe live somewhere else, but it
> should be okay either way.

Yes, this looks right to me.

[commit-bot: I haz the power, 2015-09-02 21:10:58]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-09-02 21:11:14]

Patchset 13 (id:??) landed as
https://crrev.com/510baeacbab311798d5e8795800ff773d00d062c
Cr-Commit-Position: refs/heads/master@{#30550}

[caitp (gmail), 2015-09-04 12:34:32]

On 2015/09/02 21:11:14, commit-bot: I haz the power wrote:
> Patchset 13 (id:??) landed as
> https://crrev.com/510baeacbab311798d5e8795800ff773d00d062c
> Cr-Commit-Position: refs/heads/master@{#30550}

Based on a simple microbenchmark, http://jsperf.com/v8-rest-parameters3/2, this
may have been more damaging to perf than I thought it would be :( This surprises
me a bit

[caitp (gmail), 2015-09-04 12:45:23]

On 2015/09/04 12:34:32, caitp wrote:
> On 2015/09/02 21:11:14, commit-bot: I haz the power wrote:
> > Patchset 13 (id:??) landed as
> > https://crrev.com/510baeacbab311798d5e8795800ff773d00d062c
> > Cr-Commit-Position: refs/heads/master@{#30550}
> 
> Based on a simple microbenchmark, http://jsperf.com/v8-rest-parameters3/2,
this
> may have been more damaging to perf than I thought it would be :( This
surprises
> me a bit

Actually, I may have spoken too soon. I guess the xeon in my mac pro just gets
through these tests slower than the i7 in my laptop. It still seems to be
slightly faster than the old implementation
http://jsperf.com/v8-rest-parameters3/3

[caitp (gmail), 2015-09-04 12:56:34]

On 2015/09/04 12:45:23, caitp wrote:
> On 2015/09/04 12:34:32, caitp wrote:
> > On 2015/09/02 21:11:14, commit-bot: I haz the power wrote:
> > > Patchset 13 (id:??) landed as
> > > https://crrev.com/510baeacbab311798d5e8795800ff773d00d062c
> > > Cr-Commit-Position: refs/heads/master@{#30550}
> > 
> > Based on a simple microbenchmark, http://jsperf.com/v8-rest-parameters3/2,
> this
> > may have been more damaging to perf than I thought it would be :( This
> surprises
> > me a bit
> 
> Actually, I may have spoken too soon. I guess the xeon in my mac pro just gets
> through these tests slower than the i7 in my laptop. It still seems to be
> slightly faster than the old implementation
> http://jsperf.com/v8-rest-parameters3/3

Spoke too soon again, console.log was weighing heavier on the tests than I
thought :( http://jsperf.com/v8-rest-parameters2/4 so, the old implementation
was faster :\ This is confusing since the desugaring is very similar to babel's
implementation

[caitp (gmail), 2015-09-04 22:52:50]

On 2015/09/04 12:56:34, caitp wrote:
> On 2015/09/04 12:45:23, caitp wrote:
> > On 2015/09/04 12:34:32, caitp wrote:
> > > On 2015/09/02 21:11:14, commit-bot: I haz the power wrote:
> > > > Patchset 13 (id:??) landed as
> > > > https://crrev.com/510baeacbab311798d5e8795800ff773d00d062c
> > > > Cr-Commit-Position: refs/heads/master@{#30550}
> > > 
> > > Based on a simple microbenchmark, http://jsperf.com/v8-rest-parameters3/2,
> > this
> > > may have been more damaging to perf than I thought it would be :( This
> > surprises
> > > me a bit
> > 
> > Actually, I may have spoken too soon. I guess the xeon in my mac pro just
gets
> > through these tests slower than the i7 in my laptop. It still seems to be
> > slightly faster than the old implementation
> > http://jsperf.com/v8-rest-parameters3/3
> 
> Spoke too soon again, console.log was weighing heavier on the tests than I
> thought :( http://jsperf.com/v8-rest-parameters2/4 so, the old implementation
> was faster :\ This is confusing since the desugaring is very similar to
babel's
> implementation

http://jsperf.com/v8-rest-parameters2/5 did some experimentation to see why the
babel impl does so much better, the biggest problem seems to be the use of "let"
(providing us with the proper TDZ). Some other improvements can also be obtained
via pre allocating the rest array rather than using the empty array literal.

(The bare*_func() versions do not exactly match the desugaring, due to not using
%AppendElement() and instead using babel-style assignment, which is not
technically correct per-spec).

Things to improve include making `let` declarations faster in general (quite
possibly related to hole-checking :() and pre-allocating the array size while
still using the fastest spec-correct means of filling in the values.

[caitp (gmail), 2015-09-04 22:59:00]

On 2015/09/04 22:52:50, caitp wrote:
> On 2015/09/04 12:56:34, caitp wrote:
> > On 2015/09/04 12:45:23, caitp wrote:
> > > On 2015/09/04 12:34:32, caitp wrote:
> > > > On 2015/09/02 21:11:14, commit-bot: I haz the power wrote:
> > > > > Patchset 13 (id:??) landed as
> > > > > https://crrev.com/510baeacbab311798d5e8795800ff773d00d062c
> > > > > Cr-Commit-Position: refs/heads/master@{#30550}
> > > > 
> > > > Based on a simple microbenchmark,
http://jsperf.com/v8-rest-parameters3/2,
> > > this
> > > > may have been more damaging to perf than I thought it would be :( This
> > > surprises
> > > > me a bit
> > > 
> > > Actually, I may have spoken too soon. I guess the xeon in my mac pro just
> gets
> > > through these tests slower than the i7 in my laptop. It still seems to be
> > > slightly faster than the old implementation
> > > http://jsperf.com/v8-rest-parameters3/3
> > 
> > Spoke too soon again, console.log was weighing heavier on the tests than I
> > thought :( http://jsperf.com/v8-rest-parameters2/4 so, the old
implementation
> > was faster :\ This is confusing since the desugaring is very similar to
> babel's
> > implementation
> 
> http://jsperf.com/v8-rest-parameters2/5 did some experimentation to see why
the
> babel impl does so much better, the biggest problem seems to be the use of
"let"
> (providing us with the proper TDZ). Some other improvements can also be
obtained
> via pre allocating the rest array rather than using the empty array literal.
> 
> (The bare*_func() versions do not exactly match the desugaring, due to not
using
> %AppendElement() and instead using babel-style assignment, which is not
> technically correct per-spec).
> 
> Things to improve include making `let` declarations faster in general (quite
> possibly related to hole-checking :() and pre-allocating the array size while
> still using the fastest spec-correct means of filling in the values.

Oh, the other thing is that the hole-checking isn't actually needed at all
unless there are binding patterns or default parameters in the formals. So,
using `var` instead of `let` in those cases would definitely speed things up.