Conversation
Bun's runtime transpiler escaped every non-ASCII code point in a regex
literal as \uXXXX before handing the printed source to JSC (the buffer
was passed through a Latin-1 StringImpl). That kept the matching
behaviour intact but changed RegExp.prototype.source at runtime, so
/\u00B6/u.source returned "\\u00B6" instead of the pilcrow
character.
Puppeteer's bundled parsel-js (used for ::-p-xpath / ::-p-text
selectors) derives a second pattern by string-replacing on .source and
missed because the literal character was no longer there, leaving the
captured argument empty.
Print the regex pattern verbatim (UTF-8) and switch every consumer of
the printed buffer to clone_utf8 / borrow_utf8 so non-ASCII bytes are
decoded rather than memcpy'd as Latin-1:
- jsc_hooks / RuntimeTranspilerStore / AsyncModule: clone_utf8
(BunString__fromBytes keeps the Latin-1 fast path for all-ASCII
output, which is the common case)
- VirtualMachine::ref_counted_resolved_source: fall back to
clone_utf8 when the buffer is not all-ASCII (the ref-string cache
wraps bytes in a Latin-1 external string)
- RuntimeTranspilerCache put: tag cached output UTF-8 so the
Encoding::UTF8 read path (which already branches on is_all_ascii)
round-trips it; bump the cache version so old escaped entries are
invalidated.
Fixes #13853
|
Warning Review limit reached
Next review available in: 1 minute 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 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
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.
Beyond the inline findings, I also checked the DevServer server-side HMR patch path (bake_load_server_hmr_patch at src/runtime/bake/DevServer.rs:4361), which also passes bundler output through clone_latin1 — verifiers concluded it is not affected by this change.
Extended reasoning...
The inline finding on the already_bundled fast path and the C++ bytecode generator (ZigSourceProvider.cpp) is the substantive concern here — removing the IS_BUN_PLATFORM regex escape breaks the all-ASCII invariant for --target=bun bundler output, and those two consumers were not swept alongside the four runtime-transpiler clone_latin1 → clone_utf8 conversions. The DevServer HMR clone_latin1 site was separately examined and ruled out as a sibling of the same class.
| // The pattern is printed verbatim (UTF-8), even under `IS_BUN_PLATFORM`: | ||
| // rewriting `/¶/u` as `/\u00B6/u` changes `RegExp.prototype.source` at | ||
| // runtime. The consumers of the transpiled buffer treat it as UTF-8 | ||
| // (see `String::clone_utf8` at the `ResolvedSource` construction sites). | ||
| self.print(&e.value[..]); |
There was a problem hiding this comment.
🔴 Removing the IS_BUN_PLATFORM regex escape breaks the all-ASCII invariant that two other consumers of --target=bun printer output still depend on: the already_bundled fast path (String::clone_latin1(&parse_result.source.contents) in src/runtime/jsc_hooks.rs:2759 and the same call in RuntimeTranspilerStore.rs) and the build-time bytecode generator (generateCachedModuleByteCodeFromSourceCode / the CJS twin in src/jsc/bindings/ZigSourceProvider.cpp:211-214,246-250, which construct WTF::String(std::span<const Latin1Character>(...))). A source containing /¶/u now round-trips through bun build --target=bun → bun run, or bun build --bytecode / --compile, as /¶/u — the regex no longer matches, which is a functional regression (previously it emitted /\u00B6/u and matched correctly). These are sibling sites of the four clone_latin1 → clone_utf8 conversions this PR does apply and need the same treatment (or the bundler path should keep escaping).
Extended reasoning...
What the bug is
This PR deletes the IS_BUN_PLATFORM non-ASCII-escaping branch in print_reg_exp_literal so regex patterns are printed verbatim as UTF-8, and updates the runtime-transpiler consumers of that buffer to decode UTF-8 (clone_latin1 → clone_utf8 at four sites, plus an is_all_ascii fallback in ref_counted_resolved_source). But two other consumers of the same printer's output still interpret it as Latin-1 and were not updated:
- The
already_bundledruntime fast path —bun_core::String::clone_latin1(&source.contents)atsrc/runtime/jsc_hooks.rs:2759and the identical block insrc/jsc/RuntimeTranspilerStore.rs(theAlreadyBundledarm around line 1005). - The build-time bytecode generator —
generateCachedModuleByteCodeFromSourceCodeandgenerateCachedCommonJSProgramByteCodeFromSourceCodeinsrc/jsc/bindings/ZigSourceProvider.cpp:211-214/:246-250declareconst Latin1Character* inputSourceCodeand constructWTF::String(std::span<const Latin1Character>(inputSourceCode, inputSourceCodeSize)).
Both paths feed on the bundler's --target=bun output, which after this change can contain raw multi-byte UTF-8.
The specific code path
For the bundler round-trip: bun build --target=bun input.js → LinkerContext.rs calls js_printer::print_with_writer(..., ast.target, ...) → lib.rs:7816 sees target.is_bun() and dispatches to print_with_writer_and_platform::<_, /*IS_BUN_PLATFORM=*/true, _> (the type alias at lib.rs:7863-7864 sets both ASCII_ONLY=true and IS_BUN_PLATFORM=true). Strings and identifiers are still ASCII-escaped by ASCII_ONLY, but after this PR print_reg_exp_literal writes the pattern verbatim, so /¶/u puts raw bytes 0xC2 0xB6 into the chunk. postProcessJSChunk.rs prepends // @bun and the bundle is written as UTF-8. When the bundle is executed, the // @bun pragma triggers AlreadyBundled, and both jsc_hooks.rs:2759 and the async twin in RuntimeTranspilerStore.rs build the ResolvedSource with String::clone_latin1(&source.contents) → BunString__fromLatin1, which memcpy's each byte as one Latin-1 code point.
For bytecode: --bytecode and --compile both force target=bun (Arguments.rs / build_command.rs), so the same IS_BUN_PLATFORM=true printer runs. generateChunksInParallel.rs:1057 / writeOutputFilesToDisk.rs:412 pass &code_result.buffer to generate_cached_bytecode → __bun_jsc_generate_cached_bytecode → the C++ FFI, with no transcoding. The C++ side then constructs WTF::String from a Latin1Character span — bytewise Latin-1 decode.
Why existing code doesn't prevent it
The four clone_latin1 → clone_utf8 conversions in this PR sit on the runtime-transpiler print path (freshly-transpiled source going straight to JSC). The two sites above sit on different paths that consume the bundler's --target=bun output — the // @bun fast path skips transpilation entirely and hands the raw file bytes to JSC, and bytecode generation happens at build time in C++ before any of the Rust-side runtime consumers are involved. Fixing the four runtime sites doesn't touch either of these; fixing the already_bundled sites doesn't fix the C++ bytecode path and vice versa. Before this PR the IS_BUN_PLATFORM regex escape guaranteed --target=bun output was all-ASCII, so clone_latin1 / the Latin1Character* span were equivalent to UTF-8 decoding; the PR removes that guarantee without updating these consumers.
Step-by-step proof
Take input.js = console.log(/¶/u.test("¶")).
Bundle round-trip:
bun build --target=bun input.js -o out.js→ printer emits/¶/uverbatim;out.jscontains bytes… 2F C2 B6 2F 75 …prefixed with// @bun.bun out.js→ parser sees// @bun, returnsAlreadyBundled.jsc_hooks.rs:2759callsString::clone_latin1(&source.contents)→BunString__fromLatin1treats0xC2as U+00C2 and0xB6as U+00B6.- JSC receives
console.log(/¶/u.test("¶"))(the string literal was already ASCII-escaped byASCII_ONLY, so it decodes fine); printsfalse. - Before this PR the bundle contained
/\u00B6/u, whichclone_latin1passed through byte-for-byte, and JSC parsed as a regex matching U+00B6 → printedtrue. So this is a functional regression in matching behaviour, not just.sourcecosmetics.
Bytecode / compile:
bun build --compile input.js -o app(or--target=bun --bytecode) → same printer, same0xC2 0xB6incode_result.buffer.generate_cached_bytecodeforwardsbuffer.as_ptr()/len()unchanged togenerateCachedModuleByteCodeFromSourceCode.ZigSourceProvider.cpp:213buildsWTF::String(std::span<const Latin1Character>(ptr, len))→ JSC parses/¶/uand bakes it into the.jsccache / compiled executable.- Running
./appprintsfalse; before this PR it printedtrue.
Impact
bun build --target=bun → bun run, bun build --bytecode, and bun build --compile all produce silently-wrong runtime behaviour for any source containing a non-ASCII regex literal — the regex no longer matches what it did before, and .source is mojibake. This reintroduces (in a worse form — matching is broken, not just .source) the exact bug class this PR fixes for the runtime transpiler, on the bundler / compiled-binary path. Per REVIEW.md "Fix the whole class in the same PR", these are missed sibling sites of the sweep the PR performs.
How to fix
Either:
- Convert the two
already_bundledclone_latin1calls toclone_utf8(matching the four this PR already converts), and changeZigSourceProvider.cpp's two bytecode entry points to construct theWTF::StringviaWTF::String::fromUTF8(...)(or checkcharactersAreAllASCIIand branch); or - Keep the
\uXXXXescaping for regex literals on the bundler path only (gate on something other thanIS_BUN_PLATFORM, sinceIS_BUN_PLATFORMis shared with the runtime transpiler), leaving the runtime path verbatim.
Whichever is chosen, the tests should cover the bun build --target=bun → bun run round-trip and --compile / --bytecode (per REVIEW.md "Cover the variant matrix … alternate modes (--compile, --bytecode)").
| // 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 is ASCII except for RegExp | ||
| // literals (printed verbatim so `.source` is preserved), so tag as | ||
| // UTF-8; the read path's `Encoding::UTF8` branch handles both the | ||
| // all-ASCII fast path and the rare multi-byte case. | ||
| let output_code = BunString::borrow_utf8(output_code_bytes); |
There was a problem hiding this comment.
🟡 Switching BunString::ascii → BunString::borrow_utf8 here now routes every cache write through to_file's if source_code.is_utf8() { OutputCode::Utf8(Box::from(...)) } arm — a full alloc+memcpy of the printer output — where it previously hit the zero-copy OutputCode::String(*source_code) arm. Entry::save already handles a UTF-8-tagged OutputCode::String correctly (its match checks str.is_utf8() → Encoding::UTF8), so dropping the is_utf8() special case in to_file restores zero-copy with identical on-disk output. Perf-only on the cold-cache-write path, but it makes the "Borrowed UTF-8 view: to_file only reads byte_slice() + the encoding tag" comment above factually wrong.
Extended reasoning...
What changed and what it does
The vtable put bridge for the runtime transpiler cache changed the borrowed source-code view from BunString::ascii(output_code_bytes) to BunString::borrow_utf8(output_code_bytes) (RuntimeTranspilerCache.rs:1093). The updated comment frames this as a zero-copy borrow: "Borrowed UTF-8 view: to_file only reads byte_slice() + the encoding tag". That claim is now false.
The code path
BunString::borrow_utf8 (bun_core/string/mod.rs:185) → ZigString::init_utf8 (mod.rs:1383-1386) → mark_utf8() sets the UTF-8 ptr-tag on a Tag::ZigString. String::is_utf8() (mod.rs:543-545) is matches!(tag, ZigString|StaticZigString) && self.as_zig().is_utf8(), so it now returns true for this borrowed view.
to_file then branches:
let output_code: OutputCode = if source_code.is_utf8() {
OutputCode::Utf8(Box::from(source_code.byte_slice())) // full memcpy
} else {
OutputCode::String(*source_code) // zero-copy borrow
};Before this PR, BunString::ascii (mod.rs:193) → ZigString::init produced an unmarked ZigString → is_utf8() == false → the zero-copy OutputCode::String arm was taken. After this PR, every call takes the Box::from arm and heap-allocates + memcpys the entire printed output. That branch even carries a pre-existing // PERF: add a borrowed OutputCode variant to avoid the copy TODO — before this PR that TODO was dormant because nothing on the hot path hit it; this PR routes every cache write through it.
Step-by-step proof
js_printer/lib.rs:7715callscache.put(printer.writer.slice(), ...)on thebun_ast::RuntimeTranspilerCacheafter printing, say, a 30 KBnode_modulesfile.r#impl = Some(Jsc)(set in bothRuntimeTranspilerStore.rsandjsc_hooks.rs), soputdispatches to this vtable arm.BunString::borrow_utf8(output_code_bytes)produces aTag::ZigStringwith the UTF-8 ptr-tag set.to_filereceives&output_code;source_code.is_utf8()returns true.Box::from(source_code.byte_slice())allocates 30 KB on the worker's mimalloc heap and memcpys the printer buffer into it.Entry::savereadsoutput_code.byte_slice()off the Box and writes it viapwritev.- The Box is dropped at the end of
to_file.
Contrast with the pre-PR path: step 3 produced an unmarked ZigString, step 4 returned false, step 5 was OutputCode::String(*source_code) (a 24-byte struct copy of a borrowed view), and step 6 read the same bytes directly from the printer buffer. Same on-disk output, zero extra allocation.
Note that the non-vtable RuntimeTranspilerCache::put (which uses clone_utf8 → a WTFStringImpl-tagged string) is unaffected — is_utf8() only returns true for ZigString/StaticZigString tags, so that path still hits the zero-copy arm. Only the vtable bridge regressed, but the vtable bridge is what every runtime transpile actually uses.
Why the copy is unnecessary
Entry::save already handles a UTF-8-tagged OutputCode::String correctly. Its output_encoding match arm for OutputCode::String(str) checks str.is_utf8() → Encoding::UTF8, and OutputCode::byte_slice() for the String variant returns the borrowed bytes via s.byte_slice(). So passing OutputCode::String(*source_code) unconditionally would produce identical on-disk output (same Encoding::UTF8 header byte, same body bytes) with no allocation.
Impact
Fires on every cache-miss transpile of a source file above the 4 KiB MINIMUM_CACHE_SIZE, on both the sync (jsc_hooks) and async (RuntimeTranspilerStore) paths. For a cold node_modules scan of ~1500 files at ~30 KB output each, that is ~45 MB of extra alloc+memcpy on the transpiler worker threads. This is on a path that is already I/O-bound (open/preallocate/pwritev), so wall-clock impact is small in absolute terms — hence nit severity — but it directly contradicts the code comment this PR added.
Suggested fix
Drop the is_utf8() special case in to_file and always take OutputCode::String(*source_code):
// `OutputCode::String` is a refcount-neutral by-value borrow (`BunString` is
// `Copy`, no `Drop`); `Entry::save` derives `Encoding::UTF8` from
// `str.is_utf8()` and reads `byte_slice()`, so no owning copy is needed.
let output_code = OutputCode::String(*source_code);Alternatively, gate the Box::from arm on tag == WTFStringImpl if some other caller genuinely needs the owning box (none currently does).
1 participant
Repro
Puppeteer's bundled parsel-js derives a second pattern by string-replacing on
.source(TOKENS[type].source.replace("(?<argument>¶*)", "(?<argument>.*)")). With the literal pilcrow gone from.sourcethe replace misses, theargumentgroup is never filled, and::-p-xpath(...)/::-p-text(...)selectors come back with an empty value:Cause
The runtime transpiler prints with
IS_BUN_PLATFORM = trueand hands the output to JSC as a Latin-1WTF::StringImpl(clone_latin1/ an external Latin-1 string).print_reg_exp_literaltherefore escaped every non-ASCII code point as\uXXXXso the bytes survived the Latin-1 memcpy (originally added for #2005). For strings and identifiers that rewrite is invisible, but for a regex literal the escape sequence is observable throughRegExp.prototype.source.Fix
Print the regex pattern verbatim (UTF-8, same as esbuild) and make every consumer of the printed buffer decode it rather than memcpy it as Latin-1:
jsc_hooks.rs/RuntimeTranspilerStore.rs/AsyncModule.rs:clone_latin1→clone_utf8.BunString__fromByteskeeps the Latin-1 fast path for all-ASCII output, which is the common case (everything except a non-ASCII regex literal is still escaped to ASCII).VirtualMachine::ref_counted_resolved_source(watcher path): the ref-string cache wraps bytes in a Latin-1 external string; fall back to a plainclone_utf8copy when the buffer is not all-ASCII.RuntimeTranspilerCacheput: tag cached output UTF-8 so the existingEncoding::UTF8read path (which already branches onis_all_ascii) round-trips it; bump the cache version so escaped entries from older builds are re-transpiled.Verification
New
test/regression/issue/13853/13853.test.tscovers Latin-1 / BMP / astral.source, the parsel-js.source.replaceshape, the transpiler-cache write → read round-trip, and re-asserts the original #2005 matching behaviour. End-to-end, Puppeteer'sparsePSelectors("::-p-xpath(//div)")now matches Node:Fixes #13853