Conversation
….source The printer's ASCII-only policy for the bun runtime target re-encoded codepoints > 0x7F inside tagged-template raw contents and regex literals as \uXXXX escape sequences. Both TemplateStringsArray.raw (surfaced via String.raw and every tagged-template DSL) and RegExp.prototype.source expose that text verbatim, so the escape sequences replaced the author's characters at runtime. Two layers are changed: js_printer: print_raw_template_literal and print_reg_exp_literal now write the source bytes verbatim regardless of ASCII_ONLY, matching esbuild. JSC handoff: every site that hands printer output or // @Bun source to JavaScriptCore now uses clone_utf8 (ASCII fast path unchanged) instead of clone_latin1, so multi-byte UTF-8 in the printed output is decoded correctly rather than split into Latin-1 mojibake. The watcher ref-counted external-string path is kept for pure-ASCII output and falls through to the clone path otherwise. Runtime transpiler cache entries are now tagged UTF-8 on disk and the cache version is bumped to 23. Un-skips the existing test in 14976.test.ts that covered this. Fixes #18115 Fixes #8745 Fixes #16763 Fixes #26785
|
Warning Review limit reached
Next review available in: 5 minutes 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Added Leaving #25767 alone: its repro as written ( |
|
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}
| fn print_raw_template_literal(&mut self, bytes: &[u8]) { | ||
| if IS_JSON || !ASCII_ONLY { | ||
| self.print(bytes); | ||
| return; | ||
| } | ||
|
|
||
| // Translate any non-ASCII to unicode escape sequences | ||
| // Note that this does not correctly handle malformed template literal strings | ||
| // template literal strings can contain invalid unicode code points | ||
| // and pretty much anything else | ||
| // | ||
| // we use WTF-8 here, but that's still not good enough. | ||
| // | ||
| let mut ascii_start: usize = 0; | ||
| let mut is_ascii = false; | ||
| let iter = CodepointIterator::init(bytes); | ||
| let mut cursor = strings::Cursor::default(); | ||
|
|
||
| while iter.next(&mut cursor) { | ||
| match cursor.c as u32 { | ||
| // unlike other versions, we only want to mutate > 0x7F | ||
| 0..=LAST_ASCII => { | ||
| if !is_ascii { | ||
| ascii_start = cursor.i as usize; | ||
| is_ascii = true; | ||
| } | ||
| } | ||
| _ => { | ||
| if is_ascii { | ||
| self.print(&bytes[ascii_start..(cursor.i as usize)]); | ||
| is_ascii = false; | ||
| } | ||
|
|
||
| match cursor.c as u32 { | ||
| c @ 0..=0xFFFF => self.print(&bmp_escape(c)[..]), | ||
| _ => { | ||
| self.print(b"\\u{"); | ||
| let _ = self.fmt(format_args!("{:x}", cursor.c)); | ||
| self.print(b"}"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if is_ascii { | ||
| self.print(&bytes[ascii_start..]); | ||
| } | ||
| // `TemplateStringsArray.raw` exposes these bytes verbatim at | ||
| // runtime, so re-encoding non-ASCII as escape sequences would | ||
| // change the observed string value. esbuild exempts these too. | ||
| self.print(bytes); |
There was a problem hiding this comment.
🔴 Removing these escape loops means --target=bun printer output can now contain multi-byte UTF-8, but several sibling consumers of that output still hard-code Latin-1 and were not updated: (1) Bake production prerender — OutputFile::Value::to_bun_string_ref / to_bun_string (src/bundler/OutputFile.rs:274-289, :225-263) pass is_latin1=true unconditionally, so String.raw`café`in a server module now evaluates to"café"at prerender time; **(2)**--bytecode/--compile—generateCachedModuleByteCodeFromSourceCode/...CommonJSProgram...(src/jsc/bindings/ZigSourceProvider.cpp:213/248) build the build-timeSourceCodefrom astd::spanover the printer bytes, so the cached bytecode'sSourceCodeKeyno longer matches the runtime one (nowclone_utf8) and the .jscis silently discarded; **(3)** Bake dev-server no-source-map branch —DevServer.rs:4397still usesclone_latin1(&server_bundle)(its sibling at :4379 already usesclone_utf8). The --compilepath at StandaloneModuleGraph.rs:928-935 already carries thefirst_non_asciiguard whose comment documents the invariant this PR removes — the same guard (or a UTF-8 constructor) is needed at these sites. Also note the existing14976.test.ts "bun build --target=bun outputs only ascii"` test still passes only because its fixture has non-ASCII solely in identifiers/strings; the invariant it asserts is now false in general.
Extended reasoning...
What changed and what it broke
Before this PR, the printer instantiation used for --target=bun (js_printer/lib.rs:8203-8204 — Printer<…, ASCII_ONLY=true, …, IS_BUN_PLATFORM=true, …>) guaranteed pure-ASCII output: print_raw_template_literal escaped every codepoint > 0x7F when ASCII_ONLY, and print_reg_exp_literal did the same when IS_BUN_PLATFORM. Several consumers of --target=bun printer bytes relied on that invariant to interpret the bytes as Latin-1 (byte-per-char). This PR removes both escape loops, so String.raw`café`and/café/ now emit raw UTF-8 (63 61 66 C3 A9) into the chunk buffer. The PR description enumerates six Rust-side clone_latin1` sites plus two watcher branches and updates them, but three sibling consumers of the same invariant were missed.
(1) Bake production prerender — correctness regression
src/runtime/bake/production.rs bundles server/SSR chunks with target = bun_ast::Target::Bun (bake_body.rs:1207) and serves them to JSC via BakeProdLoad (production.rs:1648-1650) → OutputFile::Value::to_bun_string_ref(). At src/bundler/OutputFile.rs:283-288:
// latin1 = true.
BunString::create_external::<*mut c_void>(
bytes,
true,
core::ptr::null_mut::<c_void>(),
noop,
)There is no first_non_ascii guard (nor in to_bun_string at :252-257). bakeModuleLoaderFetch (BakeGlobalObject.cpp:135-145) calls source.toWTFString() on this Latin-1-tagged external string and hands it to JSC::SourceCode, so the module evaluates the mis-decoded source: UTF-8 café (5 bytes) becomes the 5-char source text café, and String.raw`café`returns"café"` at prerender time. This is exactly the bug the PR is fixing, reintroduced on the Bake production static-build path.
(2) --bytecode / --compile — silently defeated
--bytecode requires --target=bun (Arguments.rs:2006). generateChunksInParallel.rs:1057 / writeOutputFilesToDisk.rs:412 pass &code_result.buffer (raw printer bytes) → CachedBytecode::generate → generateCachedModuleByteCodeFromSourceCode / generateCachedCommonJSProgramByteCodeFromSourceCode at src/jsc/bindings/ZigSourceProvider.cpp:213/248:
std::span<const Latin1Character> sourceCodeSpan(inputSourceCode, inputSourceCodeSize);
JSC::SourceCode sourceCode = JSC::makeSource(WTF::String(sourceCodeSpan), …);Each UTF-8 byte becomes one Latin-1 code unit, so JSC parses café and derives the cached SourceCodeKey from the hash of that mis-decoded string. At runtime the already_bundled path now decodes correctly via clone_utf8 (this PR's own changes at jsc_hooks.rs:2648 / RuntimeTranspilerStore.rs:1037), so the runtime SourceCodeKey differs, JSC's decodeCodeBlockImpl rejects the cached bytecode, and it silently re-parses. Net effect: for any chunk containing non-ASCII in a tagged-template raw segment or regex literal, --bytecode pays the build-time, disk-space, and runtime-load cost, then throws the bytecode away.
(3) Bake dev-server no-source-map branch
src/runtime/bake/DevServer.rs:4397 still calls BunString::clone_latin1(&server_bundle) on target=bun bundler output; its with-source-map sibling at :4379 already uses clone_utf8.
Why existing code doesn't prevent it
The --compile sibling at src/standalone_graph/StandaloneModuleGraph.rs:928-935 already carries exactly the guard needed:
// The printer escapes non-ASCII for server-side JS, but …
encoding: match output_file.loader {
Loader::Js | … if strings::first_non_ascii(buf_bytes).is_none() => Encoding::Latin1,
_ => Encoding::Binary,
},Its comment explicitly documents the invariant this PR removes. The three sites above were correct only because of the removed escape loops. Also note test/regression/issue/14976/14976.test.ts "bun build --target=bun outputs only ascii" still passes only because its fixture (import_target.ts) has non-ASCII solely in identifiers/string literals, which are still escaped; the invariant it asserts is now false in general and should probably be updated or removed.
Step-by-step proof (Bake path)
- Server module contains
const s = String.raw`café`;and is bundled withtarget = Target::Bun. - Printer instantiates with
ASCII_ONLY=true, IS_BUN_PLATFORM=true(lib.rs:8203-8204). After this PR,print_raw_template_literalwrites the 5 UTF-8 bytes63 61 66 C3 A9verbatim into the chunk buffer. - Chunk is stored in
pt.bundled_outputsasValue::Buffer { bytes }. BakeProdLoadreturnsbundled_outputs[idx].value.to_bun_string_ref()→BunString::create_external(bytes, /*is_latin1=*/true, …)(OutputFile.rs:283-288).bakeModuleLoaderFetchdoessource.toWTFString()— the external Latin-1 impl converts byte-per-char to UTF-16, yielding the 5-char stringc a f U+00C3 U+00A9="café"— and wraps it in aJSC::SourceCode.- JSC evaluates
String.raw`café`→"café"(length 5, not 4).
Fix
Apply the same strings::first_non_ascii(bytes).is_none() guard (fall through to clone_utf8 / WTF::String::fromUTF8 otherwise) at OutputFile.rs:252-257 / :283-288, ZigSourceProvider.cpp:213/248, and DevServer.rs:4397 — the same shape already applied at the two watcher branches in this PR and at StandaloneModuleGraph.rs:928-935.
| // Borrowed UTF-8 view: `to_file` only reads `byte_slice()` + the | ||
| // encoding tag, and `output_code_bytes` outlives the synchronous | ||
| // `to_file` call. Printer output may contain multi-byte UTF-8 | ||
| // (raw tagged-template / regex literals) so tag it UTF-8; the |
There was a problem hiding this comment.
🟡 nit: borrow_utf8 sets the ZigString UTF-8 tag, so source_code.is_utf8() in to_file() is now always true and routes into OutputCode::Utf8(Box::from(source_code.byte_slice())) — a fresh alloc+memcpy of the entire transpiled output on every cache write, where before BunString::ascii() took the zero-copy OutputCode::String(*source_code) arm. Entry::save already handles OutputCode::String with is_utf8()==true → Encoding::UTF8, so the Box::from branch in to_file could be dropped entirely (or gate on first_non_ascii like the two watcher sites).
Extended reasoning...
What changed
The vtable put() bridge switched the borrowed view from BunString::ascii(output_code_bytes) to BunString::borrow_utf8(output_code_bytes). borrow_utf8 calls ZigString::init_utf8 → mark_utf8(), which sets the UTF-8 pointer-tag bit on the ZigString. Consequently BunString::is_utf8() (which for the ZigString tag returns zig.is_utf8()) now returns true for this string, whereas BunString::ascii() left the tag unset and is_utf8() returned false.
Code path
Inside RuntimeTranspilerCache::to_file:
let output_code: OutputCode = if source_code.is_utf8() {
OutputCode::Utf8(Box::from(source_code.byte_slice()))
} else {
OutputCode::String(*source_code)
};Before this PR, the vtable put() passed an untagged (Latin-1) BunString, so is_utf8() was false and the zero-copy OutputCode::String(*source_code) arm ran — a refcount-neutral by-value copy of the BunString handle, no allocation. After this PR, is_utf8() is true and the OutputCode::Utf8(Box::from(...)) arm runs, which allocates a fresh Box<[u8]> and memcpys the entire printer output into it. The pre-existing comment on this line even flags it: "PERF: add a borrowed OutputCode variant to avoid the copy".
Step-by-step
js_printer/lib.rs:8055callscache.put(printer.writer.slice(), ...)for every printed module ≥MINIMUM_CACHE_SIZE(4 KiB) that missed the cache.- The
Jscvtableput()wraps the printer bytes asBunString::borrow_utf8(output_code_bytes)— UTF-8 tag set. to_file(&output_code)checkssource_code.is_utf8()→true.Box::from(source_code.byte_slice())allocatesoutput_code_bytes.len()bytes and memcpys them.Entry::savereadsoutput_code.byte_slice()(identical bytes either way) and writesEncoding::UTF8— the same encoding tag it would have written forOutputCode::String(s)wheres.is_utf8().
So the on-disk bytes and encoding tag are identical whether the copy happens or not; the allocation is pure overhead.
Why nothing prevents it
The is_utf8() branch in to_file was originally written for the other caller (RuntimeTranspilerCache::put on the struct, which does clone_utf8 and needs an owned copy anyway). The vtable bridge previously dodged it by passing an ASCII-tagged string; changing the tag to UTF-8 for correctness (so the on-disk encoding is right) accidentally routes into the copying arm.
Impact
One alloc + memcpy of the full transpiled output per cache miss — i.e., once per unique source file per cache version. Immediately followed by open + preallocate_file + pwritev + rename of the same bytes to disk, which dominates by orders of magnitude. No correctness impact. Not merge-blocking.
Fix
Entry::save already handles the OutputCode::String(str) case where str.is_utf8() → Encoding::UTF8, so the Box::from arm in to_file is redundant and can be dropped entirely — always take OutputCode::String(*source_code). Alternatively, gate on strings::first_non_ascii(output_code_bytes).is_none() and use BunString::ascii() for pure-ASCII output (matching the two watcher sites in this PR), or add the borrowed OutputCode variant the existing PERF TODO already asks for.
1 participant
Fixes #18115
Fixes #8745
Fixes #16763
Fixes #26785
Fixes #18859
Repro
Every file
bunexecutes goes through the printer, so any tagged-template DSL that reads.raw(SQL / GraphQL / CSS-in-JS / HTML /String.dedent) received ASCII escape sequences the moment an accent,©, or emoji appeared in a template. The same escape sequences were baked intobun build --target=bunoutput while--target=node/browserkept the raw text, so identical source computed different strings per target.Cause
Two layers cooperated to mangle the bytes.
Printer.
print_raw_template_literalandprint_reg_exp_literalinsrc/js_printer/lib.rsre-encoded codepoints > 0x7F as\uXXXXwhen printing for the bun runtime target. BothTemplateStringsArray.rawandRegExp.prototype.sourceexpose the source bytes verbatim by spec, so escape sequences replaced the author's characters at runtime.JSC handoff. Six Rust sites cloned the printer output (or raw
// @bunsource contents) as Latin-1 unconditionally:jsc_hooks.rs(sync load + already-bundled),RuntimeTranspilerStore.rs(async transpile + already-bundled),AsyncModule.rs(async load), and theRuntimeTranspilerCachewrite path. Plus two watcher branches that routed throughref_counted_resolved_source, which creates an external Latin-1WTFString. The Latin-1 clone was only correct because the printer had historically guaranteed ASCII output.Fix
print_raw_template_literal/print_reg_exp_literalnow write the source bytes verbatim. esbuild exempts template contents from--charset=asciifor the same reason.clone_utf8instead ofclone_latin1.BunString__fromBytesalready takes the Latin-1 fast path for pure-ASCII input, so the common case pays nothing new.first_non_asciiand keep the zero-copy external-Latin-1 path for pure-ASCII output, falling through to the encoding-aware clone otherwise. InAsyncModule.rsthe watcher'sadd_fileregistration now runs regardless of encoding.RuntimeTranspilerCachevtableput()tags the on-disk entry as UTF-8 (whose load path already has an ASCII fast path) instead of Latin-1. Cache version bumped 22 → 23.test/regression/issue/14976/14976.test.tsthat covered this exact property.Verification
test/regression/issue/18115.test.tsruns the same fixture (Latin-1, CJK, astral codepoints, multi-part tagged template, regex.source) through the sync load path, the async transpiler store (require), the// @bunalready-bundled path,--watch, the runtime transpiler cache round-trip, andbun build --target=bun→ run. The--target=node/browserarms guard that the previously-correct paths stay correct.Before: 6 of 8 fail (
.rawis\uXXXXsequences,.sourcelengths wrong). After: 8 pass.bun bd test test/bundler/transpiler/template-literal.test.ts test/regression/issue/14976/ test/cli/run/transpiler-cache.test.ts test/bundler/bundler_string.test.ts test/bundler/bundler_bun.test.ts test/bundler/transpiler/transpiler.test.js test/cli/hot/hot.test.ts: all pass.