Fully deserialize the scope chain after parsing, not before

src/ast/scopes.cc

Index: src/ast/scopes.cc
diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc
index b74d7ab824f7c2c1e6f83d342fe71beaca43e820..9acd36bc4842dc1947c70ed0e1e2c739255b7cb6 100644
--- a/src/ast/scopes.cc
+++ b/src/ast/scopes.cc
@@ -307,11 +307,25 @@ bool Scope::IsAsmFunction() const {
   return is_function_scope() && AsDeclarationScope()->asm_function();
 }
 
+Scope* Scope::DeserializeScopeChainForTesting(
+    Isolate* isolate, Zone* zone, Context* context,
+    DeclarationScope* script_scope, AstValueFactory* ast_value_factory) {
+  return DeserializeScopeChain(isolate, zone, context, script_scope,
+                               ast_value_factory);
+}
+
+void DeclarationScope::DeserializeScopeChain(ParseInfo* info) {
+  if (!info->context().is_null() && !info->context()->IsNativeContext()) {
+    ReplaceOuterScope(Scope::DeserializeScopeChain(
+        info->isolate(), zone(), *info->context(), info->script_scope(),
+        info->ast_value_factory()));
+  }
+}
+
 Scope* Scope::DeserializeScopeChain(Isolate* isolate, Zone* zone,
                                     Context* context,
                                     DeclarationScope* script_scope,
-                                    AstValueFactory* ast_value_factory,
-                                    DeserializationMode deserialization_mode) {
+                                    AstValueFactory* ast_value_factory) {
   // Reconstruct the outer scope chain from a closure's context chain.
   Scope* current_scope = nullptr;
   Scope* innermost_scope = nullptr;
@@ -372,18 +386,11 @@ Scope* Scope::DeserializeScopeChain(Isolate* isolate, Zone* zone,
     }
     if (current_scope != nullptr) {
       outer_scope->AddInnerScope(current_scope);
-      DCHECK_IMPLIES(
-          deserialization_mode == DeserializationMode::kKeepScopeInfo,
-          current_scope->scope_info_->HasOuterScopeInfo());
-      DCHECK_IMPLIES(
-          deserialization_mode == DeserializationMode::kKeepScopeInfo,
-          outer_scope->scope_info_->Equals(
-              current_scope->scope_info_->OuterScopeInfo()));
+      DCHECK(current_scope->scope_info_->HasOuterScopeInfo());
+      DCHECK(outer_scope->scope_info_->Equals(
+          current_scope->scope_info_->OuterScopeInfo()));
     }
     current_scope = outer_scope;
-    if (deserialization_mode == DeserializationMode::kDeserializeOffHeap) {
-      current_scope->DeserializeScopeInfo(isolate, ast_value_factory);
-    }
     if (innermost_scope == nullptr) innermost_scope = current_scope;
     context = context->previous();
   }
@@ -391,59 +398,17 @@ Scope* Scope::DeserializeScopeChain(Isolate* isolate, Zone* zone,
   if (innermost_scope == nullptr) return script_scope;
   script_scope->AddInnerScope(current_scope);
 #if DEBUG
-  if (deserialization_mode == DeserializationMode::kKeepScopeInfo) {
-    if (script_scope->scope_info_.is_null()) {
-      DCHECK(!current_scope->scope_info_->HasOuterScopeInfo());
-    } else {
-      DCHECK(!script_scope->scope_info_->HasOuterScopeInfo());
-      DCHECK(script_scope->scope_info_->Equals(
-          current_scope->scope_info_->OuterScopeInfo()));
-    }
+  if (script_scope->scope_info_.is_null()) {
+    DCHECK(!current_scope->scope_info_->HasOuterScopeInfo());
+  } else {
+    DCHECK(!script_scope->scope_info_->HasOuterScopeInfo());
+    DCHECK(script_scope->scope_info_->Equals(
+        current_scope->scope_info_->OuterScopeInfo()));
   }
 #endif
   return innermost_scope;
 }
 
-void Scope::DeserializeScopeInfo(Isolate* isolate,
-                                 AstValueFactory* ast_value_factory) {
-  if (scope_info_.is_null()) return;
-
-  DCHECK(ThreadId::Current().Equals(isolate->thread_id()));
-
-  // Internalize context local variables.
-  for (int var = 0; var < scope_info_->ContextLocalCount(); ++var) {
-    Handle<String> name_handle(scope_info_->ContextLocalName(var), isolate);
-    const AstRawString* name = ast_value_factory->GetString(name_handle);
-    int index = Context::MIN_CONTEXT_SLOTS + var;
-    VariableMode mode = scope_info_->ContextLocalMode(var);
-    InitializationFlag init_flag = scope_info_->ContextLocalInitFlag(var);
-    MaybeAssignedFlag maybe_assigned_flag =
-        scope_info_->ContextLocalMaybeAssignedFlag(var);
-    VariableLocation location = VariableLocation::CONTEXT;
-    VariableKind kind = NORMAL_VARIABLE;
-    if (index == scope_info_->ReceiverContextSlotIndex()) {
-      kind = THIS_VARIABLE;
-    }
-
-    Variable* result = variables_.Declare(zone(), this, name, mode, kind,
-                                          init_flag, maybe_assigned_flag);
-    result->AllocateTo(location, index);
-  }
-
-  // Internalize function proxy for this scope.
-  if (scope_info_->HasFunctionName()) {
-    Handle<String> name_handle(scope_info_->FunctionName(), isolate);
-    const AstRawString* name = ast_value_factory->GetString(name_handle);
-    int index = scope_info_->FunctionContextSlotIndex(*name_handle);
-    if (index >= 0) {
-      Variable* result = AsDeclarationScope()->DeclareFunctionVar(name);
-      result->AllocateTo(VariableLocation::CONTEXT, index);
-    }
-  }
-
-  scope_info_ = Handle<ScopeInfo>::null();
-}
-
 DeclarationScope* Scope::AsDeclarationScope() {
   DCHECK(is_declaration_scope());
   return static_cast<DeclarationScope*>(this);
@@ -470,6 +435,12 @@ int Scope::num_parameters() const {
 
 void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory,
                                                  bool* ok) {
+  if (is_eval_scope() && !outer_scope()->outer_scope()) return;

[marja, 2016/09/12 07:50:32]

Add a comment which explains that hoisting for eval scopes is done later + why.

[marja, 2016/09/12 07:50:32]

Style nit: outer_scope()->outer_scope() == nullptr

[jochen (gone - plz use gerrit), 2016/09/12 08:18:18]

done
I think that's unnecessarily verbose (read: neither in the style guide, nor used
all the time)

+  HoistSloppyBlockFunctionsInternal(factory, ok);
+}
+
+void DeclarationScope::HoistSloppyBlockFunctionsInternal(
+    AstNodeFactory* factory, bool* ok) {
   DCHECK(is_sloppy(language_mode()));
   DCHECK(is_function_scope() || is_eval_scope() || is_script_scope() ||
          (is_block_scope() && outer_scope()->is_function_scope()));
@@ -557,6 +528,12 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory,
       // scope.
       delegate->set_to(NewUnresolved(factory, name));
       delegate->set_from(delegate->scope()->NewUnresolved(factory, name));
+
+      Expression* assignment = factory->NewAssignment(

[marja, 2016/09/12 07:50:32]

Hmm, I wrote the exact same code and people had objections, and I don't think
that has changed...

(Not sure what would be an alternative solution...)

[jochen (gone - plz use gerrit), 2016/09/12 08:18:18]

AST rewriting happens around the same time. The main problem is that I first
need to deserialize the scope chain, the hoist the functions, then resolve the
variables. I guess I could introduce an explicit call to deserialize scope chain
that has to happen before ast rewriting

+          Token::ASSIGN, delegate->to(), delegate->from(), kNoSourcePosition);
+      Statement* statement =
+          factory->NewExpressionStatement(assignment, kNoSourcePosition);
+      delegate->set_statement(statement);
     }
   }
 }
@@ -565,6 +542,15 @@ void DeclarationScope::Analyze(ParseInfo* info, AnalyzeMode mode) {
   DCHECK(info->literal() != NULL);
   DeclarationScope* scope = info->literal()->scope();
 
+  scope->DeserializeScopeChain(info);
+
+  if (scope->is_eval_scope() && is_sloppy(scope->language_mode())) {
+    AstNodeFactory factory(info->ast_value_factory());
+    bool ok = true;
+    scope->HoistSloppyBlockFunctionsInternal(&factory, &ok);
+    CHECK(ok);
+  }
+
   // We are compiling one of three cases:
   // 1) top-level code,
   // 2) a function/eval/module on the top-level
@@ -576,6 +562,11 @@ void DeclarationScope::Analyze(ParseInfo* info, AnalyzeMode mode) {
   scope->AllocateVariables(info, mode);
 
 #ifdef DEBUG
+  if (scope->outer_scope()) {
+    DCHECK(is_sloppy(scope->outer_scope()->language_mode()) ||
+           is_strict(info->language_mode()));
+  }
+
   if (info->script_is_native() ? FLAG_print_builtin_scopes
                                : FLAG_print_scopes) {
     scope->Print();
@@ -747,7 +738,6 @@ void Scope::ReplaceOuterScope(Scope* outer) {
   DCHECK_NOT_NULL(outer);
   DCHECK_NOT_NULL(outer_scope_);
   DCHECK(!already_resolved_);
-  DCHECK(!outer->already_resolved_);
   DCHECK(!outer_scope_->already_resolved_);
   outer_scope_->RemoveInnerScope(this);
   outer->AddInnerScope(this);