Skip to content

Commit 2237142

Browse files
legendecaslegendecas
authored
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: #58886 Backport-PR-URL: #60000 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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
1 parent 99ea08d commit 2237142

File tree

10 files changed

+209
-50
lines changed

10 files changed

+209
-50
lines changed

‎lib/internal/modules/esm/module_job.js‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,7 @@ class ModuleJob extends ModuleJobBase {
172172
const dependencyJobs = Array(moduleRequests.length);
173173
ObjectSetPrototypeOf(dependencyJobs, null);
174174

175-
// Specifiers should be aligned with the moduleRequests array in order.
176-
const specifiers = Array(moduleRequests.length);
175+
// Modules should be aligned with the moduleRequests array in order.
177176
const modulePromises = Array(moduleRequests.length);
178177
// Iterate with index to avoid calling into userspace with `Symbol.iterator`.
179178
for (let idx = 0; idx < moduleRequests.length; idx++) {
@@ -189,11 +188,10 @@ class ModuleJob extends ModuleJobBase {
189188
return job.modulePromise;
190189
});
191190
modulePromises[idx] = modulePromise;
192-
specifiers[idx] = specifier;
193191
}
194192

195193
const modules = await SafePromiseAllReturnArrayLike(modulePromises);
196-
this.module.link(specifiers, modules);
194+
this.module.link(modules);
197195

198196
return dependencyJobs;
199197
}
@@ -386,18 +384,16 @@ class ModuleJobSync extends ModuleJobBase {
386384

387385
try {
388386
const moduleRequests = this.module.getModuleRequests();
389-
// Specifiers should be aligned with the moduleRequests array in order.
390-
const specifiers = Array(moduleRequests.length);
387+
// Modules should be aligned with the moduleRequests array in order.
391388
const modules = Array(moduleRequests.length);
392389
const jobs = Array(moduleRequests.length);
393390
for (let i = 0; i < moduleRequests.length; ++i) {
394391
const { specifier, attributes } = moduleRequests[i];
395-
const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
396-
specifiers[i] = specifier;
392+
const job = this.#loader.getModuleJobForRequire(specifier, this.url, attributes);
397393
modules[i] = job.module;
398394
jobs[i] = job;
399395
}
400-
this.module.link(specifiers, modules);
396+
this.module.link(modules);
401397
this.linked = jobs;
402398
} finally {
403399
// Restore it - if it succeeds, we'll reset in the caller; Otherwise it's

‎lib/internal/vm/module.js‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -134,23 +134,10 @@ class Module {
134134
});
135135
}
136136

137-
let registry = { __proto__: null };
138137
if (sourceText !== undefined) {
139138
this[kWrap] = new ModuleWrap(identifier, context, sourceText,
140139
options.lineOffset, options.columnOffset,
141140
options.cachedData);
142-
registry = {
143-
__proto__: null,
144-
initializeImportMeta: options.initializeImportMeta,
145-
importModuleDynamically: options.importModuleDynamically ?
146-
importModuleDynamicallyWrap(options.importModuleDynamically) :
147-
undefined,
148-
};
149-
// This will take precedence over the referrer as the object being
150-
// passed into the callbacks.
151-
registry.callbackReferrer = this;
152-
const { registerModule } = require('internal/modules/esm/utils');
153-
registerModule(this[kWrap], registry);
154141
} else {
155142
assert(syntheticEvaluationSteps);
156143
this[kWrap] = new ModuleWrap(identifier, context,
@@ -301,6 +288,19 @@ class SourceTextModule extends Module {
301288
importModuleDynamically,
302289
});
303290

291+
const registry = {
292+
__proto__: null,
293+
initializeImportMeta: options.initializeImportMeta,
294+
importModuleDynamically: options.importModuleDynamically ?
295+
importModuleDynamicallyWrap(options.importModuleDynamically) :
296+
undefined,
297+
};
298+
// This will take precedence over the referrer as the object being
299+
// passed into the callbacks.
300+
registry.callbackReferrer = this;
301+
const { registerModule } = require('internal/modules/esm/utils');
302+
registerModule(this[kWrap], registry);
303+
304304
this.#moduleRequests = ObjectFreeze(ArrayPrototypeMap(this[kWrap].getModuleRequests(), (request) => {
305305
return ObjectFreeze({
306306
__proto__: null,
@@ -314,8 +314,7 @@ class SourceTextModule extends Module {
314314
this.#statusOverride = 'linking';
315315

316316
// Iterates the module requests and links with the linker.
317-
// Specifiers should be aligned with the moduleRequests array in order.
318-
const specifiers = Array(this.#moduleRequests.length);
317+
// Modules should be aligned with the moduleRequests array in order.
319318
const modulePromises = Array(this.#moduleRequests.length);
320319
// Iterates with index to avoid calling into userspace with `Symbol.iterator`.
321320
for (let idx = 0; idx < this.#moduleRequests.length; idx++) {
@@ -342,12 +341,11 @@ class SourceTextModule extends Module {
342341
return module[kWrap];
343342
});
344343
modulePromises[idx] = modulePromise;
345-
specifiers[idx] = specifier;
346344
}
347345

348346
try {
349347
const modules = await SafePromiseAllReturnArrayLike(modulePromises);
350-
this[kWrap].link(specifiers, modules);
348+
this[kWrap].link(modules);
351349
} catch (e) {
352350
this.#error = e;
353351
throw e;

‎src/module_wrap.cc‎

Lines changed: 62 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,51 @@ using v8::UnboundModuleScript;
6262
using v8::Undefined;
6363
using v8::Value;
6464

65+
void ModuleCacheKey::MemoryInfo(MemoryTracker* tracker) const {
66+
tracker->TrackField("specifier", specifier);
67+
tracker->TrackField("import_attributes", import_attributes);
68+
}
69+
70+
template <int elements_per_attribute>
71+
ModuleCacheKey ModuleCacheKey::From(Local<Context> context,
72+
Local<String> specifier,
73+
Local<FixedArray> import_attributes) {
74+
CHECK_EQ(import_attributes->Length() % elements_per_attribute, 0);
75+
Isolate* isolate = context->GetIsolate();
76+
std::size_t h1 = specifier->GetIdentityHash();
77+
size_t num_attributes = import_attributes->Length() / elements_per_attribute;
78+
ImportAttributeVector attributes;
79+
attributes.reserve(num_attributes);
80+
81+
std::size_t h2 = 0;
82+
83+
for (int i = 0; i < import_attributes->Length();
84+
i += elements_per_attribute) {
85+
Local<String> v8_key = import_attributes->Get(context, i).As<String>();
86+
Local<String> v8_value =
87+
import_attributes->Get(context, i + 1).As<String>();
88+
Utf8Value key_utf8(isolate, v8_key);
89+
Utf8Value value_utf8(isolate, v8_value);
90+
91+
attributes.emplace_back(key_utf8.ToString(), value_utf8.ToString());
92+
h2 ^= v8_key->GetIdentityHash();
93+
h2 ^= v8_value->GetIdentityHash();
94+
}
95+
96+
// Combine the hashes using a simple XOR and bit shift to reduce
97+
// collisions. Note that the hash does not guarantee uniqueness.
98+
std::size_t hash = h1 ^ (h2 << 1);
99+
100+
Utf8Value utf8_specifier(isolate, specifier);
101+
return ModuleCacheKey{utf8_specifier.ToString(), attributes, hash};
102+
}
103+
104+
ModuleCacheKey ModuleCacheKey::From(Local<Context> context,
105+
Local<ModuleRequest> v8_request) {
106+
return From(
107+
context, v8_request->GetSpecifier(), v8_request->GetImportAttributes());
108+
}
109+
65110
ModuleWrap::ModuleWrap(Realm* realm,
66111
Local<Object> object,
67112
Local<Module> module,
@@ -493,7 +538,7 @@ void ModuleWrap::GetModuleRequests(const FunctionCallbackInfo<Value>& args) {
493538
realm, isolate, module->GetModuleRequests()));
494539
}
495540

496-
// moduleWrap.link(specifiers, moduleWraps)
541+
// moduleWrap.link(moduleWraps)
497542
void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
498543
Realm* realm = Realm::GetCurrent(args);
499544
Isolate* isolate = args.GetIsolate();
@@ -502,33 +547,28 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
502547
ModuleWrap* dependent;
503548
ASSIGN_OR_RETURN_UNWRAP(&dependent, args.This());
504549

505-
CHECK_EQ(args.Length(), 2);
550+
CHECK_EQ(args.Length(), 1);
506551

507-
Local<Array> specifiers = args[0].As<Array>();
508-
Local<Array> modules = args[1].As<Array>();
509-
CHECK_EQ(specifiers->Length(), modules->Length());
552+
Local<FixedArray> requests =
553+
dependent->module_.Get(isolate)->GetModuleRequests();
554+
Local<Array> modules = args[0].As<Array>();
555+
CHECK_EQ(modules->Length(), static_cast<uint32_t>(requests->Length()));
510556

511-
std::vector<Global<Value>> specifiers_buffer;
512-
if (FromV8Array(context, specifiers, &specifiers_buffer).IsNothing()) {
513-
return;
514-
}
515557
std::vector<Global<Value>> modules_buffer;
516558
if (FromV8Array(context, modules, &modules_buffer).IsNothing()) {
517559
return;
518560
}
519561

520-
for (uint32_t i = 0; i < specifiers->Length(); i++) {
521-
Local<String> specifier_str =
522-
specifiers_buffer[i].Get(isolate).As<String>();
562+
for (uint32_t i = 0; i < modules_buffer.size(); i++) {
523563
Local<Object> module_object = modules_buffer[i].Get(isolate).As<Object>();
524564

525565
CHECK(
526566
realm->isolate_data()->module_wrap_constructor_template()->HasInstance(
527567
module_object));
528568

529-
Utf8Value specifier(isolate, specifier_str);
530-
dependent->resolve_cache_[specifier.ToString()].Reset(isolate,
531-
module_object);
569+
ModuleCacheKey module_cache_key = ModuleCacheKey::From(
570+
context, requests->Get(context, i).As<ModuleRequest>());
571+
dependent->resolve_cache_[module_cache_key].Reset(isolate, module_object);
532572
}
533573
}
534574

@@ -872,27 +912,27 @@ MaybeLocal<Module> ModuleWrap::ResolveModuleCallback(
872912
return MaybeLocal<Module>();
873913
}
874914

875-
Utf8Value specifier_utf8(isolate, specifier);
876-
std::string specifier_std(*specifier_utf8, specifier_utf8.length());
915+
ModuleCacheKey cache_key =
916+
ModuleCacheKey::From(context, specifier, import_attributes);
877917

878918
ModuleWrap* dependent = GetFromModule(env, referrer);
879919
if (dependent == nullptr) {
880920
THROW_ERR_VM_MODULE_LINK_FAILURE(
881-
env, "request for '%s' is from invalid module", specifier_std);
921+
env, "request for '%s' is from invalid module", cache_key.specifier);
882922
return MaybeLocal<Module>();
883923
}
884924

885-
if (dependent->resolve_cache_.count(specifier_std) != 1) {
925+
if (dependent->resolve_cache_.count(cache_key) != 1) {
886926
THROW_ERR_VM_MODULE_LINK_FAILURE(
887-
env, "request for '%s' is not in cache", specifier_std);
927+
env, "request for '%s' is not in cache", cache_key.specifier);
888928
return MaybeLocal<Module>();
889929
}
890930

891931
Local<Object> module_object =
892-
dependent->resolve_cache_[specifier_std].Get(isolate);
932+
dependent->resolve_cache_[cache_key].Get(isolate);
893933
if (module_object.IsEmpty() || !module_object->IsObject()) {
894934
THROW_ERR_VM_MODULE_LINK_FAILURE(
895-
env, "request for '%s' did not return an object", specifier_std);
935+
env, "request for '%s' did not return an object", cache_key.specifier);
896936
return MaybeLocal<Module>();
897937
}
898938

‎src/module_wrap.h‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,56 @@ enum HostDefinedOptions : int {
3333
kLength = 9,
3434
};
3535

36+
/**
37+
* ModuleCacheKey is used to uniquely identify a module request
38+
* in the module cache. It is a composition of the module specifier
39+
* and the import attributes.
40+
*/
41+
struct ModuleCacheKey : public MemoryRetainer {
42+
using ImportAttributeVector =
43+
std::vector<std::pair<std::string, std::string>>;
44+
45+
std::string specifier;
46+
ImportAttributeVector import_attributes;
47+
// A hash of the specifier, and import attributes.
48+
// This does not guarantee uniqueness, but is used to reduce
49+
// the number of comparisons needed when checking for equality.
50+
std::size_t hash;
51+
52+
SET_MEMORY_INFO_NAME(ModuleCacheKey)
53+
SET_SELF_SIZE(ModuleCacheKey)
54+
void MemoryInfo(MemoryTracker* tracker) const override;
55+
56+
template <int elements_per_attribute = 3>
57+
static ModuleCacheKey From(v8::Local<v8::Context> context,
58+
v8::Local<v8::String> specifier,
59+
v8::Local<v8::FixedArray> import_attributes);
60+
static ModuleCacheKey From(v8::Local<v8::Context> context,
61+
v8::Local<v8::ModuleRequest> v8_request);
62+
63+
struct Hash {
64+
std::size_t operator()(const ModuleCacheKey& request) const {
65+
return request.hash;
66+
}
67+
};
68+
69+
// Equality operator for ModuleCacheKey.
70+
bool operator==(const ModuleCacheKey& other) const {
71+
// Hash does not provide uniqueness guarantee, so ignore it.
72+
return specifier == other.specifier &&
73+
import_attributes == other.import_attributes;
74+
}
75+
76+
private:
77+
// Use public ModuleCacheKey::From to create instances.
78+
ModuleCacheKey(std::string specifier,
79+
ImportAttributeVector import_attributes,
80+
std::size_t hash)
81+
: specifier(specifier),
82+
import_attributes(import_attributes),
83+
hash(hash) {}
84+
};
85+
3686
class ModuleWrap : public BaseObject {
3787
public:
3888
enum InternalFields {
@@ -134,7 +184,10 @@ class ModuleWrap : public BaseObject {
134184
static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);
135185

136186
v8::Global<v8::Module> module_;
137-
std::unordered_map<std::string, v8::Global<v8::Object>> resolve_cache_;
187+
std::unordered_map<ModuleCacheKey,
188+
v8::Global<v8::Object>,
189+
ModuleCacheKey::Hash>
190+
resolve_cache_;
138191
contextify::ContextifyContext* contextify_context_ = nullptr;
139192
bool synthetic_ = false;
140193
int module_hash_;

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

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
import '../common/index.mjs';
2+
import assert from 'node:assert';
3+
import Module from 'node:module';
4+
5+
// This test verifies that importing two modules with different import
6+
// attributes should result in different module instances, if the module loader
7+
// resolves to module instances.
8+
//
9+
// For example,
10+
// ```mjs
11+
// import * as secret1 from '../primitive-42.json' with { type: 'json' };
12+
// import * as secret2 from '../primitive-42.json';
13+
// ```
14+
// should result in two different module instances, if the second import
15+
// is been evaluated as a CommonJS module.
16+
//
17+
// ECMA-262 requires that in `HostLoadImportedModule`, if the operation is called
18+
// multiple times with two (referrer, moduleRequest) pairs, it should return the same
19+
// result. But if the moduleRequest is different, it should return a different,
20+
// and the module loader resolves to different module instances, it should return
21+
// different module instances.
22+
// Refs: https://tc39.es/ecma262/#sec-HostLoadImportedModule
23+
24+
const kJsonModuleSpecifier = '../primitive-42.json';
25+
const fixtureUrl = new URL('../fixtures/primitive-42.json', import.meta.url).href;
26+
27+
Module.registerHooks({
28+
resolve: (specifier, context, nextResolve) => {
29+
if (kJsonModuleSpecifier !== specifier) {
30+
return nextResolve(specifier, context);
31+
}
32+
if (context.importAttributes.type === 'json') {
33+
return {
34+
url: fixtureUrl,
35+
// Return a new importAttributes object to ensure
36+
// that the module loader treats this as a same module request.
37+
importAttributes: {
38+
type: 'json',
39+
},
40+
shortCircuit: true,
41+
format: 'json',
42+
};
43+
}
44+
return {
45+
url: fixtureUrl,
46+
// Return a new importAttributes object to ensure
47+
// that the module loader treats this as a same module request.
48+
importAttributes: {},
49+
shortCircuit: true,
50+
format: 'module',
51+
};
52+
},
53+
});
54+
55+
const { secretModule, secretJson, secretJson2 } = await import('../fixtures/es-modules/json-modules-attributes.mjs');
56+
assert.notStrictEqual(secretModule, secretJson);
57+
assert.strictEqual(secretJson, secretJson2);
58+
assert.strictEqual(secretJson.default, 42);
59+
assert.strictEqual(secretModule.default, undefined);

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@ describe('ESM: importing JSON', () => {
1616
assert.strictEqual(secret.ofLife, 42);
1717
});
1818

19+
it('should load JSON with import calls', async () => {
20+
const module = await import('../fixtures/experimental.json', { with: { type: 'json' } });
21+
assert.strictEqual(module.default.ofLife, 42);
22+
});
23+
1924
it('should not print an experimental warning', async () => {
2025
const { code, signal, stderr } = await spawnPromisified(execPath, [
2126
fixtures.path('/es-modules/json-modules.mjs'),

0 commit comments

Comments
 (0)