Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The printer escaped non-ASCII codepoints to `\uXXXX` when emitting raw template literals and regex literals. Per spec, both `TemplateStringsArray.raw` (surfaced via `String.raw`) and `RegExp.prototype.source` must expose source bytes verbatim, so the escape changed runtime string values: bun -e 'console.log(String.raw`╭─╮`.length, /╭─╮/.source.length)' # before: 18 18 # after: 3 3 Two layers cooperated to mangle the bytes: 1. The printer (`print_raw_template_literal`, `print_reg_exp_literal` in `src/js_printer/lib.rs`) re-escaped codepoints > 0x7F. Both are spec-required to expose source bytes verbatim, so they now write them through unchanged. 2. Five sites cloned the printer output (or already-bundled source contents) as Latin-1 unconditionally, mis-tagging UTF-8 bytes once the printer started emitting them verbatim. Replaced with `clone_utf8`, which auto-detects all-ASCII → Latin-1 (zero-cost, identical to the old fast path) and non-ASCII → UTF-16. The runtime transpiler cache also stored printer output via `clone_latin1` in `RuntimeTranspilerCache::put()`, so on-disk entries written before this fix mis-tag UTF-8 as Latin-1. Bumped `EXPECTED_VERSION` 22 → 23 to invalidate stale entries on read. Fixes oven-sh#18115.
kernoeb
force-pushed
the
fix-18115-string-raw-non-ascii
branch
from
May 25, 2026 15:38
dbfcb1b to
87f3c98
Compare
Contributor
|
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 (2)
WalkthroughPrinter now emits raw bytes for template and regex literals; transpiler and runtime paths decode printer output as UTF‑8; runtime transpiler cache format/version updated; tests added to verify non‑ASCII characters are preserved verbatim. ChangesNon-ASCII Preservation in Raw Literals and Transpiler Pipeline
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/RuntimeTranspilerCache.rs (1)
1105-1116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix vtable
RuntimeTranspilerCache::put()mis-encoding of non-ASCII outputThe vtable
put(line 1108) buildsBunString::ascii(output_code_bytes).BunString::asciiis justZigString::init(no UTF-8 ptr-tag), soRuntimeTranspilerCache::to_filewon’t take the UTF-8 path andEntry::saveends up classifying/storing the cache entry asLATIN1—unlike the normalputpath (line 1039) which usesBunString::clone_utf8.Suggested fix
- // Borrowed Latin-1 view: `to_file` only reads `byte_slice()` + the encoding - // tag (unmarked 8-bit ZigString -> Encoding::LATIN1, same as clone_latin1), - // and `output_code_bytes` outlives the synchronous `to_file` call. - let output_code = BunString::ascii(output_code_bytes); + // Keep cache encoding consistent with the non-ASCII printer output path. + let output_code = BunString::clone_utf8(output_code_bytes); + let _output_code_guard = scopeguard::guard(output_code, |s| s.deref()); let result = RuntimeTranspilerCache::to_file( this.input_byte_length.unwrap(), this.input_hash.unwrap(), this.features_hash.unwrap(), sourcemap, esm_record, &output_code, this.exports_kind,🤖 Prompt for 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. In `@src/jsc/RuntimeTranspilerCache.rs` around lines 1105 - 1116, The vtable implementation of RuntimeTranspilerCache::put currently constructs output_code with BunString::ascii(output_code_bytes), which creates a ZigString without the UTF-8 tag and causes RuntimeTranspilerCache::to_file / Entry::save to treat non-ASCII as LATIN1; change the construction to use the UTF-8 variant (e.g., BunString::clone_utf8 or any constructor that sets the UTF-8 ptr-tag) so RuntimeTranspilerCache::to_file follows the same UTF-8 path as the normal put path and non-ASCII output is encoded/stored correctly.
🤖 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/bundler/transpiler/transpiler.test.js`:
- Around line 3643-3644: Add an astral-plane RegExp.source case to the
regression block by adding an assertion using expectPrinted for an emoji regex,
e.g., call expectPrinted("/🐰/.source", "/🐰/.source"); so the test covers an
astral-plane character for RegExp.source alongside the existing BMP cases; place
the new expectPrinted line near the other RegExp.source assertions in
transpiler.test.js.
In `@test/regression/issue/18115.test.ts`:
- Around line 10-21: The test should capture the spawned process's stderr and
only assert exit code last; update the Bun.spawn usage around proc to await
proc.stderr.text() along with proc.stdout.text() and proc.exited (e.g., gather
stdout, stderr, exitCode), and if stdout or exitCode assertions might fail
include a conditional or explicit expect(stderr).toBe("") (or log stderr)
immediately before expect(exitCode).toBe(0); adjust the checks around
proc.stdout.text(), proc.exited, and expect(exitCode).toBe(0) to ensure stderr
is available for diagnostics on failure.
---
Outside diff comments:
In `@src/jsc/RuntimeTranspilerCache.rs`:
- Around line 1105-1116: The vtable implementation of
RuntimeTranspilerCache::put currently constructs output_code with
BunString::ascii(output_code_bytes), which creates a ZigString without the UTF-8
tag and causes RuntimeTranspilerCache::to_file / Entry::save to treat non-ASCII
as LATIN1; change the construction to use the UTF-8 variant (e.g.,
BunString::clone_utf8 or any constructor that sets the UTF-8 ptr-tag) so
RuntimeTranspilerCache::to_file follows the same UTF-8 path as the normal put
path and non-ASCII output is encoded/stored correctly.
🪄 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: 58c122b5-c4e9-470b-93d4-0edb0e19b674
📒 Files selected for processing (7)
src/js_printer/lib.rssrc/jsc/AsyncModule.rssrc/jsc/RuntimeTranspilerCache.rssrc/jsc/RuntimeTranspilerStore.rssrc/runtime/jsc_hooks.rstest/bundler/transpiler/transpiler.test.jstest/regression/issue/18115.test.ts
…re stderr - Add `/🐰/.source` to printer test for parity with the String.raw astral case. - Capture and assert stderr in the regression spawn tests for better diagnostics on failure.
…gression test
CodeRabbit caught a third site that was missed in the initial fix:
`RuntimeTranspilerCache`'s vtable `put()` (the FFI bridge from the
parser) still used `BunString::ascii`, an unmarked 8-bit ZigString
that `Entry::save` classifies as Latin-1 — mis-tagging non-ASCII
UTF-8 output exactly like the original `clone_latin1` bug.
Switched to `clone_utf8` and added a scopeguard for the WTFStringImpl
refcount, since unlike the non-vtable path the vtable variant does
not transfer ownership of the BunString to `self`.
Added a regression test that:
- writes a > 4 KiB module containing `String.raw`Redémarrage``
through the JSC vtable `put()` path (cache miss → write),
- asserts the cache file was actually created,
- re-runs with `BUN_DEBUG_ENABLE_RESTORE_FROM_TRANSPILER_CACHE=1`
to force read-back from cache (cache hit) and asserts the
output is identical and well-formed.
Verified the test fails without the fix:
Expected: "["Redémarrage","╭─╮","🐰"]"
Received: "["Redémarrage","âââ®","ð°"]"
This was referenced Jul 9, 2026
Collaborator
|
Thanks for this, and sorry it sat for so long. Four PRs ended up fixing this bug, so we are consolidating on #33866, which has just been rebased onto current main. It makes the same printer change and the same clone_latin1 -> clone_utf8 changes as this PR, and additionally handles the paths that have appeared or changed since (the --hot ref-counted source path, the transpiler cache vtable put, Bun.build artifact text, the --bytecode / --compile / NODE_COMPILE_CACHE bytecode key, and bun test --coverage). Your transpiler cache round-trip test is included there, and you are credited as a co-author on the commit. Closing this one in favour of #33866. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #18115.
Repro
Cause
Two layers cooperated to mangle the bytes:
Printer.
print_raw_template_literalandprint_reg_exp_literalinsrc/js_printer/lib.rsre-escaped codepoints > 0x7F to\uXXXX. BothTemplateStringsArray.raw(surfaced viaString.raw) andRegExp.prototype.sourceare spec-required to expose source bytes verbatim, so escaping silently changed the runtime string values.JSC handoff. Five sites cloned the printer output (or raw
source.contents) as Latin-1 unconditionally. Once the printer emits UTF-8 bytes verbatim,clone_latin1mis-tags them (RedémarragebecomesRedémarrage).Fix
print_raw_template_literal/print_reg_exp_literalnow write source bytes verbatim.All five
clone_latin1sites that ingest printer output or pre-bundled source contents switch toclone_utf8, which auto-detects all-ASCII → Latin-1 (identical to the old fast path, zero cost) and non-ASCII → UTF-16:RuntimeTranspilerStore::transpile(printer output)RuntimeTranspilerStorealready-bundled pathAsyncModule::fulfill(async printer output)transpile_source_code_inner(sync printer output,jsc_hooks)transpile_source_code_inneralready-bundled path (jsc_hooks)RuntimeTranspilerCache::put()also stored printer output viaclone_latin1, so cached entries written before this fix mis-tag UTF-8 as Latin-1. Switched toclone_utf8and bumpedEXPECTED_VERSION22 → 23 to invalidate stale entries on read.Test plan
bun bd test test/regression/issue/18115.test.ts— 2/2 pass (String.rawandRegExp.sourceacross Latin-1, BMP CJK, box-drawing, astral planes).bun bd test test/bundler/transpiler/transpiler.test.js— 157/157 including the new printer-level assertions coveringé,中,╭─╮,🐰.bun bd test test/cli/run/transpiler-cache.test.ts— 9/9.USE_SYSTEM_BUN=1— gate confirmed.Performance
Apples-to-apples, same commit, vanilla vs patched, M-series ARM64, release build:
String.raw, lots of accents in strings/comments)String.rawfileThe +4% on
String.raw-heavy files comes from the unavoidable UTF-8 → UTF-16 transcode (correct output cannot be served as Latin-1 when a codepoint > 0xFF appears). Tried a Latin-1-first fast path inBunString__fromUTF8— measured worse, reverted (simdutfconvert_utf8_to_latin1validates every byte, exceeding the alloc saving).