From 9703c057c504a2dcc202e07d027919a566630ae6 Mon Sep 17 00:00:00 2001
From: adamk <adamk@chromium.org>
Date: Wed, 11 Mar 2015 16:19:44 -0700
Subject: [PATCH] Modules: simplify logic around allocation of module internal
 variables

Since recursive modules are gone, only the top-level scope can have
module inner scopes. Rename Scope::AllocateModulesRecursively to
Scope::AllocateModules, and add test showing the module Variables
are still allocated appropriately in the top level scope.

BUG=v8:1569,v8:3940
LOG=n

Review URL: https://codereview.chromium.org/999893003

Cr-Commit-Position: refs/heads/master@{#27143}
---
 src/scopes.cc               | 27 +++++++++++++--------------
 src/scopes.h                |  2 +-
 test/cctest/test-parsing.cc | 18 +++++++++++++++---
 3 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/scopes.cc b/src/scopes.cc
index b8c357c47e..c744dd418b 100644
--- a/src/scopes.cc
+++ b/src/scopes.cc
@@ -656,9 +656,9 @@ bool Scope::AllocateVariables(ParseInfo* info, AstNodeFactory* factory) {
   PropagateScopeInfo(outer_scope_calls_sloppy_eval);
 
   // 2) Allocate module instances.
-  if (FLAG_harmony_modules && (is_script_scope() || is_module_scope())) {
+  if (FLAG_harmony_modules && is_script_scope()) {
     DCHECK(num_modules_ == 0);
-    AllocateModulesRecursively(this);
+    AllocateModules();
   }
 
   // 3) Resolve variables.
@@ -1437,19 +1437,18 @@ void Scope::AllocateVariablesRecursively(Isolate* isolate) {
 }
 
 
-void Scope::AllocateModulesRecursively(Scope* host_scope) {
-  if (already_resolved()) return;
-  if (is_module_scope()) {
-    DCHECK(module_descriptor_->IsFrozen());
-    DCHECK(module_var_ == NULL);
-    module_var_ =
-        host_scope->NewInternal(ast_value_factory_->dot_module_string());
-    ++host_scope->num_modules_;
-  }
-
+void Scope::AllocateModules() {
+  DCHECK(is_script_scope());
+  DCHECK(!already_resolved());
   for (int i = 0; i < inner_scopes_.length(); i++) {
-    Scope* inner_scope = inner_scopes_.at(i);
-    inner_scope->AllocateModulesRecursively(host_scope);
+    Scope* scope = inner_scopes_.at(i);
+    if (scope->is_module_scope()) {
+      DCHECK(!scope->already_resolved());
+      DCHECK(scope->module_descriptor_->IsFrozen());
+      DCHECK_NULL(scope->module_var_);
+      scope->module_var_ = NewInternal(ast_value_factory_->dot_module_string());
+      ++num_modules_;
+    }
   }
 }
 
diff --git a/src/scopes.h b/src/scopes.h
index 9057c35ff2..44434fa362 100644
--- a/src/scopes.h
+++ b/src/scopes.h
@@ -692,7 +692,7 @@ class Scope: public ZoneObject {
   void AllocateNonParameterLocal(Isolate* isolate, Variable* var);
   void AllocateNonParameterLocals(Isolate* isolate);
   void AllocateVariablesRecursively(Isolate* isolate);
-  void AllocateModulesRecursively(Scope* host_scope);
+  void AllocateModules();
 
   // Resolve and fill in the allocation information for all variables
   // in this scopes. Must be called *after* all scopes have been
diff --git a/test/cctest/test-parsing.cc b/test/cctest/test-parsing.cc
index 12e1fe907e..c91c7b062b 100644
--- a/test/cctest/test-parsing.cc
+++ b/test/cctest/test-parsing.cc
@@ -5220,6 +5220,8 @@ TEST(ImportExportParsingErrors) {
 
 
 TEST(ModuleParsingInternals) {
+  i::FLAG_harmony_modules = true;
+
   i::Isolate* isolate = CcTest::i_isolate();
   i::Factory* factory = isolate->factory();
   v8::HandleScope handles(CcTest::isolate());
@@ -5244,9 +5246,19 @@ TEST(ModuleParsingInternals) {
   parser.set_allow_harmony_scoping(true);
   info.set_module();
   CHECK(parser.Parse(&info));
+  CHECK(i::Compiler::Analyze(&info));
+
   i::FunctionLiteral* func = info.function();
-  CHECK_EQ(i::MODULE_SCOPE, func->scope()->scope_type());
-  i::ModuleDescriptor* descriptor = func->scope()->module();
+  i::Scope* module_scope = func->scope();
+  i::Scope* outer_scope = module_scope->outer_scope();
+  CHECK(outer_scope->is_script_scope());
+  CHECK_NULL(outer_scope->outer_scope());
+  CHECK_EQ(1, outer_scope->num_modules());
+  CHECK(module_scope->is_module_scope());
+  CHECK_NOT_NULL(module_scope->module_var());
+  CHECK_EQ(i::INTERNAL, module_scope->module_var()->mode());
+
+  i::ModuleDescriptor* descriptor = module_scope->module();
   CHECK_NOT_NULL(descriptor);
   CHECK_EQ(1, descriptor->Length());
   const i::AstRawString* export_name = avf.GetOneByteString("y");
@@ -5254,7 +5266,7 @@ TEST(ModuleParsingInternals) {
       descriptor->LookupLocalExport(export_name, &zone);
   CHECK_NOT_NULL(local_name);
   CHECK(local_name->IsOneByteEqualTo("x"));
-  i::ZoneList<i::Declaration*>* declarations = func->scope()->declarations();
+  i::ZoneList<i::Declaration*>* declarations = module_scope->declarations();
   CHECK_EQ(3, declarations->length());
   CHECK(declarations->at(0)->proxy()->raw_name()->IsOneByteEqualTo("x"));
   i::ImportDeclaration* import_decl =