diff --git a/src/ast/e.rs b/src/ast/e.rs index 2925a86e1c9e..4d36f17bbeeb 100644 --- a/src/ast/e.rs +++ b/src/ast/e.rs @@ -1685,6 +1685,23 @@ pub struct EString { // Also exported as `String`; `EString` avoids colliding with bun_core::String. pub use EString as String; +/// [`EString::flattened`] result: the node itself, or an owned copy when a rope was flattened. +pub enum Flattened<'a> { + Borrowed(&'a EString), + Owned(EString), +} + +impl core::ops::Deref for Flattened<'_> { + type Target = EString; + #[inline] + fn deref(&self) -> &EString { + match self { + Flattened::Borrowed(s) => s, + Flattened::Owned(s) => s, + } + } +} + impl Default for EString { fn default() -> Self { Self { @@ -1875,10 +1892,28 @@ impl EString { true } + /// Flatten in place. Parser only; shared-AST readers use [`Self::flattened`]. pub fn resolve_rope_if_needed(&mut self, bump: &Bump) { if self.next.is_none() || !self.is_utf8() { return; } + self.data = Str::new(self.flatten_rope(bump)); + self.next = None; + } + + /// `self` if not a rope, else a copy flattened into `bump`. Never writes to `self`. + pub fn flattened(&self, bump: &Bump) -> Flattened<'_> { + if self.next.is_none() || !self.is_utf8() { + return Flattened::Borrowed(self); + } + let mut copy = self.shallow_clone(); + copy.data = Str::new(self.flatten_rope(bump)); + copy.next = None; + Flattened::Owned(copy) + } + + /// The rope's bytes, concatenated into a fresh `bump` slice. + fn flatten_rope<'b>(&self, bump: &'b Bump) -> &'b [u8] { let mut bytes = bun_alloc::ArenaVec::::with_capacity_in(self.rope_len as usize, bump); bytes.extend_from_slice(&self.data); let mut str_ = self.next; @@ -1886,8 +1921,7 @@ impl EString { bytes.extend_from_slice(&part.get().data); str_ = part.get().next; } - self.data = Str::new(bytes.into_bump_slice()); - self.next = None; + bytes.into_bump_slice() } /// Return UTF-8 bytes, transcoding if UTF-16. diff --git a/src/bundler/linker_context/generateCodeForFileInChunkJS.rs b/src/bundler/linker_context/generateCodeForFileInChunkJS.rs index e41a1cb53ba8..8f2172128bf4 100644 --- a/src/bundler/linker_context/generateCodeForFileInChunkJS.rs +++ b/src/bundler/linker_context/generateCodeForFileInChunkJS.rs @@ -424,8 +424,10 @@ pub fn generate_code_for_file_in_chunk_js<'r, 'src>( { continue; } - let name = match &mut prop.key.as_mut().unwrap().data { - ExprData::EString(s) => s.slice(temp_arena), + let name: &[u8] = match &prop.key.as_ref().unwrap().data { + ExprData::EString(s) => { + bun_core::handle_oom(s.flattened(temp_arena).string(temp_arena)) + } _ => unreachable!(), }; if name == b"default" || name == b"__esModule" || !js_lexer::is_identifier(name) diff --git a/src/bundler/linker_context/generateCodeForLazyExport.rs b/src/bundler/linker_context/generateCodeForLazyExport.rs index ff212dadb1ad..ba694c1c49d5 100644 --- a/src/bundler/linker_context/generateCodeForLazyExport.rs +++ b/src/bundler/linker_context/generateCodeForLazyExport.rs @@ -417,11 +417,8 @@ pub(crate) fn generate_code_for_lazy_export( if let ExprData::EObject(e_object) = &expr.data { for property in e_object.properties.slice() { let _: &G::Property = property; - // `Expr`/`ExprData`/`StoreRef<_>` are `Copy`. Copy `key` out so - // `key_str: StoreRef` is a mutable local — `slice()` resolves - // the rope in-place via `DerefMut` into the arena slot. let Some(key) = property.key else { continue }; - let ExprData::EString(mut key_str) = key.data else { + let ExprData::EString(key_str) = key.data else { continue; }; let Some(value) = property.value else { @@ -436,7 +433,7 @@ pub(crate) fn generate_code_for_lazy_export( // across the `&mut self` call to `generate_named_export_in_file` below. let alloc: &bun_alloc::Arena = unsafe { bun_ptr::detach_lifetime_ref::(this.arena()) }; - let name = key_str.slice(alloc); + let name: &[u8] = bun_core::handle_oom(key_str.flattened(alloc).string(alloc)); // TODO: support non-identifier names if !js_lexer::is_identifier(name) { diff --git a/src/js_printer/lib.rs b/src/js_printer/lib.rs index 23cb2726e766..f2cb4305dcc4 100644 --- a/src/js_printer/lib.rs +++ b/src/js_printer/lib.rs @@ -3449,8 +3449,8 @@ pub(crate) mod __gated_printer { if e.optional_chain.is_none() { flags.insert(ExprFlag::HasNonOptionalChainParent); - if let Some(mut str) = e.index.data.as_e_string() { - str.resolve_rope_if_needed(self.bump); + if let Some(str) = e.index.data.as_e_string() { + let str = str.flattened(self.bump); if str.is_utf8() { if let Some(value) = self.try_to_get_imported_enum_value(e.target, str.slice8()) @@ -3762,8 +3762,7 @@ pub(crate) mod __gated_printer { return; } - let mut e = *e; - e.resolve_rope_if_needed(self.bump); + let e = e.flattened(self.bump); self.add_source_mapping(expr.loc); // If this was originally a template literal, print it as one as long as we're not minifying @@ -3926,12 +3925,12 @@ pub(crate) mod __gated_printer { } self.print(b"`"); - match &mut e.head { + match &e.head { E::TemplateContents::Raw(raw) => self.print_raw_template_literal(raw), E::TemplateContents::Cooked(cooked) => { if cooked.is_present() { - cooked.resolve_rope_if_needed(self.bump); - self.print_string_characters_e_string(cooked, b'`'); + let cooked = cooked.flattened(self.bump); + self.print_string_characters_e_string(&cooked, b'`'); } } } @@ -3944,12 +3943,7 @@ pub(crate) mod __gated_printer { E::TemplateContents::Raw(raw) => self.print_raw_template_literal(raw), E::TemplateContents::Cooked(cooked) => { if cooked.is_present() { - // `parts` is `*mut [TemplatePart]` but accessed `&[T]` - // here. We resolve a local copy of the - // EString header (the rope chain is StoreRef-linked and Copy) and - // prints from that — the arena node stays roped. - let mut local = E::EString { ..*cooked }; - local.resolve_rope_if_needed(self.bump); + let local = cooked.flattened(self.bump); self.print_string_characters_e_string(&local, b'`'); } } @@ -4652,10 +4646,9 @@ pub(crate) mod __gated_printer { self.print_symbol(priv_.ref_); } ExprData::EString(key_str) => { - let mut key_str = *key_str; + let key_str = key_str.flattened(self.bump); self.add_source_mapping(key.loc); if key_str.is_utf8() { - key_str.resolve_rope_if_needed(self.bump); self.print_space_before_identifier(); let mut allow_shorthand = true; if !IS_JSON && lexer::is_identifier(key_str.slice8()) { @@ -4908,8 +4901,7 @@ pub(crate) mod __gated_printer { match &property.key.data { ExprData::EString(str) => { - let mut str = *str; - str.resolve_rope_if_needed(self.bump); + let str = str.flattened(self.bump); self.add_source_mapping(property.key.loc); if str.is_utf8() { @@ -4960,7 +4952,9 @@ pub(crate) mod __gated_printer { ) { if Self::MAY_HAVE_MODULE_INFO && tlm.is_export { // reshaped for borrowck — bump access first. - let str8 = str.slice(self.bump); + let str8 = bun_core::handle_oom( + str.string(self.bump), + ); if let Some(mi) = self.module_info() { let name_id = mi.str(str8); mi.add_export_info_local( diff --git a/test/bundler/bun-build-api.test.ts b/test/bundler/bun-build-api.test.ts index 0fbcf59d3d75..d23ebbebeee9 100644 --- a/test/bundler/bun-build-api.test.ts +++ b/test/bundler/bun-build-api.test.ts @@ -1650,6 +1650,75 @@ test("Bun.build can be called thousands of times in one process without crashing expect(exitCode).toBe(0); }, 180_000); +// A module shared by several entry points is printed once per chunk, and those +// prints run in parallel on the thread pool against the same AST. The printer +// used to flatten `"a" + "b" + "c"` ropes in place, through the `StoreRef`, so +// one thread's write of `data` / `next = None` raced every other thread's read +// of the same node. Observed results on the unfixed printer: the tail printed +// twice ("abcbc"), the tail dropped ("a"), or a crash on a torn `next` pointer +// (a `Bus error` / `Segmentation fault` at a 4 GiB aligned address). +// +// The race needs many chunks printing many ropes at the same time, so this +// builds 64 entry points over one module with 400 folded ropes, twice, and +// checks every folded string in every output. With the in-place flatten the +// first build corrupts hundreds of strings on a 16 core machine. +// +// Needs an explicit timeout: two real 64-entry bundles on a debug build take +// well over bun:test's 5s default. +test("Bun.build does not corrupt folded string ropes shared across chunks", async () => { + const ENTRIES = 64; + const ROPES = 400; + const ROUNDS = 2; + let shared = "export function helper(...a) { return a; }\n"; + for (let i = 0; i < ROPES; i++) { + // The rope is a call argument inside an arrow body, the shape the printer + // crashed on in the field. It folds only with `minify.syntax`. + shared += + `export const fn${i} = helper("first${i}", () => { const q = ${i}; ` + + `helper(q, "alpha-${i}-" + "beta-" + "gamma-" + "delta-${i}"); return q; });\n`; + } + const files: Record = { "shared.js": shared }; + for (let i = 0; i < ENTRIES; i++) { + files[`entry${i}.js`] = `import * as s from "./shared.js";\nconsole.log(s, ${i});\n`; + } + files["run.ts"] = ` + import { join } from "node:path"; + const dir = process.argv[2]; + const entrypoints = Array.from({ length: ${ENTRIES} }, (_, i) => join(dir, "entry" + i + ".js")); + let bad = 0; + for (let round = 0; round < ${ROUNDS}; round++) { + const res = await Bun.build({ entrypoints, minify: { syntax: true }, target: "bun" }); + if (!res.success) throw new AggregateError(res.logs, "build failed"); + for (const output of res.outputs) { + const text = await output.text(); + for (let i = 0; i < ${ROPES}; i++) { + const expected = '"alpha-' + i + '-beta-gamma-delta-' + i + '"'; + if (!text.includes(expected)) { + bad++; + if (bad <= 5) { + const actual = text.match(new RegExp('"alpha-' + i + '-[^"]*"')); + console.log("BAD round " + round + " " + output.path + " expected " + expected + " got " + actual?.[0]); + } + } + } + } + } + console.log("DONE " + bad); + `; + const dir = tempDirWithFiles("bun-build-rope-print-race", files); + + await using proc = Bun.spawn({ + cmd: [bunExe(), join(dir, "run.ts"), dir], + 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(stdout.trim()).toBe("DONE 0"); + expect(exitCode).toBe(0); +}, 180_000); + test("sourcemap sourcesContent is valid JSON when source contains C0 control chars", async () => { // RFC 8259 only allows \" \\ \/ \b \f \n \r \t and six-char \u escapes; \v // and \xNN are JavaScript-only. A VT (0x0B) or BEL (0x07) in the input used diff --git a/test/internal/source-lints/printer-rope-in-place.test.ts b/test/internal/source-lints/printer-rope-in-place.test.ts new file mode 100644 index 000000000000..991e03a0d53a --- /dev/null +++ b/test/internal/source-lints/printer-rope-in-place.test.ts @@ -0,0 +1,111 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// The printer and the linker's chunk generation read ASTs that other threads +// are reading at the same time: the bundler prints a module into every chunk +// that includes it, in parallel, from one AST. An in-place rope flatten there +// (`E::String::resolve_rope_if_needed`, or anything built on it) writes `data` +// and `next = None` into the shared node while the other printers read it. +// The observed results were the tail printed twice, the tail dropped, and a +// crash on a torn `next` pointer (`Bus error at address 0x56700000000`). +// +// `StoreRef` is `Copy` and implements `DerefMut`, so `let mut e = *e; +// e.resolve_rope_if_needed(bump)` compiles and silently mutates the arena node. +// The read-only form is `e.flattened(bump)`, which returns a local copy with the +// rope flattened into `bump`. The `&mut self` rope methods are for the parser, +// which owns its nodes. +// +// x.resolve_rope_if_needed(bump) → let x = x.flattened(bump); +// x.slice(bump) → x.flattened(bump).string(bump) +// x.is_identifier(bump) → is_identifier(x.flattened(bump).slice8()) +// x.to_utf8(bump) → x.flattened(bump).string(bump) +// e_string_mut() → e_string() (a `StoreRef`, read it only) + +const root = path.resolve(import.meta.dir, "..", "..", ".."); + +// Code that runs on the shared, post-parse AST with other threads. +const SCOPE = ["src/js_printer/", "src/bundler/linker_context/", "src/bundler/LinkerContext.rs"]; + +const rustSources = globAllSources().rust.filter(abs => { + if (!abs.endsWith(".rs")) return false; + const rel = path.relative(root, abs).replaceAll(path.sep, "/"); + return SCOPE.some(s => (s.endsWith("/") ? rel.startsWith(s) : rel === s)); +}); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +const BANNED: { name: string; re: RegExp; hint: string }[] = [ + { + name: "E::String::resolve_rope_if_needed", + re: /\.resolve_rope_if_needed\(/g, + hint: "flattened(bump)", + }, + { + // The `E::String` method takes the arena; `StoreSlice::slice()` and the + // other no-argument `slice()` accessors do not match. + name: "E::String::slice(bump)", + re: /\.slice\(\s*[A-Za-z_][\w.:]*\s*\)/g, + hint: "flattened(bump).string(bump)", + }, + { + // Method form with an argument; the free fn `js_lexer::is_identifier(x)` + // has no leading `.`. + name: "E::String::is_identifier(bump)", + re: /\.is_identifier\(\s*[A-Za-z_]/g, + hint: "is_identifier(flattened(bump).slice8())", + }, + { + // `bun_core::String::to_utf8()` takes no argument and is fine. + name: "E::String::to_utf8(bump)", + re: /\.to_utf8\(\s*[A-Za-z_]/g, + hint: "flattened(bump).string(bump)", + }, + { + name: "ExprData::e_string_mut", + re: /\.e_string_mut\(/g, + hint: "e_string() and read through the StoreRef", + }, +]; + +const offenders: string[] = []; +let scanned = 0; +for (const abs of rustSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + const content = await file(abs).text(); + // Strip full-line comments so prose mentions do not count. `[ \t]*`, not + // `\s*`, so the newline before a comment survives and line numbers hold. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + const lines = stripped.split("\n"); + for (let i = 0; i < lines.length; i++) { + for (const { name, re, hint } of BANNED) { + re.lastIndex = 0; + if (re.test(lines[i])) { + offenders.push(`${source}:${i + 1}: ${name} (use ${hint}): ${lines[i].trim()}`); + } + } + } +} + +test("scans a non-empty set of tracked Rust sources", () => { + expect(scanned).toBeGreaterThan(0); +}); + +test("the printer and linker never flatten a string rope in place", () => { + expect(offenders).toEqual([]); +});