Fix CollectNonLocals

Now it actually collects free variables of the target function, rather than any reference to a non-stack-allocated variable in any of the inner scopes.

BUG=

Review-Url: https://codereview.chromium.org/2229373002
Cr-Commit-Position: refs/heads/master@{#38534}

src/ast/scopes.cc

@@ -979,18 +979,12 @@ Handle<ScopeInfo> Scope::GetScopeInfo(Isolate* isolate) {
   return scope_info_;
 }
 
-Handle<StringSet> Scope::CollectNonLocals(Handle<StringSet> non_locals) {
-  // Collect non-local variables referenced in the scope.
-  // TODO(yangguo): store non-local variables explicitly if we can no longer
-  //                rely on unresolved_ to find them.
-  for (VariableProxy* proxy = unresolved_; proxy != nullptr;
+Handle<StringSet> DeclarationScope::CollectNonLocals(
+    ParseInfo* info, Handle<StringSet> non_locals) {
+  VariableProxy* free_variables = FetchFreeVariables(this, info);
+  for (VariableProxy* proxy = free_variables; proxy != nullptr;
        proxy = proxy->next_unresolved()) {
-    if (proxy->is_resolved() && proxy->var()->IsStackAllocated()) continue;
-    Handle<String> name = proxy->name();
-    non_locals = StringSet::Add(non_locals, name);
-  }
-  for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    non_locals = scope->CollectNonLocals(non_locals);
+    non_locals = StringSet::Add(non_locals, proxy->name());
   }
   return non_locals;
 }
@@ -1003,7 +997,12 @@ void DeclarationScope::AnalyzePartially(DeclarationScope* migrate_to,
   // 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.
-  MigrateUnresolvableLocals(migrate_to, ast_node_factory, this);
+  for (VariableProxy* proxy = FetchFreeVariables(this); proxy != nullptr;
+       proxy = proxy->next_unresolved()) {
+    DCHECK(!proxy->is_resolved());
+    VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
+    migrate_to->AddUnresolved(copy);
+  }
 
   // Push scope data up to migrate_to. Note that migrate_to and this Scope
   // describe the same Scope, just in different Zones.
@@ -1359,6 +1358,11 @@ void Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy,
   BindingKind binding_kind;
   Variable* var = LookupRecursive(proxy, &binding_kind, factory);
 
+  ResolveTo(info, binding_kind, proxy, var);
+}
+
+void Scope::ResolveTo(ParseInfo* info, BindingKind binding_kind,
+                      VariableProxy* proxy, Variable* var) {
 #ifdef DEBUG
   if (info->script_is_native()) {
     // To avoid polluting the global object in native scripts
@@ -1438,30 +1442,38 @@ void Scope::ResolveVariablesRecursively(ParseInfo* info,
   }
 }
 
-void Scope::MigrateUnresolvableLocals(DeclarationScope* migrate_to,
-                                      AstNodeFactory* ast_node_factory,
-                                      DeclarationScope* max_outer_scope) {
-  BindingKind binding_kind;
+VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
+                                         ParseInfo* info,
+                                         VariableProxy* stack) {
+  BindingKind binding_kind = BOUND;
   for (VariableProxy *proxy = unresolved_, *next = nullptr; proxy != nullptr;
        proxy = next) {
     next = proxy->next_unresolved();
+    if (proxy->is_resolved()) continue;
     // Note that we pass nullptr as AstNodeFactory: this phase should not create
     // any new AstNodes, since none of the Scopes involved are backed up by
     // ScopeInfo.
-    if (LookupRecursive(proxy, &binding_kind, nullptr, max_outer_scope) ==
-        nullptr) {
-      // Re-create the VariableProxies in the right Zone and insert them into
-      // migrate_to.
-      DCHECK(!proxy->is_resolved());
-      VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
-      migrate_to->AddUnresolved(copy);
+    Variable* var =
+        LookupRecursive(proxy, &binding_kind, nullptr, max_outer_scope);
+    // Anything that was bound
+    if (var == nullptr) {
+      proxy->set_next_unresolved(stack);
+      stack = proxy;
+    } else if (info != nullptr) {
+      DCHECK_NE(UNBOUND, binding_kind);
+      DCHECK_NE(UNBOUND_EVAL_SHADOWED, binding_kind);
+      ResolveTo(info, binding_kind, proxy, var);
     }
   }
 
+  // Clear unresolved_ as it's in an inconsistent state.
+  unresolved_ = nullptr;
+
   for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    scope->MigrateUnresolvableLocals(migrate_to, ast_node_factory,
-                                     max_outer_scope);
+    stack = scope->FetchFreeVariables(max_outer_scope, info, stack);
   }
+
+  return stack;
 }
 
 void Scope::PropagateScopeInfo(bool outer_scope_calls_sloppy_eval) {

src/ast/scopes.h

@@ -414,8 +414,6 @@ class Scope: public ZoneObject {
 
   Handle<ScopeInfo> GetScopeInfo(Isolate* isolate);
 
-  Handle<StringSet> CollectNonLocals(Handle<StringSet> non_locals);
-
   // ---------------------------------------------------------------------------
   // Strict mode support.
   bool IsDeclared(const AstRawString* name) {
@@ -579,15 +577,18 @@ class Scope: public ZoneObject {
   Variable* LookupRecursive(VariableProxy* proxy, BindingKind* binding_kind,
                             AstNodeFactory* factory,
                             Scope* max_outer_scope = nullptr);
+  void ResolveTo(ParseInfo* info, BindingKind binding_kind,
+                 VariableProxy* proxy, Variable* var);
   void ResolveVariable(ParseInfo* info, VariableProxy* proxy,
                        AstNodeFactory* factory);
   void ResolveVariablesRecursively(ParseInfo* info, AstNodeFactory* factory);
 
-  // Tries to resolve local variables inside max_outer_scope; migrates those
-  // which cannot be resolved into migrate_to.
-  void MigrateUnresolvableLocals(DeclarationScope* migrate_to,
-                                 AstNodeFactory* ast_node_factory,
-                                 DeclarationScope* max_outer_scope);
+  // 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.
+  VariableProxy* FetchFreeVariables(DeclarationScope* max_outer_scope,
+                                    ParseInfo* info = nullptr,
+                                    VariableProxy* stack = nullptr);
 
   // Scope analysis.
   void PropagateScopeInfo(bool outer_scope_calls_sloppy_eval);
@@ -849,6 +850,9 @@ class DeclarationScope : public Scope {
   void AnalyzePartially(DeclarationScope* migrate_to,
                         AstNodeFactory* ast_node_factory);
 
+  Handle<StringSet> CollectNonLocals(ParseInfo* info,
+                                     Handle<StringSet> non_locals);
+
 #ifdef DEBUG
   void PrintParameters();
 #endif

src/debug/debug-scopes.cc

@@ -13,6 +13,7 @@
 #include "src/globals.h"
 #include "src/isolate-inl.h"
 #include "src/parsing/parser.h"
+#include "src/parsing/rewriter.h"
 
 namespace v8 {
 namespace internal {
@@ -107,10 +108,26 @@ ScopeIterator::ScopeIterator(Isolate* isolate, FrameInspector* frame_inspector,
     // Inner function.
     info.reset(new ParseInfo(&zone, function));
   }
-  Scope* scope = NULL;
-  if (Compiler::ParseAndAnalyze(info.get())) scope = info->literal()->scope();
-  if (!ignore_nested_scopes) RetrieveScopeChain(scope);
-  if (collect_non_locals) CollectNonLocals(scope);
+  if (Parser::ParseStatic(info.get()) && Rewriter::Rewrite(info.get())) {
+    DeclarationScope* scope = info->literal()->scope();
+    if (!ignore_nested_scopes || collect_non_locals) {
+      CollectNonLocals(info.get(), scope);
+    }
+    if (!ignore_nested_scopes) {
+      AstNodeFactory ast_node_factory(info.get()->ast_value_factory());
+      scope->AllocateVariables(info.get(), &ast_node_factory);
+      RetrieveScopeChain(scope);
+    }
+  } else if (!ignore_nested_scopes) {
+    // A failed reparse indicates that the preparser has diverged from the
+    // parser or that the preparse data given to the initial parse has been
+    // faulty. We fail in debug mode but in release mode we only provide the
+    // information we get from the context chain but nothing about
+    // completely stack allocated scopes or stack allocated locals.
+    // Or it could be due to stack overflow.
+    DCHECK(isolate_->has_pending_exception());
+    failed_ = true;
+  }
   UnwrapEvaluationContext();
 }
 
@@ -444,29 +461,16 @@ void ScopeIterator::DebugPrint() {
 }
 #endif
 
-
-void ScopeIterator::RetrieveScopeChain(Scope* scope) {
-  if (scope != NULL) {
-    int source_position = frame_inspector_->GetSourcePosition();
-    GetNestedScopeChain(isolate_, scope, source_position);
-  } else {
-    // A failed reparse indicates that the preparser has diverged from the
-    // parser or that the preparse data given to the initial parse has been
-    // faulty. We fail in debug mode but in release mode we only provide the
-    // information we get from the context chain but nothing about
-    // completely stack allocated scopes or stack allocated locals.
-    // Or it could be due to stack overflow.
-    DCHECK(isolate_->has_pending_exception());
-    failed_ = true;
-  }
+void ScopeIterator::RetrieveScopeChain(DeclarationScope* scope) {
+  DCHECK_NOT_NULL(scope);
+  int source_position = frame_inspector_->GetSourcePosition();
+  GetNestedScopeChain(isolate_, scope, source_position);
 }
 
-
-void ScopeIterator::CollectNonLocals(Scope* scope) {
-  if (scope != NULL) {
-    DCHECK(non_locals_.is_null());
-    non_locals_ = scope->CollectNonLocals(StringSet::New(isolate_));
-  }
+void ScopeIterator::CollectNonLocals(ParseInfo* info, DeclarationScope* scope) {
+  DCHECK_NOT_NULL(scope);
+  DCHECK(non_locals_.is_null());
+  non_locals_ = scope->CollectNonLocals(info, StringSet::New(isolate_));
 }
 
 

src/debug/debug-scopes.h

@@ -111,9 +111,9 @@ class ScopeIterator {
     return frame_inspector_->GetFunction();
   }
 
-  void RetrieveScopeChain(Scope* scope);
+  void RetrieveScopeChain(DeclarationScope* scope);
 
-  void CollectNonLocals(Scope* scope);
+  void CollectNonLocals(ParseInfo* info, DeclarationScope* scope);
 
   void UnwrapEvaluationContext();