Repository navigation
bundler: add every top-level symbol before renaming nested scopes - #41062
Conversation
The number renamer added a part's top-level symbols and then renamed that part's nested scopes at once. A nested local that collides with an earlier name got a numbered name before a later part's top-level symbol with that exact name was registered in the root scope. The local then shadowed the top-level binding at the call sites that reference both, which miscompiled pre-minified packages such as @vercel/otel. Collect the nested scopes during the top-level pass and rename them after every top-level symbol in the chunk is added, the same two-phase split esbuild uses in renameSymbolsInChunk. Adds coverage for the nested-let, function-parameter, __esm-wrapped, and CommonJS-wrapped variants. Fixes #41054 Fixes #14588
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughChangesThe linker now defers nested-scope symbol renaming until it registers all top-level symbols, including CommonJS export copies. Five bundler regression tests cover identifier collisions across nested parameters, ESM wrappers, local bindings, hoisted symbols, and CommonJS wrappers. Portable bytecode snapshots now match the updated generated output. Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change defers nested-scope renaming until all top-level symbols are registered, addressing a concrete symbol-collision bug. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The two-pass renaming change addresses both linked issues [ Full details: Description checkExplanation The description explains the problem, root cause, fix, affected behavior, tests, suite results, and snapshot updates. It does not use the exact template headings, but it provides the required change summary and verification details. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The two-phase split matches esbuild's renameSymbolsInChunk and the reasoning is sound, but since this reorders the non-minify renamer that every un-minified bundle goes through — and moves entry_point_cjs_export_copies registration to before nested renaming — a human familiar with the bundler should also take a look.
What was reviewed:
- Traced the deferred
*const Scopepointers: both the&raw const all_module_scopes[i]andpart.scopesentries point into arena storage that nothing mutates between collection and deref, so the later&*scopeis sound. - Verified the pool-reset claim in
src/js_printer/renamer.rs:assign_names_recursive_with_number_scopeput()s every slot it took before returning, so resetting per-scope instead of per-part (and now also after CJS module scopes, which previously had no reset) is behaviorally equivalent. - Checked that nested renaming never writes to
r.root, so registering the CJS export copies before the nested pass only adds names for nested locals to avoid and cannot change the copies' own names. - The five
itBundledcases cover parameter,let, single-file later-part,__esm-wrap, and CJS-wrap variants and all assert runtime stdout, not just bundle shape.
Extended reasoning...
Overview
This PR fixes a symbol-collision bug in the bundler's non-minify NumberRenamer path (src/bundler/linker_context/renameSymbolsInChunk.rs). The old code interleaved two operations per part: registering that part's top-level declared symbols into the root NumberScope, then immediately renaming the part's nested scopes. Because later parts' (and later files') top-level symbols hadn't been registered yet, a nested local could be number-suffixed into a name that a later top-level symbol keeps, shadowing it at runtime. The fix collects (source_index, *const Scope) pairs during the file/part loop and defers all assign_names_recursive_with_number_scope calls until after every top-level symbol — including entry_point_cjs_export_copies — is in the root scope. This mirrors esbuild's two-phase structure. Five new itBundled regression tests in test/bundler/bundler_edgecase.test.ts cover the reported reproductions (#14588, #30269, #41054) plus the __esm-wrap and CJS-wrap variants.
Security risks
None. This is bundler output-naming logic with no auth, crypto, network, or filesystem-write surface. The unsafe involved is dereferencing raw pointers to arena-allocated Scope nodes that were already being dereferenced in the same function; the change only moves where the deref happens.
Level of scrutiny
Moderate-to-high. The change is small (~50 net lines) and mechanically simple, but it sits on the path every non-minified bundle takes, so a regression would corrupt user output broadly. Two ordering-sensitive details deserve a maintainer glance: (1) entry_point_cjs_export_copies are now added to the root scope before nested renaming instead of after — the PR argues nested renaming never writes to the root scope so copy names are unchanged, and I confirmed assign_names_recursive_with_number_scope only reads the parent chain; (2) the hive-pool bitset reset now runs after every nested scope rather than once per part (and never for CJS). I read renamer.rs and confirmed each call put()s back every slot it get()s, so the reset is a no-op either way.
Other factors
The stored raw pointers are derived from all_module_scopes: &mut [Scope] (via &raw const) and from part.scopes (arena *mut Scope cast to *const); nothing between collection and the second loop reborrows or mutates those places, so provenance holds under Stacked Borrows. Test coverage is good: each case runs the bundle and asserts stdout (so a wrong rename fails at runtime, not just via a string match), and two also assert on bundle text as a stronger guard. No CODEOWNERS entry covers these paths. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. Given the correctness-critical location and the subtle ordering changes, deferring for a human sanity check rather than auto-approving.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The two-pass renamer changes numbered renames in bundles where a nested local collides with a top-level name. The library fixtures move on every platform at once. Verified locally: the regenerated hashes match what every CI lane produced.
… reference The number renamer numbered a binding in a nested scope whenever any enclosing scope, including the chunk's merged top level, had a binding of the same name. Since #41062 put every file's top-level names in place before nested scopes are named, `const Model = factory()` next to `() => { class Model {} }` printed `Model2`, chalk@5's default export got `.name === "chalk2"`, and `var Foo = class Foo {}` / tsc's `let User = class User {}` printed `Foo2` / `User2`. The parser now records, per file, the scope of every identifier reference (`Ast::scope_uses`, only when bundling without identifier minification). The renamer keeps a nested binding's name unless the symbol that owns that name in an enclosing scope is referenced inside the binding's scope, as the printer will write it (linked imports, CommonJS namespace objects, `ns.prop` bindings, cross-chunk imports). Reserved, unbound and eval-pinned names are still always avoided; uses the linker adds, the parser's generated helpers, `module`/`exports`, and class field initializers (which decorator lowering can move into the constructor) count conservatively. Class and function expression names are declared in their own scope rather than at top level. Minified output is unchanged.
… reference (#41286) ### What **1. Nested bindings keep their names.** Non-minified bundles no longer number a binding in a nested scope just because a top-level symbol in the chunk (or another enclosing binding) has the same name. A nested binding keeps its name unless the symbol that owns that name in an enclosing scope is actually referenced inside the binding's scope — so `Function#name` / `constructor.name` match unbundled code, Node and Rollup. ```js const factory = () => { class Model {} return Model; }; const Model = factory(); console.log(Model.name); // "Model2" before, "Model" now ``` | | before | after | |---|---|---| | nested `class Model` + top-level `Model` | `Model2` | `Model` | | chalk@5 default export `.name` | `chalk2` | `chalk` | | `var Foo = class Foo {}`, tsc's `let User = class User {}` | `Foo2`, `User2` | `Foo`, `User` | | `var fn = function fn() {}` | `fn2` | `fn` | | nested binding that would capture an outer reference (`import { T as U }` + `class T { m() { return U } }`, a local `parse` next to `ns.parse()`, a local `import_x` next to a CommonJS default import) | renamed | still renamed | Follow-up to #41062 (that fix stands; #41054 stays fixed). **2. `bun build` is faster**, mostly on large / many-module ESM inputs. Linux x64, LTO release builds of 1.4.0, `main` (14ab660) and this branch, hyperfine on 8 pinned cores, non-minified unless noted: | | 1.4.0 | main | this PR | |---|---|---|---| | `bench/bundle` three.js ×100 (`index.ts`) | 1.83 s | 1.51 s | **1.12 s** | | `bench/bundle` three.js ×10 (`index10.ts`) | 253 ms | 258 ms | **221 ms** | | three.js `src/` tree ×100 (~70k modules) | 2.34 s | 2.48 s | **2.03 s** | | three.js `src/` tree ×10 | 220 ms | 221 ms | **196 ms** | | vite dep chunk + rollup (3 MB ESM, `--packages=external`) | 95 ms | 97 ms | **86 ms** | | `typescript.js` as entry, `--format=cjs` | 291 ms | 304 ms | **278 ms** | | typescript+svelte+react-dom+lodash (CJS-wrapped deps) | 298 ms | 312 ms | 300 ms | | hono+mongodb+fastify+axios+express | 55 ms | 55 ms | 52 ms | | three.js ×10 `--minify` (n=30) | 134 ms | 133 ms | 127 ms | | rolldown-benchmarks `apps/10000` (10k tiny JSX files), `--production --sourcemap` | 571 ms | 584 ms | 580 ms | | same, non-minified | 619 ms | 665 ms | 635 ms | macOS arm64 (`bunx bun-pr 41286` vs 1.4 release): `bench/bundle` three.js ×100 382 → 257 ms (`--minify` 265 → 256 ms); three.js `src/` tree ×100 999 → 845 ms (`--minify` 801 → 807 ms). `--minify` builds see only the parser-side share (the minifier has its own renamer); CJS-wrapped dependencies print as one closure and see mostly parity; bundles of thousands of tiny modules are dominated by per-module resolve/link cost and see little change. ### How Renamer: - The parser records `(scope, symbol)` for each identifier reference into a flat per-file list (`Ast::scope_uses`), with scopes numbered in visit pre-order so a subtree is a range. Only when bundling without `--minify-identifiers`; references to unbound globals are skipped. - `NumberRenamer` keeps the chunk's top-level names as interned ids with one slot each (scope, count, owner). Each file's nested scopes are named by a `NestedRenamer` overlaying that table (sparse, undo-logged per scope) and run as parallel per-file tasks once top-level names are final. A nested binding may take a name held further out when the holder is not referenced in its scope subtree. - Conservative cases: uses the linker adds (`symbol::Use` carries an "unscoped" bit), parser-generated temporaries/helpers, `module`/`exports`, TypeScript decorator-metadata type references, and class field initializers/keys count as referenced across the whole file / class body. Class and function *expression* names are declared in their own scope. The TS namespace closure parameter is now visible to the renamer. Parser / linker: - Scope member tables start as a 4-slot inline block (hashes first, compared as a group; names compared only on a hash match), then 8, then a hash table — most scopes never allocate a table, and resolving an identifier up the scope chain touches one cache line per scope. - Per-part symbol-use counts are tracked in a dense side table and turned into the part's map once, instead of a hash probe per identifier reference. - Property accesses check a small bit filter before hashing the name against dot defines. - `symbols`, `scopes_in_order` and the part list are sized from the source up front; the bundler's parse workers use one mimalloc heap for AST nodes, `AstAlloc` spills and the parser arena. - A file's part range stops growing at `max(largest_source / (2 × threads), 128 KB)` of source, so a large unwrapped file prints on several threads (ranges already join the way files do). - `CharFreq::scan` (minifier) counts into four interleaved tables. Minified output can differ from main in which equally-short name a nested slot gets (scope members now iterate in declaration order, which changes the creation order of hoisted symbols); sizes move by ±0.001%. The bytecode-portability snapshot is updated for that and for the non-minified naming change. ### Tests `test/bundler/bundler_edgecase.test.ts`: `identifiers/NestedBindingKeepsNameWhenOuterIsNotReferencedInside` (fails on main), `identifiers/NestedBindingRenamedWhenOuterIsReferencedInside`, `identifiers/NestedBindingRenamedAroundLinkedAndHoistedNames`, `identifiers/NestedBindingRenamedWhenOuterIsReferencedLater` (capture cases that must still rename). Two existing expectations updated (`bar2` → `bar` in an expected runtime error, `fn2` → `fn` in a snapshot); `bundler_bytecode_portable` snapshot updated.
2 participants
Problem
@vercel/otel@2.1.3produces output that throws at load:TypeError: n2 is not a function. (In 'n2(t3.traceSampler, e)', 'n2' is "nodejs"). The renamer gave a nested local the same name as a top-level helper referenced from the same scope. No minification is involved (Bundler identifier rename collides with an existing binding, miscompiling @vercel/otel #41054, Bun is breaking a file from a dependency #14588, Bun ESM bundler scope-hoisting bug #30269).rename_symbols_in_chunk(src/bundler/linker_context/renameSymbolsInChunk.rs:436). It renamed each part's nested scopes right after it added that part's top-level symbols. A nested local that collides with an earlier name gets a numbered name (nbecomesn2) before a later part declares a top-leveln2. The root scope does not containn2yet, so the check passes and the local later shadows the function.Fix
renameSymbolsInChunk. Pass one adds every top-level symbol in the chunk to the root scope and collects the nested scopes. Pass two renames the collected nested scopes.require_*wrapper registered later (circular requires hit this).test/bundler/bundler_edgecase.test.tsunderidentifiers/, all fail on unfixed bun. Also the fulltest/bundler/suite, and the@vercel/otelrepro from Bundler identifier rename collides with an existing binding, miscompiling @vercel/otel #41054 now printsregistered ok.Background
name_countsmap, and a candidate name is checked against the scope chain up to the chunk root.Notes
const n, a function with a locallet nthat callsn2(), andfunction n2declared after it. The bundle renames the local ton2and the call becomesn2(n2).MinifyRenamerpath is slot-based and does not have this problem.assign_names_recursive_with_number_scopereturns every pool slot before it returns, so the reset stays equivalent.entry_point_cjs_export_copiessymbols are now added before nested renaming instead of after. Nested locals avoid those names too. Copy names themselves are unchanged: nested renames never write to the root scope.test/bundler/in full (6234 pass). The 9 failures are debug-build-only: bytecode tests that exceed their 5s timeout under a debug ASAN build, andcompile/HelloWorldWithProcessVersionsBun, which comparesprocess.versions.bun(1.4.1-debug) against the version with-debugstripped. Both reproduce without this diff.bundler_regressions.test.ts, reference the duplicate Bun ESM bundler scope-hoisting bug #30269).Fixes #41054. Fixes #14588.
no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_bytecode_portable.test.ts