Repository navigation
Conversation
Inside a function the React Compiler compiles, `require("x")` was lowered
to a pure `LoadGlobal` constant. Dead code elimination dropped an unused
one, so the module was never evaluated. Constant propagation moved a used
one to each of its reads, which changed the evaluation order. And a local
that gets a different module on each path read the first module, because
constant propagation compares such constants by a name that is `require`
for all of them.
Lower both nodes the way `import()` is lowered: a `CallExpression` whose
callee loads the original node. Codegen finds such a call by its callee
temporary, so `require("x")()` is not read as `require("x")`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe React Compiler now lowers ChangesRequire call preservation
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The require-call preservation change is covered across lowering, code generation, and bundler behavior, with no unresolved merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix pushed, waiting for CI. How I reproduced it (bun 1.4.3-canary.1 6a92015, and a debug build of main 4b5862f), in a directory with // side.js
console.log("side evaluated");
// c.jsx
export function App() {
require("./side");
return <div />;
}
App();
console.log("done");With this branch the second command prints Self-reviewed: 5 concerns raised, 5 addressed (rebased on main, left the |
![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 didn't find any bugs. The change closely mirrors two existing patterns in the same files (the EImport lowering shape and the object_methods identifier-keyed map), and the test matrix is thorough. A human look is still worthwhile for the HIR modeling choice and the stated coordination with #42376 (which arms of loads_same_value become dead, and landing order).
What was reviewed:
- Lowering in
expr.rs— matches theData::EImportarm directly above it;ERequireCallTarget/ERequireResolveCallTargetremain in the opaque-constant arm. - Codegen lookup — keyed on the callee temporary's
IdentifierId, not the printed expr, sorequire("./fn")(arg)(covered byCallsTheExport) still emits the outer call; falls through to normalLoadGlobalcodegen for every other binding. - Test — exact-stdout assertion over 13 forms × 2 targets, plus an
onAfterBundleguard that nouseEffect(() =>survived, so a form that silently bailed out of compilation would fail.
Extended reasoning...
Overview
The PR changes how Bun's React Compiler pass models require("x") and require.resolve("x") in HIR. Previously both were lowered as a pure LoadGlobal(BunOpaque) constant, which let DCE drop unused ones, constant propagation forward them to reads (reordering evaluation), and phi merging pick the wrong branch. The fix lowers them as a CallExpression whose callee is the opaque node — the exact shape the adjacent Data::EImport arm already uses — so HIR passes treat them as effectful calls. Codegen gains a require_call_nodes: IdMap<IdentifierId, Expr> (following the existing object_methods pattern) that records the original bun_ast::Expr at LoadGlobal time and returns it verbatim when the matching CallExpression is emitted, preserving the bundler's import_record_index. A new itBundled test in react-compiler.test.ts exercises 13 syntactic positions across both bun and browser targets with exact-stdout assertions and an onAfterBundle check that every function actually compiled.
Security risks
None. This is a bundler/compiler-pass correctness fix over the internal HIR; no auth, crypto, network, filesystem, or untrusted-input parsing is touched. The worst-case failure mode is incorrect bundled output, which the test asserts against directly.
Level of scrutiny
Moderate. The diff is small and each piece mirrors an established pattern in the same file (EImport lowering at expr.rs:253, object_methods identifier-keyed map at codegen.rs:1418), which substantially lowers the risk of a novel interaction. I checked that the new LoadGlobal match arm in codegen_instruction_nullable falls through (no return) so non-require globals still reach normal codegen, and that the CallExpression short-circuit is keyed on the callee temporary rather than the printed callee — the CallsTheExport form covers the require("./fn")(arg) ambiguity the doc comment describes. The remaining ERequireCallTarget / ERequireResolveCallTarget / EImportMetaMain / ERequireMain variants stay in the constant arm, which is correct since those are the bare identifiers, not calls.
Other factors
The test coverage is strong by REVIEW.md's standards: it asserts exact stdout (not "no panic"), covers both targets, includes a negative control (CallsTheExport) for the codegen lookup, and guards against silent bail-out via the useEffect(() => absence check. No CODEOWNERS entry covers src/react_compiler/ or test/bundler/. I'm deferring rather than approving because the PR explicitly describes coordination with #42376 (dead loads_same_value arms and duplicated test forms to delete depending on landing order) and because the choice to model these as a zero-arg call on an opaque callee — while it matches import() — is a HIR modeling decision a react_compiler maintainer should sign off on.
|
Updated 1:30 AM PT - Sep 12th, 2026
✅ @robobun, your commit 9dea2a47b23e2e78f3273f758b5944f00e26b2e0 passed in 🧪 To try this PR locally: bunx bun-pr 42432That installs a local version of the PR into your bun-42432 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
I worked on the same report and came to the same change: 1. A hook that is read off a export function InlineHook() {
const v = require("./hooks.cjs").useThing();
const [s] = useState(1);
return <div>{v + s}</div>;
}
export function LocalHook() {
const m = require("./hooks.cjs");
const v = m.useThing();
const [s] = useState(1);
return <div>{v + s}</div>;
}
2. This PR also removes a build crash. On main, an unused export function RequireInATryBlock() {
useEffect(() => {});
let found = "found";
try {
require("not-installed");
} catch {
found = "not found";
}
return found;
}Dead code elimination removed the |
This branch has not been deployed
2 participants
Problem
bun build --react-compiler, an unusedrequire("./side")in a compiled component or hook is deleted, so./sidenever runs. The plain bundler keepsinit_side().require()moves to each read of its local, soconst a = require("a"); const { b } = require("b")evaluatesbfirst. An unusedrequire.resolve("x")is deleted too.lower_expression(src/react_compiler/lowering/build_hir/expr.rs:318) loweredE::RequireStringto a pureLoadGlobal { BunOpaque }constant. Dead code elimination prunes an unused one. Constant propagation forwards a used one to every read.Fix
E::RequireStringandE::RequireResolveStringas aCallExpressionwhose callee loads the node, the shapeimport()already has. No pass prunes, forwards or merges a call. Upstream sees this source as a call too.Context::require_call_nodes). The printed callee is not enough:require("./fn")(arg)would read asrequire("./fn").require()result takes one memo slot, like any call result.test/bundler/transpiler/react-compiler.test.ts(2 new tests, 13 forms each, 12 fail on main). Alsoreact-compiler-fixtures.test.ts,bundler_dynamic_import_dce.test.ts,bundler_jsx.test.ts.Background
require("x")is oneE::RequireStringnode that holds its import record index.LoadGlobalinstruction reads a global. The compiler treats it as a constant with no side effects.BunOpaqueis aLoadGlobalthat carries a whole Bun AST node. Codegen prints it back unchanged, so the bundler keeps its import record.Notes
Forms in the new tests. Each form requires its own module, which records that it ran. "main" is a debug build of 4b5862f. The expected column is also what the plain bundler prints.
require("./s");srunsuseEffectcallback (client mode)if (yes) require("./s")void require("./s")const one = (require("./s"), 1)const unused = require("./s")no ? require("./a") : require("./b")b, runsba, runsbthenaif (no) m = require("./a"); else m = require("./b")baconst first = require("./1"); const { name } = require("./2"); first.name + name1,22,1let m = require("./1"); try { m = require("./2") } catch {}1,22onlytry { require.resolve("./not-there"); return "found" } catch { return "missing" }missingfoundrequire("./function")(yes)(control for the codegen change)The ssr output mode (
--target=bun) removes effects by design, so theuseEffectform expects nothing there.Relation to #42376. That PR makes
evaluate_phicompare twoBunOpaqueconstants by the node they carry, not byname(). It fixes the wrong-module rows above and alsoimport.meta.mainagainst!import.meta.mainand import items, which this PR does not touch. It does not fix a dropped or a movedrequire()(its body lists the dropped one as not fixed). After this PR arequire("x")/require.resolve("x")result is not a constant, so those two arms ofloads_same_valuecan no longer be reached. Whichever PR lands second can delete them, and the duplicated ternary and if/else test forms. A trial merge of the two branches is clean in both orders.What the output looks like. In ssr mode the compiled body mirrors the source (
init_side();,let mod = no ? require_a() : require_b();). In client mode the result ofrequire()is memoized like the result of any call to an unknown global:if ($[0] === sentinel) { t0 = require_other(); $[0] = t0 } else t0 = $[0]. Before, a usedrequire()was printed inline at each read with no slot. The module system caches a module after its first evaluation, so the memo block does not change when a module runs.Differential. 52 more shapes print the same lines with and without
--react-compiler, for--target=bunand--target=browser: property reads in JSX, destructure with defaults,new (require(x))(),??/||,typeof, optional chains, template literals, spreads, asset-like requires in JSX attributes,useMemo/ lazyuseStateinitializers, loops,switch, early return, nested ternary,for..in/for..of/forinit, IIFEs, labeled blocks,try/catcharound a module that throws,require(require(x)),require(require.resolve(x)),require(x)(),require(x)?.(),import(). 35 of them were also run with--format=cjs,--target=node,--minify,--minify-syntaxand--splitting. No shape that compiled before stops compiling. Four shapes that mutate a required module (m.extra = 1,delete m.name,require(x).count++,require(x).value = 5) are left uncompiled by 1.4.3-canary.1 (6a92015), and compile now.Why
require.resolve("x")too. It can throw (module not found), the plain bundler does not treat it as removable (expr_can_be_removed_if_unused), and upstream sees a method call onrequire.The codegen lookup. The first version of this change matched the call by its printed callee, as the
import()path does (if let ExprData::EImport(orig) = callee_expr.data).require("./fn")(kind)then printed asrequire("./fn"): the inner call prints as the bare node, which is also what the outer call's callee prints as.Context::object_methodsis the existing pattern for an instruction that a later instruction consumes by identity, sorequire_call_nodesfollows it. Theimport()path is unchanged.Unrelated bugs found on the way, not fixed here (each reproduces without
require):try { const unused = 1 } catch { found = "missing" }in a compiled function panics inmerge_consecutive_blocks.rs:106(Found a block with a single predecessor but where a phi has multiple (2) operands).const o = { get() { return 1 } }in a compiled function panicked with--target=buninalign_object_method_scopes.rs:52. react_compiler: fix panic on an object method shorthand in ssr output mode #42375 fixed it.--target=nodeprints!import.meta.mainas!require.main == module.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file