Put Scopes into temporary Zone

src/ast/scopes.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/ast/scopes.h"
 
 #include "src/accessors.h"
 #include "src/ast/scopeinfo.h"
 #include "src/bootstrapper.h"
 #include "src/messages.h"
@@ skipping 262 matching lines @@
       return false;
     }
   }
 
 #ifdef DEBUG
   if (info->script_is_native() ? FLAG_print_builtin_scopes
                                : FLAG_print_scopes) {
     scope->Print();
   }
   scope->CheckScopePositions();
+  scope->CheckZones();
 #endif
 
   info->set_scope(scope);
   return true;
 }
 
 void Scope::DeclareThis(AstValueFactory* ast_value_factory) {
   DCHECK(!already_resolved());
   DCHECK(is_declaration_scope());
   DCHECK(has_this_declaration());
@@ skipping 205 matching lines @@
   } else if (!scope_info_.is_null()) {
     // If we are backed by a scope info, try to lookup the variable there.
     VariableMode mode;
     int index = scope_info_->FunctionContextSlotIndex(*(name->string()), &mode);
     if (index < 0) return NULL;
     Variable* var = new (zone())
         Variable(this, name, mode, Variable::NORMAL, kCreatedInitialized);
     VariableProxy* proxy = factory->NewVariableProxy(var);
     VariableDeclaration* declaration =
         factory->NewVariableDeclaration(proxy, mode, this, kNoSourcePosition);
+    DCHECK_EQ(factory->zone(), zone());
     DeclareFunctionVar(declaration);
     var->AllocateTo(VariableLocation::CONTEXT, index);
     return var;
   } else {
     return NULL;
   }
 }
 
 
 Variable* Scope::Lookup(const AstRawString* name) {
@@ skipping 365 matching lines @@
     if (proxy->is_resolved() && proxy->var()->IsStackAllocated()) continue;
     Handle<String> name = proxy->name();
     non_locals = StringSet::Add(non_locals, name);
   }
   for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
     non_locals = scope->CollectNonLocals(non_locals);
   }
   return non_locals;
 }
 
+void Scope::CollectUnresolvableLocals(VariableProxy** still_unresolved,
+                                      Scope* max_outer_scope) {
+  BindingKind binding_kind;
+  for (VariableProxy *proxy = unresolved_, *next = nullptr; proxy != nullptr;
+       proxy = next) {
+    next = proxy->next_unresolved();
+    // Note that we pass nullptr as AstNodeFactory: this phase should not create
+    // any new AstNodes, since none of the Scopes involved are backed up by

[adamk, 2016/07/29 22:12:51]

Is there an obvious place to add a DCHECK for this invariant?

[marja, 2016/08/01 11:02:50]

I added a DCHECK_NOT_NULL(factory); statement in LookupFunctionVar which was the
only one along this code path.

They're not super useful though, since we'd crash anyway if we try to use a
nullptr as AstNodeFactory. It's a matter of style whether we should just let it
crash or DCHECK / CHECK nullity. :)

+    // ScopeInfo.
+    if (LookupRecursive(proxy, &binding_kind, nullptr, max_outer_scope) ==
+        nullptr) {
+      proxy->set_next_unresolved(*still_unresolved);
+      *still_unresolved = proxy;
+    }
+  }
+
+  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
+    scope->CollectUnresolvableLocals(still_unresolved, max_outer_scope);
+  }
+}
+
+void Scope::AnalyzePartially(Scope* migrate_to,
+                             AstNodeFactory* ast_node_factory) {
+  // Try to resolve unresolved variables for this Scope and collect those which
+  // cannot be resolved inside. It doesn't make sense to try to resolve them in
+  // the outer Scopes here, because they are incomplete.
+  VariableProxy* still_unresolved = nullptr;
+  CollectUnresolvableLocals(&still_unresolved, this);
+
+  // Re-create the VariableProxies in the right Zone and insert them into
+  // migrate_to.
+  for (VariableProxy* proxy = still_unresolved; proxy != nullptr;
+       proxy = proxy->next_unresolved()) {
+    migrate_to->NewUnresolved(
+        ast_node_factory, proxy->raw_name(),
+        proxy->is_this() ? Variable::THIS : Variable::NORMAL, proxy->position(),

[adamk, 2016/07/29 22:12:51]

I had to go read some code to understand why you didn't have to deal with
Variable::ARGUMENTS and Variable::FUNCTION. Seems kind of wrong that we re-use
the Variable::Kind enum for this purpose, but maybe it's no worse than a
boolean.

[marja, 2016/08/01 11:02:50]

Yeah, this is not good, and it's also nontrivial to see whether other
VariableProxy flags such as is_assigned() need to be migrated or not.

I added a methods into AstNodeFactory / VariableProxy to do the cloning in a
clean way. This way, if there is a new field added to VariableProxy, it's more
probable that the cloning functions get updated properly.

+        proxy->end_position());
+  }
+
+  // Gather info from inner scopes...
+  PropagateScopeInfo(false);

[adamk, 2016/07/29 22:12:51]

This is called before variable resolution in Scope::Analyze, but I guess that
doesn't matter here because we don't actually care what kind of binding we have?
Either we find a variable or not.

[marja, 2016/08/01 11:02:50]

I moved this call up, just to keep it more unified; there was no particular
reason to do it in this order.

+
+  // ... and push it up to migrate_to. Note that migrate_to and this Scope
+  // describe the same Scope, just in different Zones.
+  PropagateUsageFlagsToScope(migrate_to);
+  if (inner_scope_calls_eval_) {
+    migrate_to->inner_scope_calls_eval_ = true;
+  }
+  DCHECK(!force_eager_compilation_);
+  migrate_to->set_start_position(start_position_);
+  migrate_to->set_end_position(end_position_);
+  migrate_to->language_mode_ = language_mode_;

[adamk, 2016/07/29 22:12:51]

One thing I'd recommend before submitting would be to comment out bits of this
function (like these flags) and make sure that something breaks. If it doesn't,
needs more tests :)

[marja, 2016/08/01 11:02:50]

Done: I didn't have these things from the start, and only added them because
something broke :)

+  outer_scope_->RemoveInnerScope(this);
+  DCHECK_EQ(outer_scope_, migrate_to->outer_scope_);
+  DCHECK_EQ(outer_scope_->zone(), migrate_to->zone());
+}
 
 #ifdef DEBUG
 static const char* Header(ScopeType scope_type, FunctionKind function_kind,
                           bool is_declaration_scope) {
   switch (scope_type) {
     case EVAL_SCOPE: return "eval";
     // TODO(adamk): Should we print concise method scopes specially?
     case FUNCTION_SCOPE:
       if (IsGeneratorFunction(function_kind)) return "function*";
       if (IsAsyncFunction(function_kind)) return "async function";
@@ skipping 188 matching lines @@
   // A scope is allowed to have invalid positions if it is hidden and has no
   // inner scopes
   if (!is_hidden() && inner_scope_ == nullptr) {
     CHECK_NE(kNoSourcePosition, start_position());
     CHECK_NE(kNoSourcePosition, end_position());
   }
   for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
     scope->CheckScopePositions();
   }
 }
+
+void Scope::CheckZones() {
+  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
+    CHECK_EQ(scope->zone(), zone());
+  }
+}
 #endif  // DEBUG
 
 
 Variable* Scope::NonLocal(const AstRawString* name, VariableMode mode) {
   if (dynamics_ == NULL) dynamics_ = new (zone()) DynamicScopePart(zone());
   VariableMap* map = dynamics_->GetMap(mode);
   Variable* var = map->Lookup(name);
   if (var == NULL) {
     // Declare a new non-local.
     InitializationFlag init_flag = (mode == VAR)
         ? kCreatedInitialized : kNeedsInitialization;
     var = map->Declare(NULL,
                        name,
                        mode,
                        Variable::NORMAL,
                        init_flag);
     // Allocate it by giving it a dynamic lookup.
     var->AllocateTo(VariableLocation::LOOKUP, -1);
   }
   return var;
 }
 
-
 Variable* Scope::LookupRecursive(VariableProxy* proxy,
                                  BindingKind* binding_kind,
-                                 AstNodeFactory* factory) {
+                                 AstNodeFactory* factory,
+                                 Scope* max_outer_scope) {
   DCHECK(binding_kind != NULL);
   if (already_resolved() && is_with_scope()) {
     // Short-cut: if the scope is deserialized from a scope info, variable
     // allocation is already fixed.  We can simply return with dynamic lookup.
     *binding_kind = DYNAMIC_LOOKUP;
     return NULL;
   }
 
   // Try to find the variable in this scope.
   Variable* var = LookupLocal(proxy->raw_name());
 
   // 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 != NULL) {
     *binding_kind = BOUND;
     return var;
   }
 
   // We did not find a variable locally. Check against the function variable,
   // if any. We can do this for all scopes, since the function variable is
   // only present - if at all - for function scopes.
   *binding_kind = UNBOUND;
   var = LookupFunctionVar(proxy->raw_name(), factory);
   if (var != NULL) {
     *binding_kind = BOUND;
-  } else if (outer_scope_ != NULL) {
-    var = outer_scope_->LookupRecursive(proxy, binding_kind, factory);
+  } else if (outer_scope_ != nullptr && this != max_outer_scope) {
+    var = outer_scope_->LookupRecursive(proxy, binding_kind, factory,
+                                        max_outer_scope);
     if (*binding_kind == BOUND && (is_function_scope() || is_with_scope())) {
       var->ForceContextAllocation();
     }
   } else {
-    DCHECK(is_script_scope());
+    DCHECK(is_script_scope() || this == max_outer_scope);
   }
 
   // "this" can't be shadowed by "eval"-introduced bindings or by "with" scopes.
   // TODO(wingo): There are other variables in this category; add them.
   bool name_can_be_shadowed = var == nullptr || !var->is_this();
 
   if (is_with_scope() && name_can_be_shadowed) {
     DCHECK(!already_resolved());
     // The current scope is a with scope, so the variable binding can not be
     // statically resolved. However, note that it was necessary to do a lookup
@@ skipping 407 matching lines @@
       function_ != NULL && function_->proxy()->var()->IsContextSlot();
   return num_heap_slots() - Context::MIN_CONTEXT_SLOTS - num_global_slots() -
          (is_function_var_in_context ? 1 : 0);
 }
 
 
 int Scope::ContextGlobalCount() const { return num_global_slots(); }
 
 }  // namespace internal
 }  // namespace v8