runtime: one module record for a .json imported with and without the type attribute - #35914
runtime: one module record for a .json imported with and without the type attribute#35914robobun wants to merge 6 commits into
Conversation
…type attribute
JSC keys the module map on (specifier, ScriptFetchParameters::Type). An
attribute-less request for a .json falls back to Type::JavaScript and
`with { type: "json" }` keys on Type::JSON, so the same file became two
live module instances and a mutation via one was invisible via the
other.
For the Bun target the printer now emits `with { type: "json" }` for a
.json specifier that has no attribute (and mirrors that in module_info),
so both forms hash to the (path, JSON) slot. For dynamic import()
moduleLoaderImportModule synthesizes Type::JSON after resolving when
the caller supplied no attributes. Filenames Bun routes to the jsonc
loader (package.json, tsconfig.*, jsconfig.*) are excluded so the
synthesized attribute cannot force strict JSON there.
|
Updated 9:50 AM PT - Jul 26th, 2026
❌ @robobun, your commit 280ee2d has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35914That installs a local version of the PR into your bun-35914 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WalkthroughChangesThe Bun JS printer now marks eligible attribute-less JSON module identity
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 `@test/js/bun/resolve/json-import-identity.test.ts`:
- Around line 110-155: Extend the tests in the JSON import identity suite with
build-output assertions covering static JSON imports and re-exports, verifying
the printer still emits with { type: "json" }, plus a static explicit non-JSON
attribute that remains unchanged. Add an export * as cfg from "./cfg.json"
runtime identity case to ensure it shares the attributed JSON module, covering
paths not exercised by the existing dynamic-import fixtures.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34603d0d-f63c-409e-a427-4cc4c0492ee2
📒 Files selected for processing (3)
src/js_printer/lib.rssrc/jsc/bindings/ZigGlobalObject.cpptest/js/bun/resolve/json-import-identity.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/bindings/ZigGlobalObject.cpp:3791— The two normalization sites inspect different inputs:record_implies_json_type()checksrecord.path.text(the source-as-written specifier — the runtime transpiler never resolves it), whilenormalizeFetchParametersForResolvedPath()checksresolvedIdentifier(the absolute path afterZig__GlobalObject__resolve). So when a specifier does not literally end in.jsonbut resolves to a.jsonfile — e.g. package"exports": { "./data": "./data.json" }or a subpath import"#cfg" → "./cfg.json"— a static import keys onType::JavaScriptwhile a dynamicimport()of the same specifier now keys onType::JSON, forking one file into two module instances where before this PR both keyedJavaScriptand shared one. Either the printer needs to see the resolved path (it can't in the runtime transpiler) or the normalization needs to move to a post-resolve point that both static and dynamic requests flow through — as-is this introduces a new instance of the exact bug class the PR fixes.Extended reasoning...
What the bug is
This PR normalizes attribute-less
.jsonimports toScriptFetchParameters::Type::JSONat two sites, but the two sites key on different representations of the specifier:- Printer side (
record_implies_json_type(),src/js_printer/lib.rs:6208): inspectsrecord.path.text. In the runtime transpiler this is the literal text the user wrote —RuntimeTranspilerStore.rs:1019-1045only rewritesimport_record.path.textforHardcodedAliasentries andbun:prefix stripping; it never runs the resolver. So forimport a from "pkg/data"the printer sees the string"pkg/data". - C++ side (
normalizeFetchParametersForResolvedPath(),ZigGlobalObject.cpp:3791): runs afterZig__GlobalObject__resolveand inspectsresolvedIdentifier, i.e. the absolute on-disk path.
When a specifier does not literally end in
.jsonbut resolves to a.jsonfile, the two disagree on whether to synthesizeType::JSON.Concrete trigger
node_modules/pkg/package.json: { "name": "pkg", "exports": { "./data": "./data.json" } } node_modules/pkg/data.json: { "n": 1 } // a.mjs import a from "pkg/data"; export default a; // index.mjs const s = (await import("./a.mjs")).default; const d = (await import("pkg/data")).default; console.log(s === d);The same shape applies to subpath imports (
"imports": { "#cfg": "./cfg.json" }) and to bare specifiers whosemainis a.jsonfile.Step-by-step
Static path (
a.mjs→import a from "pkg/data"):- Runtime transpiler parses
a.mjs;add_import_recordstorespath.text = "pkg/data". RuntimeTranspilerStoredoes not resolve import records;"pkg/data"reaches the printer unchanged.record_implies_json_type():has_suffix_comptime("pkg/data", ".json")→ false → no attribute emitted.- JSC's
ModuleAnalyzerrecords the requested module withType::JavaScript. moduleLoaderResolveresolves"pkg/data"→/abs/.../data.json, but returns only anIdentifier— it cannot alter the fetch parameters.- Module map key:
(/abs/.../data.json, JavaScript).
Dynamic path (
index.mjs→await import("pkg/data")):moduleLoaderImportModuleis called withmoduleName = "pkg/data",parameters = null.Zig__GlobalObject__resolvereturnsresolvedIdentifier = /abs/.../data.json.normalizeFetchParametersForResolvedPath:parametersis null, resolved path ends in.json, filename isdata.json→ synthesizesType::JSON.- Module map key:
(/abs/.../data.json, JSON).
Before this PR: neither side synthesized anything; both keyed
(/abs/.../data.json, JavaScript)→s === dwastrue.
After this PR: static →JavaScript, dynamic →JSON→ two live module instances →s === disfalse; a mutation via one is invisible via the other.The subpath-import case (
"#cfg") is guaranteed to hit this:record_implies_json_type()treats#as a fragment delimiter (lib.rs:6215), so"#cfg"truncates to the empty string and the printer side never fires, while the C++ side sees the resolved.jsonabsolute path and does.Why nothing prevents it
The static-import fetch parameters come from parsed source text —
moduleLoaderResolvereturns only aJSC::Identifierand has no channel to rewrite them post-resolve. The printer cannot see the resolved path because the runtime transpiler does not resolve. So the two normalization sites are architecturally looking at different-shaped inputs, and any specifier where "as-written" ≠ "resolved" wrt the.jsonsuffix diverges.Impact
This is a regression: attribute-less static and dynamic imports of the same specifier that shared one module on
mainnow fork into two after this PR — the exact identity bug the PR sets out to fix, newly introduced for.jsonfiles reached via packageexports, subpathimports, or a bare package whosemainis a.json. That's a realistic pattern (config packages, generated data packages).How to fix
The safe options are:
- Normalize at a post-resolve point both static and dynamic requests flow through (e.g. in
moduleLoaderFetch/ at the module-map lookup where the resolved key is known for both), rather than in the printer +moduleLoaderImportModule; or - Drop the C++ resolved-path normalization so both sides continue to key on the as-written specifier consistently (accepting that
await import("pkg/data")vsawait import("pkg/data", { with: { type: "json" } })still fork — but at least no new fork is introduced).
Either way, the two sides must key on the same representation.
- Printer side (
-
🔴
src/js_printer/lib.rs:6208-6224— The synthesizedwith { type: "json" }reaches the fetch hook as an explicit type override (jsc_hooks.rs:4037-4041), which clobbers any other loader-selection mechanism for the same specifier. Concretely,import x from "./cfg.json?raw"now returns the parsed object instead of the raw string (the query is stripped before the.jsoncheck, so the attribute is emitted, andLoader::Jsonoverwrites theLoader::Textthat?rawset); a bunfig[loader] ".json" = "jsonc"remap is likewise silently forced back to strict JSON.record_implies_json_typeandnormalizeFetchParametersForResolvedPathshould at minimum bail when the stripped query is?raw(same mechanism the jsonc-filename exclusion already guards against).Extended reasoning...
What breaks
record_implies_json_type()(and its C++ twinnormalizeFetchParametersForResolvedPath) decides purely on the specifier ending in.jsonafter stripping the first?/#, with a hardcoded exclusion list for the built-in jsonc filenames. The synthesized attribute then reachesget_loader_and_virtual_sourceastype_attribute_str = Some(b"json"), and at jsc_hooks.rs:4037-4041 that unconditionally overwrites whatever loader was chosen by any earlier mechanism:if query == b"?raw" { loader = Some(Loader::Text); } if let Some(attr_str) = type_attribute_str { if let Some(attr_loader) = Loader::from_string(attr_str) { loader = Some(attr_loader); // <-- overwrites Text with Json } }
The PR description already names this override as the reason for excluding
package.json/tsconfig.*/jsconfig.*— but the exclusion list only covers Bun's built-in jsonc special-cases, not the two other loader-selection mechanisms that key on.jsonspecifiers.Regression 1:
?rawon a.jsonfileStep-by-step for
import x from "./cfg.json?raw":- Parser produces an import record with
path.text = "./cfg.json?raw"andrecord.loader = None(?rawis a query, not awith-attribute;record.loaderis only set from a parsedwith { type }atjs_parser/parse/mod.rs:1397). record_implies_json_typeruns:record.loader.is_some()is false; it strips at the first?→path = "./cfg.json";.jsonsuffix matches; filename is not in the exclusion list → returnstrue.- Printer emits
import x from "./cfg.json?raw" with { type: "json" }. (The PR's own?v=1test proves the query is present inrecord.path.textat print time.) - JSC's ModuleAnalyzer parses the attribute →
ScriptFetchParameters::Type::JSON. moduleLoaderFetch(ZigGlobalObject.cpp:3843-3844) mapsType::JSON→typeAttributeString = "json"_s→ passed toBun__transpileFile.get_loader_and_virtual_source:query == b"?raw"setsloader = Some(Loader::Text); thentype_attribute_str = Some(b"json")overwrites it withLoader::Json.- Result:
xis the parsed JSON object. Before this PR, no attribute was emitted →type_attribute_str = None→ step 6's overwrite didn't fire →xwas the raw file contents as a string.
The dynamic-import path is identical:
moduleLoaderImportModuleconcatenatesqueryStringback onto the resolved path (ZigGlobalObject.cpp:3779), soresolvedIdentifierforawait import("./cfg.json?raw")contains?raw;normalizeFetchParametersForResolvedPathstrips at?, sees.json, and synthesizesType::JSON— same override chain follows.?rawis a documented built-in feature tested attest/js/bun/import-attributes/import-attributes.test.ts:346, but that test only exercises.js?raw, not.json?raw, so CI won't catch this.Regression 2: bunfig
[loader]remap of.jsonloader_for_path(jsc_hooks.rs:3862-3863) consults the user's loader table first —loaders.get(ext)— before any built-in default. A user with[loader] ".json" = "jsonc" # or "json5", "text"
previously had every attribute-less
import x from "./data.json"honor that remap. After this PR the printer emitswith { type: "json" }(it has no access to the loader table, andrecord.loaderisNonesince bunfig config never sets it), which forces strict JSON at fetch time via the same override at :4037-4041 — either a parse error (comments/trailing commas) or silently the wrong content type. This is the exact failure mode the PR's jsonc-filename exclusion guards against, applied to the user-configurable equivalent (documented atdocs/runtime/bunfig.mdx:87).Fix
For
?rawthe fix is local: in bothrecord_implies_json_typeandnormalizeFetchParametersForResolvedPath, bail (returnfalse/ early-return) when the stripped query is exactly?raw. That preserves the?v=1cache-buster behavior the new test asserts while leaving?rawuntouched.The bunfig case is harder because the printer can't see the loader table. Options: (a) have the fetch hook not treat the synthesized attribute as a hard override when the user's loader table remaps
.json(e.g. only apply thetype_attribute_stroverride when it differs from whatloader_for_pathalready returned, or thread a "synthesized" bit through), or (b) skip synthesis entirely whentranspiler.options.loadersmaps.jsonto something other thanLoader::Jsonon the dynamic-import side and accept the printer-side gap as a known limitation. At minimum the?rawcase should be excluded before merge; the bunfig case is rarer but worth noting since the exclusion list is demonstrably incomplete relative to the mechanism it guards against. - Parser produces an import record with
…est.concurrent The dynamic-import hook was inspecting the resolved path while the printer can only see the as-written specifier, so a specifier like `pkg/data` or `#cfg` that resolves to a .json forked static vs dynamic. Both sides now inspect the same representation. Also skip `?raw`, which selects the text loader and must not be overridden.
|
Addressed the two findings in bfbc4cf: Specifier-vs-resolved-path mismatch.
The bunfig Also switched the suite to |
Bumping EXPECTED_VERSION to 24: the bun-target printer output and the serialized esm_record changed for attribute-less .json import records, both of which are persisted in .pile entries. Dropping `#` from the query split: a leading `#` is a Node subpath marker, not a URL fragment, so `#cfg/data.json` must be seen as ending in `.json`. `normalize_specifier_for_loader` already splits on `?` only.
|
CI status: the diff itself is green; The only The remaining annotations are Ready for review. |
There was a problem hiding this comment.
All prior review feedback is addressed (including the stderr nit — expect(stderr).toBe("") is now folded into run()). No further issues found, but this changes module-identity semantics in the JSC loader with a duplicated predicate across the printer and ZigGlobalObject.cpp and an acknowledged partition shift for bare specifiers resolving to .json, so it's worth a maintainer look at the approach.
What was reviewed:
- Verified all three printer sites (SImport, SExportFrom, SExportStar) and their
module_infotwins are gated onIS_BUN_PLATFORM; browser/node targets untouched (transpiler snapshot covers this). - Checked
record_implies_json_typeandspecifierImpliesJsonTypeagree on?-only splitting,?rawskip, and jsonc-filename exclusions. - Confirmed the
?raw,#subpath, and cache-version issues from earlier rounds are fixed and covered by tests; a candidate concern about export-from forking on explicittype: "json"was examined and ruled out.
Extended reasoning...
Overview
Normalizes attribute-less .json imports so they share a JSC module record with the with { type: "json" } form. Touches four files: src/js_printer/lib.rs (emit with { type: "json" } + FetchParameters::Json for bun-target .json specifiers across SImport/SExportFrom/SExportStar), src/jsc/bindings/ZigGlobalObject.cpp (synthesize ScriptFetchParameters::Type::JSON in moduleLoaderImportModule for attribute-less dynamic import()), src/jsc/RuntimeTranspilerCache.rs (version bump 23→24), and a new 20-test file.
Security risks
None identified. No untrusted-input parsing beyond an extension check on the as-written specifier; no auth/crypto/permissions surface. The StringView operations in specifierImpliesJsonType are bounds-safe (find/substring/left/reverseFind), and moduleNameValue->value(globalObject) already has a RETURN_IF_EXCEPTION before the new predicate runs.
Level of scrutiny
High. This is module-loader semantics — silent duplicate-module bugs and silent loader-override bugs (the ?raw regression caught in an earlier round) are exactly the failure mode here. The fix is a heuristic on the as-written specifier with a hardcoded jsonc-filename exclusion list mirrored across Rust and C++; the PR body acknowledges the structural fix lives in WebKit's hostLoadImportedModule and that a partition boundary moves for bare specifiers that resolve to .json. Whether that tradeoff is acceptable, and whether the jsonc exclusion list will stay in sync with loader_for_path, are maintainer-level calls.
Other factors
The PR has iterated well: three review rounds surfaced real issues (?raw override, #-subpath truncation, missing cache-version bump, specifier-vs-resolved-path mismatch, test.concurrent, stderr assertion) and each was fixed with matching test coverage. The 20 tests cover static/dynamic, both load orders, export-from/export-star, query strings, ?raw, #subpath, package-exports indirection, jsonc filenames, and a negative type: "text" contract; the transpiler snapshot pins printer output for all three targets. The mechanized gate confirms the suite fails on main and passes on the PR under both debug+ASAN and release. Given the semantic subtlety and the disclosed known limitation, deferring to a human rather than auto-approving.
There was a problem hiding this comment.
All prior findings from my earlier passes are addressed and the bug-hunt found nothing new this round. Deferring to a maintainer because this changes bun-target transpiler output and JSC's module-map keying via a duplicated predicate across the printer and moduleLoaderImportModule, with an acknowledged known-limitation partition shift — that's a design call worth a human sign-off.
What was reviewed:
record_implies_json_type/specifierImpliesJsonTypenow split on?only, skip?raw, and exclude the jsonc filenames — verified the two predicates agree on the as-written specifier.EXPECTED_VERSIONbumped to 24 so warm.pileentries can't serve pre-fix output.run()asserts empty stderr; suite usestest.concurrentand covers static/dynamic/export-from/export-star,#cfg/data.json,?raw, and the jsonc guard set.
Extended reasoning...
Overview
The PR normalizes attribute-less .json imports to ScriptFetchParameters::Type::JSON so they share a module-map slot with the with { type: "json" } form. It touches four files: src/js_printer/lib.rs (emit with { type: "json" } and record FetchParameters::Json for SImport / SExportFrom / SExportStar under IS_BUN_PLATFORM), src/jsc/bindings/ZigGlobalObject.cpp (synthesize Type::JSON in moduleLoaderImportModule when parameters is null), src/jsc/RuntimeTranspilerCache.rs (version 23 → 24), and a new 20-test file.
Security risks
None identified. No untrusted-input parsing beyond a .json suffix / basename check on the user-written specifier, no allocation sized from external data, and no auth/crypto/permissions surface. The StringView operations in specifierImpliesJsonType are bounds-safe WTF helpers.
Level of scrutiny
High. This sits directly on the module-identity path: the printer change alters what every bun-target transpile of an attribute-less .json import emits (visible via Bun.Transpiler output), and the C++ change alters how every dynamic import() of a .json specifier keys into JSC's module map. The two predicates must stay byte-for-byte in agreement or static and dynamic imports fork — the first revision of this PR got that wrong, and the invariant now lives only in cross-referencing comments. The PR body's Known limitation also documents that the partition of module identity moves for the bare-specifier-vs-literal-path case; net module count is unchanged but the grouping differs from main. That's a deliberate tradeoff (the true fix is inside hostLoadImportedModule in WebKit, per the PR body and #32999), and per REVIEW.md's "fix at the layer that owns the invariant" guidance a maintainer should confirm the printer-side approach is the one they want to ship.
Other factors
Test coverage is thorough: 20 tests including both load orders, export-from / export-star, #cfg/data.json, ?raw static+dynamic, query-string, the jsonc filename guard set, a negative type: "text" contract, and a Bun.Transpiler snapshot pinning the printed output for bun vs browser/node targets. The suite is verified to fail on the installed canary and pass on the debug build. All four of my prior findings (test.concurrent, cache-version bump, ?raw override, #-prefix truncation) and CodeRabbit's earlier note are resolved, and expect(stderr).toBe("") is folded into run(). CI is green on the diff per the author's build summary. Given the module-loading criticality and the visible transpiler-output change, I'm deferring rather than approving.
2 participants
Importing the same
.jsonfile both without an attribute and withwith { type: "json" }produced two live module instances, so a mutation via one was invisible via the other.Reproduction
Deterministic on main. Within a single source file the two static forms coalesce (the printer deduplicates
requestedModulesby specifier), so the fork only shows across files or via dynamicimport().Node cannot exhibit it because the attribute-less form is
ERR_IMPORT_ATTRIBUTE_MISSINGthere; Bun accepts both, so both forms resolve to the same JSON loader and must be one module.Cause
JSC's module map is keyed on
(specifier, ScriptFetchParameters::Type). An attribute-less request falls back toType::JavaScript(AbstractModuleRecord::ModuleRequest::type()/JSModuleLoader::loadModule), whilewith { type: "json" }parses toType::JSON, so the two requests hash to different slots and fetch twice.Fix
Normalize the attribute-less form to
Type::JSONat the two points Bun controls before JSC's key is built, keying the decision on the as-written specifier so both sides agree:src/js_printer/lib.rs): fortarget: "bun", when an import /export ... fromrecord has nowith { type }and its specifier ends in.json, emitwith { type: "json" }in the printed source (and mirrorFetchParameters::Jsoninmodule_infoso the isolation-cache path and the debugfallbackParsediff stay consistent). JSC'sModuleAnalyzerthen producesType::JSONfor both forms.import()(src/jsc/bindings/ZigGlobalObject.cpp): inmoduleLoaderImportModule, before resolving, if the caller supplied no attributes and the specifier ends in.json, synthesizeScriptFetchParameters::create(Type::JSON).Both predicates skip the same cases a synthesized
type: "json"would change the loader for:?raw, which selects the text loader.loader_for_pathroutes to jsonc (package.json,tsconfig.*,jsconfig.*), where it would force strict JSON and break empty/commented config files.An explicit user-written
with { type: ... }is never rewritten, and othertargetvalues are untouched (the printer path is behindIS_BUN_PLATFORM).The dynamic-import side originally inspected the resolved path; that forked a specifier like
pkg/dataor#cfg(resolves to a.json, does not end in one) between static and dynamic, which the printer cannot match because the runtime transpiler never resolves. Both sides now inspect the same representation and are covered by tests.Verification
Against the installed canary (
/usr/local/bin/bun, 1.3.14): 9 of 20 tests intest/js/bun/resolve/json-import-identity.test.tsfail; the passing half is the regression-guard set (?raw, jsonc filenames,pkg/data/#cfgstatic-vs-dynamic, the negativetype: "text"contract).bun bd test test/js/bun/resolve/json-import-identity.test.ts: 20 pass / 0 fail.Also green on the debug build:
jsonc.test.ts,import-query.test.ts,import-defer.test.ts,esModule.test.ts,import-attributes.test.ts,resolve.test.ts,require.test.ts,import-meta.test.js,bundler_loader.test.ts,transpiler.test.js,regression/issue/16476.Known limitation
A specifier that does not literally end in
.jsonbut resolves to one (e.g."pkg/data"via packageexports) keys onType::JavaScript, while the literal path to the same file keys onType::JSON. On main both wereType::JavaScript, so an attribute-lessimport "pkg/data"and an attribute-lessimport "./node_modules/pkg/data.json"shared a module; now the literal form joins theType::JSONslot alongside every attributed form instead. Net module count is unchanged, the partition moved. Closing that completely needs the key's type component rewritten afterresolve()insidehostLoadImportedModule, which is a WebKit change (same territory as #32999).A bunfig
[loader] ".json" = "jsonc"remap is not touched here: it already fails on main the same way (the fetch-timetypeoverride is unaware of the bunfig remap), verified against 1.3.14.Related
#32999 fixes the adjacent collapse where two different
with { type }values hash to one slot (HostDefined string not in the key). This PR is the converse: no-attribute vstype: "json"on a.jsonspecifier hashing to two slots when Bun loads them identically. The two are independent.[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file