PreParser: track variable declarations and parameters

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 <set>
 
 #include "src/accessors.h"
 #include "src/ast/ast.h"
 #include "src/bootstrapper.h"
 #include "src/messages.h"
 #include "src/parsing/parse-info.h"
 
 namespace v8 {
 namespace internal {
 
+namespace {
+void* kDummyPreParserVariable = reinterpret_cast<void*>(0x1);
+}  // namespace
+
 // ----------------------------------------------------------------------------
 // Implementation of LocalsMap
 //
 // Note: We are storing the handle locations as key values in the hash map.
 //       When inserting a new variable via Declare(), we rely on the fact that
 //       the handle location remains alive for the duration of that variable
 //       use. Because a Variable holding a handle with the same location exists
 //       this is ensured.
 
 VariableMap::VariableMap(Zone* zone)
@@ skipping 14 matching lines @@
   if (added) *added = p->value == nullptr;
   if (p->value == nullptr) {
     // The variable has not been declared yet -> insert it.
     DCHECK_EQ(name, p->key);
     p->value = new (zone) Variable(scope, name, mode, kind, initialization_flag,
                                    maybe_assigned_flag);
   }
   return reinterpret_cast<Variable*>(p->value);
 }
 
+void VariableMap::DeclareName(Zone* zone, const AstRawString* name) {

[Toon Verwaest, 2016/12/01 10:50:56]

We should probably have a debug mode flag to check that that we don't call this
on non-preparsed scopes; and don't call the regular DeclareVariable on preparsed
scopes.

[marja, 2016/12/05 16:05:19]

We already have is_lazily_parsed, but that's set too late (and AnalyzePartially
-> FetchFreeVariables assumes it's not set at that point yet). :/

As a brute force solution I added a is_being_lazily_parsed_.

+  Entry* p =
+      ZoneHashMap::LookupOrInsert(const_cast<AstRawString*>(name), name->hash(),
+                                  ZoneAllocationPolicy(zone));
+  if (p->value == nullptr) {
+    // The variable has not been declared yet -> insert it.
+    DCHECK_EQ(name, p->key);
+    p->value = kDummyPreParserVariable;
+  }
+}
+
 void VariableMap::Remove(Variable* var) {
   const AstRawString* name = var->raw_name();
   ZoneHashMap::Remove(const_cast<AstRawString*>(name), name->hash());
 }
 
 void VariableMap::Add(Zone* zone, Variable* var) {
   const AstRawString* name = var->raw_name();
   Entry* p =
       ZoneHashMap::LookupOrInsert(const_cast<AstRawString*>(name), name->hash(),
                                   ZoneAllocationPolicy(zone));
@@ skipping 815 matching lines @@
     *is_duplicate = IsDeclaredParameter(name);
   }
   has_rest_ = is_rest;
   params_.Add(var, zone());
   if (name == ast_value_factory->arguments_string()) {
     has_arguments_parameter_ = true;
   }
   return var;
 }
 
+void DeclarationScope::DeclareParameterName(
+    const AstRawString* name, bool is_optional, bool is_rest,
+    bool* is_duplicate, AstValueFactory* ast_value_factory) {
+  DCHECK(!already_resolved_);
+  DCHECK(is_function_scope() || is_module_scope());
+  DCHECK(!has_rest_);
+  DCHECK(!is_optional || !is_rest);
+  variables_.DeclareName(zone(), name);
+  // TODO(wingo): Avoid O(n^2) check.

[Toon Verwaest, 2016/12/01 10:50:56]

Does it make sense to still add TODO's for wingo?

[marja, 2016/12/05 16:05:19]

Was just copied over; but I deleted this whole function :)

+  *is_duplicate = IsDeclaredParameter(name);
+  has_rest_ = is_rest;

[Toon Verwaest, 2016/12/01 10:50:56]

Do we care about rest? Isn't that just used to deal with it at some point later
rewriting the parameters or so?

[marja, 2016/12/05 16:05:19]

Ditto; the downside is that now preparsed scopes don't have has_rest_ set
consistently which might be surprising if some later feature makes use of it.

+  if (name == ast_value_factory->arguments_string()) {

[Toon Verwaest, 2016/12/01 10:50:56]

Do we care about arguments? Isn't that just used to avoid allocating the
arguments object in the function?

[marja, 2016/12/05 16:05:19]

Ditto

+    has_arguments_parameter_ = true;
+  }
+}
+
 Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode,
                               InitializationFlag init_flag, VariableKind kind,
                               MaybeAssignedFlag maybe_assigned_flag) {
   DCHECK(!already_resolved_);
   // This function handles VAR, LET, and CONST modes.  DYNAMIC variables are
   // introduced during variable allocation, and TEMPORARY variables are
   // allocated via NewTemporary().
   DCHECK(IsDeclaredVariableMode(mode));
   return Declare(zone(), name, mode, kind, init_flag, maybe_assigned_flag);
 }
@@ skipping 95 matching lines @@
   //
   // This will lead to multiple declaration nodes for the
   // same variable if it is declared several times. This is not a
   // semantic issue, but it may be a performance issue since it may
   // lead to repeated DeclareEvalVar or DeclareEvalFunction calls.
   decls_.Add(declaration);
   proxy->BindTo(var);
   return var;
 }
 
+void Scope::DeclareVariableName(const AstRawString* name, VariableMode mode) {
+  DCHECK(IsDeclaredVariableMode(mode));
+  DCHECK(!already_resolved_);
+
+  if (mode == VAR && !is_declaration_scope()) {
+    return GetDeclarationScope()->DeclareVariableName(name, mode);
+  }
+  DCHECK(!is_catch_scope());
+  DCHECK(!is_with_scope());
+  DCHECK(!is_eval_scope());
+  DCHECK(is_declaration_scope() ||
+         (IsLexicalVariableMode(mode) && is_block_scope()));
+
+  // Declare the variable in the declaration scope.
+  if (LookupLocal(name) == nullptr) {
+    variables_.DeclareName(zone(), name);
+  }
+}
+
 VariableProxy* Scope::NewUnresolved(AstNodeFactory* factory,
                                     const AstRawString* name,
                                     int start_position, VariableKind kind) {
   // Note that we must not share the unresolved variables with
   // the same name because they may be removed selectively via
   // RemoveUnresolved().
   DCHECK(!already_resolved_);
   DCHECK_EQ(factory->zone(), zone());
   VariableProxy* proxy = factory->NewVariableProxy(name, kind, start_position);
   proxy->set_next_unresolved(unresolved_);
@@ skipping 254 matching lines @@
 Scope* Scope::GetOuterScopeWithContext() {
   Scope* scope = outer_scope_;
   while (scope && !scope->NeedsContext()) {
     scope = scope->outer_scope();
   }
   return scope;
 }
 
 Handle<StringSet> DeclarationScope::CollectNonLocals(
     ParseInfo* info, Handle<StringSet> non_locals) {
-  VariableProxy* free_variables = FetchFreeVariables(this, true, info);
+  VariableProxy* free_variables = FetchFreeVariables(this, info);
   for (VariableProxy* proxy = free_variables; proxy != nullptr;
        proxy = proxy->next_unresolved()) {
     non_locals = StringSet::Add(non_locals, proxy->name());
   }
   return non_locals;
 }
 
 void DeclarationScope::ResetAfterPreparsing(AstValueFactory* ast_value_factory,
                                             bool aborted) {
   DCHECK(is_function_scope());
@@ skipping 28 matching lines @@
 }
 
 void DeclarationScope::AnalyzePartially(AstNodeFactory* ast_node_factory) {
   DCHECK(!force_eager_compilation_);
   VariableProxy* unresolved = nullptr;
 
   if (!outer_scope_->is_script_scope()) {
     // 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.
-    for (VariableProxy* proxy =
-             FetchFreeVariables(this, !FLAG_lazy_inner_functions);
-         proxy != nullptr; proxy = proxy->next_unresolved()) {
+    for (VariableProxy* proxy = FetchFreeVariables(this); proxy != nullptr;
+         proxy = proxy->next_unresolved()) {
       DCHECK(!proxy->is_resolved());
       VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
       copy->set_next_unresolved(unresolved);
       unresolved = copy;
     }
 
     // Clear arguments_ if unused. This is used as a signal for optimization.
     if (arguments_ != nullptr &&
         !(MustAllocate(arguments_) && !has_arguments_parameter_)) {
       arguments_ = nullptr;
@@ skipping 256 matching lines @@
                                                       NORMAL_VARIABLE);
   }
 
   DCHECK(!is_script_scope());
 
   var = outer_scope_->LookupRecursive(proxy, outer_scope_end);
 
   // The variable could not be resolved statically.
   if (var == nullptr) return var;
 
+  if (var == kDummyPreParserVariable) return var;

[Toon Verwaest, 2016/12/01 10:50:56]

Mmh. We should probably stop sharing LookupRecursive between regular scope
resolution and preparser... Preparser is much simpler, and these additional
checks now make both slower: scope resolution of the preparser and of the
parser.

For the preparser we just need:

bool Scope::LookupRecursive(AstRawString* name, Scope* end) {
  Scope* current = this;
  do {
    if (LookupLocal(name) != nullptr) return true;
    current = current->outer_scope();
  } while (current != end);
  return false;
}

I'm also fine with improving this code in follow-up CLs.

[marja, 2016/12/05 16:05:19]

Ok, added a TODO about that. For that we should somehow track the Scope
"preparsedness" (or life cycle) better:
1. getting preparsed (also for the DCHECKs that you suggested adding;
DeclareVaribleName should only happen at this stage)
2. was preparsed (we have ran analyzePartially)
3. not preparsed

Resoving against a kDummyPreParserVariable can only happen when we are running
AnalyzePartially for a Scope which we just preparsed.

At the moment we don't have a good way tracking that. And the different code
paths merge in FetchFreeVariables which is called both by AnalyzePartially and
the debugger... yeah, I'll need to think about how to make this cleaner.

+
   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->is_this()) return var;
 
   if (is_with_scope()) {
     // The current scope is a with scope, so the variable binding can not be
@@ skipping 146 matching lines @@
 
     // Resolve unresolved variables for inner scopes.
     for (Scope* scope = inner_scope_; scope != nullptr;
          scope = scope->sibling_) {
       scope->ResolveVariablesRecursively(info);
     }
   }
 }
 
 VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
-                                         bool try_to_resolve, ParseInfo* info,
+                                         ParseInfo* info,

[Toon Verwaest, 2016/12/01 10:50:56]

That obviously makes this slightly more complicated again; but we can cleanly
separate the code with your flag from the rest. So it might even make it better
overall.

[marja, 2016/12/05 16:05:19]

I might need to introduce a new parameter here which tells us whether we're
using FetchFreeVariables via AnalyzePartially or via CollectNonLocals.

If we're using it for AnalyzePartially, we can call
LookupRecursiveInPreParsedScope() (or whatever is a descriptive name) instead.

This actually boils down to not having a function which is
JustLookupAndDoNothingElse() which I was complaining about earlier :)

                                          VariableProxy* stack) {
   // 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()->is_lazily_parsed()
           ? outer_scope()
           : this;
   for (VariableProxy *proxy = unresolved_, *next = nullptr; proxy != nullptr;
        proxy = next) {
     next = proxy->next_unresolved();
     DCHECK(!proxy->is_resolved());
-    Variable* var = nullptr;
-    if (try_to_resolve) {
-      var = lookup->LookupRecursive(proxy, max_outer_scope->outer_scope());
-    }
+    Variable* var =
+        lookup->LookupRecursive(proxy, max_outer_scope->outer_scope());
     if (var == nullptr) {
       proxy->set_next_unresolved(stack);
       stack = proxy;
-    } else 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();
+    } else if (var != kDummyPreParserVariable) {
+      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();
+      }
     }
   }
 
   // Clear unresolved_ as it's in an inconsistent state.
   unresolved_ = nullptr;
 
   for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    stack =
-        scope->FetchFreeVariables(max_outer_scope, try_to_resolve, info, stack);
+    stack = scope->FetchFreeVariables(max_outer_scope, info, stack);
   }
 
   return stack;
 }
 
 bool Scope::MustAllocate(Variable* var) {
   DCHECK(var->location() != VariableLocation::MODULE);
   // Give var a read/write use if there is a chance it might be accessed
   // via an eval() call.  This is only possible if the variable has a
   // visible name.
@@ skipping 254 matching lines @@
   Variable* function =
       is_function_scope() ? AsDeclarationScope()->function_var() : nullptr;
   bool is_function_var_in_context =
       function != nullptr && function->IsContextSlot();
   return num_heap_slots() - Context::MIN_CONTEXT_SLOTS -
          (is_function_var_in_context ? 1 : 0);
 }
 
 }  // namespace internal
 }  // namespace v8