react_compiler: preserve typeof unbound-global semantics in codegen - #36743
react_compiler: preserve typeof unbound-global semantics in codegen#36743robobun wants to merge 3 commits into
Conversation
…l doesn't become typeof (0, x)
|
Warning Review limit reached
Next review available in: 6 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 selected for processing (6)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — threads the parse-time UnaryFlags through HIR so typeof unboundGlobal keeps its no-throw semantics after react-compiler codegen.
What was reviewed:
- Verified
lower_unaryis the onlyInstructionValue::UnaryExpressionconstruction site; every other match arm already uses..so the new field compiles without further edits. - Checked the other
E::Unaryemitters in codegen.rs (delete/update/-Infinity) — none carryWAS_ORIGINALLY_TYPEOF_IDENTIFIER, so leaving them atempty()is correct; the sibling delete-flag case is #36741 as the description notes. - Tests exercise both the fix (runtime
["undefined",1]+ no(0, x)in output) and the control (typeof (0, x)still re-wraps and throws), and guard against a compiler bailout via thereact.memo_cache_sentinelassertion.
Extended reasoning...
Overview
Fixes typeof unboundGlobal inside react-compiled functions printing as typeof (0, unboundGlobal), which evaluates the reference and throws ReferenceError instead of returning "undefined". The fix threads E::UnaryFlags (specifically WAS_ORIGINALLY_TYPEOF_IDENTIFIER) from the parser AST through the HIR InstructionValue::UnaryExpression variant and back out at codegen. Touches: src/ast/e.rs (adds Debug to the bitflags so the HIR #[derive(Debug)] keeps compiling), src/react_compiler/hir/mod.rs (new flags field with a doc comment explaining the Bun-vs-Babel divergence), lowering/build_hir/expr.rs (captures unary.flags), optimization/constant_propagation.rs (adds .. to the destructure), codegen.rs (emits the captured flags instead of empty()), plus two new itBundled tests.
Security risks
None. This is a bundler/transpiler codegen correctness fix — no auth, network, filesystem, or untrusted-input parsing surface is touched. The flag is a 2-bit bitset copied by value.
Level of scrutiny
Medium-low. The change is mechanical (add a field, thread it through one construction site, emit it at one consumption site). I grepped every InstructionValue::UnaryExpression occurrence to confirm lower_unary is the sole constructor and that all other pattern-match sites already use .., so no exhaustiveness break. The other E::Unary emitters in codegen.rs are for delete, ++/--, and -Infinity, none of which carry the typeof flag; the description correctly scopes the delete-flag sibling to #36741.
Other factors
The tests are strong: both include a run: { stdout: ... } check proving runtime semantics, plus output-shape assertions that (a) confirm the hook was actually compiled (not bailed out) via react.memo_cache_sentinel, and (b) assert the specific typeof shape. The control test (TypeofSequenceUnboundGlobalStillThrows) proves the fix doesn't over-apply — typeof (0, x) in source still re-wraps because its parse-time flags are empty. The PR description reports the full react-compiler.test.ts and 3293-fixture suite pass. The HIR field's doc comment clearly records why this deviates from upstream.
|
Self-review pass came back clean (32 probes, 0 surviving). GitHub fast checks are green (clippy, miri, format, lints, comment-cop) and the evidence gate confirms the test fails on main / passes here under both ASAN and release. Buildkite #87513 is complete: 184 jobs passed, Ready for review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, targeted fix that threads parse-time UnaryFlags through the react-compiler HIR so typeof <unbound> doesn't get re-wrapped as typeof (0, x).
What was reviewed:
- Confirmed
lower_unaryis the onlyInstructionValue::UnaryExpressionconstructor; every other match site already uses..or was updated here. - Checked the remaining
E::UnaryFlags::empty()sites in codegen.rs — they'redelete(sibling #36741),++/--, and-Infinity, none of which the printer's typeof-rewrap touches. - Both tests execute the bundle (
run:) and assert the hook was compiled (not bailed) via the memo-cache sentinel; the control test pins thetypeof (0, x)throwing form.
Extended reasoning...
Overview
Threads E::UnaryFlags (specifically WAS_ORIGINALLY_TYPEOF_IDENTIFIER) through the react-compiler HIR so codegen can hand it back to the printer. Five source files touched: adds a flags field to InstructionValue::UnaryExpression, captures unary.flags at the one construction site in lower_unary, emits it in codegen_base_instruction_value, adds .. to the constant-propagation destructure, and derives Debug on UnaryFlags so the HIR's #[derive(Debug)] compiles. Two new itBundled tests.
Security risks
None. Pure transpiler-output correctness; no user input parsing, no I/O, no allocation changes. UnaryFlags is a Copy u8 bitflags — no ownership/lifetime concerns.
Level of scrutiny
Low-moderate. The fix is mechanical plumbing of an existing flag from parse → lower → codegen with no semantic transformation in between. I grepped every InstructionValue::UnaryExpression reference across src/react_compiler/ (12 files): the sole constructor is in lower_unary, and every other match site uses a .. rest pattern (or was updated in this PR). The other E::Unary { flags: empty() } sites in codegen.rs are UnDelete (explicitly deferred to sibling #36741), UnPreInc/UnPostInc/etc., and UnNeg for -Infinity — none reach the printer's typeof-identifier rewrap.
Other factors
- Tests are strong: both bundle and run the output (so a runtime
ReferenceErrorfails the test), assert the react-compiler actually compiled the hook (viareact.memo_cache_sentinel) rather than bailing, and include a control that pinstypeof (0, x)semantics so the fix can't over-apply. - The evidence block shows the new test fails on main (debug/ASAN) and passes with the fix;
react-compiler-fixtures.test.ts(3293 fixtures) also passes. - The comment-cop bot's concern about a long doc comment was addressed in eeac6ea (trimmed to one line) and the thread is resolved.
|
Updated 11:05 PM PT - Aug 1st, 2026
❌ @robobun, your commit eeac6ea has 3 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.b5159ecf-4bf9-4300-a304-acca63c64a04
🧪 To try this PR locally: bunx bun-pr 36743That installs a local version of the PR into your bun-36743 --bun |
|
@robobun conflicts |
|
Superseded by #36741, which was expanded to cover the typeof case as well and merged as 81a68d3 (same mechanism: thread |
2 participants
Problem
typeof unboundGlobalinside a react-compiled function prints astypeof (0, unboundGlobal), which throwsReferenceErrorinstead of returning"undefined".This breaks the feature-detection idiom (
typeof WebSocket !== "undefined"etc.) in any hook or component the compiler rewrites.Cause
src/react_compiler/codegen.rsatInstructionValue::UnaryExpressionemitsE::Unary { op: UnTypeof, flags: E::UnaryFlags::empty() }. The printer (src/js_printer/lib.rs:4003) re-wraps anytypeof <unbound EIdentifier>that lacksWAS_ORIGINALLY_TYPEOF_IDENTIFIERastypeof (0, ...), to preserve the throwing semantics of e.g.typeof (a ?? b)after folding.The parser sets that flag for
typeof <EIdentifier>(src/js_parser/parse/parse_prefix.rs:369-371), butlower_unary(src/react_compiler/lowering/build_hir/expr.rs:1016-1026) did not captureunary.flags, so the HIR had no way to distinguishtypeof xfromtypeof (0, x)at codegen time. Both collapse toUnaryExpression { TypeOf, <temp-for-LoadGlobal> }, because sequence lowering discards the side-effect-free0and inlines the global load.Upstream's HIR also drops the distinction, but
@babel/generatornever inserts a defensive(0, ...)wrap, so Babel emitstypeof xfor both forms.Fix
Thread the parse-time
E::UnaryFlagsthroughInstructionValue::UnaryExpressionand emit them back in codegen.typeof xthen keepsWAS_ORIGINALLY_TYPEOF_IDENTIFIERso the printer leaves it alone;typeof (0, x)has an empty flag set so the printer still re-wraps the inlined identifier.typeof (sideEffect(), x)is unaffected (the operand stays a sequence, which the printer never wraps).This is stricter than Babel's reference output (which loses the throw for
typeof (0, x)), but it matches the source semantics in every case. Sibling of #36741; that one can set the delete flag unconditionally becausePropertyDelete/ComputedDeleteare only lowered fromdelete <EDot|EIndex>, whereasUnaryExpression { TypeOf }is reached from both flagged and unflagged sources.Also adds
DebugtoE::UnaryFlagsso the HIR#[derive(Debug)]keeps compiling.Verification
Full
react-compiler.test.ts(34 pass) andreact-compiler-fixtures.test.ts(3293 pass, 320 skip) pass.[review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file