diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 74d6ac12cdd800..448334f47105ab 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -172,8 +172,7 @@ class ModuleJob extends ModuleJobBase { const dependencyJobs = Array(moduleRequests.length); ObjectSetPrototypeOf(dependencyJobs, 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); // Iterate with index to avoid calling into userspace with `Symbol.iterator`. for (let idx = 0; idx < moduleRequests.length; idx++) { @@ -189,11 +188,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 dependencyJobs; } @@ -386,18 +384,16 @@ class ModuleJobSync extends ModuleJobBase { 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 jobs = Array(moduleRequests.length); for (let i = 0; i < moduleRequests.length; ++i) { const { specifier, attributes } = moduleRequests[i]; - const job = this.#loader.getModuleJobForRequire(specifier, url, attributes); - specifiers[i] = specifier; + const job = this.#loader.getModuleJobForRequire(specifier, this.url, attributes); modules[i] = job.module; jobs[i] = job; } - this.module.link(specifiers, modules); + this.module.link(modules); this.linked = jobs; } 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 e2319e1694542e..266e019be9c3da 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -134,23 +134,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, @@ -301,6 +288,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, @@ -314,8 +314,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++) { @@ -342,12 +341,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 c52e20d7429426..2bb7eea7aec571 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -62,6 +62,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, @@ -493,7 +538,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(); @@ -502,33 +547,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); } } @@ -872,27 +912,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(); } diff --git a/src/module_wrap.h b/src/module_wrap.h index 9363ce73e51cde..ef0dcc0f6d1643 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -33,6 +33,56 @@ enum HostDefinedOptions : int { kLength = 9, }; +/** + * 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. + */ +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 { @@ -134,7 +184,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);