Code review follow-ups from: Plumb import assertions through...

Code review follow-ups from: Plumb import assertions through SourceTextModuleDescriptor's ModuleRequestMap

Address a few bits of code review feedback that came in after landing
https://chromium-review.googlesource.com/c/v8/v8/+/2493060:

- Add ModuleRequest:kAssertionEntrySize and use in place of a numeric
  literal.
- Get rid of ModuleRequestLocation and separate module_request_positions
  FixedArray, and merge these into AstModuleRequest and
  v8::internal::ModuleRequest.

Change-Id: If6d628d29bfa6fbd9933c6cdaa706623128ccc5d
Bug: v8:10958
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2530478Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#71125}

src/api/api.cc

@@ -2261,13 +2261,13 @@ Location Module::GetModuleRequestLocation(int i) const {
   i::Isolate* isolate = self->GetIsolate();
   i::HandleScope scope(isolate);
   CHECK(self->IsSourceTextModule());
-  i::Handle<i::FixedArray> module_request_positions(
-      i::Handle<i::SourceTextModule>::cast(self)
-          ->info()
-          .module_request_positions(),
+  i::Handle<i::FixedArray> module_requests(
+      i::Handle<i::SourceTextModule>::cast(self)->info().module_requests(),
       isolate);
-  CHECK_LT(i, module_request_positions->length());
-  int position = i::Smi::ToInt(module_request_positions->get(i));
+  CHECK_LT(i, module_requests->length());
+  i::Handle<i::ModuleRequest> module_request(
+      i::ModuleRequest::cast(module_requests->get(i)), isolate);
+  int position = module_request->position();
   i::Handle<i::Script> script(
       i::Handle<i::SourceTextModule>::cast(self)->GetScript(), isolate);
   i::Script::PositionInfo info;

src/ast/modules.cc

@@ -139,19 +139,20 @@ Handle<ModuleRequest> SourceTextModuleDescriptor::AstModuleRequest::Serialize(
   // 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));
+      isolate->factory()->NewFixedArray(static_cast<int>(
+          import_assertions()->size() * ModuleRequest::kAssertionEntrySize));
 
   int i = 0;
   for (auto iter = import_assertions()->cbegin();
-       iter != import_assertions()->cend(); ++iter, i += 3) {
+       iter != import_assertions()->cend();
+       ++iter, i += ModuleRequest::kAssertionEntrySize) {
     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);
+                                          import_assertions_array, position());
 }
 template Handle<ModuleRequest>
 SourceTextModuleDescriptor::AstModuleRequest::Serialize(Isolate* isolate) const;

src/ast/modules.h

@@ -126,11 +126,13 @@ class SourceTextModuleDescriptor : public ZoneObject {
 
   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) {}
+                     const ImportAssertions* import_assertions, int position,
+                     int index)
+        : specifier_(specifier),
+          import_assertions_(import_assertions),
+          position_(position),
+          index_(index) {}
 
     template <typename LocalIsolate>
     Handle<v8::internal::ModuleRequest> Serialize(LocalIsolate* isolate) const;
@@ -140,21 +142,19 @@ class SourceTextModuleDescriptor : public ZoneObject {
       return import_assertions_;
     }
 
+    int position() const { return position_; }
+    int index() const { return index_; }
+
    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;
+    int position_;
 
-    ModuleRequestLocation(int index, int position)
-        : index(index), position(position) {}
+    // The index at which we will place the request in SourceTextModuleInfo's
+    // module_requests FixedArray.
+    int index_;
   };
 
   // Custom content-based comparer for the below maps, to keep them stable
@@ -171,8 +171,7 @@ class SourceTextModuleDescriptor : public ZoneObject {
   };
 
   using ModuleRequestMap =
-      ZoneMap<const AstModuleRequest*, ModuleRequestLocation,
-              ModuleRequestComparer>;
+      ZoneSet<const AstModuleRequest*, ModuleRequestComparer>;
   using RegularExportMap =
       ZoneMultimap<const AstRawString*, Entry*, AstRawStringComparer>;
   using RegularImportMap =
@@ -274,12 +273,11 @@ class SourceTextModuleDescriptor : public ZoneObject {
     DCHECK_NOT_NULL(specifier);
     int module_requests_count = static_cast<int>(module_requests_.size());
     auto it = module_requests_
-                  .insert(std::make_pair(
-                      zone->New<AstModuleRequest>(specifier, import_assertions),
-                      ModuleRequestLocation(module_requests_count,
-                                            specifier_loc.beg_pos)))
+                  .insert(zone->New<AstModuleRequest>(
+                      specifier, import_assertions, specifier_loc.beg_pos,
+                      module_requests_count))
                   .first;
-    return it->second.index;
+    return (*it)->index();
   }
 };
 

src/diagnostics/objects-debug.cc

@@ -1387,9 +1387,11 @@ void Module::ModuleVerify(Isolate* isolate) {
 
 void ModuleRequest::ModuleRequestVerify(Isolate* isolate) {
   TorqueGeneratedClassVerifiers::ModuleRequestVerify(*this, isolate);
-  CHECK_EQ(0, import_assertions().length() % 3);
+  CHECK_EQ(0,
+           import_assertions().length() % ModuleRequest::kAssertionEntrySize);
 
-  for (int i = 0; i < import_assertions().length(); i += 3) {
+  for (int i = 0; i < import_assertions().length();
+       i += ModuleRequest::kAssertionEntrySize) {
     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

src/objects/module-inl.h

@@ -77,18 +77,13 @@ FixedArray SourceTextModuleInfo::namespace_imports() const {
   return FixedArray::cast(get(kNamespaceImportsIndex));
 }
 
-FixedArray SourceTextModuleInfo::module_request_positions() const {
-  return FixedArray::cast(get(kModuleRequestPositionsIndex));
-}
-
 #ifdef DEBUG
 bool SourceTextModuleInfo::Equals(SourceTextModuleInfo other) const {
   return regular_exports() == other.regular_exports() &&
          regular_imports() == other.regular_imports() &&
          special_exports() == other.special_exports() &&
          namespace_imports() == other.namespace_imports() &&
-         module_requests() == other.module_requests() &&
-         module_request_positions() == other.module_request_positions();
+         module_requests() == other.module_requests();
 }
 #endif
 

src/objects/scope-info.cc

@@ -1046,20 +1046,22 @@ std::ostream& operator<<(std::ostream& os, VariableAllocationInfo var_info) {
 template <typename LocalIsolate>
 Handle<ModuleRequest> ModuleRequest::New(LocalIsolate* isolate,
                                          Handle<String> specifier,
-                                         Handle<FixedArray> import_assertions) {
+                                         Handle<FixedArray> import_assertions,
+                                         int position) {
   Handle<ModuleRequest> result = Handle<ModuleRequest>::cast(
       isolate->factory()->NewStruct(MODULE_REQUEST_TYPE, AllocationType::kOld));
   result->set_specifier(*specifier);
   result->set_import_assertions(*import_assertions);
+  result->set_position(position);
   return result;
 }
 
 template Handle<ModuleRequest> ModuleRequest::New(
     Isolate* isolate, Handle<String> specifier,
-    Handle<FixedArray> import_assertions);
+    Handle<FixedArray> import_assertions, int position);
 template Handle<ModuleRequest> ModuleRequest::New(
     LocalIsolate* isolate, Handle<String> specifier,
-    Handle<FixedArray> import_assertions);
+    Handle<FixedArray> import_assertions, int position);
 
 template <typename LocalIsolate>
 Handle<SourceTextModuleInfoEntry> SourceTextModuleInfoEntry::New(
@@ -1097,14 +1099,9 @@ Handle<SourceTextModuleInfo> SourceTextModuleInfo::New(
   // Serialize module requests.
   int size = static_cast<int>(descr->module_requests().size());
   Handle<FixedArray> module_requests = isolate->factory()->NewFixedArray(size);
-  Handle<FixedArray> module_request_positions =
-      isolate->factory()->NewFixedArray(size);
   for (const auto& elem : descr->module_requests()) {
-    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));
+    Handle<ModuleRequest> serialized_module_request = elem->Serialize(isolate);
+    module_requests->set(elem->index(), *serialized_module_request);
   }
 
   // Serialize special exports.
@@ -1154,7 +1151,6 @@ Handle<SourceTextModuleInfo> SourceTextModuleInfo::New(
   result->set(kRegularExportsIndex, *regular_exports);
   result->set(kNamespaceImportsIndex, *namespace_imports);
   result->set(kRegularImportsIndex, *regular_imports);
-  result->set(kModuleRequestPositionsIndex, *module_request_positions);
   return result;
 }
 template Handle<SourceTextModuleInfo> SourceTextModuleInfo::New(

src/objects/source-text-module.h

@@ -205,7 +205,6 @@ class SourceTextModuleInfo : public FixedArray {
   inline FixedArray regular_exports() const;
   inline FixedArray regular_imports() const;
   inline FixedArray namespace_imports() const;
-  inline FixedArray module_request_positions() const;
 
   // Accessors for [regular_exports].
   int RegularExportCount() const;
@@ -227,7 +226,6 @@ class SourceTextModuleInfo : public FixedArray {
     kRegularExportsIndex,
     kNamespaceImportsIndex,
     kRegularImportsIndex,
-    kModuleRequestPositionsIndex,
     kLength
   };
   enum {
@@ -249,7 +247,12 @@ class ModuleRequest
   template <typename LocalIsolate>
   static Handle<ModuleRequest> New(LocalIsolate* isolate,
                                    Handle<String> specifier,
-                                   Handle<FixedArray> import_assertions);
+                                   Handle<FixedArray> import_assertions,
+                                   int position);
+
+  // The number of entries in the import_assertions FixedArray that are used for
+  // a single assertion.
+  static const size_t kAssertionEntrySize = 3;
 
   TQ_OBJECT_CONSTRUCTORS(ModuleRequest)
 };

src/objects/source-text-module.tq

@@ -52,6 +52,9 @@ extern class ModuleRequest extends Struct {
   // Import assertions are stored in this array in the form:
   // [key1, value1, location1, key2, value2, location2, ...]
   import_assertions: FixedArray;
+
+  // Source text position of the module request
+  position: Smi;
 }
 
 @generateCppClass