bundler: register every top-level symbol before renaming nested scopes

[robobun]

What this fixes

When bundling without minification, the NumberRenamer can rename a nested binding (function parameter or let/var) into the name of a top-level binding that is declared later in the same chunk, shadowing it in the output and breaking the bundle at runtime.

Reproduction

// dep.js
export var e = 1;
export var e2 = 2;

// entry.js
import { e as _e, e2 as _e2 } from "./dep.js";
function ZI(t, e, n) { return e3(t, e, n); }  // uses hoisted e3
function e3(a, b, c) { return "range:" + b; } // declared after ZI
console.log(ZI(1, 2, 3), _e, _e2);

$ bun build entry.js --target bun
function ZI(t, e3, n) {
  return e3(t, e3, n);  // e3 is now the parameter, not the function
}

Running the bundle throws TypeError: e3 is not a function. The same happens for a module wrapped in __esm via dynamic import() (the #14588 @react-email/tailwind case): its top-level declarations are hoisted outside the closure, so a parameter inside that module can be renamed into one of its own hoisted functions.

Root cause

rename_symbols_in_chunk iterated parts sequentially and, for each part, registered that part's top-level symbols in r.root and then immediately walked that part's nested scopes. NumberScope::find_unused_name checks only the current scope and its parent chain, so a nested scope in part N had no knowledge of top-level names declared in any later part M > N. Function hoisting makes this ordering common in real code (and in minified dependencies, which is where #14588 hit it).

Fix

Split the single interleaved loop into two passes over files_in_order:

1. Register every top-level symbol in the chunk in r.root.
2. Walk nested scopes; r.root is now fully populated so find_unused_name sees every potential top-level collision.

This matches esbuild's structure. It is also the same fix as #30272, which was closed unmerged when the Zig source it edited was removed during the Rust migration; this PR reapplies it to the Rust implementation.

Verification

Three itBundled tests in bundler_edgecase.test.ts covering the parameter case (#14588), the __esm-wrapped dynamic import case (#14588, the @react-email/tailwind shape), and the let case (#30269). All three fail on main with the shadowing TypeError and pass with the fix. Ran bundler_edgecase, bundler_cjs, bundler_naming, bundler_regressions, esbuild/default, esbuild/splitting, and bun-build-api (snapshots) with no regressions.

Fixes #14588
Fixes #30269

---

no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7d09106e-6c89-456a-8c7c-a28766ebfe75

📥 Commits

Reviewing files that changed from the base of the PR and between f91d5c9 and 1298ab5.

📒 Files selected for processing (2)

• src/bundler/linker_context/renameSymbolsInChunk.rs
• test/bundler/bundler_edgecase.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 3:02 PM PT - Jul 25th, 2026}

@robobun, your commit 561622c is building: #81584

[github-actions]

Found 1 issue this PR may fix:

1. Bun.build() minifier identifier collision persists after #14585 fix #28742 - Minifier identifier collision where a function and an unrelated object get the same renamed identifier, causing TypeError: _4 is not a function. Reporter explicitly ties it to reference collision issue with Bun.build #14585, and the prior fix (Fix 14585 — unminified identifier collisions #17930) was acknowledged as a workaround — the two-pass top-level symbol registration in this PR addresses the root cause.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #28742

🤖 Generated with Claude Code

[robobun]

Checked #28742: that one is about the MinifyRenamer path (minify: true), which early-returns before the loop this PR changes. The two-pass restructuring here only affects the NumberRenamer (non-minified bundling), so this PR does not fix #28742.

[claude]

I didn't find any bugs. The two-pass restructure looks correct and matches esbuild, but this is core bundler symbol-renaming logic in unsafe Rust, so worth a maintainer's confirmation.

What was reviewed:

• Verified Pass 1 preserves all top-level-symbol registration (wrapper refs, hoisted CJS external imports, per-part declared symbols with the same parts_live gating) and Pass 2 preserves the nested-scope walk for all three WrapKind arms.
• Checked the number_scope_pool reset moving from per-part to per-source-index — assign_names_recursive_with_number_scope already put()s every slot it takes, so the reset is a safety net and the frequency change is benign (and now also runs after CJS modules, which the old continue skipped).
• Checked that Pass 2's immutable all_parts[...].as_slice() borrow does not conflict with Pass 1's mutable borrow.

Extended reasoning...

Overview

Splits the NumberRenamer loop in rename_symbols_in_chunk into two passes over files_in_order: first register every top-level symbol in r.root, then walk nested scopes. This prevents a nested binding from being collision-renamed into a top-level name declared later in the chunk. Two itBundled regression tests cover the parameter case (#14588) and the nested let case (#30269), placed next to the existing identifiers/SameNameDifferentModulesWithMinifyIdentifiersDisabled test.

Security risks

None. This is bundler output-correctness; no untrusted-input parsing, auth, or crypto surface.

Level of scrutiny

High — this is the linker's symbol renamer, which every unminified bundle goes through, and the function carries a large CONCURRENCY note about Stacked Borrows and per-chunk parallelism. That said, the change is a control-flow reordering rather than new unsafe: the same raw-pointer patterns (addr_of_mut!(r.root), &**scope) are reused verbatim, and the SoA column borrows are unchanged. I traced each arm of the old match wrap through the split and every side effect (add_top_level_symbol for wrapper refs, the ES6-import hoist special-case for CJS, add_top_level_declared_symbols gated on parts_live, the module-scope vs per-part-scope walk) lands in the same place with the same guards.

Other factors

• The number_scope_pool.hive.used reset moves from inside the parts loop to once per source_index. assign_names_recursive_with_number_scope (renamer.rs:686-694) already balances every get_init with a put, so the reset is redundant cleanup; moving it is harmless and now covers the CJS branch too (the old code continued past it).
• The PR description states this reapplies the unmerged Zig fix from #30272 and matches esbuild's two-pass structure; the author ran the bundler_edgecase / bundler_cjs / bundler_naming / esbuild suites.
• Tests assert runtime output (run: { stdout }) plus a negative not.toContain on the specific bad rename, so they fail for the right reason on main.

Deferring rather than approving because the repo guidance treats loop-reordering in shared bundler paths as guilty-until-proven and a maintainer should confirm the pool-reset frequency change and that no snapshot suites need updating.

[robobun]

CI summary (latest build #87507 on 1298ab5):

• 177 jobs passed, 14 still scheduled. Zero hard test failures.
• Every red test is [flaky] and passed on retry: watch-many-dirs, tty-reopen-after-stdin-eof, test-error-code-done-callback, napi 7_factory_wrap, test-fastutf8stream-reopen, sourcetextmodule-leak, compile-windows-metadata, 36577, solc. None touch the bundler.
• Remaining blocked lanes are agent-provisioning errors (AWS EC2 400 Unsupported / Azure 429 TooManyRequests), not test or build failures.
• comment-cop threads addressed in the squashed commit.

The diff is green across every lane that ran. Ready for a maintainer.

[claude]

I didn't find any bugs, but this restructures the bundler's symbol renamer — a path where an off-by-one breaks user bundles at runtime — so it's worth a maintainer's eyes.

What was reviewed:

• Pass 1/Pass 2 split preserves the original per-WrapKind behavior; CJS still registers only wrapper + hoisted external-import symbols, ESM/None still walk part.scopes for live parts only.
• number_scope_pool.hive.used reset moved from per-part to per-file — benign, since assign_names_recursive_with_number_scope already balances every get_init with a put before returning.
• The four comment-cop flags: two are on the pre-existing "Reshaped for borrowck" comment that was only relocated; the other two are algorithm explanations with issue links, not workaround justifications.

Extended reasoning...

Overview

The PR splits the NumberRenamer loop in rename_symbols_in_chunk into two passes over files_in_order: pass 1 registers every top-level symbol in r.root, pass 2 walks nested scopes. This fixes a real bug where a nested binding in part N could be collision-renamed into a top-level name declared in a later part M, shadowing it in the output. Two itBundled regression tests cover the parameter case (#14588) and the nested-let case (#30269), both with run: assertions.

Security risks

None. No untrusted input handling, no auth/crypto/permissions.

Level of scrutiny

High. The symbol renamer is on the critical path for every non-minified bundle; a mistake here produces bundles that compile fine but fail at runtime with confusing shadowing errors. The change is a control-flow restructuring rather than a one-line fix, and it subtly changes when number_scope_pool is reset (per-file instead of per-part; CJS files now get a reset where they previously didn't). I traced assign_names_recursive_with_number_scope and confirmed every pool slot is put() back before the function returns, so the reset-frequency change is a no-op for correctness — but it's the kind of thing a maintainer familiar with the hive-array pool should confirm.

Other factors

• The fix matches esbuild's structure and reapplies closed-unmerged #30272 to the Rust port, so the approach has precedent.
• Linux x64 test shards passed in CI; darwin/windows/aarch64 failures are infra (step failed outside runner on build-cpp, pre-existing on main).
• The comment-cop bot flagged four comments. Two are on the pre-existing "Reshaped for borrowck" note that this PR only moved; the other two are algorithm documentation (why two passes; why the CJS branch adds no per-part top-level symbols) with issue links, not workaround justifications. I don't think they need action, but the author may want to trim the pass-1 header comment.
• Borrow structure in pass 2 (all_parts[i].as_slice() after pass 1's as_mut_slice()) is sound — the passes are sequential.

[claude]

I didn't find any issues, but this changes core bundler renaming behavior for every non-minified bundle, so it's worth a maintainer look before merging.

What was reviewed:

• Two-pass split preserves the exact registration order of top-level symbols in r.root; pass 2 only sees a superset of names, so it can't introduce new collisions.
• assign_names_recursive_with_number_scope is get/put-balanced, so moving the number_scope_pool.hive.used reset from per-part to per-source_index (and now also running it for WrapKind::Cjs) is a no-op on an already-empty bitset.
• assign_name is idempotent (early-returns on already-named refs), so no double-rename risk from the reordering.
• Both new tests assert runtime output; the first also asserts the bad rename shape is absent.

Extended reasoning...

Overview

Splits the NumberRenamer loop in rename_symbols_in_chunk into two passes over files_in_order: pass 1 registers every top-level symbol in r.root, pass 2 walks nested scopes. Previously these were interleaved per-part, so a nested scope in an earlier part could pick a name that collided with a top-level symbol declared in a later part or file. Two itBundled regression tests cover the parameter case (#14588) and the nested let case (#30269).

Security risks

None. Pure bundler symbol-renaming logic; no untrusted-input parsing, filesystem, or network.

Level of scrutiny

High. This runs on every non-minified bun build and directly determines identifier names in output — a subtle mistake here silently miscompiles user code. The change is small and the reasoning is sound (nested scopes now see strictly more root names, never fewer), but output for existing bundles may change (different suffix numbers), which is a broad blast radius.

Other factors

• Author verified locally against bundler_edgecase, bundler_cjs, bundler_naming, bundler_regressions, esbuild/default, esbuild/splitting, and bun-build-api snapshots with no regressions; linux x64 CI shards on 7c1252c passed.
• The pool-reset relocation is provably a no-op: assign_names_recursive_with_number_scope puts back every scope it gets, so hive.used is already empty at each reset point.
• This is a re-apply of previously-unmerged #30272 to the Rust port; a maintainer opting in to that fix seems appropriate.
• comment-cop nits were addressed in 561622c; all inline threads are resolved.

[robobun]

Rebased onto current main (a7838c5) and added a third test covering the __esm-wrapped dynamic-import shape from #14588 (@react-email/tailwind). All three tests fail on 1.4.0-canary.1 and pass with the fix; wider bundler suites (bundler_edgecase, bundler_naming, bundler_regressions, esbuild/default, esbuild/splitting, bundler_cjs, bun-build-api) are clean.

[claude]

No bugs found, but this restructures the bundler's NumberRenamer loop — a path every non-minified bundle goes through — so it's worth a maintainer glance before merge.

Checked that Pass 1 + Pass 2 together perform the same set of add_top_level_symbol/add_top_level_declared_symbols/assign_names_recursive calls as the old interleaved loop for all three WrapKind arms.
Checked the number_scope_pool reset moving from per-part to per-file: assign_names_recursive_with_number_scope already balances get_init/put internally, so the bitset reset is defensive and the frequency change is benign (CJS now resets where it previously didn't).
Checked the three new itBundled tests — each has a runtime run.stdout assertion plus an output-shape assertion, covering the parameter, __esm-wrap, and nested-let variants.

Extended reasoning...

Overview

The PR splits rename_symbols_in_chunk's single interleaved loop (register top-level symbols for a part, then immediately walk that part's nested scopes) into two sequential passes over files_in_order: Pass 1 registers every top-level symbol in r.root, Pass 2 walks nested scopes. This ensures NumberScope::find_unused_name sees all top-level names when picking collision-avoiding names for nested bindings, fixing #14588 and #30269 where a parameter or let was renamed into a later-declared hoisted function. Three itBundled tests are added covering the parameter case, the __esm-wrapped dynamic-import case, and the nested-let case.

Security risks

None. This is a pure bundler code-generation change with no auth, crypto, network, or filesystem surface.

Level of scrutiny

Moderate-to-high. The NumberRenamer path runs for every non-minified bun build, so a regression would produce broken bundles at runtime for a large fraction of users. The change is small and mirrors esbuild's structure (and reapplies the previously-reviewed #30272 that was lost in the Rust migration), but it is a real control-flow restructuring of a critical path rather than a mechanical tweak.

Other factors

I traced each WrapKind arm through both passes and confirmed the same registration and scope-walk calls occur as before, only reordered. The one uncalled-out behavioral delta is the number_scope_pool.hive.used reset moving from once-per-part to once-per-file (and now firing for CJS too); reading renamer.rs:623-694 shows assign_names_recursive_with_number_scope already balances get_init/put internally, so the reset is defensive and the change is benign. The new tests assert both runtime output and bundle shape (not.toContain("function ZI(t, e3, n)"), toContain("__esm")), which is strong coverage. CI test shards that ran were green; red lanes are pre-existing infra failures unrelated to this Rust-only diff. Given it touches a critical bundler path, deferring to a maintainer rather than auto-approving.

[robobun]

Closing: superseded by #41062. That PR carries the same two-pass renamer split rebased onto the current renameSymbolsInChunk.rs, and includes this branch's three tests unchanged.