Repository navigation
Conversation
…r renamer
The ESM bundler renamer used to interleave two operations per part:
1. register the part's top-level symbols in the root scope, and
2. walk the part's nested scopes to rename collisions.
Because nested-scope walking for part N ran before part M>N's top-level
symbols were registered, a local binding in part N could be renamed to a
name that later became a top-level symbol in part M, shadowing it.
Minimal repro — after bundling, `r2()` inside typecheck called the
`Expression` instance from `let r2` (renamed from `let r`) instead of
the top-level `r2` function:
// conflict.js
export function r() { return "top-level r"; }
// module.js
function typecheck(node) {
let r, t, c; // bundler renames r -> r2 (conflict with top-level r)
/* ... */
return r2().bo4(d, n, s).a(); // resolves to local r2, not top-level
}
function r2() { /* ... */ }
// main.js
import * as conflict from "./conflict.js";
import { run } from "./module.js";
conflict.r(); // forces "r" into the root scope
run();
Fix: split the single-pass loop into two passes. Pass 1 adds every
top-level symbol (across all files in the chunk) into r.root. Pass 2
walks each file's nested scopes. By the time findUnusedName runs, r.root
is fully populated, so r2 is recognized as taken and a free name is
chosen instead. Matches esbuild's structure.
Fixes #30269
|
Updated 3:06 PM PT - May 5th, 2026
❌ @robobun, your commit b117b7f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30272That installs a local version of the PR into your bun-30272 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughrenameSymbolsInChunk was restructured into two explicit passes: Pass 1 registers all chunk top-level symbols and collects per-file nested scopes; Pass 2 performs the recursive renaming per file using the fully populated root. A regression test for issue ChangesScope-Hoisting Rename Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/bundler/linker_context/renameSymbolsInChunk.zig`:
- Around line 163-168: nested_scopes currently only deinitializes the ArrayList
but not the per-file duplicated slice buffers appended into it, leaking one
allocation per file; fix by, before calling nested_scopes.deinit(), iterate the
entries in nested_scopes (and likewise entries in scopes_scratch where you
dup/alloc slices) and free each backing slice with
r.temp_allocator.free(theSlice) (or the appropriate deallocator used when
duping), then call nested_scopes.deinit() and scopes_scratch.deinit(); reference
the variables nested_scopes and scopes_scratch and perform the per-entry free
loop just prior to their .deinit() calls (also apply the same change at the
other occurrences mentioned).
In `@test/regression/issue/30269.test.ts`:
- Around line 90-109: Remove the brittle assertion that the spawned process
stderr is exactly empty: in the block that awaits const [stdout, stderr,
exitCode] = await Promise.all([run.stdout.text(), run.stderr.text(),
run.exited]); delete or stop asserting expect(stderr).toBe("") (do not assert
stderr contents at all, or only assert it does not contain fatal keywords if
necessary). Also ensure the exit code check (expect(exitCode).toBe(0)) appears
after stdout assertions so stdout failures remain easy to read; keep the
existing expect(stdout).toBe("ok: true\n") before the exitCode assertion and
leave the earlier build checks (e.g., the build stdout/stderr assertions)
unchanged.
🪄 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: 63194053-0815-4fad-b92a-1e783f680cdc
📒 Files selected for processing (2)
src/bundler/linker_context/renameSymbolsInChunk.zigtest/regression/issue/30269.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/30269.test.ts (1)
90-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubprocess assertion order and stderr checks still need adjustment.
The issues flagged in the previous review comment remain:
Lines 90-95 (build block):
expect(stderr).not.toContain("error")on line 91 will flake under ASAN (ASAN warnings can contain "error"). The assertion order should be: stdout first, then the conditional stderr pattern, then exitCode.Lines 104-109 (run block):
expect(stderr).toBe("")on line 107 will flake under ASAN. The assertion order should be: stdout first, then the conditional stderr pattern, then exitCode.Per the established house style, use the conditional stderr check pattern (
if (exitCode !== 0) { expect(stderr).toBe(""); }) immediately beforeexpect(exitCode).toBe(0)so that stderr surfaces in the diff only when the command fails.Suggested fix (same as previous review)
{ const [stdout, stderr, exitCode] = await Promise.all([build.stdout.text(), build.stderr.text(), build.exited]); - expect(stderr).not.toContain("error"); - expect(exitCode).toBe(0); // `stdout` carries the "Bundled N modules" banner; referenced to keep it. expect(stdout).toContain("out.js"); + if (exitCode !== 0) { + expect(stderr).toBe(""); + } + expect(exitCode).toBe(0); } await using run = Bun.spawn({ cmd: [bunExe(), "out.js"], env: bunEnv, cwd: String(dir), stdout: "pipe", stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([run.stdout.text(), run.stderr.text(), run.exited]); // On the buggy build this stderr contains: // TypeError: r2 is not a function. (In 'r2()', 'r2' is an instance of Expression) - expect(stderr).toBe(""); expect(stdout).toBe("ok: true\n"); + if (exitCode !== 0) { + expect(stderr).toBe(""); + } expect(exitCode).toBe(0);Based on learnings: in regression tests under
test/regression/issue/, avoid asserting spawned subprocess stderr is empty or doesn't contain generic keywords because ASAN builds emit warnings to stderr; assert exitCode last after stdout checks for better diagnostics.🤖 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 `@test/regression/issue/30269.test.ts` around lines 90 - 109, The test currently asserts stderr contents directly and checks exitCode before or alongside stdout which flakes under ASAN; update both blocks that use the variables build (stdout, stderr, exitCode) and run (stdout, stderr, exitCode) to assert stdout first, then perform a conditional stderr assertion only when the process failed (use if (exitCode !== 0) { expect(stderr).toBe(""); }), and finally assert expect(exitCode).toBe(0); remove or replace any direct expect(stderr).not.toContain("error") / expect(stderr).toBe("") checks with this conditional pattern so ASAN warnings on stderr won't cause spurious failures.
🤖 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.
Duplicate comments:
In `@test/regression/issue/30269.test.ts`:
- Around line 90-109: The test currently asserts stderr contents directly and
checks exitCode before or alongside stdout which flakes under ASAN; update both
blocks that use the variables build (stdout, stderr, exitCode) and run (stdout,
stderr, exitCode) to assert stdout first, then perform a conditional stderr
assertion only when the process failed (use if (exitCode !== 0) {
expect(stderr).toBe(""); }), and finally assert expect(exitCode).toBe(0); remove
or replace any direct expect(stderr).not.toContain("error") /
expect(stderr).toBe("") checks with this conditional pattern so ASAN warnings on
stderr won't cause spurious failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad16f757-6299-44eb-bf4f-f4d9320af736
📒 Files selected for processing (1)
test/regression/issue/30269.test.ts
- Eliminate the intermediate nested_scopes/scopes_scratch buffers. Pass 1 over files_in_order adds top-level symbols only, pass 2 over the same order walks each file's nested scopes. No per-file slice allocations, no duplicated buffers for coderabbit to flag as potential leaks. - Regression test: stop asserting stderr is literally empty. The TypeError signature is implied by stdout + exit code; stderr can hold benign debug-build warnings (MADV_DONTNEED) that we don't want to flake on. The behavior is unchanged — every top-level symbol still lands in r.root before the first nested-scope walk.
Follows the CLAUDE.md convention for subprocess tests — negative substring checks on stderr are fragile, and asserting stdout before exitCode gives a more useful error message on test failure. Pass stderr through as the exitCode assertion's message so build failures surface the actual output.
|
Thanks for the triage. Verified each against current
So only |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/30269.test.ts (1)
90-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrop the build
stderrsubstring check and assertexitCodelast.
expect(stderr).not.toContain("error")is still a brittle success-path assertion, andexpect(exitCode).toBe(0)should come after the stdout check so failures surface the build output first.As per coding guidelines, “Never write tests that check for no 'panic' or 'uncaught exception' or similar in test output” and “When spawning processes in tests, check stdout expectations before exit code expectations to provide more useful error messages on test failure”.Suggested adjustment
{ - const [stdout, stderr, exitCode] = await Promise.all([build.stdout.text(), build.stderr.text(), build.exited]); - expect(stderr).not.toContain("error"); - expect(exitCode).toBe(0); + const [stdout, , exitCode] = await Promise.all([build.stdout.text(), build.stderr.text(), build.exited]); // `stdout` carries the "Bundled N modules" banner; referenced to keep it. expect(stdout).toContain("out.js"); + expect(exitCode).toBe(0); }🤖 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 `@test/regression/issue/30269.test.ts` around lines 90 - 94, Remove the brittle stderr substring check and reorder the assertions so the process output is asserted before the exit code: drop the line asserting on stderr (`stderr`/`expect(stderr).not.toContain("error")`) and ensure you first assert stdout contains "out.js" (using the existing `stdout` variable and `expect(stdout).toContain("out.js")`) and only after that assert `expect(exitCode).toBe(0)` so build output is visible when tests fail; these changes affect the test block that awaits `build.stdout.text()`, `build.stderr.text()`, and `build.exited` and uses the `stdout`, `stderr`, and `exitCode` variables.
🤖 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.
Duplicate comments:
In `@test/regression/issue/30269.test.ts`:
- Around line 90-94: Remove the brittle stderr substring check and reorder the
assertions so the process output is asserted before the exit code: drop the line
asserting on stderr (`stderr`/`expect(stderr).not.toContain("error")`) and
ensure you first assert stdout contains "out.js" (using the existing `stdout`
variable and `expect(stdout).toContain("out.js")`) and only after that assert
`expect(exitCode).toBe(0)` so build output is visible when tests fail; these
changes affect the test block that awaits `build.stdout.text()`,
`build.stderr.text()`, and `build.exited` and uses the `stdout`, `stderr`, and
`exitCode` variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 374b21ce-4e72-407f-b82f-e66e30898fe8
📒 Files selected for processing (2)
src/bundler/linker_context/renameSymbolsInChunk.zigtest/regression/issue/30269.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/regression/issue/30269.test.ts`:
- Around line 90-95: The current assertion uses expect(exitCode, stderr).toBe(0)
which passes stderr as a custom message; change it to first check the exit code
and surface stderr on failure by adding an if-check: after collecting
stdout/stderr/exitCode (variables build, stdout, stderr, exitCode) insert if
(exitCode !== 0) { expect(stderr).toBe(""); } immediately before the final
expect(exitCode).toBe(0) so failing runs show full stderr in the test diff
instead of using the custom-message form.
- Around line 104-110: Add the standard if-pattern before the exit code
assertion: check the spawned process exitCode variable (exitCode) and, if it's
non-zero, assert that stderr contains the full output (i.e. run an assertion on
stderr) immediately before the existing expect(exitCode).toBe(0); this should be
placed in the test near stdout/stderr/exitCode handling so failures surface the
full stderr for diagnosis.
🪄 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: 26e97296-9dd0-4842-b43b-65d6a8d70f8c
📒 Files selected for processing (1)
test/regression/issue/30269.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
- 🟡
src/bundler/linker_context/renameSymbolsInChunk.zig:289-293— nit: This comment is misleading — the reset is load-bearing, not defensive.assignNamesRecursiveWithNumberScope's linear-chain while-loop (renamer.zig:600-617) maynumber_scope_pool.get()several times but the singledeferonlyput()s the last one back, so intermediate NumberScopes are only reclaimed by this bulkhive.used = initEmpty(). Also, the old code reset per-part while this resets per-file, so it doesn't quite "match the previous per-part behavior". Suggest rewording so a future reader doesn't delete a load-bearing line:
// Bulk-reclaim hive slots for NumberScopes that the linear-chain walk in
// assignNamesRecursiveWithNumberScope acquired but did not individually
// put() back (only the final scope in each chain is returned via defer).Extended reasoning...
What the issue is
The new comment at renameSymbolsInChunk.zig:289-292 reads:
Bulk-release every NumberScope we borrowed from the pool while processing this file. The recursive walk has already put each scope back individually; this is just a defensive reset that matches the previous per-part behavior.
Both bolded claims are inaccurate. The hive.used = initEmpty() line is the primary mechanism by which intermediate NumberScopes are returned to the 128-slot hive pool — it is not defensive. And the previous code reset per-part (inside for (parts)), while the new code resets per-file (inside for (files_in_order, ...)), so the behavior isn't strictly the same either.
Step-by-step proof that the reset is load-bearing
Look at assignNamesRecursiveWithNumberScope in src/js_printer/renamer.zig:592-623:
var s = initial_scope;
defer if (s != initial_scope) {
s.deinit(r.temp_allocator);
r.number_scope_pool.put(s); // ← only puts back the FINAL s
};
while (true) {
if (scope.members.count() > 0 or scope.generated.len > 0) {
const new_child_scope = r.number_scope_pool.get(); // ← may fire N times
...
s = new_child_scope; // ← reassigns s each iteration
}
if (scope.children.len == 1) { scope = scope.children.ptr[0]; } else break;
}Concrete example: a JS function with a linear chain of 3 nested non-empty scopes (fn → block → block, each declaring at least one symbol, each with exactly one child):
- Iteration 1:
get()→ slot A;s = A. - Iteration 2:
get()→ slot B;s = B. Slot A is neverput()back. - Iteration 3:
get()→ slot C;s = C. Slot B is neverput()back. - Loop breaks; recursion into children (none, or each recursive call has the same leak shape).
deferfires once:put(s)returns only slot C.
Slots A and B remain marked used in hive.used. The only thing that reclaims them is the caller's bulk hive.used = initEmpty(). So the comment's claim that "the recursive walk has already put each scope back individually" is false — it puts back at most one per call frame.
Why nothing else prevents this
number_scope_pool is a HiveArray(NumberScope, 128).Fallback backed by renamer.arena (renamer.zig:456, 533). There is no other code path that returns the intermediate chain slots; the defer is the only put() in the function and it captures s by reference, so it sees only the final value.
Impact
There is no runtime bug today — the reset line is present and correct, and even if it were removed the fallback allocator (the renamer's arena) would absorb the overflow once the 128-slot hive fills, so nothing would crash. The harm is purely to future maintainers: a comment that explicitly says "this is just a defensive reset" invites deletion. If someone removes it (or moves it further out), large files with many linear scope chains would silently exhaust the 128-slot hive and push every subsequent NumberScope onto the arena, degrading the pool's reuse benefit without any visible failure.
The second inaccuracy ("matches the previous per-part behavior") is minor on its own — per-file vs per-part reset is functionally fine here since the fallback is arena-backed — but it compounds the misdirection by implying this line was carried over verbatim for parity rather than because it does real work.
Suggested fix
Reword to reflect what the line actually does:
// Bulk-reclaim hive slots for NumberScopes that the linear-chain walk in
// assignNamesRecursiveWithNumberScope acquired but did not individually
// put() back (only the final scope in each chain is returned via defer).
r.number_scope_pool.hive.used = @TypeOf(r.number_scope_pool.hive.used).initEmpty();Or simply drop the second sentence of the existing comment and keep just "Bulk-release every NumberScope we borrowed from the pool while processing this file."
The linear-chain while loop inside assignNamesRecursiveWithNumberScope can get() several NumberScopes but its single defer only put()s the final one back. Intermediate slots stay marked used in the 128-slot hive until the bulk reset here reclaims them. The reset is load-bearing, not defensive — claude[bot] flagged the misleading wording on review.
expect(exitCode, stderr).toBe(0) passes stderr as a custom message but the repo convention is to use an if-check immediately before the exitCode assertion so stderr shows up in the diff when the exit code is non-zero.
| // `typecheck` has a nested \`let r\`. The renamer picks \`r2\` for it — | ||
| // but \`r2\` is already a top-level function in the same file. |
There was a problem hiding this comment.
🟡 Tiny cosmetic nit: the \`` sequences in these //comments render as a literal backslash followed by a backtick, since line comments don't process escapes (note ``typecheck`` on the same line is already unescaped). Just drop the four backslashes so it reads ``let r`` / ``r2` ``.
Extended reasoning...
What this is
Lines 30-31 of the new test file are ordinary // line comments sitting between two object-literal properties — after the closing backtick of the conflict.js template literal on line 29 and before the opening backtick of module.js on line 32:
`,
// `typecheck` has a nested \`let r\`. The renamer picks \`r2\` for it —
// but \`r2\` is already a top-level function in the same file.
"module.js": `Because these are plain TypeScript line comments (not inside any template literal), the source bytes are taken verbatim — \`` is not an escape sequence here, so it renders as a literal backslash followed by a backtick. The very same line already writes `` typecheck`` with an unescaped backtick, which confirms the`` instances are leftovers from when this prose lived inside a template literal and needed escaping.
Step-by-step
- The TypeScript scanner sees
//on line 30 and enters single-line-comment mode. - In single-line-comment mode, every byte up to the newline is consumed literally; there is no escape processing (ECMA-262 §12.4: SingleLineComment :: // SingleLineCommentChars, where the chars are any source character except line terminators).
- The byte sequence
\`therefore stays as two characters — backslash, backtick — in the comment text. - A reader (or anything that surfaces the comment, e.g. an IDE hover, GitHub diff view) sees
nested \`let r\`. The renamer picks \`r2\` …with stray backslashes. - Contrast:
`typecheck`earlier on line 30 is written without\and reads correctly — the inconsistency is the giveaway that the escapes are accidental.
Why nothing prevents it
There is nothing to prevent — the parser, the test runner, and the bundled fixture are all completely unaffected. Comments are discarded before execution. No linter in this repo flags ``` in comments. This is purely a readability / consistency issue in newly-added prose.
Impact
Zero functional impact. The test compiles, runs, and catches the #30269 regression identically with or without the backslashes. The only effect is that the explanatory comment reads slightly oddly with four stray \ characters, and is internally inconsistent (`typecheck` vs. \`r2\` ).
Fix
Delete the four backslashes:
- // `typecheck` has a nested \`let r\`. The renamer picks \`r2\` for it —
- // but \`r2\` is already a top-level function in the same file.
+ // `typecheck` has a nested `let r`. The renamer picks `r2` for it —
+ // but `r2` is already a top-level function in the same file.Build 51718 failed on the well-known Windows flakes: - test-http-should-emit-close-when-connection-is-aborted (100% of recent failed builds hit this) - jsc-stress.test.ts ipint-bbq-osr-with-try3 'mprotect failed: 487' (57% rate) - spawn-ipc-gc.test.ts GC-count timing - bake/dev-and-prod.test.ts 'hmr handles rapid consecutive edits' - bun-install-registry.test.ts hoisting peer 1.0.10 None of these touch bundler code. The PR's sole code change is in src/bundler/linker_context/renameSymbolsInChunk.zig.
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
1 participant
What does this PR do?
Fixes #30269.
Reproduction
bun main.jsruns correctly.bun build main.jsemits code that crashes with:Root cause
renameSymbolsInChunk(src/bundler/linker_context/renameSymbolsInChunk.zig) interleaved two operations in a single per-part loop:addTopLevelDeclaredSymbols(part.declared_symbols)— register this part's top-level refs inr.root.assignNamesRecursiveWithNumberScope(&r.root, scope, ...)— walk this part's nested scopes to rename collisions.Because (2) for part N ran before (1) for part M > N, a local binding in part N could collide-rename into a name later claimed by a top-level symbol in part M, silently shadowing it.
Concretely: the local
let rintypecheckis renamed tor2(to avoidconflict.r), but the top-levelfunction r2from the same file lives in a later part whose top-level symbols haven't been registered yet.findUnusedNameseesr2as available and picks it. The subsequentreturn r2()in the same scope then resolves to the renamed local.Fix
Split the single pass into two passes, matching esbuild's structure:
part.declared_symbolsfor ESM/no-wrap), and stashes the nested scopes to visit.r.rootis fully populated.findUnusedNamesees every top-level collision and picks a unique name (r3in this repro).Verification
test/regression/issue/30269.test.tsbundles the repro and runs the output. Fails onmainwith the aboveTypeError, passes with this change.test/bundler/esbuild/default.test.ts(150/150 pass),importstar.test.ts(72/72 pass),splitting.test.ts(20/20 pass),bundler_cjs.test.ts(6/6 pass),bundler_cjs2esm.test.ts(35/35 pass),bundler_bun.test.ts+bun-build-api.test.ts(48/48 pass).Output before / after
function typecheck(node) { - let r2, t, c; + let r3, t, c; block: { let k = node.left; - r2 = k; + r3 = k; ... } - let d = r2; + let d = r3; return r2().bo4(d, n, s).a(); // <- now resolves to the top-level r2 function } function r2() { /* ... */ }