react_compiler: preserve E::UnaryFlags through codegen (delete/typeof Reference semantics) - #36741
Conversation
PropertyDelete and ComputedDelete emitted E::Unary with UnaryFlags::empty(). The printer re-wraps any 'delete <dot|index>' missing the WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS flag as 'delete (0, obj.prop)', which evaluates the property to a value and returns true without deleting. Lowering only creates these instructions from 'delete <EDot>' / 'delete <EIndex>' (both non-optional-chain), for which the parser always sets the flag, so codegen can set it unconditionally.
WalkthroughChangesThe React Compiler now preserves unary-expression flags through HIR and code generation. Delete and Unary flag preservation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ete lowering on parse-time flag The printer's (0, x) re-wrap for typeof/delete is keyed on the parse-time UnaryFlags. Lowering discarded them, so codegen rebuilt E::Unary with empty flags and the printer re-wrapped every typeof/delete. For typeof: add bun_flags to InstructionValue::UnaryExpression and pass unary.flags through lowering -> codegen unchanged. This preserves the visitor's distinction between 'typeof x' (flag set, no throw) and 'typeof (folded-to-x)' (flag unset, visitor wraps as (0, x), still throws). For delete: gate lower_unary's EDot/EIndex arms on the parse-time flag. A flagless EDot/EIndex operand (visitor folded 'delete (true ? a.b : c.d)') now falls through to the 'Only object properties can be deleted' bailout, matching upstream's behaviour on the unfolded ConditionalExpression. This makes the codegen-side flag restoration unconditional and correct.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/transpiler/react-compiler.test.ts`:
- Around line 560-565: Remove the explanatory regression comments surrounding
the affected tests and retain only the associated issue URL comment for each
regression test, preserving all test IDs and assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4539d754-6de9-43f1-969a-6449638317ea
📒 Files selected for processing (6)
src/ast/e.rssrc/react_compiler/codegen.rssrc/react_compiler/hir/mod.rssrc/react_compiler/lowering/build_hir/expr.rssrc/react_compiler/optimization/constant_propagation.rstest/bundler/transpiler/react-compiler.test.ts
There was a problem hiding this comment.
LGTM — both concerns from my earlier review are addressed (typeof now threads bun_flags through HIR rather than guessing from operand shape; lower_unary's EDot/EIndex arms are gated on the parse-time flag so the visitor-folded delete (cond ? a.b : c.d) case falls through to the bailout), and the comment-cop nits are trimmed.
What was reviewed:
- Confirmed
lower_unaryis the only constructor ofInstructionValue::UnaryExpression; every other match site uses..so the new field needs no further updates. - Checked constant_propagation's
TypeOfarm is a no-op (returnsNone), sobun_flagscan't go stale mid-pipeline; a stale flag would be harmless anyway since the printer's wrap also requiresis_unbound_identifier. - Verified the visitor's
UnTypeofid_before/id_afterguard is what makes threading (vs. operand-shape inference) correct for thetypeof (true ? x : y)case the new test covers.
Extended reasoning...
Overview
This PR fixes two react-compiler codegen bugs where E::UnaryFlags were dropped on the HIR round-trip, causing the printer's (0, ...) guard (js_printer/lib.rs:4003-4004) to re-wrap delete obj.prop and typeof undeclaredGlobal — turning the former into a no-op and the latter into a ReferenceError. Six files touched: a Debug derive on E::UnaryFlags (needed by HIR's derive), a new bun_flags field on InstructionValue::UnaryExpression, the lowering/codegen wiring for it, a flag gate on lower_unary's delete arms, a .. on constant_propagation's match, and three new itBundled tests.
Security risks
None. This is AST-flag plumbing inside the bundler's react-compiler pass; no untrusted-input parsing, no I/O, no auth/crypto surface.
Level of scrutiny
Moderate. It's a semantics-affecting transpiler fix (incorrect output could silently change runtime behavior of user code), but the mechanism is well understood, narrowly scoped, and the printer guard it interacts with is exactly two lines. I already reviewed an earlier revision and raised the two issues that shaped the current design; both are now resolved with the more-robust threading approach (vs. my original suggestion of inferring the flag from the codegen-time operand shape, which robobun correctly pointed out would regress the folded-conditional typeof case).
Other factors
- Whole-class coverage: both
UnaryFlagsbits are handled; the delete edge case (visitor-folded conditional operand) that the first revision regressed is now explicitly guarded and tested. - Constructor audit: grepped all
InstructionValue::UnaryExpressionsites — the sole constructor islower_unary; every consumer either destructures with..or is the codegen site this PR updates, so the field addition is complete. - Tests: each of the three new tests asserts both output shape (regex on the bundle) and runtime behavior (
run: { stdout }), and each includes a sanity assertion that the function was actually compiled (not bailed) so the codegen path is on trial. Test comment style matches the file's existing// Regression: ...convention (CodeRabbit withdrew its objection). - Prior feedback: all inline threads (my two, comment-cop's five, CodeRabbit's one) are marked resolved; the follow-up commits ba9983e/0c85de39 trimmed the comments to single lines as requested.
There was a problem hiding this comment.
LGTM — all three prior concerns are addressed at 24ba9a8 (typeof threads bun_flags through HIR; lower_unary now gates PropertyDelete/ComputedDelete on the parse-time flag so the codegen invariant holds; the accidental src/ revert in ddf056e is restored).
What was reviewed:
- Verified
lower_unaryis the onlyInstructionValue::UnaryExpressionconstruction site; every other match uses.., so the new field is fully wired. git diff f91d5c95..HEADat the current tip is the full 6-file change; the net src/ diff is no longer empty.- The three new tests cover dot/index delete,
typeof windowvs.typeof (folded ? …), and the folded-conditional-delete bailout.
Extended reasoning...
Overview
Threads E::UnaryFlags through the react-compiler HIR so delete obj.prop and typeof undeclaredGlobal round-trip with their parse-time flags intact, preventing the printer's (0, ...) guard from re-wrapping them into semantically-different forms. Touches src/ast/e.rs (adds Debug to the bitflags), hir/mod.rs (new bun_flags field), lowering/build_hir/expr.rs (captures the flag; gates delete lowering on it), codegen.rs (restores the flag; sets the delete flag at both delete sites), constant_propagation.rs (adds .. to its match), plus three itBundled tests.
Security risks
None. Purely a compiler correctness fix in the react-compiler pass; no untrusted input parsing, no auth/crypto/permissions.
Level of scrutiny
Medium-low. The change is mechanical (add a field, copy it in at lowering, copy it out at codegen). The one subtle piece — gating EDot/EIndex delete arms on the parse-time flag so a visitor-folded delete (true ? o.a : o.b) bails out instead of becoming a real delete — was raised in my earlier review and is now correctly implemented, with the codegen-side comment ("lower_unary only creates PropertyDelete when this flag was set") now accurate. The typeof fix threads the visited node's flags rather than guessing from operand shape, which correctly distinguishes typeof window (flag set → no wrap) from typeof (true ? undeclared : other) (visitor already wrapped, flag unset → stays throwing).
Other factors
This PR has been through two prior review rounds; both blocking findings and the accidental-revert catch are now resolved and the threads are marked resolved. The robobun evidence block shows the new tests fail on main (delete (0, / typeof (0, window)) and pass with the fix under both ASAN-debug and release. Full react-compiler.test.ts (35), react-compiler-fixtures.test.ts (3293), bundler_edgecase.test.ts (117), and transpiler.test.js (183) pass. I grep-verified that lower_unary is the sole construction site for InstructionValue::UnaryExpression; every other reference is a .. pattern match, so no other producer needs updating. No bugs were found by the bug hunting system this run.
|
CI status: builds #87401 and #87419 both failed with all 26 The diff at 24ba9a8 is ready: |
2 participants
Problem
delete obj.propandtypeof undeclaredGlobalinside a react-compiled function both lose their parse-timeE::UnaryFlagson the round-trip through HIR, so the printer's(0, ...)guard (src/js_printer/lib.rs:4003-4004) re-wraps them:Cause
src/react_compiler/codegen.rsrebuilt everyE::Unarywithflags: UnaryFlags::empty():InstructionValue::PropertyDelete/InstructionValue::ComputedDeletedroppedWAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS, so the printer wrapped everydelete <dot|index>asdelete (0, ...), which evaluates the property to a value and returnstruewithout deleting.InstructionValue::UnaryExpressiondroppedWAS_ORIGINALLY_TYPEOF_IDENTIFIER, so the printer wrappedtypeof undeclaredGlobalastypeof (0, undeclaredGlobal), which throwsReferenceErrorinstead of returning"undefined".lower_unary(src/react_compiler/lowering/build_hir/expr.rs) discardedunary.flagsat construction time, so codegen had nothing to restore.Fix
unary.flagsthrough HIR.InstructionValue::UnaryExpressiongains abun_flags: E::UnaryFlagsfield; lowering copies the visited node's flags in and codegen copies them back out. This preserves the visitor's own distinction betweentypeof x(flag set, no wrap) andtypeof (folded-to-x)(flag unset, visitor already wrapped as a real(0, x)comma expression, stays throwing).lower_unary'sEDot/EIndexarms onunary.flagsso a flagless operand (the visitor foldeddelete (true ? a.b : c.d)to a bareEDot) falls through to the existing "Only object properties can be deleted" bailout. That matches upstream's Babel plugin, which sees the unfoldedConditionalExpressionand bails the same way, and keeps the spec no-op semantics for that edge case.E::UnaryFlagsgainsDebug(the HIR enum derivesDebug); the otherbitflags!insrc/ast/already do.Babel's reference output for the upstream
delete-property/delete-computed-propertyfixtures is plaindelete x.b/delete x["b"], which this now matches.Verification
$ bun bd test test/bundler/transpiler/react-compiler.test.tsNew tests (
PropertyDeletePreservesReferenceSemantics,TypeofUnboundIdentifierPreservesFlag) fail on main (delete (0,/typeof (0, window)in the output; runtime{"a":1,"b":2,"c":3}/ReferenceError) and pass with this change.DeleteFoldedConditionalKeepsNoOpSemanticsguards the edge case that an earlier revision of this PR regressed.Full
react-compiler.test.ts(35 pass),react-compiler-fixtures.test.ts(3293 pass),bundler_edgecase.test.ts(117 pass),transpiler.test.js(183 pass).[review] gate passed · iteration 1 · 6 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