[modules] Explicitly keep track of module requests.

We must keep track of the exact order in which modules are requested.
To do so, maintain a map from module specifiers to position while
parsing (in ModuleDescriptor). Descriptor entries now refer to that
position rather than the string.  When generating the ModuleInfo, turn
this map into an array of specifiers. We don't need the map anymore
later on, so we do not reconstruct it when deserializing again.

BUG=v8:1569

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

src/ast/modules.cc

@@ -15,7 +15,7 @@ void ModuleDescriptor::AddImport(
   Entry* entry = new (zone) Entry(loc);
   entry->local_name = local_name;
   entry->import_name = import_name;
-  entry->module_request = module_request;
+  entry->module_request = AddModuleRequest(module_request);
   AddRegularImport(entry);
 }
 
@@ -24,10 +24,9 @@ void ModuleDescriptor::AddStarImport(
     const AstRawString* local_name, const AstRawString* module_request,
     Scanner::Location loc, Zone* zone) {
   DCHECK_NOT_NULL(local_name);
-  DCHECK_NOT_NULL(module_request);
   Entry* entry = new (zone) Entry(loc);
   entry->local_name = local_name;
-  entry->module_request = module_request;
+  entry->module_request = AddModuleRequest(module_request);
   AddSpecialImport(entry, zone);
 }
 
@@ -35,7 +34,7 @@ void ModuleDescriptor::AddStarImport(
 void ModuleDescriptor::AddEmptyImport(
     const AstRawString* module_request, Scanner::Location loc, Zone* zone) {
   Entry* entry = new (zone) Entry(loc);
-  entry->module_request = module_request;
+  entry->module_request = AddModuleRequest(module_request);
   AddSpecialImport(entry, zone);
 }
 
@@ -58,7 +57,7 @@ void ModuleDescriptor::AddExport(
   Entry* entry = new (zone) Entry(loc);
   entry->export_name = export_name;
   entry->import_name = import_name;
-  entry->module_request = module_request;
+  entry->module_request = AddModuleRequest(module_request);
   AddSpecialExport(entry, zone);
 }
 
@@ -66,7 +65,7 @@ void ModuleDescriptor::AddExport(
 void ModuleDescriptor::AddStarExport(
     const AstRawString* module_request, Scanner::Location loc, Zone* zone) {
   Entry* entry = new (zone) Entry(loc);
-  entry->module_request = module_request;
+  entry->module_request = AddModuleRequest(module_request);
   AddSpecialExport(entry, zone);
 }
 
@@ -89,11 +88,12 @@ const AstRawString* FromStringOrUndefined(Isolate* isolate,
 
 Handle<ModuleInfoEntry> ModuleDescriptor::Entry::Serialize(
     Isolate* isolate) const {
-  return ModuleInfoEntry::New(isolate,
-                              ToStringOrUndefined(isolate, export_name),
+  CHECK(Smi::IsValid(module_request));  // TODO(neis): Check earlier?
+  return ModuleInfoEntry::New(
+      isolate, ToStringOrUndefined(isolate, export_name),
       ToStringOrUndefined(isolate, local_name),
       ToStringOrUndefined(isolate, import_name),
-                              ToStringOrUndefined(isolate, module_request));
+      Handle<Object>(Smi::FromInt(module_request), isolate));
 }
 
 ModuleDescriptor::Entry* ModuleDescriptor::Entry::Deserialize(
@@ -106,8 +106,7 @@ ModuleDescriptor::Entry* ModuleDescriptor::Entry::Deserialize(
       isolate, avfactory, handle(entry->local_name(), isolate));
   result->import_name = FromStringOrUndefined(
       isolate, avfactory, handle(entry->import_name(), isolate));
-  result->module_request = FromStringOrUndefined(
-      isolate, avfactory, handle(entry->module_request(), isolate));
+  result->module_request = Smi::cast(entry->module_request())->value();
   return result;
 }
 
@@ -120,9 +119,11 @@ void ModuleDescriptor::MakeIndirectExportsExplicit(Zone* zone) {
       // Found an indirect export.  Patch export entry and move it from regular
       // to special.
       DCHECK_NULL(entry->import_name);
-      DCHECK_NULL(entry->module_request);
+      DCHECK_LT(entry->module_request, 0);
       DCHECK_NOT_NULL(import->second->import_name);
-      DCHECK_NOT_NULL(import->second->module_request);
+      DCHECK_LE(0, import->second->module_request);
+      DCHECK_LT(import->second->module_request,
+                static_cast<int>(module_requests_.size()));
       entry->import_name = import->second->import_name;
       entry->module_request = import->second->module_request;
       entry->local_name = nullptr;
@@ -160,8 +161,8 @@ const ModuleDescriptor::Entry* ModuleDescriptor::FindDuplicateExport(
   const ModuleDescriptor::Entry* duplicate = nullptr;
   ZoneMap<const AstRawString*, const ModuleDescriptor::Entry*> export_names(
       zone);
-  for (const auto& it : regular_exports_) {
-    duplicate = BetterDuplicate(it.second, export_names, duplicate);
+  for (const auto& elem : regular_exports_) {
+    duplicate = BetterDuplicate(elem.second, export_names, duplicate);
   }
   for (auto entry : special_exports_) {
     if (entry->export_name == nullptr) continue;  // Star export.
@@ -188,8 +189,8 @@ bool ModuleDescriptor::Validate(ModuleScope* module_scope,
   }
 
   // Report error iff there are exports of non-existent local names.
-  for (const auto& it : regular_exports_) {
-    const Entry* entry = it.second;
+  for (const auto& elem : regular_exports_) {
+    const Entry* entry = elem.second;
     DCHECK_NOT_NULL(entry->local_name);
     if (module_scope->LookupLocal(entry->local_name) == nullptr) {
       error_handler->ReportMessageAt(

src/ast/modules.h

@@ -19,7 +19,8 @@ class ModuleInfoEntry;
 class ModuleDescriptor : public ZoneObject {
  public:
   explicit ModuleDescriptor(Zone* zone)
-      : special_exports_(1, zone),
+      : module_requests_(zone),
+        special_exports_(1, zone),
         special_imports_(1, zone),
         regular_exports_(zone),
         regular_imports_(zone) {}
@@ -78,7 +79,11 @@ class ModuleDescriptor : public ZoneObject {
     const AstRawString* export_name;
     const AstRawString* local_name;
     const AstRawString* import_name;
-    const AstRawString* module_request;
+    // The module_request value records the order in which modules are
+    // requested. It also functions as an index into the ModuleInfo's array of
+    // module specifiers and into the Module's array of requested modules.  A
+    // negative value means no module request.
+    int module_request;
 
     // TODO(neis): Remove local_name component?
     explicit Entry(Scanner::Location loc)
@@ -86,7 +91,7 @@ class ModuleDescriptor : public ZoneObject {
           export_name(nullptr),
           local_name(nullptr),
           import_name(nullptr),
-          module_request(nullptr) {}
+          module_request(-1) {}
 
     // (De-)serialization support.
     // Note that the location value is not preserved as it's only needed by the
@@ -96,6 +101,11 @@ class ModuleDescriptor : public ZoneObject {
                               Handle<ModuleInfoEntry> entry);
   };
 
+  // Module requests.
+  const ZoneMap<const AstRawString*, int>& module_requests() const {
+    return module_requests_;
+  }
+
   // Empty imports and namespace imports.
   const ZoneList<const Entry*>& special_imports() const {
     return special_imports_;
@@ -120,32 +130,34 @@ class ModuleDescriptor : public ZoneObject {
     DCHECK_NOT_NULL(entry->export_name);
     DCHECK_NOT_NULL(entry->local_name);
     DCHECK_NULL(entry->import_name);
+    DCHECK_LT(entry->module_request, 0);
     regular_exports_.insert(std::make_pair(entry->local_name, entry));
   }
 
   void AddSpecialExport(const Entry* entry, Zone* zone) {
-    DCHECK_NOT_NULL(entry->module_request);
+    DCHECK_LE(0, entry->module_request);
     special_exports_.Add(entry, zone);
   }
 
   void AddRegularImport(const Entry* entry) {
     DCHECK_NOT_NULL(entry->import_name);
     DCHECK_NOT_NULL(entry->local_name);
-    DCHECK_NOT_NULL(entry->module_request);
     DCHECK_NULL(entry->export_name);
+    DCHECK_LE(0, entry->module_request);
     regular_imports_.insert(std::make_pair(entry->local_name, entry));
     // We don't care if there's already an entry for this local name, as in that
     // case we will report an error when declaring the variable.
   }
 
   void AddSpecialImport(const Entry* entry, Zone* zone) {
-    DCHECK_NOT_NULL(entry->module_request);
     DCHECK_NULL(entry->export_name);
+    DCHECK_LE(0, entry->module_request);
     special_imports_.Add(entry, zone);
   }
 
  private:
   // TODO(neis): Use STL datastructure instead of ZoneList?
+  ZoneMap<const AstRawString*, int> module_requests_;
   ZoneList<const Entry*> special_exports_;
   ZoneList<const Entry*> special_imports_;
   ZoneMultimap<const AstRawString*, Entry*> regular_exports_;
@@ -172,6 +184,14 @@ class ModuleDescriptor : public ZoneObject {
   //   import {a as b} from "X"; export {a as c} from "X";
   // (The import entry is never deleted.)
   void MakeIndirectExportsExplicit(Zone* zone);
+
+  int AddModuleRequest(const AstRawString* specifier) {
+    DCHECK_NOT_NULL(specifier);
+    auto it = module_requests_
+                  .insert(std::make_pair(specifier, module_requests_.size()))
+                  .first;
+    return it->second;
+  }
 };
 
 }  // namespace internal

src/ast/scopeinfo.cc

@@ -853,6 +853,13 @@ Handle<ModuleInfoEntry> ModuleInfoEntry::New(Isolate* isolate,
 }
 
 Handle<ModuleInfo> ModuleInfo::New(Isolate* isolate, ModuleDescriptor* descr) {
+  // Serialize module requests.
+  Handle<FixedArray> module_requests = isolate->factory()->NewFixedArray(
+      static_cast<int>(descr->module_requests().size()));
+  for (const auto& elem : descr->module_requests()) {
+    module_requests->set(elem.second, *elem.first->string());
+  }
+
   // Serialize special exports.
   Handle<FixedArray> special_exports =
       isolate->factory()->NewFixedArray(descr->special_exports().length());
@@ -868,12 +875,13 @@ Handle<ModuleInfo> ModuleInfo::New(Isolate* isolate, ModuleDescriptor* descr) {
       static_cast<int>(descr->regular_exports().size()));
   {
     int i = 0;
-    for (const auto& it : descr->regular_exports()) {
-      regular_exports->set(i++, *it.second->Serialize(isolate));
+    for (const auto& elem : descr->regular_exports()) {
+      regular_exports->set(i++, *elem.second->Serialize(isolate));
     }
   }
 
   Handle<ModuleInfo> result = isolate->factory()->NewModuleInfo();
+  result->set(kModuleRequestsIndex, *module_requests);
   result->set(kSpecialExportsIndex, *special_exports);
   result->set(kRegularExportsIndex, *regular_exports);
   return result;

src/ast/scopes.h

@@ -855,6 +855,11 @@ class ModuleScope final : public DeclarationScope {
  public:
   ModuleScope(DeclarationScope* script_scope,
               AstValueFactory* ast_value_factory);
+
+  // Deserialization.
+  // The generated ModuleDescriptor does not preserve all information.  In
+  // particular, its module_requests map will be empty because we no longer need
+  // the map after parsing.
   ModuleScope(Isolate* isolate, Handle<ScopeInfo> scope_info,
               AstValueFactory* ast_value_factory);
 

src/objects-inl.h

@@ -7943,6 +7943,10 @@ Object* ModuleInfoEntry::module_request() const {
   return get(kModuleRequestIndex);
 }
 
+FixedArray* ModuleInfo::module_requests() const {
+  return FixedArray::cast(get(kModuleRequestsIndex));
+}
+
 FixedArray* ModuleInfo::special_exports() const {
   return FixedArray::cast(get(kSpecialExportsIndex));
 }

src/objects.h

@@ -4572,6 +4572,7 @@ class ModuleInfo : public FixedArray {
  public:
   DECLARE_CAST(ModuleInfo)
   static Handle<ModuleInfo> New(Isolate* isolate, ModuleDescriptor* descr);
+  inline FixedArray* module_requests() const;
   inline FixedArray* special_exports() const;
   inline FixedArray* regular_exports() const;
 
@@ -4581,7 +4582,12 @@ class ModuleInfo : public FixedArray {
 
  private:
   friend class Factory;
-  enum { kSpecialExportsIndex, kRegularExportsIndex, kLength };
+  enum {
+    kModuleRequestsIndex,
+    kSpecialExportsIndex,
+    kRegularExportsIndex,
+    kLength
+  };
 };
 
 // The cache for maps used by normalized (dictionary mode) objects.

test/cctest/test-parsing.cc

@@ -5962,7 +5962,7 @@ TEST(EnumReserved) {
 
 static void CheckEntry(const i::ModuleDescriptor::Entry* entry,
                        const char* export_name, const char* local_name,
-                       const char* import_name, const char* module_request) {
+                       const char* import_name, int module_request) {
   CHECK_NOT_NULL(entry);
   if (export_name == nullptr) {
     CHECK_NULL(entry->export_name);
@@ -5979,11 +5979,7 @@ static void CheckEntry(const i::ModuleDescriptor::Entry* entry,
   } else {
     CHECK(entry->import_name->IsOneByteEqualTo(import_name));
   }
-  if (module_request == nullptr) {
-    CHECK_NULL(entry->module_request);
-  } else {
-    CHECK(entry->module_request->IsOneByteEqualTo(module_request));
-  }
+  CHECK_EQ(entry->module_request, module_request);
 }
 
 TEST(ModuleParsingInternals) {
@@ -6115,74 +6111,86 @@ TEST(ModuleParsingInternals) {
   i::ModuleDescriptor* descriptor = module_scope->module();
   CHECK_NOT_NULL(descriptor);
 
+  CHECK_EQ(5, descriptor->module_requests().size());
+  for (const auto& elem : descriptor->module_requests()) {
+    if (elem.first->IsOneByteEqualTo("m.js"))
+      CHECK_EQ(elem.second, 0);
+    else if (elem.first->IsOneByteEqualTo("n.js"))
+      CHECK_EQ(elem.second, 1);
+    else if (elem.first->IsOneByteEqualTo("p.js"))
+      CHECK_EQ(elem.second, 2);
+    else if (elem.first->IsOneByteEqualTo("q.js"))
+      CHECK_EQ(elem.second, 3);
+    else if (elem.first->IsOneByteEqualTo("bar.js"))
+      CHECK_EQ(elem.second, 4);
+    else
+      CHECK(false);
+  }
+
   CHECK_EQ(3, descriptor->special_exports().length());
-  CheckEntry(descriptor->special_exports().at(0), "b", nullptr, "a", "m.js");
-  CheckEntry(descriptor->special_exports().at(1), nullptr, nullptr, nullptr,
-             "p.js");
+  CheckEntry(descriptor->special_exports().at(0), "b", nullptr, "a", 0);
+  CheckEntry(descriptor->special_exports().at(1), nullptr, nullptr, nullptr, 2);
   CheckEntry(descriptor->special_exports().at(2), "bb", nullptr, "aa",
-             "m.js");  // !!!
+             0);  // !!!
 
   CHECK_EQ(8, descriptor->regular_exports().size());
   entry = descriptor->regular_exports()
               .find(declarations->at(3)->proxy()->raw_name())
               ->second;
-  CheckEntry(entry, "foo", "foo", nullptr, nullptr);
+  CheckEntry(entry, "foo", "foo", nullptr, -1);
   entry = descriptor->regular_exports()
               .find(declarations->at(4)->proxy()->raw_name())
               ->second;
-  CheckEntry(entry, "goo", "goo", nullptr, nullptr);
+  CheckEntry(entry, "goo", "goo", nullptr, -1);
   entry = descriptor->regular_exports()
               .find(declarations->at(5)->proxy()->raw_name())
               ->second;
-  CheckEntry(entry, "hoo", "hoo", nullptr, nullptr);
+  CheckEntry(entry, "hoo", "hoo", nullptr, -1);
   entry = descriptor->regular_exports()
               .find(declarations->at(6)->proxy()->raw_name())
               ->second;
-  CheckEntry(entry, "joo", "joo", nullptr, nullptr);
+  CheckEntry(entry, "joo", "joo", nullptr, -1);
   entry = descriptor->regular_exports()
               .find(declarations->at(7)->proxy()->raw_name())
               ->second;
-  CheckEntry(entry, "default", "*default*", nullptr, nullptr);
+  CheckEntry(entry, "default", "*default*", nullptr, -1);
   entry = descriptor->regular_exports()
               .find(declarations->at(12)->proxy()->raw_name())
               ->second;
-  CheckEntry(entry, "foob", "foob", nullptr, nullptr);
+  CheckEntry(entry, "foob", "foob", nullptr, -1);
   // TODO(neis): The next lines are terrible. Find a better way.
   auto name_x = declarations->at(0)->proxy()->raw_name();
   CHECK_EQ(2, descriptor->regular_exports().count(name_x));
   auto it = descriptor->regular_exports().equal_range(name_x).first;
   entry = it->second;
   if (entry->export_name->IsOneByteEqualTo("y")) {
-    CheckEntry(entry, "y", "x", nullptr, nullptr);
+    CheckEntry(entry, "y", "x", nullptr, -1);
     entry = (++it)->second;
-    CheckEntry(entry, "x", "x", nullptr, nullptr);
+    CheckEntry(entry, "x", "x", nullptr, -1);
   } else {
-    CheckEntry(entry, "x", "x", nullptr, nullptr);
+    CheckEntry(entry, "x", "x", nullptr, -1);
     entry = (++it)->second;
-    CheckEntry(entry, "y", "x", nullptr, nullptr);
+    CheckEntry(entry, "y", "x", nullptr, -1);
   }
 
   CHECK_EQ(3, descriptor->special_imports().length());
-  CheckEntry(descriptor->special_imports().at(0), nullptr, nullptr, nullptr,
-             "q.js");
-  CheckEntry(descriptor->special_imports().at(1), nullptr, "loo", nullptr,
-             "bar.js");
-  CheckEntry(descriptor->special_imports().at(2), nullptr, "foob", nullptr,
-             "bar.js");
+  CheckEntry(descriptor->special_imports().at(0), nullptr, nullptr, nullptr, 3);
+  CheckEntry(descriptor->special_imports().at(1), nullptr, "loo", nullptr, 4);
+  CheckEntry(descriptor->special_imports().at(2), nullptr, "foob", nullptr, 4);
 
   CHECK_EQ(4, descriptor->regular_imports().size());
   entry = descriptor->regular_imports().find(
       declarations->at(1)->proxy()->raw_name())->second;
-  CheckEntry(entry, nullptr, "z", "q", "m.js");
+  CheckEntry(entry, nullptr, "z", "q", 0);
   entry = descriptor->regular_imports().find(
       declarations->at(2)->proxy()->raw_name())->second;
-  CheckEntry(entry, nullptr, "n", "default", "n.js");
+  CheckEntry(entry, nullptr, "n", "default", 1);
   entry = descriptor->regular_imports().find(
       declarations->at(9)->proxy()->raw_name())->second;
-  CheckEntry(entry, nullptr, "mm", "m", "m.js");
+  CheckEntry(entry, nullptr, "mm", "m", 0);
   entry = descriptor->regular_imports().find(
       declarations->at(10)->proxy()->raw_name())->second;
-  CheckEntry(entry, nullptr, "aa", "aa", "m.js");
+  CheckEntry(entry, nullptr, "aa", "aa", 0);
 }