Let LookupRecursive bind to NonLocals properly.

Let LookupRecursive bind to NonLocals properly.

This gets rid of the BindingsKind flag. It replaces the factory argument with a bool that indicates whether free variables should be resolved as well.

BUG=

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

[Toon Verwaest, 2016-08-23 17:25:45]

verwaest@chromium.org changed reviewers:
+ adamk@chromium.org, marja@chromium.org

[Toon Verwaest, 2016-08-23 17:25:46]

ptal

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1265: if (var == nullptr) return var;
If we decided we didn't want to resolve free variables, we also don't want to
mask them with with / eval info which would've been done by ResolveTo
previously.

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1267: if (is_function_scope() && !var->is_dynamic()) {
There's no point in context allocating dynamic variables.

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1282: if (!var->is_dynamic() && var->IsUnallocated()) {
Again, don't context allocate a dynamic var.

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1291: return nullptr;
If we reach the end and don't want to resolve free variables, return nullptr.

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1309: if (var->IsGlobalObjectProperty()) {
Replace dynamic scope-bound globals with dynamic lookup globals.

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1313: if (var->is_dynamic()) return var;
don't do anything if it's already dynamic.

https://codereview.chromium.org/2262393004/diff/20001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1343: if (var->IsGlobalObjectProperty()) {
if we reached a global native property it's unbound statically.

[Toon Verwaest, 2016-08-23 18:05:51]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 18:05:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adamk, 2016-08-23 18:15:10]

lgtm besides a few nits, feel free to submit once handled.

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:425: // Allocate the variables.
Let's kill this comment while we're here.

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1298: 
Can you add a few DCHECKs?

DCHECK(resolve_free);
DCHECK(!is_script_scope())

The above if/else is tricky enough to parse that those would help me read the
rest of this function.

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1383: // Note that we pass nullptr as AstNodeFactory: this
phase should not create
Dead comment

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.h#newcod...
src/ast/scopes.h:537: Variable* LookupRecursive(VariableProxy* proxy, bool
resolve_free,
Can you add a comment for the resolve_free argument? It's not clear from the
name what that does ("resolve_nonlocal" might be better). I guess the main thing
to convey is that this _creates_ variables. Maybe "declare_free"?

[commit-bot: I haz the power, 2016-08-23 18:27:28]

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

[commit-bot: I haz the power, 2016-08-23 18:27:29]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11456)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[Toon Verwaest, 2016-08-23 20:01:05]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 20:01:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-08-23 20:08:45]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 20:08:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-08-23 20:09:11]

Addressed comments.

ptal again, the change wasn't entirely trivial.

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:425: // Allocate the variables.
On 2016/08/23 18:15:10, adamk wrote:
> Let's kill this comment while we're here.

Done.

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1298: 
On 2016/08/23 18:15:09, adamk wrote:
> Can you add a few DCHECKs?
> 
> DCHECK(resolve_free);
> DCHECK(!is_script_scope())
> 
> The above if/else is tricky enough to parse that those would help me read the
> rest of this function.

I've changed the branches to make it more obvious. I did add
DCHECK(!is_script_scope()) which apparently was wrong in case of a deserialized
script scope and caused the failures on the bots. I fixed deserialization to not
generate nested scriptscopes.

https://codereview.chromium.org/2262393004/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1383: // Note that we pass nullptr as AstNodeFactory: this
phase should not create
On 2016/08/23 18:15:09, adamk wrote:
> Dead comment

Done.

[commit-bot: I haz the power, 2016-08-23 20:36:13]

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

[commit-bot: I haz the power, 2016-08-23 20:36:14]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-08-23 20:44:42]

lgtm

https://codereview.chromium.org/2262393004/diff/80001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2262393004/diff/80001/src/ast/scopes.h#newcod...
src/ast/scopes.h:641: scope_info_ = scope_info;
What ScopeInfo was in there before? Can we DCHECK that it's empty in some way?

[Toon Verwaest, 2016-08-23 20:51:46]

Addressed, thanks!

[Toon Verwaest, 2016-08-23 20:53:05]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-23 20:53:05]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2262393004/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-23 20:53:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 21:17:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-23 21:17:32]

Description was changed from

==========
Let LookupRecursive bind to NonLocals properly.

This gets rid of the BindingsKind flag. It replaces the factory argument with a
bool that indicates whether free variables should be resolved as well.

BUG=
==========

to

==========
Let LookupRecursive bind to NonLocals properly.

This gets rid of the BindingsKind flag. It replaces the factory argument with a
bool that indicates whether free variables should be resolved as well.

BUG=

Committed: https://crrev.com/ff40125ed882b0ead44d773f651f268d7b06506e
Cr-Commit-Position: refs/heads/master@{#38844}
==========

[commit-bot: I haz the power, 2016-08-23 21:17:33]

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