Repository navigation
Conversation
… is rebound later When bundling to ESM, visit_decls turns any identifier declaration whose initializer is a require() of a package on the CommonJS unwrap list into the import binding itself, so later assignments to the variable went to the import namespace while reads of its properties had already been bound to the package's exports. Redeclaring the variable produced a bundle that throws. The parser now records, during the parse pass, every name the file assigns to (plain, compound and update assignments, destructuring patterns, for-in/of heads) or declares more than once, and visit_decls keeps such a declaration as a variable initialized from the import's namespace, as require() in any other expression position already is. Declarations that are never rebound, and const declarations, are still replaced by the import, so the React packages bundle exactly as before.
|
Status: CI green on 5dbd732 (Buildkite build 99258), ready for review. Reproduced on 1.4.0 and main by bundling an entry that does Tests, all in Rework: the first revision matched rebindings by bare name, which would have stopped minified consumers ( |
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 selected for processing (5)
Comment |
|
@robobun why doesnt esbuild have this problem? |
|
Because esbuild never performs the rewrite this bug lives in. When esbuild bundles CommonJS, That rewrite is Bun-specific: for the packages in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and found no bugs. Because it hooks the hot new_expr path (with a monomorphization claim about the added match compiling away), adds name-keyed over-approximation state to the parser, and changes when the cjs2esm require-unwrap tree-shaking optimization is disabled, a maintainer look at the design trade-off would be worthwhile.
What was reviewed:
record_rebound_targethandles identifiers, arrays, objects, and spreads; member targets fall through to_and are not hashed.- The
visit_declsis_reboundbranch replaces the marker withdeferred.namespace.ref_and falls through (nocontinue 'outer), sovisit_declstill runs on the kept declarator. record_redeclared_nameindeclare_symbolfires onentry.found_existingin the current scope only, so a same-namedletin an inner scope does not count (verified by the tree-shaking test'slet top = "shadow"); thehoist_symbolshook covers hoistedvarcollisions.- All recording is gated on
should_unwrap_common_js_to_esm(), so the map stays empty outside ESM bundling.
Extended reasoning...
Overview
This PR fixes a bundler correctness bug where let/var x = require("react") (for packages on the CommonJS unwrap list) is rewritten into an import namespace binding even when x is later reassigned, causing the reassignment to be lost or the bundle to throw. It adds a rebound_names: HashMap<&[u8], ()> to P, populated during the parse pass from four sites: assignment/update targets in new_expr, for-in/of loop heads in parse_stmt, same-scope redeclarations in declare_symbol, and hoisted-var collisions in hoist_symbols. visit_decls then consults it: for a non-const declarator whose name is in the set, it keeps the declarator and initializes it from the deferred import's namespace instead of replacing the binding with the import.
Three new itBundled tests cover 18 rebinding forms, redeclaration (same-scope and hoisted), and a negative test pinning that non-rebound var/let/const still become the import and tree-shake.
Security risks
None. This is bundler output-shape logic; no untrusted input parsing beyond what the JS parser already does, no filesystem/network/auth surface.
Level of scrutiny
High. new_expr is called for every expression built during parsing across the entire bundler, and the PR relies on a compiler-optimization claim ("T fixes the variant at each inlined call site, so this compiles to nothing except where a binary or unary expression is built") for its cost model. That claim is plausible given #[inline] on new_expr and the generic T: IntoExprData, but it depends on Expr::init inlining and LLVM propagating the discriminant — not verified here, and definitely not true in debug builds. The name-based over-approximation is deliberately conservative (assignment to a same-named variable in any scope disables the rewrite for every let/var of that name), which the description argues only costs tree-shaking, never correctness; that trade-off, and whether the declare_symbol/hoist_symbols hooks over-fire in practice (TS overloads, function redeclarations, catch bindings), is a design call for a maintainer.
Other factors
- The description is unusually thorough and cites the
npm/ReactSSRbyte-position test as unchanged, plus eight other bundler suites passing. - The three new tests exercise each recording site independently (the description states removing only the
hoist_symbolsor for-in/of hook fails exactly the corresponding case). HashMap<&[u8], ()>with.contains(&name)matches the existingbun_collections::HashMap::containssignature;load_name_from_refreturns&'a [u8]so the lifetime works.- The change composes with #39184 and #39244 per the description;
was_constis already threaded intovisit_declsfrom #39244.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Because it hooks new_expr (every expression construction) and declare_symbol/hoist_symbols, and encodes a design choice (name-keyed over-approximation rather than symbol-keyed tracking), a human sign-off on the approach and the hot-path cost would still be worthwhile.
Checked: the is_rebound fallback in visit_decls reuses the namespace_ref that transpose_require already record_usage'd and inserted into import_items_for_namespace, matching the non-decl expression path — no missing/extra usage count. The !was_const guard means a same-named assignment in another scope cannot disable the rewrite for a const (pinned by NotReboundIsTreeShaken). The for-in/of hook only fires on the expression-init arm, so for (let x of ...) isn't recorded (correct — that's a fresh binding).
Extended reasoning...
Overview
The PR fixes a bundler correctness bug where a let/var initialized by require() of a package on the CommonJS-unwrap list is aliased to the import namespace even when the file later reassigns or redeclares it, so subsequent reads keep seeing the original module instead of the new value (and redeclaration produces a bundle that throws). It adds a rebound_names set on P, populated during the parse pass from four recording sites: the EBinary/EUnary arm of new_expr (via binary_assign_target/unary_assign_target), the expression-init branch of for (x in/of ...) in t_for, the found_existing branch of declare_symbol, and the ancestor-scope collision branch of hoist_symbols. visit_decls consults the set for non-const declarators carrying the unwrapped_id marker and, if the name is present, keeps the declarator with the deferred namespace ref as its initializer instead of dropping it and renaming the pending import.
Security risks
None. This is bundler output-shape logic; no untrusted input parsing beyond what the JS parser already does, no auth/crypto/fs/network.
Level of scrutiny
High. new_expr is called for every expression the parser and visitor construct, so any per-call cost here is multiplied across every file bundled or transpiled. The PR description reports an assembly-level check that the added match compiles to a range check on the op enum inside the EBinary/EUnary constructor sites and nothing elsewhere, gated by the should_unwrap_common_js_to_esm() flag before any hashing — plausible given the code shape, but I can't verify the emitted assembly. declare_symbol and hoist_symbols are also on every declaration; the added call there is a single flag test in the common (unwrap-off) case. The name-keyed over-approximation is a conscious design trade-off (parse pass hasn't bound refs to symbols yet); it only ever falls back to a correct-but-less-tree-shakable output, and the PR pins that the important var React = require('react') shape inside the React packages themselves is unaffected. That trade-off, and whether the hot-path cost is acceptable, is the kind of call a maintainer should confirm.
Other factors
Test coverage is thorough: UnwrappedModuleRequireRebound exercises plain/compound assignment, pre/post-increment, array/object/rest destructuring targets, for-in/for-of, an assignment inside a nested function, an assignment before the hoisted var, and a wrapped (module.exports) target as a control; UnwrappedModuleRequireRedeclared covers same-scope and hoisted-block redeclaration; UnwrappedModuleRequireNotReboundIsTreeShaken uses dce: true to pin that non-rebound var/let and a const whose name is assigned in another scope still take the alias path. The comment-cop bot flagged long comments in earlier commits; those threads are all resolved and the current diff has one-line comments. alii asked a clarifying question that was answered; no explicit approval or change request from them yet.
…name
Keying the rebound set by bare name disabled the require() unwrap for any
file that assigns to a same-named variable in another scope, which is the
normal shape of minified CommonJS (var e = require("react") at the top,
e reused as a local everywhere else) and kept all of react in such
bundles. Record assignment targets as (scope, name) while parsing and
bind them against the scope tree the first time an unwrapped require()
binding asks, after hoisting; record redeclarations as the symbols
involved. A debug assertion checks that a variable turned into its
import was never assigned to.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds new cross-pass state to the parser (unresolved_rebound_targets / rebound_refs) and hooks into new_expr, declare_symbol, and hoist_symbols, a human look at the design would still be worthwhile.
Checked that the rebound path in visit_decls reuses the namespace ref whose usage transpose_require already recorded, so use counts stay balanced.
Checked that declare_symbol's MR::Forbidden arm returns before record_redeclared, so genuine redeclaration errors aren't recorded.
Checked that hoist_symbols and declare_symbol over-recording (e.g. private get/set pairs, hoist-into-let collisions) only costs an optimization, never correctness.
The debug_assert on has_been_assigned_to for a user-written const x = require(...); x = 1 was examined and ruled out.
Extended reasoning...
Overview
This PR fixes a bundler correctness bug in the CommonJS-unwrap path: when let/var x = require("react") is later reassigned or redeclared, the current bundler still aliases x to the import namespace and rewrites x.prop to named imports, so the reassignment is lost (or the bundle throws on redeclaration). The fix records, during the parse pass, every assignment/update target and every redeclaration, then in visit_decls resolves those against the scope tree and keeps the declarator as a real variable (initialized from the namespace) when its symbol is in the rebound set. Changes span p.rs (new rebound_refs / unresolved_rebound_targets fields, record_rebound_target / record_redeclared / binding_is_rebound, hooks in new_expr / declare_symbol / hoist_symbols), visit/mod.rs (the is_rebound branch), parse_stmt.rs (for-in/of head), parse_entry.rs (a debug assertion), and three new bundler tests.
Security risks
None. This is bundler AST-transform logic with no I/O, auth, or untrusted-input parsing beyond what the parser already handles.
Level of scrutiny
High. new_expr is called for every constructed expression in every parsed file; declare_symbol and hoist_symbols are core to scope construction. The recording is gated on should_unwrap_common_js_to_esm() (an inlined flag test), and the PR description argues the compiled code leaves work only in the assignment-operator arms of EBinary/EUnary, but the placement of a persistent match inside new_expr and the choice to defer scope resolution to first use are design decisions a maintainer should confirm. The first revision of this PR was already reworked once (name-keyed → scope-resolved) after it regressed react tree-shaking, which underlines that the interaction with minified consumers is subtle.
Other factors
- Tests are thorough: a positive test covering 15+ rebinding forms (assignment, compound, ++/--, array/object/spread destructuring, for-in/of, hoisted-var-in-block, cross-function), a redeclaration test, and a negative test pinning that same-named locals/parameters/block-lets in other scopes do not disable the rewrite (
dce: trueverifies tree-shaking still works). The evidence gate confirms the new tests fail on release main and pass on the branch, and the wider bundler suites were run under the debug assertion. - I traced the
is_reboundbranch:transpose_requirealready callsrecord_usage(namespace_ref)and insertsnamespace_refintoimport_items_for_namespacebefore returning the marker, so replacing the marker withEIdentifier(namespace_ref)here yields a correctly-counted reference, and property reads of the user variable stay as property reads (its ref is not inimport_items_for_namespace). record_redeclaredfires broadly (private get/set pairs, function overloads, hoist collisions) but is gated on the unwrap flag and only over-approximates "rebound", which can only skip an optimization, not miscompile.- The comment-cop bot flagged long comments six times; all were resolved by the author trimming to one-liners in follow-up commits.
- No CODEOWNERS-gated paths appear to be involved beyond normal parser ownership.
This branch has not been deployed
2 participants
Problem
bun build(ESM output, the default) loses later assignments to alet/varthat was initialized by arequire()of a package on the CommonJS unwrap list (react,react-dom,scheduler, ...) when the required module converts to ESM (it usesexports.x = ..., notmodule.exports = ...). Same on 1.4.0 and main:r = ...,r += ...,r++,[r] = ...,({ r } = ...),for (r of ...), an assignment inside a function, an assignment that precedes the declaration (varhoisting), and a declaration that itself reaches module scope by being hoisted out of a block. Declaring the variable a second time (var r = require("react"); ... var r = {...}, or avar rin a nested block) bundles to code that throwsTypeError: undefined is not an object (evaluating 'exports_react.react'): the package's namespace object is never emitted, but the redeclared variable has been renamed to it.var React = require('react'); React = React && React.hasOwnProperty('default') ? React['default'] : React;.visit_decls(src/js_parser/visit/mod.rs) visits every declarator's initializer withis_immediately_assigned_to_decl;transpose_require(src/js_parser/p.rs) answers it with anE::RequireStringmarker, andvisit_declsconsumes the marker by renaming the pendingimport * as nsto the declared variable and deleting the declarator. From then onr.schedis rewritten into a named import ofschedandritself is the import namespace. That treats the variable as an immutable alias of the module, which holds forconstand for a variable nothing else touches, but not for one the file assigns to or declares again. Whenvisit_declsruns, the rest of the file has not been visited yet, so it cannot tell the two apart from what it has seen. (When the target stays a CommonJS wrapper the namespace happens to print asvar r = __toESM(require_x(), 1), a real variable, which is why only converted targets show it.)Fix
new_expr, using the samebinary_assign_target/unary_assign_targetclassifiers as the visit pass, looking through destructuring patterns) and the head of everyfor (x in/of ...)(parse_stmt), as(scope it was parsed in, name)inunresolved_rebound_targets. Identifiers are not bound to symbols until the visit pass, so this is all that is known at that point;rebound_refs: a second declaration in the same scope (declare_symbol, which also covers avarsharing a parameter's name, since parameters are copied into the body scope) and a nestedvarhoisted into a scope that already has the name (hoist_symbols).visit_declsmeets the marker for a non-constdeclarator it callsbinding_is_rebound, which first binds the recorded targets by walking each one's scope chain (hoisting has run by then, so avardeclared later or inside a block is found where it ends up) and adds the symbols they hit torebound_refs, then asks whether the declarator's own symbol is in the set. If it is, the declarator is kept and its initializer becomes the import's namespace, which is whattranspose_requirereturns for arequire()in any other expression position:var r = exports_scheduler;for a converted target,var ns = __toESM(require_x(), 1); var r = ns;for a wrapped one, withr.schedleft as a property read of the variable. Otherwise the declarator takes the unchanged replace-with-import path.parse_entry.rsnowdebug_assert!s that no namespace symbol was assigned to. A fresh namespace never is, so this only fires ifvisit_declsreplaced a variable that the visit pass later saw assigned, i.e. if the recording above ever misses a form of assignment (redeclaration has no symbol flag to check).varhoisted into some other function does not count. That matters because minified CommonJS looks exactly like that (var e=require("react")at the top,ereused as a local in every function): a first version of this change keyed the set by bare name and therefore kept all of react in such bundles;cjs2esm/UnwrappedModuleRequireNotReboundIsTreeShakenfails on that version. With this version a tsdx-shaped consumer of the real react 18.3.1 bundles byte-for-byte the same as on 1.4.0 (1093 bytes minified, no namespace object), andnpm/ReactSSR(byte positions in the real react-dom 18.3.1 output, whose ownvar React = require('react')/var Scheduler = require('scheduler')depend on this rewrite) is unchanged.constdeclarators skip the question: they cannot be assigned or redeclared, and this keeps the TypeScript-emittedconst react_1 = require("react")shape from binding the file's targets at all.let/var, doing it here confines it to files that actually have such a declarator. The marker does not survive either way, it is replaced by the namespace identifier before the declarator continues down the normal path.exports.x = ...push nothing), freed when the targets are bound or when the parser is dropped; two hash inserts per redeclaration; the binding walk only in files with a non-constunwrappedrequire()declarator. Outside ESM bundling nothing is recorded:record_rebound_targetis an inlined flag test, and in the optimized assembly thenew_exprmatch leaves code only in theE::Binary/E::Unaryconstructor paths (a range check on the operator, then the flag test for assignment operators only), nothing in any other instantiation.Pgrows by an emptyRefMapand an emptyVec.test/bundler/bundler_cjs2esm.test.ts:cjs2esm/UnwrappedModuleRequireRebound: the forms listed above against a converted package, plus one against a wrapped package, checked against what the entry prints unbundled. 14 of its 19 lines are wrong on the current release, including the declaration hoisted out of a block.cjs2esm/UnwrappedModuleRequireRedeclared: same-scope redeclaration and a hoisted block-levelvar; the current release's bundle throws. Removing only thehoist_symbolshook fails exactly the second case, removing only the for-in/of hook fails exactly theforlines of the first test.cjs2esm/UnwrappedModuleRequireNotReboundIsTreeShaken(passes on main, fails on the name-keyed version): a top-levelvar, aletinside a function and aconstall still become the import (dcepins that the package's unused export is gone) while the same name is assigned as a parameter, assigned after being hoisted out of aforinside another function, redeclared over a parameter, assigned as a block-levellet, and used as an arrow default.bundler_cjs,bundler_edgecase,bundler_regressions,bundler_jsx,bundler_npm,esbuild/default,esbuild/importstar,esbuild/dce,esbuild/tsandtranspiler/transpiler.test.jswith the debug build (so with the new assertion armed): no failures.exported andusingdeclarators). Both change which declarators get the marker; this one changes what happens to a marker for an identifier declarator, so the three compose. bundler: keep exported and using declarations initialized by an unwrapped require() #39244 keeps a localwas_constinvisit_decls, which is what this change reads.Background
DEFAULT_UNWRAP_COMMONJS_PACKAGES(src/bundler/options.rs) are parsed withexports.x = ...turned into ESM exports, and everyrequire()that resolves into one of them, from any file, becomes animport * as ns from "..."statement (emitted at the end of the parse fromimports_to_convert_from_require) plus a reference tons.ns.xis then rewritten into a named import ofx, so a file that only reads properties never needs the namespace object and the package tree-shakes. esbuild has no equivalent: thererequire()of CommonJS stays a call to the__commonJSwrapper, and the namespace machinery this reuses only ever applies to realimport * as nsbindings, which cannot be assigned to (that is a bundling error).exports.xconverts to ESM, and its namespace prints as the namespace object the linker generates for it (exports_scheduler), shared by every importer. A file that assignsmodule.exportsstays a CommonJS wrapper, and each importer gets its ownvar ns = __toESM(require_x(), 1).is_immediately_assigned_to_decl/E::RequireString.unwrapped_id: the handshake betweenvisit_declsandtranspose_require. The flag asks for the marker; the marker'sunwrapped_idindexes the pending import;visit_declsuses it to rename the import's namespace to the declared variable and drop the declarator, which is howconst React = require("react")becomesimport * as React from "react".Scope::members;hoist_symbolsthen moves nestedvars up into their function or module scope; only the visit pass binds identifiers to symbols and performs rewrites such as this one, in source order. WalkingScope::parentand checkingmembersis the same lookup the visit pass'sfind_symbolperforms, which is why binding the recorded targets after hoisting reproduces what the visit pass will bind them to.Symbol::has_been_assigned_to: a flag the visit pass sets on a symbol when it visits an assignment to it (HMR uses it to decide which exports need live bindings). It is final once the visit pass is over, which is when the import-emitting loop runs, so it can double-check the decisions made earlier.Earlier revision of this PR
The first revision recorded bare names (
rebound_names: HashMap<&[u8], ()>) and compared the declarator's name against them. That is correct but over-approximate: an assignment to a same-named variable in any scope disabled the rewrite, and since minified CommonJS reuses the same short names in every function, it kept all of react in bundles that 1.4.0 tree-shakes. The current revision records scopes alongside the names and binds them to symbols instead, and adds the tree-shaking test cases that distinguish the two.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file