[es6] Fix scoping for default parameters in arrow functions

[es6] Fix scoping for default parameters in arrow functions

When eagerly parsing arrow functions, expressions in default
parameter initializers are parsed in the enclosing scope,
rather than in the function's scope (since that scope does not
yet exist). This leads to VariableProxies being added to the
wrong scope, and scope chains for FunctionLiterals being incorrect.

This patch addresses these problems by adding a subclass of
AstExpressionVisitor that moves VariableProxies to the proper
scope and fixes up scope chains of FunctionLiterals.

More work likely still needs to be done to make this work completely,
but it's very close to correct.

BUG=v8:4395
LOG=y

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

[adamk, 2015-10-16 08:18:32]

adamk@chromium.org changed reviewers:
+ caitpotter88@gmail.com, rossberg@chromium.org

[adamk, 2015-10-16 08:18:33]

This is still not perfect, but I finally got enough cleanup pushed for
AstVIsitors that this works without too much code duplication.

The approach still strikes me as horribly brittle, but I haven't been able to
find anything else that works (though I do have a few ideas of ways to avoid the
tree walk for VariableProxy re-scoping).

[rossberg, 2015-10-16 11:55:41]

lgtm

https://codereview.chromium.org/1405313002/diff/1/src/ast-expression-visitor.cc
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1405313002/diff/1/src/ast-expression-visitor....
src/ast-expression-visitor.cc:346: void
AstExpressionVisitor::VisitClassLiteral(ClassLiteral* expr) {
Oh, how did this even work before?

https://codereview.chromium.org/1405313002/diff/1/test/mjsunit/harmony/regres...
File test/mjsunit/harmony/regress/regress-4395.js (right):

https://codereview.chromium.org/1405313002/diff/1/test/mjsunit/harmony/regres...
test/mjsunit/harmony/regress/regress-4395.js:56: })(3, 4);
Perhaps also return x and y and verify on the outside that they were indeed
bound to 3 and 4.

[adamk, 2015-10-16 13:38:29]

https://codereview.chromium.org/1405313002/diff/1/src/ast-expression-visitor.cc
File src/ast-expression-visitor.cc (right):

https://codereview.chromium.org/1405313002/diff/1/src/ast-expression-visitor....
src/ast-expression-visitor.cc:346: void
AstExpressionVisitor::VisitClassLiteral(ClassLiteral* expr) {
On 2015/10/16 11:55:41, rossberg wrote:
> Oh, how did this even work before?

Since this was only designed for asm, it didn't have to support this syntax.

https://codereview.chromium.org/1405313002/diff/1/test/mjsunit/harmony/regres...
File test/mjsunit/harmony/regress/regress-4395.js (right):

https://codereview.chromium.org/1405313002/diff/1/test/mjsunit/harmony/regres...
test/mjsunit/harmony/regress/regress-4395.js:56: })(3, 4);
On 2015/10/16 11:55:41, rossberg wrote:
> Perhaps also return x and y and verify on the outside that they were indeed
> bound to 3 and 4.

Done (though I just put the test inside the function instead).

[adamk, 2015-10-16 13:39:54]

Hmm, I think the strong mode failures are due to the poor class scoping, so I'll
likely have to fix that before landing. It'll require a bit of refactoring in
AstExpressionVisitor most likely.

[adamk, 2015-10-19 09:51:55]

Waiting on https://codereview.chromium.org/1413903002/ to fix class scoping
(also may do a little more ClassLiteral cleanup before landing this).

[caitp (gmail), 2015-10-19 12:37:39]

On 2015/10/19 09:51:55, adamk wrote:
> Waiting on https://codereview.chromium.org/1413903002/ to fix class scoping
> (also may do a little more ClassLiteral cleanup before landing this).

I have a question:

How does the scope rewriting work for lazily parsed functions with default
parameters / initializers in binding elements? In those cases, you only know
about the outer scope as being a script scope, and don't really have any
knowledge of the available variables, right? Or is it not an issue

[caitp (gmail), 2015-10-19 12:38:02]

On 2015/10/19 12:37:39, caitp wrote:
> On 2015/10/19 09:51:55, adamk wrote:
> > Waiting on https://codereview.chromium.org/1413903002/ to fix class scoping
> > (also may do a little more ClassLiteral cleanup before landing this).
> 
> I have a question:
> 
> How does the scope rewriting work for lazily parsed functions with default
> parameters / initializers in binding elements? In those cases, you only know
> about the outer scope as being a script scope, and don't really have any
> knowledge of the available variables, right? Or is it not an issue

Sorry, this question applies to non-arrow functions as well

[adamk, 2015-10-19 13:55:58]

On 2015/10/19 12:38:02, caitp wrote:
> On 2015/10/19 12:37:39, caitp wrote:
> > On 2015/10/19 09:51:55, adamk wrote:
> > > Waiting on https://codereview.chromium.org/1413903002/ to fix class
scoping
> > > (also may do a little more ClassLiteral cleanup before landing this).
> > 
> > I have a question:
> > 
> > How does the scope rewriting work for lazily parsed functions with default
> > parameters / initializers in binding elements? In those cases, you only know
> > about the outer scope as being a script scope, and don't really have any
> > knowledge of the available variables, right? Or is it not an issue
> 
> Sorry, this question applies to non-arrow functions as well

Even for lazily parsed functions, the parameters are parsed eagerly, so I don't
foresee any difficulties here. Can you give an example of a particular case
you're worried about?

[caitp (gmail), 2015-10-19 13:58:07]

On 2015/10/19 13:55:58, adamk wrote:
> On 2015/10/19 12:38:02, caitp wrote:
> > On 2015/10/19 12:37:39, caitp wrote:
> > > On 2015/10/19 09:51:55, adamk wrote:
> > > > Waiting on https://codereview.chromium.org/1413903002/ to fix class
> scoping
> > > > (also may do a little more ClassLiteral cleanup before landing this).
> > > 
> > > I have a question:
> > > 
> > > How does the scope rewriting work for lazily parsed functions with default
> > > parameters / initializers in binding elements? In those cases, you only
know
> > > about the outer scope as being a script scope, and don't really have any
> > > knowledge of the available variables, right? Or is it not an issue
> > 
> > Sorry, this question applies to non-arrow functions as well
> 
> Even for lazily parsed functions, the parameters are parsed eagerly, so I
don't
> foresee any difficulties here. Can you give an example of a particular case
> you're worried about?

I was thinking of cases like `function f(a = b = c = 10) {}`. The lazily parsed
block begins at the `(`, and formal parameters are re-parsed. So it's possible
that `b` and `c` become dynamic lookups in that case, which seems unfortunate.

It's probably likely that this doesn't happen and I'm just missing something in
my mental model of how this works, though

[adamk, 2015-10-19 14:09:07]

On 2015/10/19 13:58:07, caitp wrote:
> On 2015/10/19 13:55:58, adamk wrote:
> > On 2015/10/19 12:38:02, caitp wrote:
> > > On 2015/10/19 12:37:39, caitp wrote:
> > > > On 2015/10/19 09:51:55, adamk wrote:
> > > > > Waiting on https://codereview.chromium.org/1413903002/ to fix class
> > scoping
> > > > > (also may do a little more ClassLiteral cleanup before landing this).
> > > > 
> > > > I have a question:
> > > > 
> > > > How does the scope rewriting work for lazily parsed functions with
default
> > > > parameters / initializers in binding elements? In those cases, you only
> know
> > > > about the outer scope as being a script scope, and don't really have any
> > > > knowledge of the available variables, right? Or is it not an issue
> > > 
> > > Sorry, this question applies to non-arrow functions as well
> > 
> > Even for lazily parsed functions, the parameters are parsed eagerly, so I
> don't
> > foresee any difficulties here. Can you give an example of a particular case
> > you're worried about?
> 
> I was thinking of cases like `function f(a = b = c = 10) {}`. The lazily
parsed
> block begins at the `(`, and formal parameters are re-parsed. So it's possible
> that `b` and `c` become dynamic lookups in that case, which seems unfortunate.

I think lazy parsing starts at the '{', see
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/parser.cc&l...
and note that above that in the function the parameters are all parsed eagerly.
Am I missing something?

[caitp (gmail), 2015-10-19 14:23:54]

On 2015/10/19 14:09:07, adamk wrote:
> On 2015/10/19 13:58:07, caitp wrote:
> > On 2015/10/19 13:55:58, adamk wrote:
> > > On 2015/10/19 12:38:02, caitp wrote:
> > > > On 2015/10/19 12:37:39, caitp wrote:
> > > > > On 2015/10/19 09:51:55, adamk wrote:
> > > > > > Waiting on https://codereview.chromium.org/1413903002/ to fix class
> > > scoping
> > > > > > (also may do a little more ClassLiteral cleanup before landing
this).
> > > > > 
> > > > > I have a question:
> > > > > 
> > > > > How does the scope rewriting work for lazily parsed functions with
> default
> > > > > parameters / initializers in binding elements? In those cases, you
only
> > know
> > > > > about the outer scope as being a script scope, and don't really have
any
> > > > > knowledge of the available variables, right? Or is it not an issue
> > > > 
> > > > Sorry, this question applies to non-arrow functions as well
> > > 
> > > Even for lazily parsed functions, the parameters are parsed eagerly, so I
> > don't
> > > foresee any difficulties here. Can you give an example of a particular
case
> > > you're worried about?
> > 
> > I was thinking of cases like `function f(a = b = c = 10) {}`. The lazily
> parsed
> > block begins at the `(`, and formal parameters are re-parsed. So it's
possible
> > that `b` and `c` become dynamic lookups in that case, which seems
unfortunate.
> 
> I think lazy parsing starts at the '{', see
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/parser.cc&l...
> and note that above that in the function the parameters are all parsed
eagerly.
> Am I missing something?

It looks like you're right, only the beginning of the body is logged
(https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/preparser.c...).

But then, if the lazily parsed function has no knowledge of the formal
parameters, how did lazily parsed parameters that need desugaring ever work?

This doesn't really require an answer, I'll figure it out later on when I have
some time.

[adamk, 2015-10-20 08:16:08]

Description was changed from

==========
[es6] Fix scoping for default parameters in arrow functions

When eagerly parsing arrow functions, expressions in default
parameter initializers are parsed in the enclosing scope,
rather than in the function's scope (since that scope does not
yet exist). This leads to VariableProxies being added to the
wrong scope, and scope chains for FunctionLiterals being incorrect.

This patch addresses these problems by adding a subclass of
AstExpressionVisitor that moves VariableProxies to the proper
scope and fixes up scope chains of FunctionLiterals.

More work still needs to be done to make this work completely:
ClassLiterals are not yet properly re-scoped, at least.

BUG=v8:4395
LOG=y
==========

to

==========
[es6] Fix scoping for default parameters in arrow functions

When eagerly parsing arrow functions, expressions in default
parameter initializers are parsed in the enclosing scope,
rather than in the function's scope (since that scope does not
yet exist). This leads to VariableProxies being added to the
wrong scope, and scope chains for FunctionLiterals being incorrect.

This patch addresses these problems by adding a subclass of
AstExpressionVisitor that moves VariableProxies to the proper
scope and fixes up scope chains of FunctionLiterals.

More work likely still needs to be done to make this work completely,
but it's very close to correct.

BUG=v8:4395
LOG=y
==========

[adamk, 2015-10-20 08:17:36]

Class literals should now be working properly. I suspect there are still
problems here, but it passes all the regression tests I handed it at the moment.

[rossberg, 2015-10-20 09:12:35]

lgtm

[adamk, 2015-10-20 09:13:50]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-10-20 09:14:03]

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

[commit-bot: I haz the power, 2015-10-20 09:15:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-20 09:15:45]

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

[Benedikt Meurer, 2015-10-20 10:35:59]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1417463004/ by bmeurer@chromium.org.

The reason for reverting is: Breaks nosnap:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20....