-
Notifications
You must be signed in to change notification settings - Fork 5.1k
runtime: preserve non-ASCII characters in RegExp literal .source #35670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,10 @@ | |
| /// path reinstates the bug for any previously-cached TLA module (#30887). | ||
| /// Version 23: `jsx.runtime`/`jsx.development` participate in the features hash, | ||
| /// and tsconfig `"jsx": "react-jsx"` now emits the production runtime (#4227). | ||
| const EXPECTED_VERSION: u32 = 23; | ||
| /// Version 24: RegExp literals are printed verbatim (no `\uXXXX` escaping of | ||
| /// non-ASCII) so `RegExp.prototype.source` matches the source text (#13853); | ||
| /// cached output is now tagged `Encoding::UTF8`. | ||
| const EXPECTED_VERSION: u32 = 24; | ||
|
|
||
| /// Source files smaller than this are not written to / read from the on-disk | ||
| /// transpiler cache. Originally 50 KiB, which excluded almost every file in a | ||
|
|
@@ -1009,7 +1012,10 @@ | |
| return; | ||
| } | ||
| debug_assert!(self.entry.is_none()); | ||
| let output_code = BunString::clone_latin1(output_code_bytes); | ||
| // Printer output is ASCII except for RegExp literals (printed verbatim | ||
| // so `.source` is preserved); `clone_utf8` keeps the Latin-1 fast path | ||
| // for the common all-ASCII case and transcodes to UTF-16 otherwise. | ||
| let output_code = BunString::clone_utf8(output_code_bytes); | ||
| // Refcount stays at 1, sole owner. | ||
| // BunString is Copy with no Drop, so an extra dupe_ref here would leak. | ||
| self.output_code = Some(output_code); | ||
|
|
@@ -1028,7 +1034,7 @@ | |
| } | ||
| #[cfg(debug_assertions)] | ||
| { | ||
| bun_core::scoped_log!(cache, "put() = {} bytes", output_code.latin1().len()); | ||
| bun_core::scoped_log!(cache, "put() = {} bytes", output_code_bytes.len()); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1078,10 +1084,13 @@ | |
| } | ||
| debug_assert!(this.entry.is_none()); | ||
|
|
||
| // 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); | ||
| // 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); | ||
|
Check warning on line 1093 in src/jsc/RuntimeTranspilerCache.rs
|
||
|
Comment on lines
+1087
to
+1093
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Switching Extended reasoning...What changed and what it doesThe vtable The code path
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, Step-by-step proof
Contrast with the pre-PR path: step 3 produced an unmarked ZigString, step 4 returned false, step 5 was Note that the non-vtable Why the copy is unnecessary
ImpactFires on every cache-miss transpile of a source file above the 4 KiB Suggested fixDrop the // `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 |
||
| let result = RuntimeTranspilerCache::to_file( | ||
| this.input_byte_length.unwrap(), | ||
| this.input_hash.unwrap(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| // https://github.com/oven-sh/bun/issues/13853 | ||
| // RegExp literal .source must preserve non-ASCII characters from the original | ||
| // source text. Bun's runtime transpiler used to rewrite /¶/u as /\u00B6/u | ||
| // (to keep the printed output ASCII-only for a Latin-1 source pipeline), | ||
| // which changed the observable value of RegExp.prototype.source and broke | ||
| // packages such as parsel-js/Puppeteer that string-replace on .source. | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, tempDir } from "harness"; | ||
| import { join } from "node:path"; | ||
|
|
||
| test("RegExp literal .source preserves non-ASCII characters (#13853)", async () => { | ||
| // Spawn a fresh process so the fixture is run through the runtime transpiler | ||
| // (this test file itself is also transpiled, but the fixture's bytes are | ||
| // what we want the assertion to observe). | ||
| using dir = tempDir("issue-13853", { | ||
| "index.js": ` | ||
| const results = { | ||
| latin1_no_u: /\u00b6/.source, | ||
| latin1_u: /\u00b6/u.source, | ||
| latin1_v: /\u00b6/v.source, | ||
| cjk: /\u8981\u66ff\u6362/u.source, | ||
| astral: /\u{1d54f}/u.source, | ||
| // via new RegExp the source text is already a runtime string, | ||
| // so this was never broken; kept as a sanity check | ||
| runtime: new RegExp("\u00b6", "u").source, | ||
| }; | ||
| process.stdout.write(JSON.stringify(results)); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "run", join(String(dir), "index.js")], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).toBe(""); | ||
| const got = JSON.parse(stdout); | ||
| expect(got).toEqual({ | ||
| latin1_no_u: "\u00b6", | ||
| latin1_u: "\u00b6", | ||
| latin1_v: "\u00b6", | ||
| cjk: "\u8981\u66ff\u6362", | ||
| astral: "\u{1d54f}", | ||
| runtime: "\u00b6", | ||
| }); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("parsel-js .source.replace pattern works (#13853)", async () => { | ||
| // Minimal reduction of what Puppeteer's bundled parsel-js does for | ||
| // ::-p-xpath() / ::-p-text(): build a RegExp with a literal PILCROW SIGN | ||
| // placeholder, then .source.replace("\u00b6*", ".*") to derive a second | ||
| // pattern. If .source escaped \u00b6 to "\\u00B6" the replace would miss | ||
| // and the derived pattern would fail to capture the argument. | ||
| using dir = tempDir("issue-13853-parsel", { | ||
| "index.js": ` | ||
| const TOKEN = /::(?<name>[-\\w]+)(?:\\((?<argument>\u00b6*)\\))?/gu; | ||
| const src = TOKEN.source.replace("(?<argument>\u00b6*)", "(?<argument>.*)"); | ||
| const derived = new RegExp(src, "gu"); | ||
| derived.lastIndex = 0; | ||
| const m = derived.exec("::-p-xpath(//div)"); | ||
| process.stdout.write(JSON.stringify(m && m.groups)); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "run", join(String(dir), "index.js")], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).toBe(""); | ||
| expect(JSON.parse(stdout)).toEqual({ name: "-p-xpath", argument: "//div" }); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("transpiler cache round-trip preserves non-ASCII RegExp .source (#13853)", async () => { | ||
| // The on-disk transpiler cache used to tag printer output as Latin-1, so a | ||
| // non-ASCII RegExp literal would be corrupted when read back on a cache hit. | ||
| // The file must exceed the 4 KiB minimum cache size. | ||
| const pad = Buffer.alloc(8 * 1024, "a").toString(); | ||
| using dir = tempDir("issue-13853-cache", { | ||
| "a.js": `/* ${pad} */\nprocess.stdout.write(/\u00b6\u65e5/u.source);\n`, | ||
| }); | ||
| const cacheDir = join(String(dir), ".cache"); | ||
| const env = { | ||
| ...bunEnv, | ||
| BUN_RUNTIME_TRANSPILER_CACHE_PATH: cacheDir, | ||
| BUN_DEBUG_ENABLE_RESTORE_FROM_TRANSPILER_CACHE: "1", | ||
| }; | ||
|
|
||
| // First run: cache miss, writes the cache entry. | ||
| { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "run", join(String(dir), "a.js")], | ||
| env, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout).toBe("\u00b6\u65e5"); | ||
| expect(exitCode).toBe(0); | ||
| } | ||
|
|
||
| // Second run: cache hit, reads the entry back. | ||
| { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "run", join(String(dir), "a.js")], | ||
| env, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout).toBe("\u00b6\u65e5"); | ||
| expect(exitCode).toBe(0); | ||
| } | ||
| }); | ||
|
|
||
| test("non-ASCII RegExp literal still matches correctly (#2005 stays fixed)", async () => { | ||
| using dir = tempDir("issue-13853-match", { | ||
| "index.js": ` | ||
| const text = "\u8fd9\u662f\u4e00\u6bb5\u8981\u66ff\u6362\u7684\u6587\u5b57"; | ||
| process.stdout.write(JSON.stringify({ | ||
| literal: text.replace(/\u8981\u66ff\u6362/, ""), | ||
| ctor: text.replace(new RegExp("\u8981\u66ff\u6362"), ""), | ||
| })); | ||
| `, | ||
| }); | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "run", join(String(dir), "index.js")], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(JSON.parse(stdout)).toEqual({ | ||
| literal: "\u8fd9\u662f\u4e00\u6bb5\u7684\u6587\u5b57", | ||
| ctor: "\u8fd9\u662f\u4e00\u6bb5\u7684\u6587\u5b57", | ||
| }); | ||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Removing the
IS_BUN_PLATFORMregex escape breaks the all-ASCII invariant that two other consumers of--target=bunprinter output still depend on: thealready_bundledfast path (String::clone_latin1(&parse_result.source.contents)insrc/runtime/jsc_hooks.rs:2759and the same call inRuntimeTranspilerStore.rs) and the build-time bytecode generator (generateCachedModuleByteCodeFromSourceCode/ the CJS twin insrc/jsc/bindings/ZigSourceProvider.cpp:211-214,246-250, which constructWTF::String(std::span<const Latin1Character>(...))). A source containing/¶/unow round-trips throughbun build --target=bun→bun run, orbun build --bytecode/--compile, as/¶/u— the regex no longer matches, which is a functional regression (previously it emitted/\u00B6/uand matched correctly). These are sibling sites of the fourclone_latin1 → clone_utf8conversions 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_PLATFORMnon-ASCII-escaping branch inprint_reg_exp_literalso regex patterns are printed verbatim as UTF-8, and updates the runtime-transpiler consumers of that buffer to decode UTF-8 (clone_latin1→clone_utf8at four sites, plus anis_all_asciifallback inref_counted_resolved_source). But two other consumers of the same printer's output still interpret it as Latin-1 and were not updated: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).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=bunoutput, 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.rscallsjs_printer::print_with_writer(..., ast.target, ...)→lib.rs:7816seestarget.is_bun()and dispatches toprint_with_writer_and_platform::<_, /*IS_BUN_PLATFORM=*/true, _>(the type alias atlib.rs:7863-7864sets bothASCII_ONLY=trueandIS_BUN_PLATFORM=true). Strings and identifiers are still ASCII-escaped byASCII_ONLY, but after this PRprint_reg_exp_literalwrites the pattern verbatim, so/¶/uputs raw bytes0xC2 0xB6into the chunk.postProcessJSChunk.rsprepends// @bunand the bundle is written as UTF-8. When the bundle is executed, the// @bunpragma triggersAlreadyBundled, and bothjsc_hooks.rs:2759and the async twin inRuntimeTranspilerStore.rsbuild theResolvedSourcewithString::clone_latin1(&source.contents)→BunString__fromLatin1, which memcpy's each byte as one Latin-1 code point.For bytecode:
--bytecodeand--compileboth forcetarget=bun(Arguments.rs / build_command.rs), so the sameIS_BUN_PLATFORM=trueprinter runs.generateChunksInParallel.rs:1057/writeOutputFilesToDisk.rs:412pass&code_result.buffertogenerate_cached_bytecode→__bun_jsc_generate_cached_bytecode→ the C++ FFI, with no transcoding. The C++ side then constructsWTF::Stringfrom aLatin1Characterspan — bytewise Latin-1 decode.Why existing code doesn't prevent it
The four
clone_latin1 → clone_utf8conversions 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=bunoutput — the// @bunfast 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 thealready_bundledsites doesn't fix the C++ bytecode path and vice versa. Before this PR theIS_BUN_PLATFORMregex escape guaranteed--target=bunoutput was all-ASCII, soclone_latin1/ theLatin1Character*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.console.log(/¶/u.test("¶"))(the string literal was already ASCII-escaped byASCII_ONLY, so it decodes fine); printsfalse./\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../appprintsfalse; before this PR it printedtrue.Impact
bun build --target=bun→bun run,bun build --bytecode, andbun build --compileall produce silently-wrong runtime behaviour for any source containing a non-ASCII regex literal — the regex no longer matches what it did before, and.sourceis 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:
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\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 runround-trip and--compile/--bytecode(per REVIEW.md "Cover the variant matrix … alternate modes (--compile, --bytecode)").