[parser] Better separate scope_info-backed lookup from other lookup

Change-Id: Id81b028629d552e2f3ebbab8bc3ab1f0e9cff3fb
Reviewed-on: https://chromium-review.googlesource.com/c/1337572Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57537}

src/ast/scopes.cc

@@ -555,7 +555,9 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
       // example, that does not prevent hoisting of the function in
       // `{ let e; try {} catch (e) { function e(){} } }`
       do {
-        var = query_scope->LookupLocal(name);
+        var = query_scope->scope_info_.is_null()
+                  ? query_scope->LookupLocal(name)
+                  : query_scope->LookupInScopeInfo(name);
         if (var != nullptr && IsLexical(var)) {
           should_hoist = false;
           break;
@@ -910,6 +912,10 @@ void Scope::ReplaceOuterScope(Scope* outer) {
 }
 
 Variable* Scope::LookupInScopeInfo(const AstRawString* name) {
+  DCHECK(!scope_info_.is_null());
+  Variable* cached = variables_.Lookup(name);
+  if (cached) return cached;
+
   Handle<String> name_handle = name->string();
   // The Scope is backed up by ScopeInfo. This means it cannot operate in a
   // heap-independent mode, and all strings must be internalized immediately. So
@@ -959,14 +965,6 @@ Variable* Scope::LookupInScopeInfo(const AstRawString* name) {
   return var;
 }
 
-Variable* Scope::Lookup(const AstRawString* name) {
-  for (Scope* scope = this; scope != nullptr; scope = scope->outer_scope()) {
-    Variable* var = scope->LookupLocal(name);
-    if (var != nullptr) return var;
-  }
-  return nullptr;
-}
-
 Variable* DeclarationScope::DeclareParameter(const AstRawString* name,
                                              VariableMode mode,
                                              bool is_optional, bool is_rest,
@@ -1399,7 +1397,8 @@ void Scope::CollectNonLocals(DeclarationScope* max_outer_scope,
   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());
+    Variable* var =
+        Lookup<kParsedScope>(proxy, lookup, max_outer_scope->outer_scope());
     if (var == nullptr) {
       *non_locals = StringSet::Add(isolate, *non_locals, proxy->name());
     } else {
@@ -1428,7 +1427,8 @@ void Scope::AnalyzePartially(
   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());
+    Variable* var =
+        Lookup<kParsedScope>(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
@@ -1797,10 +1797,11 @@ Variable* Scope::NonLocal(const AstRawString* name, VariableMode mode) {
 }
 
 // static
+template <Scope::ScopeLookupMode mode>
 Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
                         Scope* outer_scope_end, bool force_context_allocation) {
-  while (true) {
-    DCHECK_NE(outer_scope_end, scope);
+  do {
+    DCHECK_IMPLIES(mode == kParsedScope, !scope->is_debug_evaluate_scope_);
     // 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
@@ -1808,18 +1809,22 @@ Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
     // stack-allocated variables.
     // TODO(yangguo): Remove once debug-evaluate creates proper ScopeInfo for
     // the scopes in which it's evaluating.
-    if (V8_UNLIKELY(scope->is_debug_evaluate_scope_)) {
+    if (mode == kDeserializedScope &&
+        V8_UNLIKELY(scope->is_debug_evaluate_scope_)) {
       return scope->NonLocal(proxy->raw_name(), VariableMode::kDynamic);
     }
 
     // Try to find the variable in this scope.
-    Variable* var = scope->LookupLocal(proxy->raw_name());
+    Variable* var = mode == kParsedScope
+                        ? scope->LookupLocal(proxy->raw_name())
+                        : scope->LookupInScopeInfo(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 != nullptr) {
-      if (force_context_allocation && !var->is_dynamic()) {
+      if (mode == kParsedScope && force_context_allocation &&
+          !var->is_dynamic()) {
         var->ForceContextAllocation();
       }
       return var;
@@ -1840,7 +1845,11 @@ Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
 
     force_context_allocation |= scope->is_function_scope();
     scope = scope->outer_scope_;
-  }
+    // TODO(verwaest): Separate through AnalyzePartially.
+    if (mode == kParsedScope && !scope->scope_info_.is_null()) {
+      return Lookup<kDeserializedScope>(proxy, scope, outer_scope_end);
+    }
+  } while (mode == kParsedScope || !scope->is_script_scope());
 
   // We may just be trying to find all free variables. In that case, don't
   // declare them in the outer scope.
@@ -1853,6 +1862,13 @@ Variable* Scope::Lookup(VariableProxy* proxy, Scope* scope,
                                                            NORMAL_VARIABLE);
 }
 
+template Variable* Scope::Lookup<Scope::kParsedScope>(
+    VariableProxy* proxy, Scope* scope, Scope* outer_scope_end,
+    bool force_context_allocation);
+template Variable* Scope::Lookup<Scope::kDeserializedScope>(
+    VariableProxy* proxy, Scope* scope, Scope* outer_scope_end,
+    bool force_context_allocation);
+
 namespace {
 bool CanBeShadowed(Scope* scope, Variable* var) {
   if (var == nullptr) return false;
@@ -1875,8 +1891,13 @@ Variable* Scope::LookupWith(VariableProxy* proxy, Scope* scope,
                             bool force_context_allocation) {
   DCHECK(scope->is_with_scope());
 
-  Variable* var = Lookup(proxy, scope->outer_scope_, outer_scope_end,
-                         force_context_allocation);
+  Variable* var =
+      scope->outer_scope_->scope_info_.is_null()
+          ? Lookup<kParsedScope>(proxy, scope->outer_scope_, outer_scope_end,
+                                 force_context_allocation)
+          : Lookup<kDeserializedScope>(proxy, scope->outer_scope_,
+                                       outer_scope_end);
+
   if (!CanBeShadowed(scope, var)) return var;
 
   // The current scope is a with scope, so the variable binding can not be
@@ -1900,8 +1921,12 @@ Variable* Scope::LookupSloppyEval(VariableProxy* proxy, Scope* scope,
   DCHECK(scope->is_declaration_scope() &&
          scope->AsDeclarationScope()->calls_sloppy_eval());
 
-  Variable* var = Lookup(proxy, scope->outer_scope_, outer_scope_end,
-                         force_context_allocation);
+  Variable* var =
+      scope->outer_scope_->scope_info_.is_null()
+          ? Lookup<kParsedScope>(proxy, scope->outer_scope_, outer_scope_end,
+                                 force_context_allocation)
+          : Lookup<kDeserializedScope>(proxy, scope->outer_scope_,
+                                       outer_scope_end);
   if (!CanBeShadowed(scope, var)) return var;
 
   // A variable binding may have been found in an outer scope, but the current
@@ -1927,7 +1952,7 @@ Variable* Scope::LookupSloppyEval(VariableProxy* proxy, Scope* scope,
 bool Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy) {
   DCHECK(info->script_scope()->is_script_scope());
   DCHECK(!proxy->is_resolved());
-  Variable* var = Lookup(proxy, this, nullptr);
+  Variable* var = Lookup<kParsedScope>(proxy, this, nullptr);
   if (var == nullptr) {
     DCHECK(proxy->is_private_name());
     info->pending_error_handler()->ReportMessageAt(
@@ -2041,7 +2066,7 @@ bool Scope::ResolveVariablesRecursively(ParseInfo* info) {
   if (is_declaration_scope() && AsDeclarationScope()->was_lazily_parsed()) {
     DCHECK_EQ(variables_.occupancy(), 0);
     for (VariableProxy* proxy : unresolved_list_) {
-      Variable* var = Lookup(proxy, outer_scope(), nullptr);
+      Variable* var = Lookup<kParsedScope>(proxy, outer_scope(), nullptr);
       if (var == nullptr) {
         info->pending_error_handler()->ReportMessageAt(
             proxy->position(), proxy->position() + 1,

src/ast/scopes.h

@@ -184,17 +184,12 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // Lookup a variable in this scope. Returns the variable or nullptr if not
   // found.
   Variable* LookupLocal(const AstRawString* name) {
-    Variable* result = variables_.Lookup(name);
-    if (result != nullptr || scope_info_.is_null()) return result;
-    return LookupInScopeInfo(name);
+    DCHECK(scope_info_.is_null());
+    return variables_.Lookup(name);
   }
 
   Variable* LookupInScopeInfo(const AstRawString* name);
 
-  // Lookup a variable in this scope or outer scopes.
-  // Returns the variable or nullptr if not found.
-  Variable* Lookup(const AstRawString* name);
-
   // Declare a local variable in this scope. If the variable has been
   // declared before, the previously declared variable is returned.
   Variable* DeclareLocal(const AstRawString* name, VariableMode mode,
@@ -486,6 +481,16 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
     return false;
   }
 
+  Variable* LookupForTesting(const AstRawString* name) {
+    for (Scope* scope = this; scope != nullptr; scope = scope->outer_scope()) {
+      Variable* var = scope->scope_info_.is_null()
+                          ? scope->LookupLocal(name)
+                          : scope->LookupInScopeInfo(name);
+      if (var != nullptr) return var;
+    }
+    return nullptr;
+  }
+
   static void* const kDummyPreParserVariable;
   static void* const kDummyPreParserLexicalVariable;
 
@@ -594,11 +599,17 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // These variables are looked up dynamically at runtime.
   Variable* NonLocal(const AstRawString* name, VariableMode mode);
 
+  enum ScopeLookupMode {
+    kParsedScope,
+    kDeserializedScope,
+  };
+
   // Variable resolution.
   // Lookup a variable reference given by name starting with this scope, and
   // stopping when reaching the outer_scope_end scope. If the code is executed
   // because of a call to 'eval', the context parameter should be set to the
   // calling context of 'eval'.
+  template <ScopeLookupMode mode>
   static Variable* Lookup(VariableProxy* proxy, Scope* scope,
                           Scope* outer_scope_end,
                           bool force_context_allocation = false);
@@ -696,6 +707,13 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
 
   bool was_lazily_parsed() const { return was_lazily_parsed_; }
 
+  Variable* LookupInModule(const AstRawString* name) {
+    DCHECK(is_module_scope());
+    Variable* var = LookupInScopeInfo(name);
+    DCHECK_NOT_NULL(var);
+    return var;
+  }
+
 #ifdef DEBUG
   void set_is_being_lazily_parsed(bool is_being_lazily_parsed) {
     is_being_lazily_parsed_ = is_being_lazily_parsed;

src/interpreter/bytecode-generator.cc

@@ -1299,8 +1299,7 @@ void BytecodeGenerator::VisitModuleNamespaceImports() {
         ->LoadLiteral(Smi::FromInt(entry->module_request))
         .StoreAccumulatorInRegister(module_request)
         .CallRuntime(Runtime::kGetModuleNamespace, module_request);
-    Variable* var = closure_scope()->LookupLocal(entry->local_name);
-    DCHECK_NOT_NULL(var);
+    Variable* var = closure_scope()->LookupInModule(entry->local_name);
     BuildVariableAssignment(var, Token::INIT, HoleCheckMode::kElided);
   }
 }

test/cctest/test-parsing.cc

@@ -1059,8 +1059,8 @@ TEST(ScopeUsesArgumentsSuperThis) {
       if ((source_data[i].expected & THIS) != 0) {
         // Currently the is_used() flag is conservative; all variables in a
         // script scope are marked as used.
-        CHECK(
-            scope->Lookup(info.ast_value_factory()->this_string())->is_used());
+        CHECK(scope->LookupForTesting(info.ast_value_factory()->this_string())
+                  ->is_used());
       }
       if (is_sloppy(scope->language_mode())) {
         CHECK_EQ((source_data[i].expected & EVAL) != 0,
@@ -3204,7 +3204,7 @@ TEST(SerializationOfMaybeAssignmentFlag) {
   CHECK_NOT_NULL(name);
 
   // Get result from h's function context (that is f's context)
-  i::Variable* var = s->Lookup(name);
+  i::Variable* var = s->LookupForTesting(name);
 
   CHECK_NOT_NULL(var);
   // Maybe assigned should survive deserialization
@@ -3252,7 +3252,7 @@ TEST(IfArgumentsArrayAccessedThenParametersMaybeAssigned) {
   CHECK(s != script_scope);
 
   // Get result from f's function context (that is g's outer context)
-  i::Variable* var_x = s->Lookup(name_x);
+  i::Variable* var_x = s->LookupForTesting(name_x);
   CHECK_NOT_NULL(var_x);
   CHECK_EQ(var_x->maybe_assigned(), i::kMaybeAssigned);
 }
@@ -3424,7 +3424,7 @@ TEST(InnerAssignment) {
         DCHECK(scope->is_function_scope());
         const i::AstRawString* var_name =
             info->ast_value_factory()->GetOneByteString("x");
-        i::Variable* var = scope->Lookup(var_name);
+        i::Variable* var = scope->LookupForTesting(var_name);
         bool expected = outers[i].assigned || inners[j].assigned;
         CHECK_NOT_NULL(var);
         CHECK(var->is_used() || !expected);
@@ -3521,7 +3521,7 @@ TEST(MaybeAssignedParameters) {
       CHECK(scope->is_function_scope());
       const i::AstRawString* var_name =
           info->ast_value_factory()->GetOneByteString("arg");
-      i::Variable* var = scope->Lookup(var_name);
+      i::Variable* var = scope->LookupForTesting(var_name);
       CHECK(var->is_used() || !assigned);
       bool is_maybe_assigned = var->maybe_assigned() == i::kMaybeAssigned;
       CHECK_EQ(is_maybe_assigned, assigned);
@@ -3565,7 +3565,7 @@ static void TestMaybeAssigned(Input input, const char* variable, bool module,
     scope = i::ScopeTestHelper::FindScope(scope, input.location);
     const i::AstRawString* var_name =
         info->ast_value_factory()->GetOneByteString(variable);
-    var = scope->Lookup(var_name);
+    var = scope->LookupForTesting(var_name);
   }
 
   CHECK_NOT_NULL(var);
@@ -10025,7 +10025,7 @@ TEST(NoPessimisticContextAllocation) {
       DCHECK(scope->is_function_scope());
       const i::AstRawString* var_name =
           info.ast_value_factory()->GetOneByteString("my_var");
-      i::Variable* var = scope->Lookup(var_name);
+      i::Variable* var = scope->LookupForTesting(var_name);
       CHECK_EQ(inners[i].ctxt_allocate,
                i::ScopeTestHelper::MustAllocateInContext(var));
     }