Let LookupRecursive bind to NonLocals properly.

src/ast/scopes.cc

Index: src/ast/scopes.cc
diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc
index 7689786ce46606f07090155b3e0194f777c3cdad..c7b86a3bd354a9e9f8f3d574c9a73b25089b5de8 100644
--- a/src/ast/scopes.cc
+++ b/src/ast/scopes.cc
@@ -423,10 +423,7 @@ void Scope::Analyze(ParseInfo* info) {
          scope->outer_scope()->already_resolved_);
 
   // Allocate the variables.

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

Let's kill this comment while we're here.

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

Done.

-  {
-    AstNodeFactory ast_node_factory(info->ast_value_factory());
-    scope->AllocateVariables(info, &ast_node_factory);
-  }
+  scope->AllocateVariables(info);
 
 #ifdef DEBUG
   if (info->script_is_native() ? FLAG_print_builtin_scopes
@@ -844,13 +841,12 @@ void Scope::CollectStackAndContextLocals(ZoneList<Variable*>* stack_locals,
   }
 }
 
-void DeclarationScope::AllocateVariables(ParseInfo* info,
-                                         AstNodeFactory* factory) {
+void DeclarationScope::AllocateVariables(ParseInfo* info) {
   // 1) Propagate scope information.
   PropagateScopeInfo();
 
   // 2) Resolve variables.
-  ResolveVariablesRecursively(info, factory);
+  ResolveVariablesRecursively(info);
 
   // 3) Allocate variables.
   AllocateVariablesRecursively();
@@ -1229,13 +1225,9 @@ Variable* Scope::NonLocal(const AstRawString* name, VariableMode mode) {
   return var;
 }
 
-Variable* Scope::LookupRecursive(VariableProxy* proxy,
-                                 BindingKind* binding_kind,
-                                 AstNodeFactory* factory,
+Variable* Scope::LookupRecursive(VariableProxy* proxy, bool resolve_free,
                                  Scope* outer_scope_end) {
   DCHECK_NE(outer_scope_end, this);
-  DCHECK_NOT_NULL(binding_kind);
-  DCHECK_EQ(UNBOUND, *binding_kind);
   // Short-cut: whenever we find a debug-evaluate scope, just look everything up
   // dynamically. Debug-evaluate doesn't properly create scope info for the
   // lookups it does. It may not have a valid 'this' declaration, and anything
@@ -1244,8 +1236,8 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy,
   // TODO(yangguo): Remove once debug-evaluate creates proper ScopeInfo for the
   // scopes in which it's evaluating.
   if (is_debug_evaluate_scope_) {
-    *binding_kind = DYNAMIC_LOOKUP;
-    return nullptr;
+    if (!resolve_free) return nullptr;
+    return NonLocal(proxy->raw_name(), DYNAMIC);
   }
 
   // Try to find the variable in this scope.
@@ -1254,31 +1246,31 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy,
   // We found a variable and we are done. (Even if there is an 'eval' in this
   // scope which introduces the same variable again, the resulting variable
   // remains the same.)
-  if (var != nullptr) {
-    *binding_kind = BOUND;
-    return var;
-  }
+  if (var != nullptr) return var;
 
   // We did not find a variable locally. Check against the function variable, if
   // any.
   if (is_function_scope()) {
     var = AsDeclarationScope()->LookupFunctionVar(proxy->raw_name());
     if (var != nullptr) {
-      *binding_kind = calls_sloppy_eval() ? BOUND_EVAL_SHADOWED : BOUND;
+      if (calls_sloppy_eval()) return NonLocal(proxy->raw_name(), DYNAMIC);
       return var;
     }
   }
 
   if (outer_scope_ != outer_scope_end) {
-    var = outer_scope_->LookupRecursive(proxy, binding_kind, factory,
-                                        outer_scope_end);
-    if (*binding_kind == BOUND && is_function_scope()) {
+    var = outer_scope_->LookupRecursive(proxy, resolve_free, outer_scope_end);
+
+    // The variable could not be resolved statically.
+    if (var == nullptr) return var;
+
+    if (is_function_scope() && !var->is_dynamic()) {
       var->ForceContextAllocation();
     }
     // "this" can't be shadowed by "eval"-introduced bindings or by "with"
     // scopes.
     // TODO(wingo): There are other variables in this category; add them.
-    if (var != nullptr && var->is_this()) return var;
+    if (var->is_this()) return var;
 
     if (is_with_scope()) {
       // The current scope is a with scope, so the variable binding can not be
@@ -1287,21 +1279,24 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy,
       // scope, the associated variable has to be marked as potentially being
       // accessed from inside of an inner with scope (the property may not be in
       // the 'with' object).
-      if (var != nullptr && var->IsUnallocated()) {
+      if (!var->is_dynamic() && var->IsUnallocated()) {
         DCHECK(!already_resolved_);
         var->set_is_used();
         var->ForceContextAllocation();
         if (proxy->is_assigned()) var->set_maybe_assigned();
       }
-      *binding_kind = DYNAMIC_LOOKUP;
-      return nullptr;
+      return NonLocal(proxy->raw_name(), DYNAMIC);
     }
+  } else if (!resolve_free) {
+    return nullptr;
   } else {
-    DCHECK(!is_with_scope());
-    DCHECK(is_function_scope() || is_script_scope() || is_eval_scope());
+    DCHECK(is_script_scope());
+    // No binding has been found. Declare a variable on the global object.
+    return AsDeclarationScope()->DeclareDynamicGlobal(proxy->raw_name(),
+                                                      Variable::NORMAL);
   }
 

[adamk, 2016/08/23 18:15:09]

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.

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

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.

-  if (calls_sloppy_eval() && is_declaration_scope() && !is_script_scope()) {
+  if (calls_sloppy_eval() && is_declaration_scope()) {
     // A variable binding may have been found in an outer scope, but the current
     // scope makes a sloppy 'eval' call, so the found variable may not be the
     // correct one (the 'eval' may introduce a binding with the same name). In
@@ -1309,18 +1304,21 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy,
     // scopes that can host var bindings (declaration scopes) need be considered
     // here (this excludes block and catch scopes), and variable lookups at
     // script scope are always dynamic.
-    if (*binding_kind == BOUND) {
-      *binding_kind = BOUND_EVAL_SHADOWED;
-    } else if (*binding_kind == UNBOUND) {
-      *binding_kind = UNBOUND_EVAL_SHADOWED;
+    if (var->IsGlobalObjectProperty()) {
+      return NonLocal(proxy->raw_name(), DYNAMIC_GLOBAL);
     }
+
+    if (var->is_dynamic()) return var;
+
+    Variable* invalidated = var;
+    var = NonLocal(proxy->raw_name(), DYNAMIC_LOCAL);
+    var->set_local_if_not_shadowed(invalidated);
   }
 
   return var;
 }
 
-void Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy,
-                            AstNodeFactory* factory) {
+void Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy) {
   DCHECK(info->script_scope()->is_script_scope());
 
   // If the proxy is already resolved there's nothing to do
@@ -1328,21 +1326,19 @@ void Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy,
   if (proxy->is_resolved()) return;
 
   // Otherwise, try to resolve the variable.
-  BindingKind binding_kind = UNBOUND;
-  Variable* var = LookupRecursive(proxy, &binding_kind, factory);
+  Variable* var = LookupRecursive(proxy, true, nullptr);
 
-  ResolveTo(info, binding_kind, proxy, var);
+  ResolveTo(info, proxy, var);
 }
 
-void Scope::ResolveTo(ParseInfo* info, BindingKind binding_kind,
-                      VariableProxy* proxy, Variable* var) {
+void Scope::ResolveTo(ParseInfo* info, VariableProxy* proxy, Variable* var) {
 #ifdef DEBUG
   if (info->script_is_native()) {
     // To avoid polluting the global object in native scripts
     //  - Variables must not be allocated to the global scope.
     CHECK_NOT_NULL(outer_scope());
     //  - Variables must be bound locally or unallocated.
-    if (BOUND != binding_kind) {
+    if (var->IsGlobalObjectProperty()) {
       // The following variable name may be minified. If so, disable
       // minification in js2c.py for better output.
       Handle<String> name = proxy->raw_name()->string();
@@ -1357,62 +1353,23 @@ void Scope::ResolveTo(ParseInfo* info, BindingKind binding_kind,
   }
 #endif
 
-  switch (binding_kind) {
-    case BOUND:
-      break;
-
-    case BOUND_EVAL_SHADOWED:
-      // We either found a variable binding that might be shadowed by eval  or
-      // gave up on it (e.g. by encountering a local with the same in the outer
-      // scope which was not promoted to a context, this can happen if we use
-      // debugger to evaluate arbitrary expressions at a break point).
-      if (var->IsGlobalObjectProperty()) {
-        var = NonLocal(proxy->raw_name(), DYNAMIC_GLOBAL);
-      } else if (var->is_dynamic()) {
-        var = NonLocal(proxy->raw_name(), DYNAMIC);
-      } else {
-        Variable* invalidated = var;
-        var = NonLocal(proxy->raw_name(), DYNAMIC_LOCAL);
-        var->set_local_if_not_shadowed(invalidated);
-      }
-      break;
-
-    case UNBOUND:
-      // No binding has been found. Declare a variable on the global object.
-      var = info->script_scope()->DeclareDynamicGlobal(proxy->raw_name(),
-                                                       Variable::NORMAL);
-      break;
-
-    case UNBOUND_EVAL_SHADOWED:
-      // No binding has been found. But some scope makes a sloppy 'eval' call.
-      var = NonLocal(proxy->raw_name(), DYNAMIC_GLOBAL);
-      break;
-
-    case DYNAMIC_LOOKUP:
-      // The variable could not be resolved statically.
-      var = NonLocal(proxy->raw_name(), DYNAMIC);
-      break;
-  }
-
-  DCHECK(var != NULL);
+  DCHECK_NOT_NULL(var);
   if (proxy->is_assigned()) var->set_maybe_assigned();
-
   proxy->BindTo(var);
 }
 
-void Scope::ResolveVariablesRecursively(ParseInfo* info,
-                                        AstNodeFactory* factory) {
+void Scope::ResolveVariablesRecursively(ParseInfo* info) {
   DCHECK(info->script_scope()->is_script_scope());
 
   // Resolve unresolved variables for this scope.
   for (VariableProxy* proxy = unresolved_; proxy != nullptr;
        proxy = proxy->next_unresolved()) {
-    ResolveVariable(info, proxy, factory);
+    ResolveVariable(info, proxy);
   }
 
   // Resolve unresolved variables for inner scopes.
   for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    scope->ResolveVariablesRecursively(info, factory);
+    scope->ResolveVariablesRecursively(info);
   }
 }
 
@@ -1426,16 +1383,13 @@ VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
     // Note that we pass nullptr as AstNodeFactory: this phase should not create

[adamk, 2016/08/23 18:15:09]

Dead comment

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

Done.

     // any new AstNodes, since none of the Scopes involved are backed up by
     // ScopeInfo.
-    BindingKind binding_kind = UNBOUND;
-    Variable* var = LookupRecursive(proxy, &binding_kind, nullptr,
-                                    max_outer_scope->outer_scope());
+    Variable* var =
+        LookupRecursive(proxy, false, max_outer_scope->outer_scope());
     if (var == nullptr) {
       proxy->set_next_unresolved(stack);
       stack = proxy;
     } else if (info != nullptr) {
-      DCHECK_NE(UNBOUND, binding_kind);
-      DCHECK_NE(UNBOUND_EVAL_SHADOWED, binding_kind);
-      ResolveTo(info, binding_kind, proxy, var);
+      ResolveTo(info, proxy, var);
     }
   }