Don't use different function scopes when parsing with temp zones

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"
@@ skipping 935 matching lines @@
 }
 
 VariableProxy* Scope::NewUnresolved(AstNodeFactory* factory,
                                     const AstRawString* name,
                                     int start_position, int end_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());
+  // DCHECK_EQ(factory->zone(), zone());

[Toon Verwaest, 2016/09/26 14:24:30]

We should probably just drop this?

[marja, 2016/09/27 07:14:03]

Hmmmm... I think this was a nice DCHECK for guarding that we don't have Zone ->
different Zone pointers in Scope. It sounds risky to remove that invariant.
Would it be possible to do an equivalent check that we're in a consistent state
after AnalyzePartially? Maybe not b/c VariableProxy's don't know their Zone
(yet)?

[marja, 2016/09/27 11:17:34]

Suggestion: flag scopes w/ needs_migration, here we DHCECK(needs_migration_ ||
factory->zone() == zone());, then in AnalyzePartially we set needs_migration_ =
false; and finally as part of scope analysis somewhere we
DCHECK(!needs_migration);

That would solve my worry about having dangling pointers to a different Zone.

   VariableProxy* proxy =
       factory->NewVariableProxy(name, kind, start_position, end_position);
   proxy->set_next_unresolved(unresolved_);
   unresolved_ = proxy;
   return proxy;
 }
 
 void Scope::AddUnresolved(VariableProxy* proxy) {
   DCHECK(!already_resolved_);
   DCHECK(!proxy->is_resolved());
@@ skipping 214 matching lines @@
 Handle<StringSet> DeclarationScope::CollectNonLocals(
     ParseInfo* info, Handle<StringSet> non_locals) {
   VariableProxy* free_variables = FetchFreeVariables(this, true, 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::AnalyzePartially(DeclarationScope* migrate_to,
-                                        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()) {
-    DCHECK(!proxy->is_resolved());
-    VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
-    migrate_to->AddUnresolved(copy);
+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()) {
+      DCHECK(!proxy->is_resolved());
+      VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
+      copy->set_next_unresolved(unresolved);
+      unresolved = copy;
+    }
   }
 
-  // Push scope data up to migrate_to. Note that migrate_to and this Scope
-  // describe the same Scope, just in different Zones.
-  PropagateUsageFlagsToScope(migrate_to);
-  if (scope_uses_super_property_) migrate_to->scope_uses_super_property_ = true;
-  if (inner_scope_calls_eval_) migrate_to->inner_scope_calls_eval_ = true;
-  if (is_lazily_parsed_) migrate_to->is_lazily_parsed_ = true;
-  DCHECK(!force_eager_compilation_);
-  migrate_to->set_start_position(start_position_);
-  migrate_to->set_end_position(end_position_);
-  migrate_to->set_language_mode(language_mode());
-  migrate_to->arity_ = arity_;
-  migrate_to->force_context_allocation_ = force_context_allocation_;
-  outer_scope_->RemoveInnerScope(this);
-  DCHECK_EQ(outer_scope_, migrate_to->outer_scope_);
-  DCHECK_EQ(outer_scope_->zone(), migrate_to->zone());
-  DCHECK_EQ(NeedsHomeObject(), migrate_to->NeedsHomeObject());
-  DCHECK_EQ(asm_function_, migrate_to->asm_function_);
+  ResetAfterPreparsing();

[marja, 2016/09/27 07:14:03]

This is somewhat surprising now since we haven't necessarily preparsed... but as
the "full parse but throw the code away" option is going away, maybe it's fine.

+
+  unresolved_ = unresolved;
 }
 
 #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*";
@@ skipping 627 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