Fully deserialize the scope chain after parsing, not before

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 289 matching lines @@
 }
 
 bool Scope::IsAsmModule() const {
   return is_function_scope() && AsDeclarationScope()->asm_module();
 }
 
 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()) {

[adamk, 2016/09/12 16:57:47]

Can you make this an early return instead? I'd find that easier to parse.

[jochen (gone - plz use gerrit), 2016/09/12 19:00:43]

done

+    ReplaceOuterScope(Scope::DeserializeScopeChain(

[adamk, 2016/09/12 16:57:47]

Nit: You shouldn't need to qualify DeserializeScopeChain here.

[jochen (gone - plz use gerrit), 2016/09/12 19:00:43]

done

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

Actually, the compiler disagrees so I reverted this back

+        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;
   Scope* outer_scope = nullptr;
   while (!context->IsNativeContext()) {
     if (context->IsWithContext() || context->IsDebugEvaluateContext()) {
       // For scope analysis, debug-evaluate is equivalent to a with scope.
       outer_scope = new (zone)
           Scope(zone, WITH_SCOPE, Handle<ScopeInfo>(context->scope_info()));
 
@@ skipping 40 matching lines @@
           isolate, Handle<ScopeInfo>(scope_info), ast_value_factory);
     } else {
       DCHECK(context->IsCatchContext());
       String* name = context->catch_name();
       outer_scope = new (zone)
           Scope(zone, ast_value_factory->GetString(handle(name, isolate)),
                 Handle<ScopeInfo>(context->scope_info()));
     }
     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();
   }
 
   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);
 }
 
 const DeclarationScope* Scope::AsDeclarationScope() const {
   DCHECK(is_declaration_scope());
   return static_cast<const DeclarationScope*>(this);
 }
 
 ModuleScope* Scope::AsModuleScope() {
   DCHECK(is_module_scope());
   return static_cast<ModuleScope*>(this);
 }
 
 const ModuleScope* Scope::AsModuleScope() const {
   DCHECK(is_module_scope());
   return static_cast<const ModuleScope*>(this);
 }
 
 int Scope::num_parameters() const {
   return is_declaration_scope() ? AsDeclarationScope()->num_parameters() : 0;
 }
 
 void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory,
                                                  bool* ok) {

[adamk, 2016/09/12 16:57:47]

It doesn't look like this out parameter should ever get set to false, so I think
it should go away.

[jochen (gone - plz use gerrit), 2016/09/12 19:00:43]

done

+  // For the outer most eval scope (which is inside a script scope), don't
+  // hoist during parsing, but later during DeclarationScope::Analyze when the
+  // actual scope chain is available.
+  if (is_eval_scope() && !outer_scope()->outer_scope()) return;
+  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()));
   DCHECK(HasSimpleParameters() || is_block_scope());
   bool has_simple_parameters = HasSimpleParameters();
   // For each variable which is used as a function declaration in a sloppy
   // block,
   SloppyBlockFunctionMap* map = sloppy_block_function_map();
   for (ZoneHashMap::Entry* p = map->Start(); p != nullptr; p = map->Next(p)) {
     AstRawString* name = static_cast<AstRawString*>(p->key);
@@ skipping 58 matching lines @@
         var_created = true;
         VariableProxy* proxy = NewUnresolved(factory, name);
         Declaration* declaration =
             factory->NewVariableDeclaration(proxy, this, kNoSourcePosition);
         // Based on the preceding check, it doesn't matter what we pass as
         // allow_harmony_restrictive_generators and
         // sloppy_mode_block_scope_function_redefinition.
         DeclareVariable(declaration, VAR,
                         Variable::DefaultInitializationFlag(VAR), false,
                         nullptr, ok);
         DCHECK(*ok);  // Based on the preceding check, this should not fail

[adamk, 2016/09/12 16:57:47]

Let's make this a CHECK instead and remove the out parameter.

[jochen (gone - plz use gerrit), 2016/09/12 19:00:43]

done

         if (!*ok) return;
       }
 
-      // Create VariableProxies for creating an assignment statement
-      // (later). Read from the local lexical scope and write to the function
-      // scope.
-      delegate->set_to(NewUnresolved(factory, name));
-      delegate->set_from(delegate->scope()->NewUnresolved(factory, name));
+      Expression* assignment = factory->NewAssignment(
+          Token::ASSIGN, NewUnresolved(factory, name),
+          delegate->scope()->NewUnresolved(factory, name), kNoSourcePosition);
+      Statement* statement =
+          factory->NewExpressionStatement(assignment, kNoSourcePosition);
+      delegate->set_statement(statement);
     }
   }
 }
 
 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
   // 3) a function/eval in a scope that was already resolved.
   DCHECK(scope->scope_type() == SCRIPT_SCOPE ||
          scope->outer_scope()->scope_type() == SCRIPT_SCOPE ||
          scope->outer_scope()->already_resolved_);
 
   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();
   }
   scope->CheckScopePositions();
   scope->CheckZones();
 #endif
 }
 
 void DeclarationScope::DeclareThis(AstValueFactory* ast_value_factory) {
@@ skipping 151 matching lines @@
     }
   }
   outer_closure->locals_.Rewind(top_local_);
   outer_closure->decls_.Rewind(top_decl_);
 }
 
 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);
   outer_scope_ = outer;
 }
 
 
 void Scope::PropagateUsageFlagsToScope(Scope* other) {
   DCHECK_NOT_NULL(other);
   DCHECK(!already_resolved_);
@@ skipping 1084 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