Repository navigation
Conversation
…its ESM wrapper Lowering a top-level `using` moves the module's top-level `var`s into a generated try block after the statements were visited, so they were never relocated to the top level like other nested `var`s. The linker's `__esm` wrapper only hoists top-level statements out of the closure, so a module with a top-level `using` that was reached through `import()` kept its declarations inside the closure and its export getters threw a ReferenceError. When bundling, relocate those `var`s the same way `maybe_relocate_vars_to_top_level` does, keeping only the assignments inside the try block.
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Comment |
|
Status: reproduced with released 1.4.0 by bundling a module containing a top-level |
…op-level using declarations
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes parser-level AST rewriting (var relocation inside LowerUsingDeclarationsContext::finalize) and the interaction with the linker's __esm wrapper hoist is subtle, a human look would still be worthwhile.
What was reviewed:
- The extracted
relocate_vars_to_top_levelis a pure refactor of the existing tail ofmaybe_relocate_vars_to_top_level; behavior of the original caller is unchanged. - The
relocate_varsgate (options.bundle && current_scope == module_scope) — confirmed the non-module-scopefinalizecall sites (for-of body, switch) are excluded, and--no-bundleoutput is untouched. - Ordering:
finalizeruns insidevisit_stmts_and_prepend_temp_refs, andappend_partdrainsrelocated_top_level_varsimmediately after, so the pushed refs land in the same part. - The comment-cop bot comments were addressed in ab52a68/8d4422cd — the current comments are one-liners.
Extended reasoning...
Overview
The PR fixes a bundler bug where a module containing a top-level using/await using produces a broken bundle when reached through import() (or otherwise wrapped in __esm). The lowering moves the module's top-level vars into a generated try block, and the linker's wrapper hoist only scans literal top-level statements, so the export getters referenced undeclared bindings. The fix reuses the existing relocated_top_level_vars mechanism: finalize now converts each module-level var decl inside the try block into an assignment and pushes the binding refs to relocated_top_level_vars, which append_part then emits as a single top-level var list. The conversion body was extracted from maybe_relocate_vars_to_top_level into relocate_vars_to_top_level so it can be called at module scope (the wrapper deliberately no-ops there).
Four files touched: src/js_parser/fold.rs (extract helper), src/js_parser/p.rs (call it in finalize), test/bundler/bundler_edgecase.test.ts (6 new itBundled cases covering using/await using × browser/node × wrapped/inlined), and test/bundler/bundler_npm.test.ts (updated pinned source-map positions and filesize for npm/ReactSSR, whose entry has a top-level using).
Security risks
None. This is a compile-time AST transformation with no untrusted-input parsing, no auth/crypto/permissions surface, and no runtime FFI.
Level of scrutiny
Medium-high. The change itself is small and mechanical, but it sits at the intersection of three subsystems (using lowering, var relocation via to_expr_wrapper_hoisted → wrap_identifier_hoisting → relocated_top_level_vars, and the linker's WrapKind::Esm hoist), and it changes generated code for every bundled module that has a top-level using. The record_usage call inside wrap_identifier_hoisting now fires for these bindings — that mirrors what already happens for nested vars under the same relocation, so tree-shaking bookkeeping should be consistent, but a maintainer familiar with the symbol-use accounting should confirm there's no unintended interaction with the __stack/handle refs the lowering itself generates.
Other factors
- Test coverage is thorough: both
usingandawait using, bothbrowserandnodetargets, both the wrapped (__esm) and inlined shapes, plain exports, destructuring, an initializer-less decl, and a hoisted function reading non-exported bindings. TheonAfterBundleregex asserts the exact hoistedvar __stack, state, handle, result, val, assignedLater;line, andrun.stdoutverifies runtime behavior. - The
npm/ReactSSRpinned-position update (-4/-8/-16 columns, -1 byte filesize) is explained in the description as fivevarprefixes becoming assignments plus one trailingvarlist; the arithmetic is plausible but I did not re-derive it independently — CI will catch a mismatch. - The github-actions comment-cop flagged both source files twice for long comments; the author responded with two follow-up commits that condensed them to single lines, so those are addressed.
- The description explicitly scopes out three neighbouring bugs in the same lowering (#38279, dropped
export default function/class, class decls staying in the try block), which is reasonable for a focused fix.
|
On the symbol-use accounting question from the review: the relocation records exactly what it records for a nested |
|
Updated 5:50 AM PT - Aug 14th, 2026
❌ @robobun, your commit 4d6da34 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38284That installs a local version of the PR into your bun-38284 --bun |
…mode, not only when bundling The REPL transform hoists a line's declarations the same way the linker's ESM wrapper does, by looking at top-level statements only, so declarations sharing a line with a top-level `using` were lost between REPL inputs too. Also cover `export default <expr>` in the bundler fixture.
using module out of its ESM wrapperusing reachable from the ESM wrapper and the REPL
using reachable from the ESM wrapper and the REPLusing out of the ESM wrapper and the REPL IIFE
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes parser output for every module with a lowered top-level using (not only wrapped ones — the final commit dropped the options.bundle gate the description still mentions, so Bun.Transpiler output changed too), a human familiar with the bundler's symbol-use accounting should confirm.
What was reviewed:
- The
relocate_vars_to_top_levelextraction is a pure refactor of the existing tail ofmaybe_relocate_vars_to_top_level; behavior identical for the existing caller. relocate_module_varsgate: verified the otherfinalizecallers (for-of body, switch case) run in a nested scope, so only the module-level lowering is affected.- Traced
wrap_identifier_hoisting→relocated_top_level_vars→append_part's emission loop; the extrarecord_usageper binding matches what a nested relocatedvaralready does. - Snapshot change is semantically equivalent (
varinside a module-level try was already module-scoped).
Extended reasoning...
Overview
Two Rust files: fold.rs splits the assignment-generation half of maybe_relocate_vars_to_top_level into a callable relocate_vars_to_top_level; p.rs calls it from LowerUsingDeclarationsContext::finalize for every SLocal of kind KVar when current_scope == module_scope, replacing the declaration with the joined assignments and letting append_part emit the trailing var x; list. Tests: six new itBundled cases in bundler_edgecase.test.ts (using / await using × wrapped / inlined × browser / node), a REPL end-to-end test, a replMode transform-output test, updated pinned sourcemap positions in bundler_npm.test.ts, and one updated transpiler snapshot.
Security risks
None. Pure code-transformation change in the JS parser; no I/O, no untrusted-input parsing surface added, no allocator/refcount changes.
Level of scrutiny
High. This is bundler correctness code where the invariant being restored ("every top-level binding of a module is a top-level statement") is what the linker's __esm hoist and tree-shaking rely on. The mechanism reused (relocated_top_level_vars + wrap_identifier_hoisting) also touches symbol-use counts, which feed DCE decisions. The change is small and reuses an existing well-understood path, but the blast radius of a mistake here is any module with a top-level using on a non-bun target.
Other factors
- The PR description says the change is "gated on
options.bundle"; the final commit (4d6da34) changed the gate tocurrent_scope == module_scope, which now affectsBun.Transpilerand--no-bundleoutput (visible in the updated snapshot). This is semantically a no-op for unwrapped output — avarinside a module-level try block was already module-scoped — but it is a user-visible output change the description doesn't reflect, and it also fixes the REPL case (covered by the two new REPL tests). A human should confirm the broader gate is intentional and that the extrarecord_usageper relocated binding doesn't perturb tree-shaking for the unwrapped/inlined path (theLoweredTopLevelUsingInlinedtests exercise this by running the output). - Test coverage is thorough: the wrapped tests run the bundle and assert the exact hoisted
varline, and the author confirmed the four*InEsmWrapper*tests fail with released 1.4.0. The comment-cop feedback about long comments was addressed in ab52a68 (all resolved). - Checked that the other two
finalizecall sites (for-of/for-in withusinginit, switch-case bodies) always run with a nestedcurrent_scope, so the new relocation path cannot fire for them.
|
The description was rewritten for the current shape right after that push: the relocation is no longer limited to bundling (it runs whenever the lowering handles the module body itself), which is what fixes the REPL as well, and the updated On tree shaking for the inlined path: a module with a lowered top-level |
Problem
bun buildfor a target that lowersusing(anything but--target=bun) produces a broken bundle when a module containing a top-levelusing/await usingis reached throughimport(): reading any of its exports throwsReferenceError: result is not definedfrom the export getter (result: () => resultin the generated__export(...)call). A static import of the same module fails the same way once the module is wrapped. Reproduces with released 1.4.0; esbuild 0.25.12 has the same bug.bun build --target=browser entry.mjs --outfile=out.mjs && node out.mjs):bun repllowersusingon purpose (see the comment insrc/runtime/cli/repl.rs), so afterusing h = ...; const y = 1on one line,yon the next line throwsReferenceError: y is not defined. Same withnew Bun.Transpiler({ replMode: true }).LowerUsingDeclarationsContext::finalize(src/js_parser/p.rs) runs after the module body has been visited and moves the module's top-levelvars into the try block it generates.vars nested inside other statements get relocated to the top level while they are visited (maybe_relocate_vars_to_top_level), but these were top-level statements at that point, so nothing relocated them. Both consumers that hoist a module's declarations, the linker's ESM wrapper (src/bundler/linker_context/generateCodeForFileInChunkJS.rs,WrapKind::Esm) and the REPL transform (src/js_parser/repl_transforms.rs), only look at the module's top-level statements, so thevars stayed inside__esm(() => { try { var result = ... } ... })(or the REPL's IIFE) while__exportand the importers (or the next REPL line) reference them outside of it.Fix
finalizenow relocates the module body'svars through the same mechanism as nestedvars (relocated_top_level_vars, emitted byappend_part): the declarations become top-levelvarstatements and the try block keeps only the assignments. The conversion is split out ofmaybe_relocate_vars_to_top_level(src/js_parser/fold.rs) intorelocate_vars_to_top_level, since the existing function deliberately does nothing at module scope.var __stack, state, handle, result;beforevar init_lazy = __esm(...), and the REPL transform hoistsvar y;next to its IIFE, with no changes to either. Static imports, export getters, hoisted functions reading non-exported module variables and later REPL lines all see the same bindings. Disposal is unchanged: the try/catch/finally still runs where it did, and thevars were module-scoped before, so making the declarations explicit does not change what unwrapped output does.finalizehandles the module body itself (current_scope == module_scope), regardless of mode, like the nested-varrelocation it reuses; the otherfinalizecall sites (for-of bodies, switch statements) run in nested scopes, where the visit pass already relocated everything. Non-bundled output for a module with a top-levelusingtherefore changes shape too (x = ...in the try plus trailingvar x;statements, the shape nestedvars already get), which is what the updatedusing top levelsnapshot intest/bundler/transpiler/__snapshots__/transpiler.test.js.snapshows; that output still runs and exports correctly.var:wrap_identifier_hoistingrecords one use per relocated binding for the assignment that now references it, andappend_partre-declares it as a top-level symbol of the same part. Tree shaking is not affected either: a module with a lowered top-levelusingis always emitted as a single part (parse_entry.rsdisables per-statement parts for it, since everything lives in one try block), that part is never removable because of the try statement, and the relocated declarations are added to that same part, so the recorded uses only point at the part they are in. The lowering's own temporaries are not affected:__stackis declared by theletthatfinalizeemits outside the try block, and_catch/_err/_hasErr/_promiselive in the catch and finally blocks, so none of them go through the relocation.test/bundler/bundler_edgecase.test.ts(edgecase/LoweredTopLevel*): a module with a top-levelusingorawait usingthat is imported both statically and withimport(), built forbrowserandnode. The fixture has every kind of declaration the lowering moves into the try block: exported and localconst, theusingitself, a destructuring pattern, aletwithout initializer, a function reading the local ones, andexport default <expr>(whichvisit_stmt.rsturns into its ownvarwhen a top-levelusingis present). Plus the same module imported only statically (inlined, not wrapped). The four wrapped cases fail before this change with the ReferenceError above.test/js/bun/repl/repl.test.ts(declarations on a line with a top-level using persist) andtest/js/bun/transpiler/repl-transform.test.ts(declarations next to a top-level using are hoisted ...): both fail before this change.test/bundler/bundler_npm.test.ts(npm/ReactSSR) pins the exact size and three source map positions of a minified entry point that has a top-levelusing; its fivevar x=...statements inside the try block becomex=...plus one trailingvarlist, so the output is 1 byte smaller and the pinned positions move by the removedvarprefixes (4, 8 and 16 bytes). The pinned values are updated accordingly.test/bundler/bundler_edgecase.test.ts,test/bundler/transpiler/transpiler.test.js, the rest ofrepl.test.ts/repl-transform.test.ts,test/js/bun/resolve/lower-using-bun-target.test.ts,test/regression/issue/11100.test.ts,test/js/web/explicit-resource-management.test.ts,test/bundler/bundler_browser.test.ts,test/bundler/esbuild/dce.test.ts,test/bundler/esbuild/default.test.ts. Manually checked--minify,--format=cjs/iife,--splitting,require()of the module from a CommonJS file, a CommonJS file with a top-levelusing,--format=internal_bake_devoutput, and runningBun.Transpiler(non-bundled) output of a module with exports under bun.Background
using: for targets without native support, a top-levelusing x = vmakes the parser rewrite the whole module body intolet __stack = []; try { var x = __using(__stack, v); ...rest of the module as var declarations... } catch (e) { ... } finally { __callDispose(__stack, ...) }, with imports and function declarations kept outside the try and exports collected into a trailingexport { ... }clause.import(),require()of ESM), the linker emits its body asvar init_x = __esm(() => { ... })and anexports_xobject whose getters read the module's bindings. For that to work the module's top-level declarations are moved out of the closure asvar a, b;with the initializers left inside as assignments.vardeclarations plus an IIFE containing the rest, so the variables become properties of the REPL's context and survive to the next input. It finds the declarations the same way, by walking the input's top-level statements.varthat sits inside a block (if (c) { var x = 1 }) into a top-levelvar x;plus an assignment in place, which is what lets those two passes look at top-level statements only. This change applies that same transformation to the declarations theusinglowering moves into its try block.export default function/classbeing dropped; class declarations staying inside the try block) are tracked separately and are not changed here.