Skip to content

ModuleMap: key the module registry on the host-defined import attribute string #258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ModuleMap: key the module registry on the host-defined import attribute string #258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Source/JavaScriptCore/parser/ModuleAnalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ ModuleAnalyzer::ModuleAnalyzer(JSGlobalObject* globalObject, const Identifier& m

void ModuleAnalyzer::appendRequestedModule(const Identifier& specifier, RefPtr<ScriptFetchParameters>&& attributes, AbstractModuleRecord::ModulePhase phase)
{
ModuleMapKey key { specifier.impl(), attributes ? attributes->type() : ScriptFetchParameters::Type::JavaScript };
ScriptFetchParameters::Type type = attributes ? attributes->type() : ScriptFetchParameters::Type::JavaScript;
ModuleMapKey key = makeModuleMapKey(specifier.impl(), type, attributes.get());
if (m_requestedModules[phase].add(key).isNewEntry)
moduleRecord()->appendRequestedModule(specifier, WTF::move(attributes), phase);
}
Expand Down
41 changes: 34 additions & 7 deletions Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ ScriptFetchParameters::Type AbstractModuleRecord::ModuleRequest::type(ScriptFetc
return fallback;
}

ModuleMapKey AbstractModuleRecord::ModuleRequest::moduleMapKey() const
{
return makeModuleMapKey(m_specifier.impl(), type(), m_attributes.get());
}

AbstractModuleRecord::LoadedModuleRequest::LoadedModuleRequest(VM& vm, ModuleRequest moduleRequest, AbstractModuleRecord* loadedModule, JSCell* owner)
: ModuleRequest(WTF::move(moduleRequest))
, m_module(vm, owner, loadedModule)
Expand All @@ -141,8 +146,15 @@ bool AbstractModuleRecord::ModuleRequest::operator==(const ModuleRequest& other)
if (!!m_attributes != !!other.m_attributes)
return false;

if (m_attributes)
return m_attributes->type() == other.m_attributes->type();
if (m_attributes) {
if (m_attributes->type() != other.m_attributes->type())
return false;
#if USE(BUN_JSC_ADDITIONS)
// ModuleRequestsEqual compares the whole attribute list, and for Bun's
// host-defined types the `type` attribute string is the discriminant.
return m_attributes->hostDefinedImportType() == other.m_attributes->hostDefinedImportType();
#endif
}

return true;
}
Expand Down Expand Up @@ -221,7 +233,23 @@ auto AbstractModuleRecord::Resolution::ambiguous() -> Resolution

AbstractModuleRecord* AbstractModuleRecord::hostResolveImportedModule(JSGlobalObject*, const Identifier& moduleName, ScriptFetchParameters::Type moduleRequestType)
{
if (auto iter = m_loadedModules.find(ModuleMapKey { moduleName.impl(), moduleRequestType }); iter != m_loadedModules.end())
#if USE(BUN_JSC_ADDITIONS)
// Import and export entries record only the attribute Type, not the host-defined
// attribute string that completes a HostDefined ModuleMapKey, so recover the full
// key from this record's own requested modules. They are in source order, so when
// one specifier is requested with several host-defined types the binding resolves
// to the first of them, which is what the old specifier-keyed lookup resolved to.
if (moduleRequestType == ScriptFetchParameters::Type::HostDefined) {
for (const ModuleRequest& request : m_requestedModules) {
if (request.m_specifier.impl() != moduleName.impl() || request.type() != moduleRequestType)
continue;
if (auto iter = m_loadedModules.find(request.moduleMapKey()); iter != m_loadedModules.end())
return iter->value.m_module.get();
}
return nullptr;
}
#endif
if (auto iter = m_loadedModules.find(makeModuleMapKey(moduleName.impl(), moduleRequestType, nullptr)); iter != m_loadedModules.end())
return iter->value.m_module.get();
return nullptr;
}
Expand All @@ -237,11 +265,10 @@ void AbstractModuleRecord::setImportedModule(JSGlobalObject* globalObject, const
// getImportedModule(), so records that are linked outside the loader (Bun's
// node:vm SourceTextModule) need this map populated too. Reuse the original
// ModuleRequest (specifier + attributes) so a `with { type: "json" }` /
// HostDefined import lands in the same (specifier, type) bucket that
// getImportedModule()'s typed lookup will use.
// HostDefined import lands in the same bucket that getImportedModule()'s
// typed lookup will use.
Locker locker { cellLock() };
ModuleMapKey key { request.m_specifier.impl(), request.type() };
m_loadedModules.set(key, LoadedModuleRequest { vm, request, record, this });
m_loadedModules.set(request.moduleMapKey(), LoadedModuleRequest { vm, request, record, this });
Comment thread
claude[bot] marked this conversation as resolved.
}

auto AbstractModuleRecord::resolveImport(JSGlobalObject* globalObject, const Identifier& localName) -> Resolution
Expand Down
5 changes: 5 additions & 0 deletions Source/JavaScriptCore/runtime/AbstractModuleRecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ class AbstractModuleRecord : public JSInternalFieldObjectImpl<2> {
ModulePhase m_phase { ModulePhase::Evaluation };

ScriptFetchParameters::Type type(ScriptFetchParameters::Type fallback = ScriptFetchParameters::Type::JavaScript) const;
// The (specifier, type, host-defined attribute string) key this request
// resolves to in a ModuleMap. Always use this instead of hand-building a
// ModuleMapKey from m_specifier + type() so the HostDefined attribute
// string is never dropped.
ModuleMapKey moduleMapKey() const;
bool operator==(const ModuleRequest&) const;
};

Expand Down
8 changes: 4 additions & 4 deletions Source/JavaScriptCore/runtime/JSMicrotask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,7 +1167,7 @@ static void moduleLoadTopSettled(JSGlobalObject* globalObject, VM& vm, ThrowScop
auto type = context->moduleRequest().type();
ScriptFetcher* scriptFetcher = context->scriptFetcher();

globalObject->moduleLoader()->provideFetch(globalObject, specifier, type, jsSourceCode);
globalObject->moduleLoader()->provideFetch(globalObject, specifier, type, context->moduleRequest().m_attributes.get(), jsSourceCode);
if (scope.exception()) {
intermediatePromise->rejectWithCaughtException(vm, scope);
return;
Expand Down Expand Up @@ -1223,7 +1223,7 @@ static void moduleLoadTopSettled(JSGlobalObject* globalObject, VM& vm, ThrowScop
// onFetchRejected logic
const Identifier& specifier = context->moduleRequest().m_specifier;
auto type = context->moduleRequest().type();
ModuleRegistryEntry* entry = globalObject->moduleLoader()->ensureRegistered(globalObject, specifier, type);
ModuleRegistryEntry* entry = globalObject->moduleLoader()->ensureRegistered(globalObject, specifier, type, context->moduleRequest().m_attributes.get());
if (scope.exception()) {
intermediatePromise->rejectWithCaughtException(vm, scope);
return;
Expand Down Expand Up @@ -1259,7 +1259,7 @@ static void moduleLoadTopRejected(JSGlobalObject* globalObject, VM& vm, ThrowSco
else {
const Identifier& specifier = context->moduleRequest().m_specifier;
auto type = context->moduleRequest().type();
ModuleRegistryEntry* entry = globalObject->moduleLoader()->ensureRegistered(globalObject, specifier, type);
ModuleRegistryEntry* entry = globalObject->moduleLoader()->ensureRegistered(globalObject, specifier, type, context->moduleRequest().m_attributes.get());
if (scope.exception()) {
resultPromise->rejectWithCaughtException(vm, scope);
return;
Expand Down Expand Up @@ -1429,7 +1429,7 @@ static void moduleLoadStoreError(JSGlobalObject* globalObject, ThrowScope& scope
JSValue errorValue = arguments[1];
const Identifier& specifier = context->moduleRequest().m_specifier;
auto type = context->moduleRequest().type();
ModuleRegistryEntry* entry = globalObject->moduleLoader()->ensureRegistered(globalObject, specifier, type);
ModuleRegistryEntry* entry = globalObject->moduleLoader()->ensureRegistered(globalObject, specifier, type, context->moduleRequest().m_attributes.get());
RETURN_IF_EXCEPTION(scope, void());
if (auto* error = dynamicDowncast<ErrorInstance>(errorValue)) {
auto failure = JSModuleLoader::getErrorInfo(globalObject, error);
Expand Down
52 changes: 31 additions & 21 deletions Source/JavaScriptCore/runtime/JSModuleLoader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,10 +297,10 @@ JSArray* JSModuleLoader::dependencyKeysIfEvaluated(JSGlobalObject* globalObject,

Identifier ident = Identifier::fromString(vm, key);

auto iter = m_moduleMap.find({ ident.impl(), ScriptFetchParameters::Type::JavaScript });
auto iter = m_moduleMap.find(makeModuleMapKey(ident.impl(), ScriptFetchParameters::Type::JavaScript, nullptr));

if (iter == m_moduleMap.end())
iter = m_moduleMap.find({ ident.impl(), ScriptFetchParameters::Type::WebAssembly });
iter = m_moduleMap.find(makeModuleMapKey(ident.impl(), ScriptFetchParameters::Type::WebAssembly, nullptr));

if (iter == m_moduleMap.end())
RELEASE_AND_RETURN(scope, nullptr);
Expand Down Expand Up @@ -334,14 +334,24 @@ JSArray* JSModuleLoader::dependencyKeysIfEvaluated(JSGlobalObject* globalObject,

void JSModuleLoader::provideFetch(JSGlobalObject* globalObject, const Identifier& key, ScriptFetchParameters::Type type, SourceCode&& sourceCode)
{
ModuleRegistryEntry* entry = ensureRegistered(globalObject, key, type);
provideFetch(globalObject, key, type, nullptr, WTF::move(sourceCode));
}

void JSModuleLoader::provideFetch(JSGlobalObject* globalObject, const Identifier& key, ScriptFetchParameters::Type type, JSSourceCode* jsSourceCode)
{
provideFetch(globalObject, key, type, nullptr, jsSourceCode);
}

void JSModuleLoader::provideFetch(JSGlobalObject* globalObject, const Identifier& key, ScriptFetchParameters::Type type, const ScriptFetchParameters* parameters, SourceCode&& sourceCode)
{
ModuleRegistryEntry* entry = ensureRegistered(globalObject, key, type, parameters);
if (entry->status() == ModuleRegistryEntry::Status::New)
entry->provideFetch(globalObject, WTF::move(sourceCode)); // can throw
}

void JSModuleLoader::provideFetch(JSGlobalObject* globalObject, const Identifier& key, ScriptFetchParameters::Type type, JSSourceCode* jsSourceCode)
void JSModuleLoader::provideFetch(JSGlobalObject* globalObject, const Identifier& key, ScriptFetchParameters::Type type, const ScriptFetchParameters* parameters, JSSourceCode* jsSourceCode)
{
ModuleRegistryEntry* entry = ensureRegistered(globalObject, key, type);
ModuleRegistryEntry* entry = ensureRegistered(globalObject, key, type, parameters);
if (entry->status() == ModuleRegistryEntry::Status::New)
entry->provideFetch(globalObject, jsSourceCode); // can throw
}
Expand All @@ -360,7 +370,7 @@ JSPromise* JSModuleLoader::loadModule(JSGlobalObject* globalObject, const Identi
RefPtr<ScriptFetchParameters> contextParameters = parameters ? parameters : ScriptFetchParameters::create(type);
#endif

if (ModuleRegistryEntry* entry = getRegisteredMayBeNull(specifier, type)) {
if (ModuleRegistryEntry* entry = getRegisteredMayBeNull(specifier, type, parameters.get())) {
JSValue error = entry->error(globalObject);
RETURN_IF_EXCEPTION(scope, nullptr);
if (error)
Expand Down Expand Up @@ -399,7 +409,7 @@ JSPromise* JSModuleLoader::linkAndEvaluateModule(JSGlobalObject* globalObject, c
auto scope = DECLARE_THROW_SCOPE(vm);

ScriptFetchParameters::Type type = parameters ? parameters->type() : ScriptFetchParameters::Type::JavaScript;
ModuleRegistryEntry* entry = ensureRegistered(globalObject, moduleKey, type);
ModuleRegistryEntry* entry = ensureRegistered(globalObject, moduleKey, type, parameters.get());
RETURN_IF_EXCEPTION(scope, nullptr);

AbstractModuleRecord* record = entry->record();
Expand Down Expand Up @@ -586,7 +596,7 @@ AbstractModuleRecord* JSModuleLoader::getImportedModule(AbstractModuleRecord* re
// https://tc39.es/ecma262/#sec-GetImportedModule

// 1. Let records be a List consisting of each LoadedModuleRequest Record r of referrer.[[LoadedModules]] such that ModuleRequestsEqual(r, request) is true.
auto iter = referrer->loadedModules().find(ModuleMapKey { request.m_specifier.impl(), request.type() });
auto iter = referrer->loadedModules().find(request.moduleMapKey());
// 2. Assert: records has exactly one element, since LoadRequestedModules has completed successfully on referrer prior to invoking this abstract operation.
ASSERT(iter != referrer->loadedModules().end());
// 3. Let record be the sole element of records.
Expand Down Expand Up @@ -614,7 +624,7 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject,
const Identifier& specifier = moduleRequest.m_specifier;
auto type = moduleRequest.type();

ModuleMapKey moduleMapKey { specifier.impl(), type };
ModuleMapKey moduleMapKey = moduleRequest.moduleMapKey();

// HostLoadImportedModule is required to be idempotent for the same
// (referrer, moduleRequest) pair. referrer.[[LoadedModules]] is that cache;
Expand All @@ -625,7 +635,7 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject,
auto& loadedModules = record ? record->loadedModules() : m_loadedModules;
if (auto iter = loadedModules.find(moduleMapKey); iter != loadedModules.end()) {
AbstractModuleRecord* loaded = iter->value.m_module.get();
ModuleRegistryEntry* loadedEntry = getRegisteredMayBeNull(loaded->moduleKey(), type);
ModuleRegistryEntry* loadedEntry = getRegisteredMayBeNull(loaded->moduleKey(), type, moduleRequest.m_attributes.get());
ASSERT(loadedEntry);
ASSERT(loadedEntry->record() == loaded);
ASSERT(loadedEntry->loadPromise());
Expand All @@ -636,7 +646,7 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject,
}

if (specifier.isSymbol())
mapEntry = getRegisteredMayBeNull(specifier, type);
mapEntry = getRegisteredMayBeNull(specifier, type, moduleRequest.m_attributes.get());

ResolutionMapKey resolutionKey { referrerKey.impl(), specifier.impl() };

Expand Down Expand Up @@ -680,7 +690,7 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject,
RELEASE_AND_RETURN(scope, promise);
}

moduleMapKey.first = resolved.impl();
std::get<0>(moduleMapKey) = resolved.impl();

if (auto iter = m_moduleMap.find(moduleMapKey); iter != m_moduleMap.end())
mapEntry = iter->value.get();
Expand Down Expand Up @@ -860,7 +870,7 @@ void JSModuleLoader::innerModuleLoading(JSGlobalObject* globalObject, ModuleGrap
// 2.d.i.2. Perform ContinueModuleLoading(state, error).
// (Not possible.)
// 2.d.ii. Else if module.[[LoadedModules]] contains a LoadedModuleRequest Record record such that ModuleRequestsEqual(record, request) is true, then
if (auto iter = module->loadedModules().find(ModuleMapKey { request.m_specifier.impl(), request.type() }); iter != module->loadedModules().end()) {
if (auto iter = module->loadedModules().find(request.moduleMapKey()); iter != module->loadedModules().end()) {
// 2.d.ii.1. Perform InnerModuleLoading(state, record.[[Module]]).
AbstractModuleRecord* loaded = iter->value.m_module.get();
if (state->containsVisited(loaded))
Expand All @@ -886,7 +896,7 @@ void JSModuleLoader::innerModuleLoading(JSGlobalObject* globalObject, ModuleGrap
// there is no need to attach a ModuleGraphLoadingError reaction, so we skip it.
bool needsErrorReaction = module->loadedModules().size() == loadedModulesCountBefore;
ASSERT(module->loadedModules().size() <= loadedModulesCountBefore + 1);
ASSERT(needsErrorReaction != module->loadedModules().contains(ModuleMapKey { request.m_specifier.impl(), request.type() }));
ASSERT(needsErrorReaction != module->loadedModules().contains(request.moduleMapKey()));
if (needsErrorReaction)
promise->performPromiseThenWithInternalMicrotask(vm, InternalMicrotask::ModuleGraphLoadingError, nullptr, state);
// 2.d.iv. If state.[[IsLoading]] is false, return UNUSED.
Expand Down Expand Up @@ -950,13 +960,13 @@ void JSModuleLoader::finishLoadingImportedModule(JSGlobalObject* globalObject, c
ASSERT(owner);

// 1.a. If referrer.[[LoadedModules]] contains a LoadedModuleRequest Record record such that ModuleRequestsEqual(record, moduleRequest) is true, then
if (auto iter = loadedModules.find(ModuleMapKey { moduleRequest.m_specifier.impl(), moduleRequest.type() }); iter != loadedModules.end()) {
if (auto iter = loadedModules.find(moduleRequest.moduleMapKey()); iter != loadedModules.end()) {
// 1.a.i. Assert: record.[[Module]] and result.[[Value]] are the same Module Record.
ASSERT(iter->value.m_module.get() == *resultRecord);
// 1.b. Else,
} else {
// 1.b.i. Append the LoadedModuleRequest Record { [[Specifier]]: moduleRequest.[[Specifier]], [[Attributes]]: moduleRequest.[[Attributes]], [[Module]]: result.[[Value]] } to referrer.[[LoadedModules]].
ModuleMapKey key { moduleRequest.m_specifier.impl(), moduleRequest.type() };
ModuleMapKey key = moduleRequest.moduleMapKey();
Locker locker { owner->cellLock() };
AbstractModuleRecord::LoadedModuleRequest value { vm, moduleRequest, *resultRecord, owner };
loadedModules.add(WTF::move(key), WTF::move(value));
Expand Down Expand Up @@ -1075,11 +1085,11 @@ JSPromise* JSModuleLoader::loadRequestedModules(JSGlobalObject* globalObject, Ab
return pc;
}

ModuleRegistryEntry* JSModuleLoader::ensureRegistered(JSGlobalObject* globalObject, const Identifier& key, ScriptFetchParameters::Type type)
ModuleRegistryEntry* JSModuleLoader::ensureRegistered(JSGlobalObject* globalObject, const Identifier& key, ScriptFetchParameters::Type type, const ScriptFetchParameters* parameters)
{
VM& vm = globalObject->vm();

ModuleMapKey moduleMapKey { key.impl(), type };
ModuleMapKey moduleMapKey = makeModuleMapKey(key.impl(), type, parameters);

if (auto iter = m_moduleMap.find(moduleMapKey); iter != m_moduleMap.end())
return iter->value.get();
Expand All @@ -1092,9 +1102,9 @@ ModuleRegistryEntry* JSModuleLoader::ensureRegistered(JSGlobalObject* globalObje
return entry;
}

ModuleRegistryEntry* JSModuleLoader::getRegisteredMayBeNull(const Identifier& key, ScriptFetchParameters::Type type)
ModuleRegistryEntry* JSModuleLoader::getRegisteredMayBeNull(const Identifier& key, ScriptFetchParameters::Type type, const ScriptFetchParameters* parameters)
{
if (auto iter = m_moduleMap.find({ key.impl(), type }); iter != m_moduleMap.end())
if (auto iter = m_moduleMap.find(makeModuleMapKey(key.impl(), type, parameters)); iter != m_moduleMap.end())
return iter->value.get();
return nullptr;
}
Expand All @@ -1109,7 +1119,7 @@ int64_t JSModuleLoader::asyncEvaluationOrderForKey(const Identifier& key)
// EvaluatingAsync); once the module finishes it is cleared to DONE.
if (key.isNull() || key.isEmpty())
return -1;
auto* entry = getRegisteredMayBeNull(key, ScriptFetchParameters::Type::JavaScript);
auto* entry = getRegisteredMayBeNull(key, ScriptFetchParameters::Type::JavaScript, nullptr);
if (!entry)
return -1;
auto* cyclic = dynamicDowncast<CyclicModuleRecord>(entry->record());
Expand Down
Loading
Loading