ModuleMap: key the module registry on the host-defined import attribute string - #258
ModuleMap: key the module registry on the host-defined import attribute string#258robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughChangesModule map keys now include host-defined import attributes. Registry and module-loading operations propagate these parameters through lookup, registration, fetch, evaluation, and imported-module tracking paths. Attribute-aware module loading
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
|
End-to-end verification against this PR's preview artifact (
Bun's existing module loader suite ( For the record, the one red lane in the first Preview Build attempt ( |
Importing the same path with two different `with { type }` attributes
returned one shared module: JSC's ModuleMapKey was
(specifier, ScriptFetchParameters::Type), and every Bun-defined attribute
value ("text", "file", "toml", "html", ...) maps onto the single
Type::HostDefined enum value, so whichever import evaluated first decided
what both callers got.
The key now also carries the host-defined attribute string. The fix lives
in oven-sh/WebKit (oven-sh/WebKit#258); this commit
bumps WEBKIT_VERSION, updates Bun's three ModuleMapKey users to the widened
key (NodeVMSourceTextModule uses the new ModuleRequest::moduleMapKey()
helper; two diagnostic iterations switch from .first to std::get<0>), and
adds the regression test, including the HTML reproduction from issue 19834.
Fixes #19834
|
Cross-referencing a sibling bug found while reproducing a concurrent variant of this one. // bad.ts contains `import {` (one parser error -> a BuildMessage)
// other.ts contains `import "./bad.ts"`
await import("./bad.ts").catch(() => {});
await import("./other.ts"); // never settlesFixed separately in #262; it is independent of this PR (this one threads the attribute into the Two data points that may be useful here:
Also confirmed empirically that the static-import gap described in "What this fixes, and what it does not" is real: with both diffs, |
…262) In moduleLoadTopSettled's rejected branch, the entire fetch/evaluation-error classification is gated on dynamicDowncast<ErrorInstance>. Bun rejects a single-message transpile failure with a BuildMessage, which is a plain JSDestructibleObject, so nothing is recorded on the registry entry at all. moduleLoadTopRejected then falls through its fetchError() check and records the value with setEvaluationError. That leaves the entry at EvaluationFailed with no fetch, module, or load promise. The next importer of the same key that goes through hostLoadImportedModule (a static import, or a dynamic import inside another module's graph load) finds a non-New entry with no fetchError and no loadPromise, creates a fresh pending fetchPromise via ensureModulePromise, and waits on it forever. Nothing ever settles it. // bad.ts contains "import {" (exactly one parser error, so the // rejection value is a BuildMessage, not an AggregateError) await import("./bad.ts").catch(() => {}); await import("./other.ts"); // other.ts statically imports bad.ts; hangs This microtask reacts to the embedder fetch promise, so every rejection that reaches it is a fetch failure by construction: the promise is either a fresh fetch() or a cached entry->ensureFetchPromise(), and the only rejector of an entry's fetch promise is setFetchError. Add the else branch so a non-Error rejection value is recorded the same way. This matches what the sibling nested-import path already does: moduleRegistryFetchSettled calls setFetchError unconditionally and only gates the attachErrorInfo metadata on the value being an ErrorInstance. With the entry at FetchFailed, moduleLoadTopRejected rejects with the cached fetchError() instead of calling setEvaluationError, and later importers take hostLoadImportedModule's fetchError short circuit instead of parking on a promise nothing settles. Independent of #258 (the import-attribute module map key): that change threads the attribute into ensureRegistered here but keeps the ErrorInstance gate, and this hang reproduces with no import attributes at all. The two apply cleanly together.
adc8aeb to
9435ca1
Compare
Importing the same path with two different `with { type }` attributes
returned one shared module: JSC's ModuleMapKey was
(specifier, ScriptFetchParameters::Type), and every Bun-defined attribute
value ("text", "file", "toml", "html", ...) maps onto the single
Type::HostDefined enum value, so whichever import evaluated first decided
what both callers got.
The key now also carries the host-defined attribute string. The fix lives
in oven-sh/WebKit (oven-sh/WebKit#258); this commit
bumps WEBKIT_VERSION, updates Bun's three ModuleMapKey users to the widened
key (NodeVMSourceTextModule uses the new ModuleRequest::moduleMapKey()
helper; two diagnostic iterations switch from .first to std::get<0>), and
adds the regression test, including the HTML reproduction from issue 19834.
Fixes #19834
|
Rebased onto main (c9ad581, the The new preview tag is |
9435ca1 to
41bae54
Compare
There was a problem hiding this comment.
Thanks for reworking the description — the new "What this fixes, and what it does not" section fully addresses my earlier note on static imports, and the end-to-end verification against the preview artifact is convincing. I didn't find any correctness issues on this pass, but this rekeys the module registry and widens ensureRegistered/provideFetch in a way that requires a coordinated embedder change, so I'd like a human to sign off.
What was reviewed:
makeModuleMapKeynormalization — non-HostDefined types always get a nullString, so(key, JSON, params)and(key, JSON, nullptr)cannot diverge;ModuleMapHash::equaluses WTFString::operator==(content compare, null==null).- All former
ModuleMapKey{impl, type}/.firstsites now go throughmoduleMapKey()/makeModuleMapKey()/std::get<0>;m_resolutionFailures(still astd::pairkey) correctly kept.first/.second. ModuleRequest::operator=='s newhostDefinedImportType()compare — for non-HostDefined params both sides return nullString, so behavior is unchanged there.provideFetchType-only overloads assert non-HostDefined and every existing caller passes an entry-point/attribute-less type.
Extended reasoning...
Overview
Widens ModuleMapKey from std::pair<UniquedStringImpl*, ScriptFetchParameters::Type> to std::tuple<..., ..., String>, adding the host-defined with { type: "..." } attribute string as a third key component so that e.g. type: "text" and type: "file" (both Type::HostDefined) land in distinct registry buckets. Threads const ScriptFetchParameters* through ensureRegistered / getRegisteredMayBeNull / new provideFetch overloads, centralizes key construction in makeModuleMapKey() / ModuleRequest::moduleMapKey(), updates ModuleMapHash and ModuleRequest::operator==, and adjusts every construction/lookup site in JSModuleLoader.{h,cpp}, JSMicrotask.cpp, and AbstractModuleRecord.{h,cpp}. Six files changed.
Security risks
None identified. This is a module-identity/caching change; it does not touch auth, sandboxing, memory layout, or untrusted-input parsing. The added String in the key is the already-parsed hostDefinedImportType from ScriptFetchParameters, hashed via StringImpl::hash().
Level of scrutiny
High. The module registry is on the hot path for every ESM import in Bun, the key shape governs module identity per the tc39 import-attributes proposal, and the ModuleMapKey type change is embedder-visible (Bun's NodeVMSourceTextModule.cpp must be updated in lockstep with the WEBKIT_VERSION bump). Hash/equality changes on a core map are exactly the kind of thing where a subtle mismatch (e.g. two spellings of "no attribute") silently splits or merges cache entries. The makeModuleMapKey helper is designed to prevent that, and I traced every call site, but a maintainer familiar with the loader's history should confirm the API shape and the !USE(BUN_JSC_ADDITIONS) behavior (where the third component is always null and the change is effectively a no-op).
Other factors
- My earlier inline comment (static-import binding path not fixed) was addressed by rewriting the description to explicitly scope it out; the comment is resolved and robobun independently confirmed the analysis empirically.
- End-to-end verified via oven-sh/bun#32999 against both preview artifacts: 6/6 new attribute tests pass, 48 existing module-loader tests pass, all CI lanes (incl. debug + ASAN) green after rebase.
- No bugs found by the automated bug-hunting pass on this revision.
safeToCompareToEmptyOrDeleted = falsewas preserved, appropriate now that the key carries aString.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/runtime/ModuleMap.h`:
- Around line 48-57: The HostDefined/null-parameters invariant is not enforced
at the ModuleMapKey construction chokepoint. In
Source/JavaScriptCore/runtime/ModuleMap.h#L48-L57, add an ASSERT_WITH_MESSAGE in
makeModuleMapKey’s fallback path rejecting HostDefined when parameters is null;
in Source/JavaScriptCore/runtime/JSModuleLoader.cpp#L337-L360, the existing
caller asserts are redundant and may be removed or retained as early
diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 422fab78-2fbf-4367-9d56-ebcf05b60b8e
📒 Files selected for processing (6)
Source/JavaScriptCore/runtime/AbstractModuleRecord.cppSource/JavaScriptCore/runtime/AbstractModuleRecord.hSource/JavaScriptCore/runtime/JSMicrotask.cppSource/JavaScriptCore/runtime/JSModuleLoader.cppSource/JavaScriptCore/runtime/JSModuleLoader.hSource/JavaScriptCore/runtime/ModuleMap.h
Importing the same path with two different `with { type }` attributes
returned one shared module: JSC's ModuleMapKey was
(specifier, ScriptFetchParameters::Type), and every Bun-defined attribute
value ("text", "file", "toml", "html", ...) maps onto the single
Type::HostDefined enum value, so whichever import evaluated first decided
what both callers got.
The key now also carries the host-defined attribute string. The fix lives
in oven-sh/WebKit (oven-sh/WebKit#258); this commit
bumps WEBKIT_VERSION, updates Bun's three ModuleMapKey users to the widened
key (NodeVMSourceTextModule uses the new ModuleRequest::moduleMapKey()
helper; two diagnostic iterations switch from .first to std::get<0>), and
adds the regression test, including the HTML reproduction from issue 19834.
Fixes #19834
|
Rebased onto main again (4895f45, after the shared-allocator build matrix additions). Clean cherry-pick: none of the six files this PR touches changed in the intervening five commits. New preview tag is |
Importing the same path with two different `with { type }` attributes
returned one shared module: JSC's ModuleMapKey was
(specifier, ScriptFetchParameters::Type), and every Bun-defined attribute
value ("text", "file", "toml", "html", ...) maps onto the single
Type::HostDefined enum value, so whichever import evaluated first decided
what both callers got.
The key now also carries the host-defined attribute string. The fix lives
in oven-sh/WebKit (oven-sh/WebKit#258); this commit
bumps WEBKIT_VERSION, updates Bun's three ModuleMapKey users to the widened
key (NodeVMSourceTextModule uses the new ModuleRequest::moduleMapKey()
helper; two diagnostic iterations switch from .first to std::get<0>), and
adds the regression test, including the HTML reproduction from issue 19834.
Fixes #19834
ac29374 to
a024b4c
Compare
There was a problem hiding this comment.
The ModuleAnalyzer.cpp build break I flagged last pass is now fixed (it uses makeModuleMapKey() with attributes.get()), and this run found no further issues. Given the scope — a widened ModuleMapKey threaded through the realm registry, per-record [[LoadedModules]], hostResolveImportedModule, and the provideFetch/ensureRegistered API surface, plus a coordinated Bun-side change — a human sign-off is still worthwhile.
What was reviewed:
- Confirmed no remaining 2-arg
ModuleMapKey { … }brace-init sites in-tree; every construction goes throughmakeModuleMapKey()/ModuleRequest::moduleMapKey(). ModuleMapHash::equalcompares theStringcomponent by value (WTFString::operator==), so twohostDefinedImportType()copies with distinctStringImpl*still match.- The new
hostResolveImportedModuleHostDefined branch scansm_requestedModulesand falls through tonullptrrather than hitting the fallthrough assert; non-HostDefined lookups are unchanged. .first/.secondaccesses onResolutionMapKeyinremoveEntry/ResolutionMapHashare unaffected (still astd::pair).
Extended reasoning...
Overview
The PR widens ModuleMapKey from (UniquedStringImpl*, ScriptFetchParameters::Type) to a 3-tuple that also carries the host-defined with { type: … } attribute string, and threads that key through every registry and [[LoadedModules]] lookup: ensureRegistered, getRegisteredMayBeNull, provideFetch (two new overloads), hostLoadImportedModule, innerModuleLoading, finishLoadingImportedModule, getImportedModule, setImportedModule, and ModuleRequest::operator==. hostResolveImportedModule gains a Bun-only linear scan of m_requestedModules to recover the attribute string that ImportEntry/ExportEntry do not carry. ModuleAnalyzer::appendRequestedModule now builds its dedup key through the same helper. Seven files, roughly +90/−30.
What changed since my last review
My previous run flagged a build break: ModuleAnalyzer.cpp:46 still brace-constructed a 2-arg ModuleMapKey after the type became a 3-tuple. The current diff adds ModuleAnalyzer.cpp to the change set with exactly the suggested fix (makeModuleMapKey(specifier.impl(), type, attributes.get())). I re-grepped the tree for ModuleMapKey\s*\{ and there are no remaining 2-arg sites. Both earlier inline threads (the static-import scope note and the CodeRabbit assert-placement nit) are resolved and reflected in the current diff and PR body.
Security risks
None identified. This is module-identity bookkeeping; it does not touch auth, crypto, permissions, or untrusted parsing. The change narrows a key (fewer collisions), which if anything reduces the surface for one import observing another's module instance.
Level of scrutiny
High. This is core JSModuleLoader / AbstractModuleRecord machinery that every ES module import flows through, it modifies spec-mapped algorithms (GetImportedModule, FinishLoadingImportedModule, ModuleRequestsEqual, HostLoadImportedModule), and it changes an embedder-visible type in a way that requires a coordinated Bun-side commit (NodeVMSourceTextModule.cpp and the WEBKIT_VERSION bump in oven-sh/bun#32999). The PR body also explicitly scopes out the static-import binding path, which is a design boundary a maintainer should ratify.
Other factors
The author has done thorough end-to-end verification against three preview artifacts (6/6 new tests pass, 48 existing module-loader tests pass, ASAN/debug lanes green), and the diffs interact cleanly with #262. That gives good confidence the change is correct, but the combination of registry-key semantics, the new HostDefined recovery loop in hostResolveImportedModule, and the cross-repo coordination is more than I'd approve without a human look.
Importing the same path with two different `with { type }` attributes
returned one shared module: JSC's ModuleMapKey was
(specifier, ScriptFetchParameters::Type), and every Bun-defined attribute
value ("text", "file", "toml", "html", ...) maps onto the single
Type::HostDefined enum value, so whichever import evaluated first decided
what both callers got.
The key now also carries the host-defined attribute string. The fix lives
in oven-sh/WebKit (oven-sh/WebKit#258); this commit
bumps WEBKIT_VERSION, updates Bun's three ModuleMapKey users to the widened
key (NodeVMSourceTextModule uses the new ModuleRequest::moduleMapKey()
helper; two diagnostic iterations switch from .first to std::get<0>), and
adds the regression test, including the HTML reproduction from issue 19834.
Fixes #19834
|
Rebased onto main (3547373). This one was not a clean cherry-pick, so summarizing what changed:
Preview |
ModuleMapKey was (specifier, ScriptFetchParameters::Type). Every Bun-defined
`with { type }` value ("text", "file", "toml", ...) maps onto the single
Type::HostDefined enum value, so two imports of the same path with different
attribute types collided on one registry entry and whichever ran first
decided what both got:
// data.txt contains "hello\n"
const a = await import("./data.txt", { with: { type: "text" } });
const b = await import("./data.txt", { with: { type: "file" } });
// both a.default and b.default were "hello\n", and a === b
The distinguishing string already lives on
ScriptFetchParameters::m_hostDefinedImportType; it just never reached the
key. Widen ModuleMapKey to (specifier, Type, hostDefinedImportType) and
route every key construction through makeModuleMapKey() (or
ModuleRequest::moduleMapKey()) so the string component is null for every
non-HostDefined type and never dropped for a HostDefined one.
makeModuleMapKey() asserts that a HostDefined key is never built without
its parameters, so the invariant is enforced at the one chokepoint instead
of at individual callers.
tc39/proposal-import-attributes requires the same specifier imported with
different attribute lists to designate different modules, which is what the
(specifier, Type) key was already trying to do; it just could not tell the
HostDefined types apart. ModuleRequest::operator== (ModuleRequestsEqual) had
the same gap and is fixed the same way.
Import, export and star-export entries carry only the attribute Type, so
AbstractModuleRecord::hostResolveImportedModule() cannot build a HostDefined
key directly. It recovers the full key from the record's own requested
modules (source order), so a binding resolves to the first request for its
specifier and type: the same module the old specifier-keyed lookup produced.
Threading the attribute string through the entries themselves, so that two
differently-attributed static imports of one specifier bind independently,
is a follow-up; this change makes the loading graph and registry hold the
distinct modules that follow-up will bind to.
ModuleAnalyzer deduplicates a module's requested modules with the same key,
so it builds the key the same way: two imports of one specifier with
different host-defined types now both reach [[RequestedModules]] and both
get loaded, instead of the second being dropped at parse time.
provideFetch's existing (key, Type, source) overloads keep their signature;
all their callers are attribute-less entry points. The post-fetch
registration in moduleLoadTopSettled is the one path that registers a
HostDefined entry and now passes the request's ScriptFetchParameters
through. ensureRegistered and getRegisteredMayBeNull gained a
`const ScriptFetchParameters*` parameter.
a024b4c to
b30b5ab
Compare
There was a problem hiding this comment.
Both issues I raised earlier are addressed in the current revision (b30b5abf): ModuleAnalyzer::appendRequestedModule now goes through makeModuleMapKey(), and hostResolveImportedModule recovers the full HostDefined key from [[RequestedModules]]. This automated pass found no further bugs. Given the scope — reshaping ModuleMapKey and threading it through the registry, [[LoadedModules]], and the binding-resolution path, plus the coordinated Bun-side change — a human sign-off on the overall approach would still be worthwhile.
What was reviewed:
makeModuleMapKey()chokepoint and theASSERTguardingHostDefinedwithout parameters; verified all in-tree key constructions route through it orModuleRequest::moduleMapKey().ModuleMapHash::hash/equalfor the third component — nullStringhandled;String::operator==gives value equality across the WTF null/empty distinction.- The new
hostResolveImportedModuleHostDefined branch — linear scan overm_requestedModulesin source order, fallthrough tonullptrmatches prior specifier-keyed miss behavior; non-HostDefined path unchanged. provideFetchoverload fan-out and theJSMicrotask.cppcall sites —m_attributes.get()is threaded consistently toensureRegistered.
Extended reasoning...
Overview
The PR widens ModuleMapKey from (UniquedStringImpl*, ScriptFetchParameters::Type) to a 3-tuple adding the host-defined import-attribute string, so that with { type: "text" } and with { type: "file" } (both of which collapse onto Type::HostDefined) get distinct registry entries. It routes every key construction through makeModuleMapKey() / ModuleRequest::moduleMapKey(), updates ensureRegistered / getRegisteredMayBeNull / provideFetch to accept a ScriptFetchParameters*, extends ModuleRequest::operator== to compare hostDefinedImportType(), makes ModuleAnalyzer::appendRequestedModule dedupe on the full key, and adds a HostDefined-specific recovery path in hostResolveImportedModule that scans m_requestedModules to reconstruct the key that ImportEntry/ExportEntry cannot yet carry. Seven files, ~150 lines net.
Security risks
None identified. The change is confined to how module-registry keys are hashed and compared; there is no new parsing, no new external input surface, and the attribute string was already stored on ScriptFetchParameters. The String in the key is held by value, and the map's lifetime is bounded by JSModuleLoader / AbstractModuleRecord, both GC-owned.
Level of scrutiny
High. Module loading is core runtime — the registry key shape determines identity for every ES module, and hostResolveImportedModule sits on the binding-resolution path used by initializeEnvironment and namespace construction. The change also has cross-repo coupling: ModuleMapKey is part of the embedder-visible surface, and Bun has three sites (per the description) plus a paired WEBKIT_VERSION bump in oven-sh/bun#32999 that must land together. There is also an explicitly documented residual behavior gap (two static imports of the same specifier with different host-defined types in one file bind to the first request), which is a design trade-off a human should ratify.
Other factors
- Both of my earlier findings (the over-claimed static-import scope, and the
ModuleAnalyzer.cpp2-arg brace-init build break) are addressed in the current revision; both threads are resolved. - CodeRabbit's suggestion to centralize the HostDefined-needs-parameters invariant inside
makeModuleMapKeyis applied. - The author reports green preview builds on all lanes for
a024b4c8, 7/7 new regression tests passing on Bun pinned to that artifact, and 48 existing module-loader tests passing. That is good empirical coverage but does not substitute for a human reading thehostResolveImportedModulerecovery approach and theoperator==change. - I did not approve because the change is neither small nor mechanical: it modifies a fundamental identity key, adds a new lookup strategy, and requires coordinated embedder changes.
1 participant
What
Importing the same file with two different
with { type: ... }attributes returns one shared module; whichever import evaluated first decides what both callers get, and reversing the import order flips the result.Why
ModuleMapKeyis(UniquedStringImpl* specifier, ScriptFetchParameters::Type).ScriptFetchParameters::parseTypemaps every non-json/ non-webassemblyattribute value onto the singleType::HostDefinedenum value, sowith { type: "text" }andwith { type: "file" }hash to the same bucket (ModuleMap.h:34-49). The string that distinguishes them already exists onScriptFetchParameters::m_hostDefinedImportType; it just never reaches the key, the hash, or the equality check.The tc39 import-attributes proposal requires the same specifier imported with different attribute lists to designate different modules. The existing
(specifier, Type)key was already trying to do that; it just cannot tell the HostDefined types apart.How
ModuleMapKeybecomesstd::tuple<UniquedStringImpl*, ScriptFetchParameters::Type, String>. The third component is the host-defined attribute string, null for every non-HostDefined type.makeModuleMapKey()/ModuleRequest::moduleMapKey(), so a key can never be built with a dropped attribute string, and(key, JSON, "json")vs(key, JSON, null)can never diverge.ensureRegistered/getRegisteredMayBeNulltake aconst ScriptFetchParameters*so the string reaches the key.provideFetch's existing(key, Type, source)overloads keep their signature; every caller is an attribute-less entry point. New(key, Type, const ScriptFetchParameters*, source)overloads exist for the one internal path that registers a HostDefined entry, the post-fetch registration inmoduleLoadTopSettled.AbstractModuleRecord::ModuleRequest::operator==(the spec'sModuleRequestsEqual) compared only theTypeenum and had the same gap; it now compares the host-defined string too.[[LoadedModules]]maps and the realm-level map use the sameModuleMapKey, so they are fixed by the same change.AbstractModuleRecord::hostResolveImportedModule()(which Remove the December 2025 LTO de-inlining stopgap now that WTF::opaque() is volatile #403 moved onto[[LoadedModules]]keyed by(specifier, Type)) receives only the attributeTypefrom import/export entries, so forHostDefinedit recovers the full key from the record's own[[RequestedModules]](source order). A binding therefore resolves to the module's first request for that specifier and type, which is the module the previous specifier-keyed lookup produced; non-HostDefinedlookups are unchanged.ModuleAnalyzer::appendRequestedModule()dedupes requested modules with the same key, so it builds it the same way: two imports of one specifier with different host-defined types now both reach[[RequestedModules]]and both get loaded, instead of the second being dropped at parse time.makeModuleMapKey()asserts that aHostDefinedkey is never built without its parameters, so the invariant is enforced at the one chokepoint rather than at individual callers (the earlier per-overload asserts inprovideFetchare gone).registryEntry()/removeEntry()/clearAll()already match on the specifier component only and are unaffected beyond thestd::get<0>spelling.Embedder impact
ModuleMapKeygained a third component, so code that brace-constructs one from{ impl, type }no longer compiles; it should useModuleRequest::moduleMapKey()ormakeModuleMapKey(). Bun has three such sites (one key construction inNodeVMSourceTextModule.cpp, two diagnostic.firstreads inInspectorLifecycleAgent.cpp/ZigGlobalObject.cpp); oven-sh/bun#32999 fixes them together with theWEBKIT_VERSIONbump.provideFetch(key, Type, source)callers are unchanged.What this fixes, and what it does not
Dynamic
import()is fully fixed: the registry is the only lookup on that path, so two dynamic imports of one path with different attributes return two distinct modules regardless of order.Static imports are fixed except for one narrow case. With #403's
moduleRequestTypeon import/export entries plus this PR, a static import or re-exportwith { type: "text" }loads and binds the right module, and two differently-attributed imports of different paths are independent. What remains is two static imports of the same path with different host-defined types inside one module: both modules now load (see theModuleAnalyzerchange), but the entries carry only theType, so both bindings resolve to the first such request. Finishing that means carrying the attribute string throughImportEntry,ExportEntryand the star-export set, plus the embedder'sadd*Entrybindings (seven FFI entry points on Bun's side), and is a separate PR; this one makes the loader hold the distinct modules it will bind to.Other out-of-scope notes
typeattribute participates in the key (it is the only attribute that decides what a module is).with { type: "sqlite", embed: "true" }vswith { type: "sqlite" }still share an entry;ScriptFetchParameters::m_attributesis not populated consistently enough to key on today.JSModuleLoader::ModuleFailurestill tags errors with(key, Type). That only matters when two HostDefined variants of the same path are both real executing modules that both throw, and the worst outcome is asetEvaluationErrorvssetFetchErrormisclassification.Verification
I will point a Bun build at this PR's preview artifact and run a two-attribute dynamic import test end to end, then link the Bun PR here. Bun's bundler has the same collapse in its own module graph and is being reported separately on the Bun side.