bundler: escape what is spliced into JS chunks (ASCII for bun), and fix the column count source map shifts use after non-ASCII text - #38666
Conversation
…n front of non-ASCII in source map shifts Chunks printed for bun start with "// @Bun", which the runtime loads as Latin-1 without re-parsing, so the printer escapes everything in them to ASCII. The paths and HTML import manifests spliced in afterwards were written as raw UTF-8, and the manifest module itself was printed as browser code because generate_server_html_module left its target at the parser default. A non-ASCII file name therefore produced a bundle that failed to load (or resolved mojibake paths). LineColumnOffset::advance skipped the ASCII run in front of every newline or non-ASCII character it found, so a shift recorded after any non-ASCII text on the same line was too small and the mappings around the splice point were off.
|
Updated 7:59 PM PT - Aug 14th, 2026
✅ @robobun, your commit 9590770fb616178b4faeca31db168d9c050403b6 passed in 🧪 To try this PR locally: bunx bun-pr 38666That installs a local version of the PR into your bun-38666 --bun |
|
Status: reproduced both reported symptoms on a build of current main (with #38429): Fix and tests are in this PR (description has the details and repro transcript). The six new tests fail on that build and pass with the branch, on Linux and, for the ones that can run there, on Windows; the metafile regression found in review is fixed in aede9d4. All review threads are resolved. CI on 12f0113 (build 96742): every lane that exercises this change is green, including the two Windows failures from the first push. The only test red on every retry was |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
WalkthroughThe bundler centralizes splice escaping and path writing for raw output, JavaScript, Bun ASCII-only output, and metafile JSON. HTML manifests and metafiles use dedicated generation paths. Source-map offset accounting and regression tests cover non-ASCII and quoted paths. ChangesSplice output generation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…e same way in both assembly passes The sizing pass used to measure the path before the Windows separator normalization the writing pass applies, which was harmless while both copied the bytes through but over-counted once the path was escaped (each backslash counted twice), leaving NUL bytes at the end of the chunk on Windows. Both passes now resolve the path through one helper. Escaping now applies to every JS chunk, not only to chunks built for bun: the placeholder is always the body of a double-quoted literal, so a quote in an asset name broke the output for every target. ASCII-only escaping stays specific to chunks that carry the bun pragma. The manifest test no longer asserts the manifest's input paths, which are a separate Windows problem.
|
On the three PRs listed above:
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bundler/Chunk.rs:478-491—SpliceEscape::for_chunkis also applied to the metafile JSON:MetafileBuilder::generatecallsintermediate.code(..., &chunks[0], ...)on the assembled metafile, andchunks[0]is always a JS chunk, so under--target=bunthis returnsJsString { ascii_only: true }andwrite_spliced_pathescapes the replacement path with\xNN(json=falseat js_printer/lib.rs:1037) — not a valid JSON escape. Repro:await import("./modülé.ts")built with--target=bun --splitting --metafile=meta.json→ the metafile'simports[].pathbecomes"./mod\xFCl\xE9-<hash>.js"andJSON.parserejects it (before this PR the path was written raw, valid UTF-8). The metafile call site needsSpliceEscape::Raw(or ajson=truevariant).Extended reasoning...
What the bug is
code_with_source_map_shiftsnow derives its escape mode from thechunkargument viaSpliceEscape::for_chunk(chunk, linker_graph)(Chunk.rs:780). That is correct for the per-chunk call sites ingenerateChunksInParallel, but there is one more caller:MetafileBuilder::generate(linker_context/MetafileBuilder.rs:442) resolves unique-key placeholders in the assembled metafile JSON by callingintermediate.code(..., &chunks[0], chunks, ...).chunks[0]is a stand-in — the metafile is not a JS chunk — butfor_chunktreats it as one.Code path
computeChunkssorts JS chunks first (computeChunks.rs), sochunks[0]is always a JS entry chunk. Under--target=bun,linker_graph.ast.items_target()[chunks[0].entry_point.source_index()].is_bun()istrue, soSpliceEscape::for_chunkreturnsJsString { ascii_only: true }.computeCrossChunkDependenciesrewrites each dynamic-import record that points at another chunk torecord.path.text = other_chunk.unique_keyand setsrecord.source_index = INVALID(computeCrossChunkDependencies.rs:177-178).MetafileBuilderreadsparse_graph.ast.items_import_records()and, becauserecord.source_index.is_valid()isfalse, falls through torecord.path.text(MetafileBuilder.rs:300-311) and JSON-quotes it viaformat_json_string_utf8. The 25-byte unique key is pure ASCII, so it survives quoting byte-for-byte.break_output_into_pieces(MetafileBuilder.rs:433) finds that key inside the JSON"..."literal and produces aQueryKind::Chunkpiece for it.write_spliced_path(Chunk.rs:598-609) runs the replacementfinal_rel_paththroughwrite_pre_quoted_string_inner::<_, Utf8>(path, writer, b'"', ascii_only=true, json=false). Forc <= 0xFF && !json(js_printer/lib.rs:1037-1039) that emits\xNN, which RFC 8259 §7 does not permit in a JSON string.
Step-by-step repro
// entry.ts const m = await import("./modülé.ts"); console.log(m.v); // modülé.ts export const v = 1;bun build ./entry.ts --target=bun --splitting --metafile=meta.json --outdir out- The dynamic import becomes a separate chunk
modülé-<hash>.js; its import record'spath.textis rewritten to the unique key andsource_indexcleared. - The metafile's
outputs["out/entry.js"].importsentry for it is written as"path": "<unique key>", whichbreak_output_into_piecessplits. - With
escape = JsString { ascii_only: true },ü(U+00FC) is written as\xFCandé(U+00E9) as\xE9, giving"path": "./mod\xFCl\xE9-<hash>.js". JSON.parse(await Bun.file("meta.json").text())throwsSyntaxError: JSON Parse errorat the\x.
Before this PR,
code_with_source_map_shiftsdidremain[..].copy_from_slice(cheap_normalizer[i])— the raw UTF-8 bytes — which is valid inside a JSON string (only",\, and C0 controls need escaping, and those cannot appear in a chunk file name). So this is a regression.Even without
--target=bun,chunks[0]is still a JS chunk sofor_chunkreturnsJsString { ascii_only: false }, which would emit\v/\x07for control characters — also invalid JSON — but that case requires a control character in a chunk name and is much less likely in practice than the non-ASCII case above.Why nothing else prevents it
SpliceEscape::for_chunkkeys offchunk.content.is_javascript(); there is no variant for "the surrounding text is JSON" (or "raw, as before"), and the metafile call site cannot supply one. The PR description says "CSS and HTML chunks are written exactly as before", which is true forpostProcess{CSS,HTML}Chunk, but this call site is neither. The PR's new tests cover JS chunk output (bun/NonAsciiChunkAndAssetPathsuses--splittingbut notmetafile) and the PR ranmetafile.test.ts, but that suite has no non-ASCII chunk names.Impact
--metafileis a documented flag; a build with--target=bun --splitting --metafileand any dynamically-imported file whose name contains a Latin-1 non-ASCII character produces a metafile that no JSON parser accepts. This is exactly the class of input (--target=bun+ non-ASCII names) the PR sets out to fix, so it is in scope for this PR rather than pre-existing.Fix
The metafile's placeholders sit inside JSON
"..."literals, not JS ones, so the two correct choices are:SpliceEscape::Rawfor this call site (byte-identical to the pre-PR behavior; UTF-8 is valid in a JSON string, andformat_json_string_utf8already escaped anything that needed it in the surrounding text), or- a
SpliceEscape::JsonStringvariant that callswrite_pre_quoted_string_innerwithjson = true(emitsüinstead of\xFC).
Either way the escape mode has to be supplied by the caller here rather than derived from the dummy
&chunks[0]— e.g. thread anOption<SpliceEscape>intocode()/code_with_source_map_shifts()that overridesfor_chunk, and haveMetafileBuilder::generatepassSome(SpliceEscape::Raw). Add a metafile test with a non-ASCII dynamically-imported chunk name thatJSON.parses the result.
The metafile resolves its chunk references through the same assembly routine as the chunks, with the first chunk standing in, so the escape mode picked for that chunk (a JS string, ASCII-only for bun) was applied inside JSON. The metafile now asks for JSON escaping explicitly.
|
The metafile finding from the review above was real: the metafile resolves its chunk references through the same routine with |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the two-pass chunk assembly (sizing and writing must agree byte-for-byte) across four output modes and overlaps with three other open PRs (#38621, #38376, #34188), a human look would still be worthwhile.
What was reviewed:
SpliceEscape::for_chunkuses the sameitems_target()[entry_point.source_index()].is_bun()predicate aspostProcessJSChunkfor the pragma, soascii_onlymatches when// @bunis emitted.spliced_path_partsnormalizes separators before both passes, and both passes size/write via the samewrite_spliced_path→write_pre_quoted_string_inner, so the escaped byte counts agree.LineColumnOffset::advancenow addsi - offsetfor the skipped ASCII run before handling the stop character; the newline arm resetscolumnsafterward so the addition is a no-op there, and the trailing-remain path is unchanged.code_for_metafileforcesSpliceEscape::JsonStringregardless ofchunks[0]'s content, so a bun-target first chunk no longer leaks\xNNinto the metafile JSON.
Extended reasoning...
Overview
This PR fixes three related bundler bugs around non-ASCII and quote characters in file names when they are spliced into already-printed chunks: (1) the generated HTML manifest module was not tagged with the importing side's target, so its identifier was printed unescaped in a // @bun chunk; (2) chunk/asset paths and the manifest JSON were spliced raw into "..." literals, breaking Latin-1 loading for bun and producing unparseable output for quotes on every target; (3) LineColumnOffset::advance under-counted columns by dropping the ASCII run before each non-ASCII stop, mis-placing the source-map shift boundary. It touches Chunk.rs (new SpliceEscape enum, shared spliced_path_parts/write_spliced_path helpers, code_for_metafile), HTMLImportManifest.rs (adds ascii_only param, deletes the Display/EscapedJson scaffolding), bundle_v2.rs (one-line target assignment), MetafileBuilder.rs, sourcemap/lib.rs, and adds six tests across five test files.
Security risks
None identified. The change escapes more (paths inside string literals and JSON), not less; input is file names the user already controls, and the escaper is the existing write_pre_quoted_string_inner used by the printer. No auth, network, or untrusted-input parsing paths are touched.
Level of scrutiny
High. code_with_source_map_shifts is the two-pass assembly of every output chunk: the sizing pass allocates exactly count bytes and the writing pass fills them, ending with debug_assert!(remain.is_empty()). Any divergence between the passes either leaves NUL bytes (over-count) or panics/UB (under-count into a fixed buffer). The PR itself documents that the first push over-counted on Windows (pre-normalization path measured, post-normalization path written) and that review caught the metafile picking up the JS chunk's escape mode. Both are fixed, and the shared-helper design (spliced_path_parts + write_spliced_path + DiscardingWriter) structurally prevents the class, but the surface area — JS/CSS/HTML/standalone/metafile × ascii-only/not × Windows/POSIX paths — is large enough that a maintainer should confirm the escape-mode selection is right for every caller and that CSS url() splices genuinely want Raw.
Other factors
- The
bundle_v2.rshunk is deliberately byte-identical to #38621 and #38376, and theChunk.rschange supersedes #34188's approach. A human should decide landing order and whether #34188's extra tests should be folded in. - All comment-cop bot threads are resolved (comments were shortened or justified as doc comments).
- Test coverage is thorough: each of the three root causes has a test that fails on main, plus the metafile regression found in review; Windows-incompatible file names are correctly
skipIf(isWindows). - The
code_with_source_map_shiftsvisibility change frompub(crate)to private is safe — the only external caller (MetafileBuilder) now goes throughcode_for_metafile. - The
file_path_bufscratch buffer is now used in the sizing pass too; it was already acquired at the top of the function, so no new allocation.
|
On the one open question in the review above (whether CSS and HTML chunks should really get Current state for whoever picks this up: head is 9590770 (an empty retrigger on top of 12f0113, because build 96742 was red only on |
Problem
bun build --target=bunof a server file that imports an HTML file with a non-ASCII name (import h from "./é.html") writes a bundle thatbuncannot load:SyntaxError: Invalid character '\u00a9'. With--minifyit loads, but the manifest'sindex/pathvalues come back as./é.html, and with--sourcemapevery stack frame after the manifest maps to the wrong position (s.ts:6:6for both frames of a throw at4:13called from6:1; the same build with an ASCII name is right since bundler: shift source maps past the spliced HTML import manifest #38429).--target=bun --splittingwithimport("./é.ts")fails at runtime withCannot find module './é-630mkta0.js', and a file-loader import of./é.txtevaluates to./é-hvs6h2nf.txt. For every target, an asset namedq"b.txtis emitted asvar q_b_default = "./q"b-t6x98h26.txt";, which does not parse (the bug bundler: JS-escape file-loader asset paths at chunk assembly #34188 is open for).generate_server_html_module(src/bundler/bundle_v2.rs:6843) builds the manifest module's AST but never sets itstarget, so it keeps the parser defaultBrowser. The printer picks its escaping from the per-file target, sovar é_default = ...is printed raw while every reference to it elsewhere in the chunk is printed as\u{e9}_default.computeChunksalso classifies files by this target: with--splitting, the shared server chunk holding the manifest got flagged as browser output and listed in the manifest itself.IntermediateOutput::code_with_source_map_shifts(src/bundler/Chunk.rs) copies each chunk/asset path into the chunk unescaped, although the placeholder it replaces is the body of a"..."literal, andHTMLImportManifest::write_escaped_jsonescaped the manifest only for quotes (ascii_only = false). In a chunk whose first line says// @bun, which the runtime loads as Latin-1 (String::clone_latin1insrc/runtime/jsc_hooks.rs), the raw UTF-8é(C3 A9) is then read asé.LineColumnOffset::advance(src/sourcemap/lib.rs:158) jumps from one newline/non-ASCII byte to the next and only counts the character it stopped at plus the trailing ASCII run; the ASCII between stops is never added tocolumns. The manifest above (5é, 384 bytes) advanced the shift by 111 columns instead of 379. This is independent of the target: in a browser build with--minify-whitespace, a non-ASCII string anywhere before an asset or chunk placeholder on the line moves the recorded splice position left, and the mappings between the two get shifted although they sit before the splice (repro in the details below, mappings off by 5 columns). Ported unchanged from the Zig version.Fix
generate_server_html_modulesets the manifest module'stargetto the importing file's target. The hunk is byte-identical to the one the open bundler: keep the import kind of server-side HTML imports so import() and require() of the manifest link #38621 and bundler: give the browser side of a server build its own copy of the runtime #38376 carry for their own symptoms (bundler: keep the import kind of server-side HTML imports so import() and require() of the manifest link #38621 also adds a test for the--splittingclassification), sobundle_v2.rsmerges cleanly in any order; it is needed here because the identifier escaping depends on it.code_with_source_map_shiftspicks aSpliceEscapeper chunk: CSS and HTML chunks are written as before; in JS chunks every spliced path goes throughwrite_pre_quoted_string_innerwith the arguments the printer uses for a"..."literal, withascii_onlyset by the predicatepostProcessJSChunkuses to emit the pragma (entry file's targetis_bun()), and the same flag goes towrite_escaped_json. Every placeholder in a JS chunk is the body of a"..."literal (import paths,require()/import()arguments, asset strings,__jsonParse("...")), so"./\xE9-630mkta0.js","./q\"b-....txt"and\u00E9inside the manifest evaluate to the strings the raw bytes were meant to be. Paths without quotes, backslashes or control characters come out byte-identical for browser and node (checked CLI and API builds of ASCII names against a build of main); this covers what bundler: JS-escape file-loader asset paths at chunk assembly #34188 fixes in the same two passes, with the printer's escaper instead of a separate one.spliced_path_parts). The sizing pass used to measure the path before the separator normalization the writing pass applies, which was harmless while both copied the bytes through but over-counted once the path was escaped (a Windows\counts as two bytes, then gets normalized to/and written as one), leaving NUL bytes at the end of the chunk;test/regression/issue/31575.test.tscaught that on the Windows lanes of the first push. The manifest is likewise sized with the same function that writes it (throughbun_io::DiscardingWriter) instead of aDisplayadapter;EscapedJson, theHTMLImportManifeststruct and thehtml_import_manifestfacade module existed only for that adapter and are removed. The shift is advanced over the bytes actually written, so the recorded width is the escaped width, which is also what the Latin-1 load sees."./mod\xFCl\xE9-....js", whichJSON.parserejects; caught in review). The escape mode is now passed in explicitly:code()derives it from the chunk, andMetafileBuildercallscode_for_metafile(), which escapes as JSON string content. That is what the release writes for non-ASCII names (raw UTF-8, checked byte for byte on the repro) and additionally makes a quote in a chunk name valid JSON, which the release gets wrong.advanceaddsi - offset(the ASCII run the search skipped) before handling the character ati. For a newline this is immediately reset, as before; for non-ASCII it makes the count match what the printer andsource-mapconsumers count (UTF-16 units). Callers that pass ASCII-only input take the same path as before.test/bundler/html-import-manifest.test.ts:non-ascii-file-name-loads-under-target-bun(fails on main on the ASCII check and with theSyntaxError),non-ascii-file-name-source-map-columns(fails on main witherror: ./sidé.htmland frames5:6/5:6instead of3:13/5:1).test/bundler/bundler_bun.test.tsbun/NonAsciiChunkAndAssetPaths: fails on main (Cannot find module).test/bundler/bundler_loader.test.ts{bun,node,browser}/loader-file-path-with-quote(POSIX only, Windows cannot create the file): on main the output contains"./q"b-for all three targets.test/bundler/metafile.test.ts"metafile escapes resolved chunk paths as JSON" (POSIX only): a dynamically importedq"modülé.jschunk in atarget: "bun"build; the metafile is unparseable on main (raw quote) and on the intermediate version of this branch (\xFC), and parses with the chunk path intact now.test/bundler/bun-build-api.test.ts"generated columns before and after a spliced asset path on a line with non-ASCII text": browser build; on main the mapping for the function between the"é"and the asset splice is recorded 5 columns left of the function (101vs106), the ones before and after are right both ways.bundler_html*.test.ts,bundler_splitting.test.ts,bundler_naming.test.ts,metafile.test.ts,bundler_banner.test.ts,bundler_compile.test.ts,bundler_edgecase.test.ts,esbuild/loader.test.ts,bun-serve-html-manifest.test.ts,test/regression/issue/{27465,25628,31575}.test.ts;cargo fmt --checkand clippy onbun_bundler/bun_sourcemap. On a Windows x64 debug build:31575.test.ts, the two html-import tests andbun-build-api.test.tspass, and manual builds with nested entry points (relative paths between directories), a--public-pathcontaining a quote, and--target=bunproduce chunks without NUL bytes that run. (Bun.build can be called thousands of timesinbun-build-api.test.tsexceeds its 180 s budget in this debug/ASAN container with and without the change, about 550 ms per build either way;compile/HelloWorldWithProcessVersionsBunfails on every debug build and is tracked separately.)inputfields as../C:\...\sidé.html; that is the separate Windows bug bundler: emit posix-relative paths in the HTML-import manifest on Windows #34557 is for, so the new test does not assertinput./* é.html */path comment in front of each module still contains the name verbatim. It is harmless to the load, it shows aséonly in error excerpts of builds without source maps, and escaping it would make the file itself less readable.Background
// @bunpragma:bun build --target=bunputs this on the first line of every JS chunk. Whenbunlater runs such a file it skips the transpiler and hands the bytes to JavaScriptCore as a Latin-1 string (one character per byte), which is only correct if the file is pure ASCII. The printer guarantees that for the code it prints (ASCII_ONLYinsrc/js_printer, selected per file fromast.target): non-ASCII in strings becomes\xE9/\u...., in identifiers\u{e9}. Standalone executables do not go through this path (a--compilebuild with a non-ASCII asset name works on main); the escaped form means the same thing there, and the one above still prints/$bunfs/root/é-....binand reads the file.break_output_into_piecescuts the chunk at those keys andcode_with_source_map_shiftswrites the final text, resolving each key to a path (or, for the HTML import, to the manifest JSON). This runs once to size the buffer and once to fill it, which is why both passes have to agree byte for byte.before(position in the printed text, advanced over the key) andafter(position in the final text, advanced over the replacement);SourceMapPieces::finalizeaddsafter - beforeto every mapping located pastbeforeon that line. Both positions are computed withLineColumnOffset::advanceover the text in between, so an under-count there movesbeforeleft and applies the shift to mappings that precede the splice.import h from "./x.html"bundles the HTML as a browser build and bindshto a generated module whose body is__jsonParse("<key>"); the key is replaced by the JSON listing the browser outputs.generate_server_html_modulebuilds that module directly instead of going through a parse task, which is where the target assignment was missed.Repro from the report, before and after
Manifest line before (raw, inside a
// @bunfile) and after:Chunk and asset paths:
advanceunder-count in a browser build (no// @buninvolved):Decoded mappings for
a.tsline 2 and 3 (function second,function third), generated column recorded vs. where the token is:The shift for
./logo-7cr55x2a.svg(19 bytes replacing a 25-byte key, so -6) was applied to everything past column ~46 instead of past the key, because everything in front of theéwas left out of the recorded position.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts test/bundler/bundler_loader.test.ts test/bundler/metafile.test.ts