[scopes] Separate CollectNonLocals from AnalyzePartially

The functions are sufficiently different and AnalyzePartially is
performance-sensitive so it's helpful to see what exact code is in there.

Change-Id: I8807684e58a7af48bdba62f0adf41e78f7b6557d
Reviewed-on: https://chromium-review.googlesource.com/c/1335701Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57514}

BUILD.gn

@@ -1608,7 +1608,6 @@ v8_source_set("v8_base") {
     "src/ast/modules.h",
     "src/ast/prettyprinter.cc",
     "src/ast/prettyprinter.h",
-    "src/ast/scopes-inl.h",
     "src/ast/scopes.cc",
     "src/ast/scopes.h",
     "src/ast/variables.cc",

src/ast/scopes-inl.h

@@ -10,55 +10,6 @@
 namespace v8 {
 namespace internal {
 
-template <typename T>
-void Scope::ResolveScopesThenForEachVariable(DeclarationScope* max_outer_scope,
-                                             T variable_proxy_stackvisitor,
-                                             ParseInfo* info) {
-  // Module variables must be allocated before variable resolution
-  // to ensure that UpdateNeedsHoleCheck() can detect import variables.
-  if (info != nullptr && is_module_scope()) {
-    AsModuleScope()->AllocateModuleVariables();
-  }
-  // Lazy parsed declaration scopes are already partially analyzed. If there are
-  // unresolved references remaining, they just need to be resolved in outer
-  // scopes.
-  Scope* lookup =
-      is_declaration_scope() && AsDeclarationScope()->was_lazily_parsed()
-          ? outer_scope()
-          : this;
-
-  for (VariableProxy *proxy = unresolved_list_.first(), *next = nullptr;
-       proxy != nullptr; proxy = next) {
-    next = proxy->next_unresolved();
-
-    DCHECK(!proxy->is_resolved());
-    Variable* var = Lookup(proxy, lookup, max_outer_scope->outer_scope());
-    if (var == nullptr) {
-      variable_proxy_stackvisitor(proxy);
-    } else if (var != Scope::kDummyPreParserVariable &&
-               var != Scope::kDummyPreParserLexicalVariable) {
-      if (info != nullptr) {
-        // In this case we need to leave scopes in a way that they can be
-        // allocated. If we resolved variables from lazy parsed scopes, we need
-        // to context allocate the var.
-        ResolveTo(info, proxy, var);
-        if (!var->is_dynamic() && lookup != this) var->ForceContextAllocation();
-      } else {
-        var->set_is_used();
-        if (proxy->is_assigned()) var->set_maybe_assigned();
-      }
-    }
-  }
-
-  // Clear unresolved_list_ as it's in an inconsistent state.
-  unresolved_list_.Clear();
-
-  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    scope->ResolveScopesThenForEachVariable(max_outer_scope,
-                                            variable_proxy_stackvisitor, info);
-  }
-}
-
 }  // namespace internal
 }  // namespace v8
 

src/ast/scopes.cc

@@ -8,7 +8,6 @@
 
 #include "src/accessors.h"
 #include "src/ast/ast.h"
-#include "src/ast/scopes-inl.h"
 #include "src/base/optional.h"
 #include "src/bootstrapper.h"
 #include "src/counters.h"
@@ -1382,14 +1381,82 @@ Scope* Scope::GetOuterScopeWithContext() {
   return scope;
 }
 
+void Scope::CollectNonLocals(DeclarationScope* max_outer_scope,
+                             Isolate* isolate, ParseInfo* info,
+                             Handle<StringSet>* non_locals) {
+  // Module variables must be allocated before variable resolution
+  // to ensure that UpdateNeedsHoleCheck() can detect import variables.
+  if (is_module_scope()) AsModuleScope()->AllocateModuleVariables();
+
+  // Lazy parsed declaration scopes are already partially analyzed. If there are
+  // unresolved references remaining, they just need to be resolved in outer
+  // scopes.
+  Scope* lookup =
+      is_declaration_scope() && AsDeclarationScope()->was_lazily_parsed()
+          ? outer_scope()
+          : this;
+
+  for (VariableProxy* proxy = unresolved_list_.first(); proxy != nullptr;
+       proxy = proxy->next_unresolved()) {
+    DCHECK(!proxy->is_resolved());
+    Variable* var = Lookup(proxy, lookup, max_outer_scope->outer_scope());
+    if (var == nullptr) {
+      *non_locals = StringSet::Add(isolate, *non_locals, proxy->name());
+    } else {
+      // In this case we need to leave scopes in a way that they can be
+      // allocated. If we resolved variables from lazy parsed scopes, we need
+      // to context allocate the var.
+      ResolveTo(info, proxy, var);
+      if (!var->is_dynamic() && lookup != this) var->ForceContextAllocation();
+    }
+  }
+
+  // Clear unresolved_list_ as it's in an inconsistent state.
+  unresolved_list_.Clear();
+
+  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
+    scope->CollectNonLocals(max_outer_scope, isolate, info, non_locals);
+  }
+}
+
+void Scope::AnalyzePartially(
+    DeclarationScope* max_outer_scope, AstNodeFactory* ast_node_factory,
+    base::ThreadedList<VariableProxy>* new_unresolved_list) {
+  DCHECK_IMPLIES(is_declaration_scope(),
+                 !AsDeclarationScope()->was_lazily_parsed());
+
+  for (VariableProxy* proxy = unresolved_list_.first(); proxy != nullptr;
+       proxy = proxy->next_unresolved()) {
+    DCHECK(!proxy->is_resolved());
+    Variable* var = Lookup(proxy, this, max_outer_scope->outer_scope());
+    if (var == nullptr) {
+      // Don't copy unresolved references to the script scope, unless it's a
+      // reference to a private name or method. In that case keep it so we
+      // can fail later.
+      if (!max_outer_scope->outer_scope()->is_script_scope() ||
+          proxy->is_private_name()) {
+        VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
+        new_unresolved_list->AddFront(copy);
+      }
+    } else if (var != Scope::kDummyPreParserVariable &&
+               var != Scope::kDummyPreParserLexicalVariable) {
+      var->set_is_used();
+      if (proxy->is_assigned()) var->set_maybe_assigned();
+    }
+  }
+
+  // Clear unresolved_list_ as it's in an inconsistent state.
+  unresolved_list_.Clear();
+
+  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
+    scope->AnalyzePartially(max_outer_scope, ast_node_factory,
+                            new_unresolved_list);
+  }
+}
+
 Handle<StringSet> DeclarationScope::CollectNonLocals(
     Isolate* isolate, ParseInfo* info, Handle<StringSet> non_locals) {
-  ResolveScopesThenForEachVariable(this,
-                                   [=, &non_locals](VariableProxy* proxy) {
-                                     non_locals = StringSet::Add(
-                                         isolate, non_locals, proxy->name());
-                                   },
-                                   info);
+  Scope::CollectNonLocals(this, isolate, info, &non_locals);
   return non_locals;
 }
 
@@ -1462,16 +1529,7 @@ void DeclarationScope::AnalyzePartially(AstNodeFactory* ast_node_factory) {
     // Try to resolve unresolved variables for this Scope and migrate 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.
-    ResolveScopesThenForEachVariable(
-        this, [=, &new_unresolved_list](VariableProxy* proxy) {
-          // Don't copy unresolved references to the script scope, unless it's a
-          // reference to a private name or method. In that case keep it so we
-          // can fail later.
-          if (!outer_scope_->is_script_scope() || proxy->is_private_name()) {
-            VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
-            new_unresolved_list.AddFront(copy);
-          }
-        });
+    Scope::AnalyzePartially(this, ast_node_factory, &new_unresolved_list);
 
     // Migrate function_ to the right Zone.
     if (function_ != nullptr) {

src/ast/scopes.h

@@ -615,11 +615,11 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
 
   // Finds free variables of this scope. This mutates the unresolved variables
   // list along the way, so full resolution cannot be done afterwards.
-  // If a ParseInfo* is passed, non-free variables will be resolved.
-  template <typename T>
-  void ResolveScopesThenForEachVariable(DeclarationScope* max_outer_scope,
-                                        T variable_proxy_stackvisitor,
-                                        ParseInfo* info = nullptr);
+  void AnalyzePartially(DeclarationScope* max_outer_scope,
+                        AstNodeFactory* ast_node_factory,
+                        base::ThreadedList<VariableProxy>* new_unresolved_list);
+  void CollectNonLocals(DeclarationScope* max_outer_scope, Isolate* isolate,
+                        ParseInfo* info, Handle<StringSet>* non_locals);
 
   // Predicates.
   bool MustAllocate(Variable* var);