Plumb import assertions through SourceTextModuleDescriptor's ModuleRequestMap

This change plumbs import assertions from SourceTextModuleDescriptor's
ModuleRequestMap into SourceTextModuleInfo via a new ModuleRequest
type, where previously there had been only the specifier.

SourceTextModuleDescriptor::module_map now deduplicates module requests
using the specifier and the import assertions.  Continuing to use the
specifier alone would cause a loss of information in the event that
a module imports from the same specifier multiple times using different
sets of assertions.  Failing to deduplicate at all would result in
multiple requests for statements like `import {a,b,c} from "foo.js"`,
which would be a potential performance issue.  See design doc at
https://docs.google.com/document/d/1yuXgNHSbTAPubT1Mg0JXp5uTrfirkvO1g5cHHCe-LmY
for more detail on this decision.

v8::internal::ModuleRequest holds the assertions as an array of the form
[key1, value1, position1, key2, value2, assertion2, ...].  However the
parser still needs to use a map, since duplicate assertion keys need to
be detected at parse time.  A follow-up  change will ensure that
assertions are sorted using a proper lexicographic sort.

Bug: v8:10958
Change-Id: Iff13fb9a37d58fc1622cd3cce78925ad2b7a14bb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2493060
Commit-Queue: Dan Clark <daniec@microsoft.com>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71066}

src/api/api.cc

@@ -2249,7 +2249,9 @@ Local<String> Module::GetModuleRequest(int i) const {
       i::Handle<i::SourceTextModule>::cast(self)->info().module_requests(),
       isolate);
   CHECK_LT(i, module_requests->length());
-  return ToApiHandle<String>(i::handle(module_requests->get(i), isolate));
+  i::Handle<i::ModuleRequest> module_request(
+      i::ModuleRequest::cast(module_requests->get(i)), isolate);
+  return ToApiHandle<String>(i::handle(module_request->specifier(), isolate));
 }
 
 Location Module::GetModuleRequestLocation(int i) const {

src/ast/modules.cc

@@ -16,17 +16,47 @@ namespace internal {
 
 bool SourceTextModuleDescriptor::AstRawStringComparer::operator()(
     const AstRawString* lhs, const AstRawString* rhs) const {
+  return ThreeWayCompare(lhs, rhs) < 0;
+}
+
+int SourceTextModuleDescriptor::AstRawStringComparer::ThreeWayCompare(
+    const AstRawString* lhs, const AstRawString* rhs) {
   // Fast path for equal pointers: a pointer is not strictly less than itself.
   if (lhs == rhs) return false;
 
   // Order by contents (ordering by hash is unstable across runs).
   if (lhs->is_one_byte() != rhs->is_one_byte()) {
-    return lhs->is_one_byte();
+    return lhs->is_one_byte() ? -1 : 1;
   }
   if (lhs->byte_length() != rhs->byte_length()) {
-    return lhs->byte_length() < rhs->byte_length();
+    return lhs->byte_length() - rhs->byte_length();
+  }
+  return memcmp(lhs->raw_data(), rhs->raw_data(), lhs->byte_length());
+}
+
+bool SourceTextModuleDescriptor::ModuleRequestComparer::operator()(
+    const AstModuleRequest* lhs, const AstModuleRequest* rhs) const {
+  if (int specifier_comparison = AstRawStringComparer::ThreeWayCompare(
+          lhs->specifier(), rhs->specifier()))
+    return specifier_comparison < 0;
+
+  if (lhs->import_assertions()->size() != rhs->import_assertions()->size())
+    return (lhs->import_assertions()->size() <
+            rhs->import_assertions()->size());
+
+  auto lhsIt = lhs->import_assertions()->cbegin();
+  auto rhsIt = rhs->import_assertions()->cbegin();
+  for (; lhsIt != lhs->import_assertions()->cend(); ++lhsIt, ++rhsIt) {
+    if (int assertion_key_comparison =
+            AstRawStringComparer::ThreeWayCompare(lhsIt->first, rhsIt->first))
+      return assertion_key_comparison < 0;
+
+    if (int assertion_value_comparison = AstRawStringComparer::ThreeWayCompare(
+            lhsIt->second.first, rhsIt->second.first))
+      return assertion_value_comparison < 0;
   }
-  return memcmp(lhs->raw_data(), rhs->raw_data(), lhs->byte_length()) < 0;
+
+  return false;
 }
 
 void SourceTextModuleDescriptor::AddImport(
@@ -34,11 +64,11 @@ void SourceTextModuleDescriptor::AddImport(
     const AstRawString* module_request,
     const ImportAssertions* import_assertions, const Scanner::Location loc,
     const Scanner::Location specifier_loc, Zone* zone) {
-  // TODO(v8:10958) Add import_assertions to the Entry.
   Entry* entry = zone->New<Entry>(loc);
   entry->local_name = local_name;
   entry->import_name = import_name;
-  entry->module_request = AddModuleRequest(module_request, specifier_loc);
+  entry->module_request =
+      AddModuleRequest(module_request, import_assertions, specifier_loc, zone);
   AddRegularImport(entry);
 }
 
@@ -46,19 +76,18 @@ void SourceTextModuleDescriptor::AddStarImport(
     const AstRawString* local_name, const AstRawString* module_request,
     const ImportAssertions* import_assertions, const Scanner::Location loc,
     const Scanner::Location specifier_loc, Zone* zone) {
-  // TODO(v8:10958) Add import_assertions to the Entry.
   Entry* entry = zone->New<Entry>(loc);
   entry->local_name = local_name;
-  entry->module_request = AddModuleRequest(module_request, specifier_loc);
+  entry->module_request =
+      AddModuleRequest(module_request, import_assertions, specifier_loc, zone);
   AddNamespaceImport(entry, zone);
 }
 
 void SourceTextModuleDescriptor::AddEmptyImport(
     const AstRawString* module_request,
     const ImportAssertions* import_assertions,
-    const Scanner::Location specifier_loc) {
-  // TODO(v8:10958) Add import_assertions to the Entry.
-  AddModuleRequest(module_request, specifier_loc);
+    const Scanner::Location specifier_loc, Zone* zone) {
+  AddModuleRequest(module_request, import_assertions, specifier_loc, zone);
 }
 
 void SourceTextModuleDescriptor::AddExport(const AstRawString* local_name,
@@ -75,13 +104,13 @@ void SourceTextModuleDescriptor::AddExport(
     const AstRawString* module_request,
     const ImportAssertions* import_assertions, const Scanner::Location loc,
     const Scanner::Location specifier_loc, Zone* zone) {
-  // TODO(v8:10958) Add import_assertions to the Entry.
   DCHECK_NOT_NULL(import_name);
   DCHECK_NOT_NULL(export_name);
   Entry* entry = zone->New<Entry>(loc);
   entry->export_name = export_name;
   entry->import_name = import_name;
-  entry->module_request = AddModuleRequest(module_request, specifier_loc);
+  entry->module_request =
+      AddModuleRequest(module_request, import_assertions, specifier_loc, zone);
   AddSpecialExport(entry, zone);
 }
 
@@ -89,9 +118,9 @@ void SourceTextModuleDescriptor::AddStarExport(
     const AstRawString* module_request,
     const ImportAssertions* import_assertions, const Scanner::Location loc,
     const Scanner::Location specifier_loc, Zone* zone) {
-  // TODO(v8:10958) Add import_assertions to the Entry.
   Entry* entry = zone->New<Entry>(loc);
-  entry->module_request = AddModuleRequest(module_request, specifier_loc);
+  entry->module_request =
+      AddModuleRequest(module_request, import_assertions, specifier_loc, zone);
   AddSpecialExport(entry, zone);
 }
 
@@ -104,6 +133,32 @@ Handle<PrimitiveHeapObject> ToStringOrUndefined(LocalIsolate* isolate,
 }
 }  // namespace
 
+template <typename LocalIsolate>
+Handle<ModuleRequest> SourceTextModuleDescriptor::AstModuleRequest::Serialize(
+    LocalIsolate* isolate) const {
+  // The import assertions will be stored in this array in the form:
+  // [key1, value1, location1, key2, value2, location2, ...]
+  Handle<FixedArray> import_assertions_array =
+      isolate->factory()->NewFixedArray(
+          static_cast<int>(import_assertions()->size() * 3));
+
+  int i = 0;
+  for (auto iter = import_assertions()->cbegin();
+       iter != import_assertions()->cend(); ++iter, i += 3) {
+    import_assertions_array->set(i, *iter->first->string());
+    import_assertions_array->set(i + 1, *iter->second.first->string());
+    import_assertions_array->set(i + 2,
+                                 Smi::FromInt(iter->second.second.beg_pos));
+  }
+  return v8::internal::ModuleRequest::New(isolate, specifier()->string(),
+                                          import_assertions_array);
+}
+template Handle<ModuleRequest>
+SourceTextModuleDescriptor::AstModuleRequest::Serialize(Isolate* isolate) const;
+template Handle<ModuleRequest>
+SourceTextModuleDescriptor::AstModuleRequest::Serialize(
+    LocalIsolate* isolate) const;
+
 template <typename LocalIsolate>
 Handle<SourceTextModuleInfoEntry> SourceTextModuleDescriptor::Entry::Serialize(
     LocalIsolate* isolate) const {

src/ast/modules.h

@@ -13,6 +13,7 @@ namespace internal {
 
 
 class AstRawString;
+class ModuleRequest;
 class SourceTextModuleInfo;
 class SourceTextModuleInfoEntry;
 class PendingCompilationErrorHandler;
@@ -55,7 +56,7 @@ class SourceTextModuleDescriptor : public ZoneObject {
   // export {} from "foo.js";  (sic!)
   void AddEmptyImport(const AstRawString* module_request,
                       const ImportAssertions* import_assertions,
-                      const Scanner::Location specifier_loc);
+                      const Scanner::Location specifier_loc, Zone* zone);
 
   // export {x};
   // export {x as y};
@@ -123,20 +124,55 @@ class SourceTextModuleDescriptor : public ZoneObject {
   enum CellIndexKind { kInvalid, kExport, kImport };
   static CellIndexKind GetCellIndexKind(int cell_index);
 
-  struct ModuleRequest {
+  class AstModuleRequest : public ZoneObject {
+   public:
+    // TODO(v8:10958): Consider storing module request location here
+    // instead of using separate ModuleRequestLocation struct.
+    AstModuleRequest(const AstRawString* specifier,
+                     const ImportAssertions* import_assertions)
+        : specifier_(specifier), import_assertions_(import_assertions) {}
+
+    template <typename LocalIsolate>
+    Handle<v8::internal::ModuleRequest> Serialize(LocalIsolate* isolate) const;
+
+    const AstRawString* specifier() const { return specifier_; }
+    const ImportAssertions* import_assertions() const {
+      return import_assertions_;
+    }
+
+   private:
+    const AstRawString* specifier_;
+    const ImportAssertions* import_assertions_;
+  };
+
+  struct ModuleRequestLocation {
+    // The index at which we will place the request in SourceTextModuleInfo's
+    // module_requests FixedArray.
     int index;
+
+    // The JS source code position of the request, used for reporting errors.
     int position;
-    ModuleRequest(int index, int position) : index(index), position(position) {}
+
+    ModuleRequestLocation(int index, int position)
+        : index(index), position(position) {}
   };
 
   // Custom content-based comparer for the below maps, to keep them stable
   // across parses.
   struct V8_EXPORT_PRIVATE AstRawStringComparer {
     bool operator()(const AstRawString* lhs, const AstRawString* rhs) const;
+    static int ThreeWayCompare(const AstRawString* lhs,
+                               const AstRawString* rhs);
+  };
+
+  struct V8_EXPORT_PRIVATE ModuleRequestComparer {
+    bool operator()(const AstModuleRequest* lhs,
+                    const AstModuleRequest* rhs) const;
   };
 
   using ModuleRequestMap =
-      ZoneMap<const AstRawString*, ModuleRequest, AstRawStringComparer>;
+      ZoneMap<const AstModuleRequest*, ModuleRequestLocation,
+              ModuleRequestComparer>;
   using RegularExportMap =
       ZoneMultimap<const AstRawString*, Entry*, AstRawStringComparer>;
   using RegularImportMap =
@@ -233,13 +269,15 @@ class SourceTextModuleDescriptor : public ZoneObject {
   void AssignCellIndices();
 
   int AddModuleRequest(const AstRawString* specifier,
-                       Scanner::Location specifier_loc) {
+                       const ImportAssertions* import_assertions,
+                       Scanner::Location specifier_loc, Zone* zone) {
     DCHECK_NOT_NULL(specifier);
     int module_requests_count = static_cast<int>(module_requests_.size());
     auto it = module_requests_
-                  .insert(std::make_pair(specifier,
-                                         ModuleRequest(module_requests_count,
-                                                       specifier_loc.beg_pos)))
+                  .insert(std::make_pair(
+                      zone->New<AstModuleRequest>(specifier, import_assertions),
+                      ModuleRequestLocation(module_requests_count,
+                                            specifier_loc.beg_pos)))
                   .first;
     return it->second.index;
   }

src/compiler/types.cc

@@ -309,6 +309,7 @@ Type::bitset BitsetType::Lub(const MapRefLike& map) {
     case EVAL_CONTEXT_TYPE:
     case FUNCTION_CONTEXT_TYPE:
     case MODULE_CONTEXT_TYPE:
+    case MODULE_REQUEST_TYPE:
     case NATIVE_CONTEXT_TYPE:
     case SCRIPT_CONTEXT_TYPE:
     case WITH_CONTEXT_TYPE:

src/diagnostics/objects-debug.cc

@@ -1385,6 +1385,17 @@ void Module::ModuleVerify(Isolate* isolate) {
   CHECK_NE(hash(), 0);
 }
 
+void ModuleRequest::ModuleRequestVerify(Isolate* isolate) {
+  TorqueGeneratedClassVerifiers::ModuleRequestVerify(*this, isolate);
+  CHECK_EQ(0, import_assertions().length() % 3);
+
+  for (int i = 0; i < import_assertions().length(); i += 3) {
+    CHECK(import_assertions().get(i).IsString());      // Assertion key
+    CHECK(import_assertions().get(i + 1).IsString());  // Assertion value
+    CHECK(import_assertions().get(i + 2).IsSmi());     // Assertion location
+  }
+}
+
 void SourceTextModule::SourceTextModuleVerify(Isolate* isolate) {
   TorqueGeneratedClassVerifiers::SourceTextModuleVerify(*this, isolate);
 

src/objects/module-inl.h

@@ -25,6 +25,7 @@ OBJECT_CONSTRUCTORS_IMPL(Module, HeapObject)
 TQ_OBJECT_CONSTRUCTORS_IMPL(JSModuleNamespace)
 
 NEVER_READ_ONLY_SPACE_IMPL(Module)
+NEVER_READ_ONLY_SPACE_IMPL(ModuleRequest)
 NEVER_READ_ONLY_SPACE_IMPL(SourceTextModule)
 NEVER_READ_ONLY_SPACE_IMPL(SyntheticModule)
 

src/objects/objects-definitions.h

@@ -135,6 +135,7 @@ namespace internal {
     function_template_rare_data)                                              \
   V(_, INTERCEPTOR_INFO_TYPE, InterceptorInfo, interceptor_info)              \
   V(_, INTERPRETER_DATA_TYPE, InterpreterData, interpreter_data)              \
+  V(_, MODULE_REQUEST_TYPE, ModuleRequest, module_request)                    \
   V(_, PROMISE_CAPABILITY_TYPE, PromiseCapability, promise_capability)        \
   V(_, PROMISE_REACTION_TYPE, PromiseReaction, promise_reaction)              \
   V(_, PROPERTY_DESCRIPTOR_OBJECT_TYPE, PropertyDescriptorObject,             \

src/objects/scope-info.cc

@@ -1043,6 +1043,24 @@ std::ostream& operator<<(std::ostream& os, VariableAllocationInfo var_info) {
   return os;
 }
 
+template <typename LocalIsolate>
+Handle<ModuleRequest> ModuleRequest::New(LocalIsolate* isolate,
+                                         Handle<String> specifier,
+                                         Handle<FixedArray> import_assertions) {
+  Handle<ModuleRequest> result = Handle<ModuleRequest>::cast(
+      isolate->factory()->NewStruct(MODULE_REQUEST_TYPE, AllocationType::kOld));
+  result->set_specifier(*specifier);
+  result->set_import_assertions(*import_assertions);
+  return result;
+}
+
+template Handle<ModuleRequest> ModuleRequest::New(
+    Isolate* isolate, Handle<String> specifier,
+    Handle<FixedArray> import_assertions);
+template Handle<ModuleRequest> ModuleRequest::New(
+    LocalIsolate* isolate, Handle<String> specifier,
+    Handle<FixedArray> import_assertions);
+
 template <typename LocalIsolate>
 Handle<SourceTextModuleInfoEntry> SourceTextModuleInfoEntry::New(
     LocalIsolate* isolate, Handle<PrimitiveHeapObject> export_name,
@@ -1082,7 +1100,9 @@ Handle<SourceTextModuleInfo> SourceTextModuleInfo::New(
   Handle<FixedArray> module_request_positions =
       isolate->factory()->NewFixedArray(size);
   for (const auto& elem : descr->module_requests()) {
-    module_requests->set(elem.second.index, *elem.first->string());
+    Handle<ModuleRequest> serialized_module_request =
+        elem.first->Serialize(isolate);
+    module_requests->set(elem.second.index, *serialized_module_request);
     module_request_positions->set(elem.second.index,
                                   Smi::FromInt(elem.second.position));
   }

src/objects/source-text-module-inl.h

@@ -17,6 +17,7 @@ namespace internal {
 
 #include "torque-generated/src/objects/source-text-module-tq-inl.inc"
 
+TQ_OBJECT_CONSTRUCTORS_IMPL(ModuleRequest)
 TQ_OBJECT_CONSTRUCTORS_IMPL(SourceTextModule)
 TQ_OBJECT_CONSTRUCTORS_IMPL(SourceTextModuleInfoEntry)
 

src/objects/source-text-module.cc

@@ -234,16 +234,20 @@ MaybeHandle<Cell> SourceTextModule::ResolveExport(
 
 MaybeHandle<Cell> SourceTextModule::ResolveImport(
     Isolate* isolate, Handle<SourceTextModule> module, Handle<String> name,
-    int module_request, MessageLocation loc, bool must_resolve,
+    int module_request_index, MessageLocation loc, bool must_resolve,
     Module::ResolveSet* resolve_set) {
   Handle<Module> requested_module(
-      Module::cast(module->requested_modules().get(module_request)), isolate);
-  Handle<String> specifier(
-      String::cast(module->info().module_requests().get(module_request)),
+      Module::cast(module->requested_modules().get(module_request_index)),
+      isolate);
+  Handle<ModuleRequest> module_request(
+      ModuleRequest::cast(
+          module->info().module_requests().get(module_request_index)),
       isolate);
+  Handle<String> module_specifier(String::cast(module_request->specifier()),
+                                  isolate);
   MaybeHandle<Cell> result =
-      Module::ResolveExport(isolate, requested_module, specifier, name, loc,
-                            must_resolve, resolve_set);
+      Module::ResolveExport(isolate, requested_module, module_specifier, name,
+                            loc, must_resolve, resolve_set);
   DCHECK_IMPLIES(isolate->has_pending_exception(), result.is_null());
   return result;
 }
@@ -312,7 +316,10 @@ bool SourceTextModule::PrepareInstantiate(
   Handle<FixedArray> module_requests(module_info->module_requests(), isolate);
   Handle<FixedArray> requested_modules(module->requested_modules(), isolate);
   for (int i = 0, length = module_requests->length(); i < length; ++i) {
-    Handle<String> specifier(String::cast(module_requests->get(i)), isolate);
+    Handle<ModuleRequest> module_request(
+        ModuleRequest::cast(module_requests->get(i)), isolate);
+    Handle<String> specifier(module_request->specifier(), isolate);
+    // TODO(v8:10958) Pass import assertions to the callback
     v8::Local<v8::Module> api_requested_module;
     if (!callback(context, v8::Utils::ToLocal(specifier),
                   v8::Utils::ToLocal(Handle<Module>::cast(module)))

src/objects/source-text-module.h

@@ -126,7 +126,7 @@ class SourceTextModule
       MessageLocation loc, bool must_resolve, ResolveSet* resolve_set);
   static V8_WARN_UNUSED_RESULT MaybeHandle<Cell> ResolveImport(
       Isolate* isolate, Handle<SourceTextModule> module, Handle<String> name,
-      int module_request, MessageLocation loc, bool must_resolve,
+      int module_request_index, MessageLocation loc, bool must_resolve,
       ResolveSet* resolve_set);
 
   static V8_WARN_UNUSED_RESULT MaybeHandle<Cell> ResolveExportUsingStarExports(
@@ -238,6 +238,20 @@ class SourceTextModuleInfo : public FixedArray {
   OBJECT_CONSTRUCTORS(SourceTextModuleInfo, FixedArray);
 };
 
+class ModuleRequest
+    : public TorqueGeneratedModuleRequest<ModuleRequest, Struct> {
+ public:
+  NEVER_READ_ONLY_SPACE
+  DECL_VERIFIER(ModuleRequest)
+
+  template <typename LocalIsolate>
+  static Handle<ModuleRequest> New(LocalIsolate* isolate,
+                                   Handle<String> specifier,
+                                   Handle<FixedArray> import_assertions);
+
+  TQ_OBJECT_CONSTRUCTORS(ModuleRequest)
+};
+
 class SourceTextModuleInfoEntry
     : public TorqueGeneratedSourceTextModuleInfoEntry<SourceTextModuleInfoEntry,
                                                       Struct> {

src/objects/source-text-module.tq

@@ -47,6 +47,16 @@ extern class SourceTextModule extends Module {
   flags: SmiTagged<SourceTextModuleFlags>;
 }
 
+@generateCppClass
+@generatePrint
+extern class ModuleRequest extends Struct {
+  specifier: String;
+
+  // Import assertions are stored in this array in the form:
+  // [key1, value1, location1, key2, value2, location2, ...]
+  import_assertions: FixedArray;
+}
+
 @generateCppClass
 extern class SourceTextModuleInfoEntry extends Struct {
   export_name: String|Undefined;

src/parsing/parser.cc

@@ -1306,8 +1306,8 @@ void Parser::ParseImportDeclaration() {
     const AstRawString* module_specifier = ParseModuleSpecifier();
     const ImportAssertions* import_assertions = ParseImportAssertClause();
     ExpectSemicolon();
-    module()->AddEmptyImport(module_specifier, import_assertions,
-                             specifier_loc);
+    module()->AddEmptyImport(module_specifier, import_assertions, specifier_loc,
+                             zone());
     return;
   }
 
@@ -1377,7 +1377,7 @@ void Parser::ParseImportDeclaration() {
   if (named_imports != nullptr) {
     if (named_imports->length() == 0) {
       module()->AddEmptyImport(module_specifier, import_assertions,
-                               specifier_loc);
+                               specifier_loc, zone());
     } else {
       for (const NamedImport* import : *named_imports) {
         module()->AddImport(import->import_name, import->local_name,
@@ -1567,7 +1567,7 @@ Statement* Parser::ParseExportDeclaration() {
 
         if (export_data->is_empty()) {
           module()->AddEmptyImport(module_specifier, import_assertions,
-                                   specifier_loc);
+                                   specifier_loc, zone());
         } else {
           for (const ExportClauseData& data : *export_data) {
             module()->AddExport(data.local_name, data.export_name,