PreParser: track variable declarations and parameters

src/ast/scopes.cc

Index: src/ast/scopes.cc
diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc
index 4ce1c0d85f7970f0b1cd5807d0519aac825d88c3..b5516fdd913d77c6a1f82e6bb0e02d6cc08af681 100644
--- a/src/ast/scopes.cc
+++ b/src/ast/scopes.cc
@@ -15,6 +15,10 @@
 namespace v8 {
 namespace internal {
 
+namespace {
+void* kDummyPreParserVariable = reinterpret_cast<void*>(0x1);
+}  // namespace
+
 // ----------------------------------------------------------------------------
 // Implementation of LocalsMap
 //
@@ -49,6 +53,17 @@ Variable* VariableMap::Declare(Zone* zone, Scope* scope,
   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());
@@ -884,6 +899,22 @@ Variable* DeclarationScope::DeclareParameter(
   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) {
@@ -999,6 +1030,25 @@ Variable* Scope::DeclareVariable(
   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) {
@@ -1273,7 +1323,7 @@ Scope* Scope::GetOuterScopeWithContext() {
 
 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());
@@ -1322,9 +1372,8 @@ 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.
-    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);
@@ -1601,6 +1650,8 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy, Scope* 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();
   }
@@ -1767,7 +1818,7 @@ void Scope::ResolveVariablesRecursively(ParseInfo* 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
@@ -1780,21 +1831,21 @@ VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
        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();
+      }
     }
   }
 
@@ -1802,8 +1853,7 @@ VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
   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;