Fix eval calls in initializers of arrow function parameters

Fix eval calls in initializers of arrow function parameters

This requires copying usage flags from the outer scope to the
arrow scope upon encountering the arrow token.

In order to properly pass-on the calls_eval bit, now record
that bit on script scopes just like everywhere else, and add
necessary code to scopes.cc to handle that change in behavior.

Also factored out scope flag propagation to its own method to
make the call site simple (though note that only the eval
bit makes any difference for arrows).

BUG=v8:4395
LOG=n

Committed: https://crrev.com/0bdaa4d877f0823dbecb08fd9d409c1ebdbc1706
Cr-Commit-Position: refs/heads/master@{#31660}

[adamk, 2015-10-22 12:07:27]

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

[rossberg, 2015-10-22 12:21:28]

https://codereview.chromium.org/1423613002/diff/1/src/parameter-initializer-r...
File src/parameter-initializer-rewriter.cc (right):

https://codereview.chromium.org/1423613002/diff/1/src/parameter-initializer-r...
src/parameter-initializer-rewriter.cc:80: if (old_scope->calls_eval())
new_scope->RecordEvalCall();
I wonder about all the other scope flags. Do we know they aren't relevant?

[adamk, 2015-10-22 12:31:07]

On 2015/10/22 12:21:28, rossberg wrote:
>
https://codereview.chromium.org/1423613002/diff/1/src/parameter-initializer-r...
> File src/parameter-initializer-rewriter.cc (right):
> 
>
https://codereview.chromium.org/1423613002/diff/1/src/parameter-initializer-r...
> src/parameter-initializer-rewriter.cc:80: if (old_scope->calls_eval())
> new_scope->RecordEvalCall();
> I wonder about all the other scope flags. Do we know they aren't relevant?

Good question. There are three relevant flags:

- eval
- super
- arguments

This patch fixes eval.

'super' is already tested, and doesn't need to be propagated because it's
already in the "right" place (since the arrow function itself will never have
its own super, and will always get it from the enclosing scope anyway, where we
already set the bit as part of initial parsing).

'arguments' is similar to 'super': arrow functions have no 'arguments' of their
own, only the original outer scope needs the bit.

I could set the bits here for "completeness" (though they'd be untestable) or
add a comment. Thoughts?

[adamk, 2015-10-22 12:32:44]

Hmm, the bots don't look too happy, will investigate whether this is the
is_script_scope() bit.

[adamk, 2015-10-22 12:40:01]

Yep, it's that bit (setting calls_eval on a script scope). No idea why TurboFan
barfs on this.

But this makes me wonder if we should change how we record eval calls instead,
for example by adding a bit to ExpressionClassifier and then only calling
RecordEvalCall() on the right scope once we've classified the expression.

[rossberg, 2015-10-22 13:00:44]

On 2015/10/22 12:31:07, adamk wrote:
> On 2015/10/22 12:21:28, rossberg wrote:
> >
>
https://codereview.chromium.org/1423613002/diff/1/src/parameter-initializer-r...
> > File src/parameter-initializer-rewriter.cc (right):
> > 
> >
>
https://codereview.chromium.org/1423613002/diff/1/src/parameter-initializer-r...
> > src/parameter-initializer-rewriter.cc:80: if (old_scope->calls_eval())
> > new_scope->RecordEvalCall();
> > I wonder about all the other scope flags. Do we know they aren't relevant?
> 
> Good question. There are three relevant flags:
> 
> - eval
> - super
> - arguments
> 
> This patch fixes eval.
> 
> 'super' is already tested, and doesn't need to be propagated because it's
> already in the "right" place (since the arrow function itself will never have
> its own super, and will always get it from the enclosing scope anyway, where
we
> already set the bit as part of initial parsing).
> 
> 'arguments' is similar to 'super': arrow functions have no 'arguments' of
their
> own, only the original outer scope needs the bit.
> 
> I could set the bits here for "completeness" (though they'd be untestable) or
> add a comment. Thoughts?

It's arguably more correct to do that, even if there are no tests. Similarly
with some of the other flags (such as contains_with, which will likely be
relevant for do-expressions). I think it makes sense to just consistently update
all flags.

[adamk, 2015-10-26 21:14:21]

I've factored out the flag propagation logic from FinalizeBlockScope() into a
new method, and called it from the rewriter.

Haven't added contains_with yet, partly because I'm not totally sure what it's
for: I removed it and only a few debugger tests failed.

Still failing on TurboFan, but I think I fixed this during my flight on Friday,
with the fix being on my laptop. Will try to resurrect that tomorrow.

[adamk, 2015-10-27 22:41:55]

Description was changed from

==========
Fix eval calls in initializers of arrow function parameters

This requires rolling back what looks like a cleanup in
https://codereview.chromium.org/7904008/ that skipped setting
the scope_calls_eval_ bit for global (script) scopes. My
understanding is that that bit has no effect on script scopes
anyway, but I'll let the tests be the judge of that.

BUG=v8:4395
LOG=n
==========

to

==========
Fix eval calls in initializers of arrow function parameters

This requires copying usage flags from the outer scope to the
arrow scope upon encountering the arrow token.

In order to properly pass-on the calls_eval bit, now record
that bit on script scopes just like everywhere else, and add
necessary code to scopes.cc to handle that change in behavior.

Also factored out scope flag propagation to its own method to
make the call site simple (though note that only the eval
bit makes any difference for arrows).

BUG=v8:4395
LOG=n
==========

[adamk, 2015-10-27 22:44:19]

Okay, this is ready for re-review, finally. See updated CL description for
what's changed (tl;dr: only propagate flags in ParserBase, instead of in the
rewriter)

[adamk, 2015-10-27 23:07:27]

Re: various flags, I'm still not sure it makes sense to propagate anything other
than eval in this patch. This is in fact the only way for uses_super_ to get set
on an arrow scope (normally it's set on ReceiverScope()). And uses_arguments
probably _should_ be set on ReceiverScope.

But I can clean that stuff up in a followup if you'd like this to maintain
consistency (I'd really love to understand what contains_with_ is for...)

[adamk, 2015-10-27 23:19:37]

On 2015/10/27 23:07:27, adamk wrote:
> Re: various flags, I'm still not sure it makes sense to propagate anything
other
> than eval in this patch. This is in fact the only way for uses_super_ to get
set
> on an arrow scope (normally it's set on ReceiverScope()). And uses_arguments
> probably _should_ be set on ReceiverScope.
> 
> But I can clean that stuff up in a followup if you'd like this to maintain
> consistency (I'd really love to understand what contains_with_ is for...)

Ah, the reason we didn't need contains_with_ for FinalizeBlockScope() is that it
gets set on the DeclarationScope (so it's never set on block scopes). I'll add
that bit to PropagateUsageFlags, but it'll be a no-op for now.

[adamk, 2015-10-28 18:04:41]

To be clear: this is ready for review again.

[rossberg, 2015-10-29 10:41:47]

lgtm

https://codereview.chromium.org/1423613002/diff/150001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1423613002/diff/150001/src/scopes.h#newcode122
src/scopes.h:122: void PropagateUsageFlagsToScope(Scope* other);
Nit: It would seem more natural to do this the other way round, i.e. have a
method PropagateUsageFlagsFrom(Scope*). But I leave that up to you.

[adamk, 2015-10-29 14:44:59]

https://codereview.chromium.org/1423613002/diff/150001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1423613002/diff/150001/src/scopes.h#newcode122
src/scopes.h:122: void PropagateUsageFlagsToScope(Scope* other);
On 2015/10/29 10:41:47, rossberg wrote:
> Nit: It would seem more natural to do this the other way round, i.e. have a
> method PropagateUsageFlagsFrom(Scope*). But I leave that up to you.

This way felt natural enough to me, I'm inclined to leave as-is (as discussed
elsewhere in this review, this particular method may not be long for this
world).

[adamk, 2015-10-29 14:49:56]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-10-29 14:49:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1423613002/#ps170001
(title: "Add comment, rebase")

[commit-bot: I haz the power, 2015-10-29 14:49:59]

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

[commit-bot: I haz the power, 2015-10-29 15:16:11]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-10-29 15:16:52]

Patchset 10 (id:??) landed as
https://crrev.com/0bdaa4d877f0823dbecb08fd9d409c1ebdbc1706
Cr-Commit-Position: refs/heads/master@{#31660}