From ed81a847d844b7fe3120f090bc05ec430918d459 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Sun, 29 Jun 2025 14:15:19 +0100 Subject: [PATCH] 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. --- lib/internal/modules/esm/module_job.js | 12 +-- lib/internal/vm/module.js | 32 +++--- src/module_wrap.cc | 98 +++++++++++++------ src/module_wrap.h | 56 ++++++++++- .../test-esm-import-attributes-identity.mjs | 59 +++++++++++ test/es-module/test-esm-json.mjs | 5 + .../es-module-loaders/hooks-input.mjs | 2 + .../es-modules/json-modules-attributes.mjs | 5 + test/fixtures/primitive-42.json | 1 + test/parallel/test-internal-module-wrap.js | 2 +- 10 files changed, 216 insertions(+), 56 deletions(-) create mode 100644 test/es-module/test-esm-import-attributes-identity.mjs create mode 100644 test/fixtures/es-modules/json-modules-attributes.mjs create mode 100644 test/fixtures/primitive-42.json diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index a7a65e50a1607b..2dd9ce80c7618a 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/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 diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index e6f3737645434f..aba5c2872daa39 100644 --- a/lib/internal/vm/module.js +++ b/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; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index b38134f9a8f768..3d27b878fd2220 100644 --- a/src/module_wrap.cc +++ b/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 +ModuleCacheKey ModuleCacheKey::From(Local context, + Local specifier, + Local 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 v8_key = import_attributes->Get(context, i).As(); + Local v8_value = + import_attributes->Get(context, i + 1).As(); + 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, + Local v8_request) { + return From( + context, v8_request->GetSpecifier(), v8_request->GetImportAttributes()); +} + ModuleWrap::ModuleWrap(Realm* realm, Local object, Local module, @@ -509,7 +554,7 @@ void ModuleWrap::GetModuleRequests(const FunctionCallbackInfo& args) { realm, isolate, module->GetModuleRequests())); } -// moduleWrap.link(specifiers, moduleWraps) +// moduleWrap.link(moduleWraps) void ModuleWrap::Link(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); Isolate* isolate = args.GetIsolate(); @@ -518,33 +563,28 @@ void ModuleWrap::Link(const FunctionCallbackInfo& args) { ModuleWrap* dependent; ASSIGN_OR_RETURN_UNWRAP(&dependent, args.This()); - CHECK_EQ(args.Length(), 2); + CHECK_EQ(args.Length(), 1); - Local specifiers = args[0].As(); - Local modules = args[1].As(); - CHECK_EQ(specifiers->Length(), modules->Length()); + Local requests = + dependent->module_.Get(isolate)->GetModuleRequests(); + Local modules = args[0].As(); + CHECK_EQ(modules->Length(), static_cast(requests->Length())); - std::vector> specifiers_buffer; - if (FromV8Array(context, specifiers, &specifiers_buffer).IsNothing()) { - return; - } std::vector> modules_buffer; if (FromV8Array(context, modules, &modules_buffer).IsNothing()) { return; } - for (uint32_t i = 0; i < specifiers->Length(); i++) { - Local specifier_str = - specifiers_buffer[i].Get(isolate).As(); + for (uint32_t i = 0; i < modules_buffer.size(); i++) { Local module_object = modules_buffer[i].Get(isolate).As(); 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()); + dependent->resolve_cache_[module_cache_key].Reset(isolate, module_object); } } @@ -924,27 +964,27 @@ MaybeLocal ModuleWrap::ResolveModuleCallback( return MaybeLocal(); } - 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(); } - 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(); } Local 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(); } @@ -965,27 +1005,27 @@ MaybeLocal ModuleWrap::ResolveSourceCallback( return MaybeLocal(); } - 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(); } - 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(); } Local 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(); } diff --git a/src/module_wrap.h b/src/module_wrap.h index 3a79bd4ba61e72..dbab9946c7210b 100644 --- a/src/module_wrap.h +++ b/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::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 + static ModuleCacheKey From(v8::Local context, + v8::Local specifier, + v8::Local import_attributes); + static ModuleCacheKey From(v8::Local context, + v8::Local 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::Global module_; - std::unordered_map> resolve_cache_; + std::unordered_map, + ModuleCacheKey::Hash> + resolve_cache_; contextify::ContextifyContext* contextify_context_ = nullptr; bool synthetic_ = false; int module_hash_; diff --git a/test/es-module/test-esm-import-attributes-identity.mjs b/test/es-module/test-esm-import-attributes-identity.mjs new file mode 100644 index 00000000000000..0f5b98ff046645 --- /dev/null +++ b/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); diff --git a/test/es-module/test-esm-json.mjs b/test/es-module/test-esm-json.mjs index 083194ab4237b0..b85767803531eb 100644 --- a/test/es-module/test-esm-json.mjs +++ b/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'), diff --git a/test/fixtures/es-module-loaders/hooks-input.mjs b/test/fixtures/es-module-loaders/hooks-input.mjs index 854b8e619281e4..e4eb159b398797 100644 --- a/test/fixtures/es-module-loaders/hooks-input.mjs +++ b/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 diff --git a/test/fixtures/es-modules/json-modules-attributes.mjs b/test/fixtures/es-modules/json-modules-attributes.mjs new file mode 100644 index 00000000000000..d0eb79f4f5dd72 --- /dev/null +++ b/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 }; diff --git a/test/fixtures/primitive-42.json b/test/fixtures/primitive-42.json new file mode 100644 index 00000000000000..d81cc0710eb6cf --- /dev/null +++ b/test/fixtures/primitive-42.json @@ -0,0 +1 @@ +42 diff --git a/test/parallel/test-internal-module-wrap.js b/test/parallel/test-internal-module-wrap.js index 3839338bc2da98..760d717c69ef8c 100644 --- a/test/parallel/test-internal-module-wrap.js +++ b/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);