Repository navigation
Conversation
…again to one symbol
A `var` in a nested block at the top level of a module is the same
variable as a top-level `var` of that name. Hoisting linked the block's
symbol to the top-level one, but the block's scope kept its own symbol,
so a read in the block was a use of a symbol that declares nothing.
Tree shaking then removed `var x = {...}` from
`var x = {...}; if (c) { var x; x.v }`, and the bundle threw. A
destructured `import()` local that only such a block read lost its export
the same way.
The merge in `hoist_symbols` now removes the block's member for the name,
so a lookup in the block goes up to the module scope's symbol. That
symbol gets a REDECLARED_IN_NESTED_SCOPE mark. Three places that take one
declaration as the only source of a variable's value check it: a macro
result is not inlined into such a `var`, an unwrapped `require()` does
not make it the import namespace, and the `__esm` wrapper keeps its
initializer inside.
Left as they are: `module` and `exports` in a CommonJS file, a file where
a top-level function and a `var` share a name, and a `var` inside a
function.
|
Updated 10:47 PM PT - Oct 25th, 2026
❌ @robobun, your commit 695abf9 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 46284That installs a local version of the PR into your bun-46284 --bun |
|
Status Reproduction: save this as var x = { v: 1 };
if (globalThis.process) {
var x;
console.log(x.v);
}
The fix is in this pull request (#46284). CI on 695abf9 (build 132936)
The PR is a draft. It waits for a decision on the order with #44001 (see "Why this is a draft" in the description). |
|
No actionable comments were generated in the recent review. ℹ️ Recent review info
WalkthroughThe parser now tracks ChangesNested-scope var redeclarations
Suggested reviewers: Priority: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the primary change: resolving a module-level var redeclared in a nested block to one symbol. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The description is complete and relevant. It explains the problem, fix, scope, known limitations, performance impact, and verification results. It does not use the exact template headings, but it prov… |
Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, fix, scope, known limitations, performance impact, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new scope.members.remove_hashed(hash.unwrap(), name) in hoist_symbols (src/js_parser/p.rs:3628): hash is unconditionally filled at p.rs:3549-3550 before any merge path, matching the sibling get_or_put_member_with_hash sites, so the unwrap cannot panic; and the removal mutates scope.members while the loop walks the separate member_snapshot copy, so iteration is not invalidated. The needs_wrapper_ref early return (p.rs:9525) was also examined and ruled out as a correctness issue (it only widens when a file gets a wrapper symbol).
Extended reasoning...
The change touches the parser's symbol hoisting (src/js_parser/p.rs), scope member storage (src/ast/scope.rs), a new SymbolFlags bit, three single-declaration optimizations in visit/mod.rs, and the __esm lift guard in the linker; it touches no security-sensitive surface. Five confirmed findings (an __esm for-head regression, order-dependent application of the function/var exclusion, quadratic linker cost on many top-level loops, and the require() namespace size growth) are posted inline, so a human needs to weigh them. This note records only the additional concerns examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| if _scope_ptr == self.module_scope | ||
| && value.ref_ == original_member_ref | ||
| && self.symbols[symbol_idx].kind | ||
| == js_ast::symbol::Kind::Hoisted | ||
| && existing_kind == js_ast::symbol::Kind::Hoisted | ||
| && !self.has_top_level_function_merged_with_var | ||
| && (self.has_es_module_syntax | ||
| || (name != b"module" && name != b"exports")) | ||
| { | ||
| self.symbols[existing_idx].set_redeclared_in_nested_scope(true); | ||
| self.has_var_redeclared_in_nested_scope = true; | ||
| scope.members.remove_hashed(hash.unwrap(), name); |
There was a problem hiding this comment.
🟡 (optional) Users bundling a module with thousands of top-level statements that re-declare one var (typically for (var i = 0; …) loops) now get a build that takes seconds and gigabytes instead of milliseconds. Once the nested member is removed at src/js_parser/p.rs:3628 every read of i resolves to the one module symbol, so in the linker each loop part depends on every other loop part (K² edges). Fix: keep one-symbol resolution but bound the linker cost, e.g. record a dependency on the symbol's declaring-part list once per use site in step 5 instead of one edge per (use, declaring part) pair, so the edge count stays O(K).
Why this was flagged
Input: one module-level file with K top-level for (var i = 0; i < n; i++) {...} loops (generated fixtures, benchmark or concatenated legacy scripts), bundled with bun build. hoist_symbols at src/js_parser/p.rs:3617-3628 flags the module-level i and removes each loop scope's own member, so every loop's uses of i become uses of the single module symbol. The linker's step-5 dependency pass then makes each loop part depend on every part that declares i, i.e. K parts, giving K² edges; the PR's own measurement is 50 loops: 50 edges on base, 2,500 after, and K=20,000: 0.07 s / 56 MB on base versus 11.8 s / 11.3 GB after, K=5,000: 40 ms / 26 MB versus 521 ms / 774 MB. On the base branch each loop's i kept its own linked symbol so each part depended only on itself. No safeguard bounds the edge count; the author notes the same-scope var t = f() form was already quadratic, but that form is far rarer at module level than repeated for (var i …) loops, so the change widens the population that hits it.
Verification: In src/js_parser/p.rs:3617-3628, when a nested scope's Hoisted var merges into an existing module-scope Hoisted var, the nested scope's own member is dropped. Then src/bundler/linker_context/doStep5.rs:376-410 iterates part.symbol_uses.keys() and pushes one Dependency per listed part. On head, every part uses i_1 and so pushes K Dependency entries, giving K² edges.
There was a problem hiding this comment.
No change here. Step 5 adds one dependency for each part that declares the symbol. K same-scope var statements pay the same on main (var t = f(); use(t); x 20,000: about 10 s and 8.2 GB). A bounded form changes what each reader of a part's dependencies sees, so it is a linker change of its own, and it helps both forms. The PR body has the numbers for this form: 500 loops cost 7 MB more, 2,000 loops cost 70 ms and 109 MB more.
| if self.is_redeclared_in_nested_scope(ref_) { | ||
| // The other declaration can assign the variable, so | ||
| // it holds the namespace, as `x = require()` does. | ||
| decl.value = Some(Expr::init_identifier( | ||
| self.imports_to_convert_from_require | ||
| [unwrapped_id.get_usize()] | ||
| .namespace | ||
| .ref_, | ||
| value.loc, | ||
| )); |
There was a problem hiding this comment.
🟡 (optional) Users bundling a CommonJS file that does var R = require("react") and later has any nested var R; now ship the whole package namespace instead of the handful of import items they got on the base branch (the author's probe grows 133 to 7,334 bytes per file). At visit/mod.rs:405-414 the flagged decl is rewritten to var R = <namespace ref>, so R.useState is no longer an import-item access and nothing in react can be tree-shaken. Fix: only fall back to the namespace object when the nested declaration can store to the variable (a var R = ... or an assignment in the block); a bare var R; re-declaration does not change the value and should keep the base import-item conversion.
Why this was flagged
Trigger: unwrap_commonjs_to_esm is on for the package (src/js_parser/p.rs:1652-1660, true for the default unwrap list, e.g. react) and the file has var R = require("react") at module level plus any var R; in a nested block, which hoist_symbols flags at src/js_parser/p.rs:3626 regardless of whether the block stores. At src/js_parser/visit/mod.rs:405 the guard is_redeclared_in_nested_scope(ref_) takes the new branch and replaces the ERequireString with an identifier of the generated namespace (lines 408-414), and skips the continue 'outer that on the base made R itself the namespace with an import_items_for_namespace entry (lines 416-422). Every R.x in the file is then a property read on a materialized namespace object, so the linker cannot drop unused exports; the author measures 133 to 7,334 bytes for one probe. No store analysis is performed before paying that cost. Population: every bundled CJS file hitting that shape, with output size for the consumer, not a correctness break.
Verification: src/js_parser/p.rs:3617-3628 sets REDECLARED_IN_NESTED_SCOPE whenever a nested-block var merges into a module-scope var, never testing whether the inner declaration is assigned. src/js_parser/visit/mod.rs:405-414 then replaces the ERequireString with the namespace ref instead of the base-branch import_items_for_namespace path (lines 416-422), so the linker retains every export.
There was a problem hiding this comment.
No change here. The fold is decided at the declaration, where the parser does not know what the block does with the variable. A store does not need an initializer: var R = require("react"); if (c) { var R; R = other; } prints the package's export from main's bundle and other from this one. A bare var R; can be told from that only after the visit pass, and #45343 asks where the fold is decided. The size cost is in Downsides: 884 to 7,789 bytes for a minified probe with react 18.3.1.
…nction/var flag in the hoist merge
The linker again moves the value of a `var` that a nested block declares
again above the `__esm` wrapper, as it does for every other `var`. The
guard that kept that store inside the wrapper is removed. A `for` head
without initializer and a body without braces keep their own `var` inside
the wrapper, so the store did not reach the exported variable, and the
importer read `undefined`.
`hoist_symbols` sets `has_top_level_function_merged_with_var` part of the
way through its walk. A block after a `function f` / `{ var f }` pair
therefore kept its own symbol, and a block before the pair did not. The
merge now reads the value that the parse pass left.
…ow-merged-var-uses
|
I moved this PR to draft. The pushed commit has two regressions. 1. The guard in the linker (the finding above). It keeps the store of a flagged 2. A function and a function f() { return 1; }
if (globalThis.process) { var f; }
var x = f;
if (globalThis.process) { var x = 5; console.log(x); }Node and a bundle from main print #44001 changes how that pair is printed, so the clean order is #44001 first and this PR without the carve-out after it. My next push makes this branch correct without #44001 too, and answers the review threads. The PR stays a draft until the order is decided. |
… a file with a function and a `var` of one name
A `var`-onto-`var` merge sets `REDECLARED_IN_NESTED_SCOPE` at every scope
level, so a macro constant and an unwrapped `require()` of a `var` that a
block inside a function declares again are right too. The member removal
stays at module level. `SymbolFlags` is 16 bits wide and the flag is bit 9.
A bundle of a file with `function f() {}` beside `var f` loads only while
tree shaking drops the pair, because module code rejects it. The read of
a redeclared `var` can keep a statement that uses `f`. After the hoist
walk, an ESM or CommonJS bundle of such a file gets the removed block
members back. The earlier check read a flag that the walk itself sets.
A `with` body now keeps a read of a module-level `var` that it declares
again, so the runtime transpiler cache version is 39. Decorator metadata
follows the link of a type that the parse pass resolved before hoisting.
…ow-merged-var-uses # Conflicts: # src/jsc/RuntimeTranspilerCache.rs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Pushed f3ac595. The PR stays a draft: #44001 should land first (see "Why this is a draft" in the description). Changes since the review:
Two cases that main has too have no tracking issue:
|
…ow-merged-var-uses
Related to #2814
Problem
bun builddrops a live declaration:var x = { v: 1 }; if (c) { var x; console.log(x.v); }loses its first statement and throwsTypeError: Cannot read properties of undefined (reading 'v').hoist_symbols(src/js_parser/p.rs:3586) links the block'sxto the top-levelx, but the block keeps its own symbol, which the linker does not look up (doStep5.rs:394).Fix
varmerge setsREDECLARED_IN_NESTED_SCOPE, which macro constants and the unwrappedrequire()fold check.function f() {}besidevar fis built as before, until js_parser: print a var merged with a top-level function declaration as an assignment #44001 lands.test/bundler/esbuild/dce.test.ts(17 new cases fail on main) and 7 more files. Self-reviewed: 29 concerns raised, 27 addressed.Background
to_ast(js_parser: record the merged symbol for an export that is re-declared later #42409): both run after the parser used the unmerged symbol.Downsides
vardepend on each other: 27 to 97 ms, 20 to 129 MB. Same-scopevarstatements cost that on main.var R = require("react")with a nestedvar Rholds the namespace object: 884 to 7,789 bytes, minified. Release binary: +4,608 bytes.vartoo early (the__esmlift): 58 of 11,994 generated cells.Notes
Why this is a draft
function f() {} var f;does not load (SyntaxError: Identifier 'f' has already been declared) when something usesf. This change can makefused:function f() { return 1; } if (c) { var f; } var x = f; if (c) { var x = 5; console.log(x); }prints5from main's bundle, because main dropsvar x = f. So an ESM or CommonJS bundle of a file with such a pair gets its block members back after the hoist walk (keep_block_symbols_beside_function_merged_with_var), and the bug stays in such a file. Bun reads a CommonJS bundle that has no CommonJS syntax as module code, so that format is included. An IIFE bundle is a function body and gets the fix. js_parser: print a var merged with a top-level function declaration as an assignment #44001 prints the pair as an assignment. After it, this rule and its list of removed members are deleted.SymbolFlagstou16with the same line as this PR, and each takes bit 8. js_parser: print a var merged with a top-level function declaration as an assignment #44001 takes bit 0. This PR takes bit 9. Both merge orders with bundler: do not bind a redeclared import() / require() local, or a pattern off a {...rest} copy #45016 keep both flags, and no output changes until a guard here readsshares_variable(). That read is a separate change with its own tests.require()fold that the fourth guard edits (src/js_parser/visit/mod.rs:405). The PR that lands second takes the other's arm. The fold is decided at the declaration, where a later store is not known. That is why a bare nestedvar R;costs the namespace object.Context
test/node_modulesand 1,217 package bundles, no output changes.Also fixed by the same change
var { a } = await import("./m.js"); { var a; console.log(a); }at module level printedundefined: the read did not count for the local, and its export was dropped.require()of avarthat a block inside a function declares again:function f(c) { var x = macro(); if (c) { var x = 7; } return x; }returned the macro result.export var x = 1out of the module when only a block that declaresxagain read it (test/bake/dev/esm.test.ts).withbody keeps a read of a module-levelvarthat it declares again:var x; with (obj) { var x; x; }runs a getter ofobj. This changesbun runoutput, so the runtime transpiler cache version is 44.var(decorator_metadata/TypeIsVarRedeclaredInNestedBlock). The parse pass resolves the type before hoisting, so this reader follows the link.Not fixed here
__esmlift (bundler: keep a top-levelvarstore of a lazily loaded ES module at its statement #45529). A file behindimport()orrequire()is wrapped in__esm(() => {...}), and the linker movesvar x = valueabove the wrapper if the value has no side effects (a function, a class, a number, a string). A read that runs before the declaration then sees the value. Main does that for everyvar. Here it also reaches a statement that main dropped, for example a block that declaresxagain and reads it first. A test on that read takes the other branch.dce/VarReadInNestedBlockBeforeItsDeclarationOfWrappedFileis atodocase.var i = 10; for (var i; i < 12; i++)in such a file, and avarin a body without braces (if (c) var x = 1;): the statement keeps its ownvarinside the wrapper, where it is another variable (js_parser: relocate module-level vars from brace-less bodies for the bundler and the REPL #45705).dce/VarRedeclaredInLoopHeadOfWrappedFileis atodocase. With js_parser: relocate module-level vars from brace-less bodies for the bundler and the REPL #45705 merged in (main 449877f and both heads), that case passes, so the PR that lands second removes thetodo. The 12 test files that the two PRs change pass on that merge (debug build). The merge has one conflict, insrc/ast/symbol.rs, where each PR adds its flag.var(function f() {} if (c) { var f; f() }): the bundle still drops the function (js_parser: print a var merged with a top-level function declaration as an assignment #44001).dce/FunctionRedeclaredAsVarInNestedBlockis atodocase.function f() {}besidevar f: see "Why this is a draft".moduleandexportsin a file with no ESM syntax keep the block's own symbol, because the parser rewrites uses of the CommonJS wrapper'smoduleandexports.var module; if (c) { var module; console.log(module.require("./dep.cjs").name); }throwsTypeError: Cannot read properties of undefined (reading 'require')in an ESM bundle, on main too. No issue or PR tracks it.withbody whosevarhas no other declaration loses a bare read of it, on main too (with (obj) { var x; x; }does not run a getter ofobj). The same holds inside a function. No issue or PR tracks it.dce/VarDeclaredOnlyInWithBodyis atodocase.import()destructure tracker there belongs to bundler: do not bind a redeclared import() / require() local, or a pattern off a {...rest} copy #45016.varagain resolves to the outer block's symbol, as on main. The flag is set there.--minify-syntaxand macro #12067, js_parser: admit a macro result to const_values only forconst, and narrow a destructured result into a new node #35548), and a plain rebind of an unwrappedrequire()variable (bundler: do not fold an unwrapped require() declaration when its variable is rebound #45343).Why the block's member is removed and not overwritten
evalpins every member of the scope that it is in. An overwritten member is the top-level symbol, so the pin reached a flat ES module's top-levelvar, and two files could end up with one variable.dce/VarRedeclaredInNestedBlockWithDirectEvalreads both variables after both files ran. It passes on main.evalin the block reads the name as written. A flat ES module does not reserve that name, as on main for avarthat no block declares again.dce/VarRedeclaredInNestedBlockReadByDirectEvalis the single-file case, which fails on main.varonly there (src/js_parser/fold.rs:75), and inside a function it would only rename minified locals.Measurements (release builds of main 2d6d445 and of this branch on that base, x64 Linux)
.text: +4,608 bytes.hoist_symbols: +288 B in each of 2 copies (nm -S).hoist_symbolsper hoistedvarwith no merge: -2.5 instructions. Per merge inside a function: +20. Per merge at module level: +205 (gdb stepi, 200-statement synthetic files).typescript.js,lodash.jsandreact-dom.development.jshave 0 merges of avaronto avar(gdb breakpoint on the flag store).SymbolFlagsis 16 bits wide, andSymbolstays 48 bytes. Per call (gdb stepi):declare_symbol158 to 160.visit_declforvar v = 230 to 30, for an inlinedconst c = 1115 to 118.s_localforvar v = 1392 to 395, forconst c = 1443 to 449.find_symbol_with_record_usage,record_usage_impl,handle_identifier,take_symbol_uses,declare_symbol,do_step5,needs_wrapper_ref,generate_code_for_file_in_chunk_js): 7 of 17 copies have the same instructions as main. The other 10 differ because the fields ofSymbolmoved: 12,419 to 12,417 instructions in the 17 copies (objdump).for (var i = 0; ...)loops, time and peak RSS ofbun build: K=500 28 ms / 17 MB to 21 ms / 24 MB. K=2,000 27 ms / 20 MB to 97 ms / 129 MB. K=5,000 33 ms / 26 MB to 530 ms / 793 MB. K=20,000 0.07 s / 57 MB to 8.6 s / 11.4 GB. The same-scope form (var t = f(); use(t);x 20,000) takes about 10 s and 8.2 GB on main. 50 loops make 2,500 step-5 dependency edges (main: 50).var React = require("react"); if (c) { var React; } console.log(React.version);with react 18.3.1 and--minify: 884 to 7,789 bytes.var a = { v: "kept" }; if (c) { var a; a = { v: "assigned" }; }: 82 to 112 bytes, because the plain store is a use of the top-level symbol, as it is without the block'svar.Right on main stays right (the same two builds, bundles run under node v26.3.0)
import(), unwrappedrequire()with lifted and wrapped targets,module/exports, catch, parameter, in esm, cjs, iife, with and without--minify-syntax, and unbundled). 32 iife cells print nothing on both builds and check nothing. Of the other 132: 107 match node on main, 121 here, 0 match on main and differ here.vardeclared at the top level and again inif, block,for,switch,try,catch, label,doandwithbodies, with reads, stores, closures,eval, and top-level functions of the same name. Flat, behindrequire()and behindimport(). In esm, cjs,--minify-syntaxand--minify-identifiers): 47,837 cells. 43,870 match node on main, 44,623 here.varbefore its declaration runs (the__esmlift above).moduleandexportsas the names: 4,560 cells. 13 become right, 0 become wrong.for-in/for-ofheads, behindimport(),require()and a static import): 15,749 cells. Every cell prints the same as on main.test/node_modules):--no-bundleof 32,082 files: 0 differ. Bundles of 1,217 package entries in esm, cjs, iife,--minify-identifiersand--minify: 0 differ. Each of 7,792 ES module files behindawait import()(5,885 get an__esmwrapper): 0 differ.Review history
var, inside functions too, and guardedmodule/exportswith the flag. It broke programs that main bundles correctly. This PR is the re-cut.varinside the__esmwrapper. It is removed: the store then depends on the wrapper, and a statement that keeps its ownvarthere takes the store. The guard changed 0 of 5,885 wrapped corpus files.has_top_level_function_merged_with_varin the hoist merge, whichhoist_symbolsalso sets part of the way through its walk. The rule for such a file now runs after the walk.Suites
esbuild/dce, 2 inbundler_cjs2esm, and 1 each inbundler_dynamic_import_dce,bundler_decorator_metadata,transpiler/macro-test,transpiler/runtime-transpiler,transpiler/transpilerandbake/dev/esm.esbuild/dce(115 pass),bundler_cjs2esm(117),bundler_dynamic_import_dce(334),bundler_decorator_metadata(3),transpiler/macro-test(35),transpiler/runtime-transpiler(16),transpiler/transpiler(260),bake/dev/esm(19).test/bundler,test/bundler/esbuild,test/bundler/transpiler,run-cjsandbake/dev/esm: 7,607 pass.bundler_bytecode_portable(2 cases) fails on main too in this container.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/esm.test.ts