module: link module with a module request record

When a module is being statically linked with module requests, if two
module requests with a same specifier but different attributes are
resolved to two modules, the module requests should be linked to these
two modules.

PR-URL: https://github.com/nodejs/node/pull/58886
Refs: https://tc39.es/ecma262/#sec-HostLoadImportedModule
Refs: https://github.com/tc39/proposal-import-attributes?tab=readme-ov-file#how-would-this-proposal-work-with-caching
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

lib/internal/modules/esm/module_job.js

@@ -190,8 +190,7 @@ class ModuleJob extends ModuleJobBase {
     const evaluationDepJobs = Array(moduleRequests.length);
     ObjectSetPrototypeOf(evaluationDepJobs, null);
 
-    // Specifiers should be aligned with the moduleRequests array in order.
-    const specifiers = Array(moduleRequests.length);
+    // Modules should be aligned with the moduleRequests array in order.
     const modulePromises = Array(moduleRequests.length);
     // Track each loop for whether it is an evaluation phase or source phase request.
     let isEvaluation;
@@ -217,11 +216,10 @@ class ModuleJob extends ModuleJobBase {
         return job.modulePromise;
       });
       modulePromises[idx] = modulePromise;
-      specifiers[idx] = specifier;
     }
 
     const modules = await SafePromiseAllReturnArrayLike(modulePromises);
-    this.module.link(specifiers, modules);
+    this.module.link(modules);
 
     return evaluationDepJobs;
   }
@@ -433,22 +431,20 @@ class ModuleJobSync extends ModuleJobBase {
     this.#loader.loadCache.set(this.url, this.type, this);
     try {
       const moduleRequests = this.module.getModuleRequests();
-      // Specifiers should be aligned with the moduleRequests array in order.
-      const specifiers = Array(moduleRequests.length);
+      // Modules should be aligned with the moduleRequests array in order.
       const modules = Array(moduleRequests.length);
       const evaluationDepJobs = Array(moduleRequests.length);
       let j = 0;
       for (let i = 0; i < moduleRequests.length; ++i) {
         const { specifier, attributes, phase } = moduleRequests[i];
         const job = this.#loader.getModuleJobForRequire(specifier, this.url, attributes, phase);
-        specifiers[i] = specifier;
         modules[i] = job.module;
         if (phase === kEvaluationPhase) {
           evaluationDepJobs[j++] = job;
         }
       }
       evaluationDepJobs.length = j;
-      this.module.link(specifiers, modules);
+      this.module.link(modules);
       this.linked = evaluationDepJobs;
     } finally {
       // Restore it - if it succeeds, we'll reset in the caller; Otherwise it's

lib/internal/vm/module.js

@@ -148,23 +148,10 @@ class Module {
       });
     }
 
-    let registry = { __proto__: null };
     if (sourceText !== undefined) {
       this[kWrap] = new ModuleWrap(identifier, context, sourceText,
                                    options.lineOffset, options.columnOffset,
                                    options.cachedData);
-      registry = {
-        __proto__: null,
-        initializeImportMeta: options.initializeImportMeta,
-        importModuleDynamically: options.importModuleDynamically ?
-          importModuleDynamicallyWrap(options.importModuleDynamically) :
-          undefined,
-      };
-      // This will take precedence over the referrer as the object being
-      // passed into the callbacks.
-      registry.callbackReferrer = this;
-      const { registerModule } = require('internal/modules/esm/utils');
-      registerModule(this[kWrap], registry);
     } else {
       assert(syntheticEvaluationSteps);
       this[kWrap] = new ModuleWrap(identifier, context,
@@ -315,6 +302,19 @@ class SourceTextModule extends Module {
       importModuleDynamically,
     });
 
+    const registry = {
+      __proto__: null,
+      initializeImportMeta: options.initializeImportMeta,
+      importModuleDynamically: options.importModuleDynamically ?
+        importModuleDynamicallyWrap(options.importModuleDynamically) :
+        undefined,
+    };
+    // This will take precedence over the referrer as the object being
+    // passed into the callbacks.
+    registry.callbackReferrer = this;
+    const { registerModule } = require('internal/modules/esm/utils');
+    registerModule(this[kWrap], registry);
+
     this.#moduleRequests = ObjectFreeze(ArrayPrototypeMap(this[kWrap].getModuleRequests(), (request) => {
       return ObjectFreeze({
         __proto__: null,
@@ -329,8 +329,7 @@ class SourceTextModule extends Module {
     this.#statusOverride = 'linking';
 
     // Iterates the module requests and links with the linker.
-    // Specifiers should be aligned with the moduleRequests array in order.
-    const specifiers = Array(this.#moduleRequests.length);
+    // Modules should be aligned with the moduleRequests array in order.
     const modulePromises = Array(this.#moduleRequests.length);
     // Iterates with index to avoid calling into userspace with `Symbol.iterator`.
     for (let idx = 0; idx < this.#moduleRequests.length; idx++) {
@@ -357,12 +356,11 @@ class SourceTextModule extends Module {
           return module[kWrap];
         });
       modulePromises[idx] = modulePromise;
-      specifiers[idx] = specifier;
     }
 
     try {
       const modules = await SafePromiseAllReturnArrayLike(modulePromises);
-      this[kWrap].link(specifiers, modules);
+      this[kWrap].link(modules);
     } catch (e) {
       this.#error = e;
       throw e;

src/module_wrap.cc

@@ -63,6 +63,51 @@ using v8::UnboundModuleScript;
 using v8::Undefined;
 using v8::Value;
 
+void ModuleCacheKey::MemoryInfo(MemoryTracker* tracker) const {
+  tracker->TrackField("specifier", specifier);
+  tracker->TrackField("import_attributes", import_attributes);
+}
+
+template <int elements_per_attribute>
+ModuleCacheKey ModuleCacheKey::From(Local<Context> context,
+                                    Local<String> specifier,
+                                    Local<FixedArray> import_attributes) {
+  CHECK_EQ(import_attributes->Length() % elements_per_attribute, 0);
+  Isolate* isolate = context->GetIsolate();
+  std::size_t h1 = specifier->GetIdentityHash();
+  size_t num_attributes = import_attributes->Length() / elements_per_attribute;
+  ImportAttributeVector attributes;
+  attributes.reserve(num_attributes);
+
+  std::size_t h2 = 0;
+
+  for (int i = 0; i < import_attributes->Length();
+       i += elements_per_attribute) {
+    Local<String> v8_key = import_attributes->Get(context, i).As<String>();
+    Local<String> v8_value =
+        import_attributes->Get(context, i + 1).As<String>();
+    Utf8Value key_utf8(isolate, v8_key);
+    Utf8Value value_utf8(isolate, v8_value);
+
+    attributes.emplace_back(key_utf8.ToString(), value_utf8.ToString());
+    h2 ^= v8_key->GetIdentityHash();
+    h2 ^= v8_value->GetIdentityHash();
+  }
+
+  // Combine the hashes using a simple XOR and bit shift to reduce
+  // collisions. Note that the hash does not guarantee uniqueness.
+  std::size_t hash = h1 ^ (h2 << 1);
+
+  Utf8Value utf8_specifier(isolate, specifier);
+  return ModuleCacheKey{utf8_specifier.ToString(), attributes, hash};
+}
+
+ModuleCacheKey ModuleCacheKey::From(Local<Context> context,
+                                    Local<ModuleRequest> v8_request) {
+  return From(
+      context, v8_request->GetSpecifier(), v8_request->GetImportAttributes());
+}
+
 ModuleWrap::ModuleWrap(Realm* realm,
                        Local<Object> object,
                        Local<Module> module,
@@ -509,7 +554,7 @@ void ModuleWrap::GetModuleRequests(const FunctionCallbackInfo<Value>& args) {
       realm, isolate, module->GetModuleRequests()));
 }
 
-// moduleWrap.link(specifiers, moduleWraps)
+// moduleWrap.link(moduleWraps)
 void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
   Realm* realm = Realm::GetCurrent(args);
   Isolate* isolate = args.GetIsolate();
@@ -518,33 +563,28 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
   ModuleWrap* dependent;
   ASSIGN_OR_RETURN_UNWRAP(&dependent, args.This());
 
-  CHECK_EQ(args.Length(), 2);
+  CHECK_EQ(args.Length(), 1);
 
-  Local<Array> specifiers = args[0].As<Array>();
-  Local<Array> modules = args[1].As<Array>();
-  CHECK_EQ(specifiers->Length(), modules->Length());
+  Local<FixedArray> requests =
+      dependent->module_.Get(isolate)->GetModuleRequests();
+  Local<Array> modules = args[0].As<Array>();
+  CHECK_EQ(modules->Length(), static_cast<uint32_t>(requests->Length()));
 
-  std::vector<Global<Value>> specifiers_buffer;
-  if (FromV8Array(context, specifiers, &specifiers_buffer).IsNothing()) {
-    return;
-  }
   std::vector<Global<Value>> modules_buffer;
   if (FromV8Array(context, modules, &modules_buffer).IsNothing()) {
     return;
   }
 
-  for (uint32_t i = 0; i < specifiers->Length(); i++) {
-    Local<String> specifier_str =
-        specifiers_buffer[i].Get(isolate).As<String>();
+  for (uint32_t i = 0; i < modules_buffer.size(); i++) {
     Local<Object> module_object = modules_buffer[i].Get(isolate).As<Object>();
 
     CHECK(
         realm->isolate_data()->module_wrap_constructor_template()->HasInstance(
             module_object));
 
-    Utf8Value specifier(isolate, specifier_str);
-    dependent->resolve_cache_[specifier.ToString()].Reset(isolate,
-                                                          module_object);
+    ModuleCacheKey module_cache_key = ModuleCacheKey::From(
+        context, requests->Get(context, i).As<ModuleRequest>());
+    dependent->resolve_cache_[module_cache_key].Reset(isolate, module_object);
   }
 }
 
@@ -924,27 +964,27 @@ MaybeLocal<Module> ModuleWrap::ResolveModuleCallback(
     return MaybeLocal<Module>();
   }
 
-  Utf8Value specifier_utf8(isolate, specifier);
-  std::string specifier_std(*specifier_utf8, specifier_utf8.length());
+  ModuleCacheKey cache_key =
+      ModuleCacheKey::From(context, specifier, import_attributes);
 
   ModuleWrap* dependent = GetFromModule(env, referrer);
   if (dependent == nullptr) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is from invalid module", specifier_std);
+        env, "request for '%s' is from invalid module", cache_key.specifier);
     return MaybeLocal<Module>();
   }
 
-  if (dependent->resolve_cache_.count(specifier_std) != 1) {
+  if (dependent->resolve_cache_.count(cache_key) != 1) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is not in cache", specifier_std);
+        env, "request for '%s' is not in cache", cache_key.specifier);
     return MaybeLocal<Module>();
   }
 
   Local<Object> module_object =
-      dependent->resolve_cache_[specifier_std].Get(isolate);
+      dependent->resolve_cache_[cache_key].Get(isolate);
   if (module_object.IsEmpty() || !module_object->IsObject()) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' did not return an object", specifier_std);
+        env, "request for '%s' did not return an object", cache_key.specifier);
     return MaybeLocal<Module>();
   }
 
@@ -965,27 +1005,27 @@ MaybeLocal<Object> ModuleWrap::ResolveSourceCallback(
     return MaybeLocal<Object>();
   }
 
-  Utf8Value specifier_utf8(isolate, specifier);
-  std::string specifier_std(*specifier_utf8, specifier_utf8.length());
+  ModuleCacheKey cache_key =
+      ModuleCacheKey::From(context, specifier, import_attributes);
 
   ModuleWrap* dependent = GetFromModule(env, referrer);
   if (dependent == nullptr) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is from invalid module", specifier_std);
+        env, "request for '%s' is from invalid module", cache_key.specifier);
     return MaybeLocal<Object>();
   }
 
-  if (dependent->resolve_cache_.count(specifier_std) != 1) {
+  if (dependent->resolve_cache_.count(cache_key) != 1) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is not in cache", specifier_std);
+        env, "request for '%s' is not in cache", cache_key.specifier);
     return MaybeLocal<Object>();
   }
 
   Local<Object> module_object =
-      dependent->resolve_cache_[specifier_std].Get(isolate);
+      dependent->resolve_cache_[cache_key].Get(isolate);
   if (module_object.IsEmpty() || !module_object->IsObject()) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' did not return an object", specifier_std);
+        env, "request for '%s' did not return an object", cache_key.specifier);
     return MaybeLocal<Object>();
   }
 

src/module_wrap.h

@@ -38,6 +38,57 @@ enum ModulePhase : int {
   kEvaluationPhase = 2,
 };
 
+/**
+ * ModuleCacheKey is used to uniquely identify a module request
+ * in the module cache. It is a composition of the module specifier
+ * and the import attributes. ModuleImportPhase is not included
+ * in the key.
+ */
+struct ModuleCacheKey : public MemoryRetainer {
+  using ImportAttributeVector =
+      std::vector<std::pair<std::string, std::string>>;
+
+  std::string specifier;
+  ImportAttributeVector import_attributes;
+  // A hash of the specifier, and import attributes.
+  // This does not guarantee uniqueness, but is used to reduce
+  // the number of comparisons needed when checking for equality.
+  std::size_t hash;
+
+  SET_MEMORY_INFO_NAME(ModuleCacheKey)
+  SET_SELF_SIZE(ModuleCacheKey)
+  void MemoryInfo(MemoryTracker* tracker) const override;
+
+  template <int elements_per_attribute = 3>
+  static ModuleCacheKey From(v8::Local<v8::Context> context,
+                             v8::Local<v8::String> specifier,
+                             v8::Local<v8::FixedArray> import_attributes);
+  static ModuleCacheKey From(v8::Local<v8::Context> context,
+                             v8::Local<v8::ModuleRequest> v8_request);
+
+  struct Hash {
+    std::size_t operator()(const ModuleCacheKey& request) const {
+      return request.hash;
+    }
+  };
+
+  // Equality operator for ModuleCacheKey.
+  bool operator==(const ModuleCacheKey& other) const {
+    // Hash does not provide uniqueness guarantee, so ignore it.
+    return specifier == other.specifier &&
+           import_attributes == other.import_attributes;
+  }
+
+ private:
+  // Use public ModuleCacheKey::From to create instances.
+  ModuleCacheKey(std::string specifier,
+                 ImportAttributeVector import_attributes,
+                 std::size_t hash)
+      : specifier(specifier),
+        import_attributes(import_attributes),
+        hash(hash) {}
+};
+
 class ModuleWrap : public BaseObject {
  public:
   enum InternalFields {
@@ -149,7 +200,10 @@ class ModuleWrap : public BaseObject {
   static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);
 
   v8::Global<v8::Module> module_;
-  std::unordered_map<std::string, v8::Global<v8::Object>> resolve_cache_;
+  std::unordered_map<ModuleCacheKey,
+                     v8::Global<v8::Object>,
+                     ModuleCacheKey::Hash>
+      resolve_cache_;
   contextify::ContextifyContext* contextify_context_ = nullptr;
   bool synthetic_ = false;
   int module_hash_;

test/es-module/test-esm-import-attributes-identity.mjs

@@ -0,0 +1,59 @@
+import '../common/index.mjs';
+import assert from 'node:assert';
+import Module from 'node:module';
+
+// This test verifies that importing two modules with different import
+// attributes should result in different module instances, if the module loader
+// resolves to module instances.
+//
+// For example,
+// ```mjs
+//   import * as secret1 from '../primitive-42.json' with { type: 'json' };
+//   import * as secret2 from '../primitive-42.json';
+// ```
+// should result in two different module instances, if the second import
+// is been evaluated as a CommonJS module.
+//
+// ECMA-262 requires that in `HostLoadImportedModule`, if the operation is called
+// multiple times with two (referrer, moduleRequest) pairs, it should return the same
+// result. But if the moduleRequest is different, it should return a different,
+// and the module loader resolves to different module instances, it should return
+// different module instances.
+// Refs: https://tc39.es/ecma262/#sec-HostLoadImportedModule
+
+const kJsonModuleSpecifier = '../primitive-42.json';
+const fixtureUrl = new URL('../fixtures/primitive-42.json', import.meta.url).href;
+
+Module.registerHooks({
+  resolve: (specifier, context, nextResolve) => {
+    if (kJsonModuleSpecifier !== specifier) {
+      return nextResolve(specifier, context);
+    }
+    if (context.importAttributes.type === 'json') {
+      return {
+        url: fixtureUrl,
+        // Return a new importAttributes object to ensure
+        // that the module loader treats this as a same module request.
+        importAttributes: {
+          type: 'json',
+        },
+        shortCircuit: true,
+        format: 'json',
+      };
+    }
+    return {
+      url: fixtureUrl,
+      // Return a new importAttributes object to ensure
+      // that the module loader treats this as a same module request.
+      importAttributes: {},
+      shortCircuit: true,
+      format: 'module',
+    };
+  },
+});
+
+const { secretModule, secretJson, secretJson2 } = await import('../fixtures/es-modules/json-modules-attributes.mjs');
+assert.notStrictEqual(secretModule, secretJson);
+assert.strictEqual(secretJson, secretJson2);
+assert.strictEqual(secretJson.default, 42);
+assert.strictEqual(secretModule.default, undefined);

test/es-module/test-esm-json.mjs

@@ -16,6 +16,11 @@ describe('ESM: importing JSON', () => {
     assert.strictEqual(secret.ofLife, 42);
   });
 
+  it('should load JSON with import calls', async () => {
+    const module = await import('../fixtures/experimental.json', { with: { type: 'json' } });
+    assert.strictEqual(module.default.ofLife, 42);
+  });
+
   it('should not print an experimental warning', async () => {
     const { code, signal, stderr } = await spawnPromisified(execPath, [
       fixtures.path('/es-modules/json-modules.mjs'),

test/fixtures/es-module-loaders/hooks-input.mjs

@@ -30,6 +30,8 @@ export async function resolve(specifier, context, next) {
     assert.deepStrictEqual(context.importAttributes, {
       type: 'json',
     });
+  } else {
+    throw new Error(`Unexpected resolve call: ${specifier}`);
   }
 
   // Ensure `context` has all and only the properties it's supposed to

test/fixtures/es-modules/json-modules-attributes.mjs

@@ -0,0 +1,5 @@
+import * as secretJson from '../primitive-42.json' with { type: 'json' };
+import * as secretModule from '../primitive-42.json';
+import * as secretJson2 from '../primitive-42.json' with { type: 'json' };
+
+export { secretModule, secretJson, secretJson2 };

test/fixtures/primitive-42.json

@@ -0,0 +1 @@
+42

test/parallel/test-internal-module-wrap.js

@@ -14,7 +14,7 @@ const bar = new ModuleWrap('bar', undefined, 'export const five = 5', 0, 0);
   assert.strictEqual(moduleRequests.length, 1);
   assert.strictEqual(moduleRequests[0].specifier, 'bar');
 
-  foo.link(['bar'], [bar]);
+  foo.link([bar]);
   foo.instantiate();
 
   assert.strictEqual(await foo.evaluate(-1, false), undefined);