Repository navigation
Conversation
…al and then assigns it `[i, (i = i >> 1), i]` reads `i`, assigns it, and uses the first read after the assignment. Two things went wrong in the memo block that builds the array. The block depended on `i` by name. At the block `i` already holds the new value, so the block hit its cache when only the old value changed. With a property path the dependency (`o.y.z`) was read from the new `o` and could throw. The temporaries sidemap maps the result of `LoadLocal i` to `i`, because codegen normally prints such a temporary as `i` at its use. That is only valid when `i` is not reassigned between the load and the use. Such a temporary is now left out of the sidemap, like a temporary that is used outside the scope that declares it. The block then depends on the temporary, and an enclosing block that contains the load still depends on `i` by name. A switch case test and the property of a method call are not checked: both print where they are used. The assignment could print inside the block. When the value it stores is not reactive (`i = G.v`) it gets no name from a dependency, and codegen prints a temporary without a name at its use. A cache hit then skips the assignment, and a later read of `i` gets the old value. An assignment to a local that is pending when a memo block starts now gets a name, so it prints as a statement ahead of the block.
|
Warning Review limit reached
On-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Or wait 6 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Status How I reproduced it: Self-reviewed: 8 concerns raised, 8 addressed. The largest one: with only the dependency fix, a cache hit skipped an assignment that printed inside the memo block, so the second part of the fix is in this PR. A second round of adversarial tests then found 2 regressions (a Merge order: #42482 is best merged first. The PR body says why under "Right on main by luck". |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs, but since it adds ~150 lines of non-upstream dependency-tracking logic to a crate documented as a byte-for-byte port (plus a new Effect::Store variant that changes when assignments are hoisted ahead of memo blocks), a human look at the design choices would still be worthwhile.
What was reviewed:
LoadsOfReassignedLocalsinpropagate_scope_dependencies_hir.rs: theLet/Reassignstore recording, thevisit_loads_beforefallback for reassigned parameters, and theSwitchtest /MethodCallproperty exemptions — each traced against the corresponding test component.Effect::Storeinpromote_used_temporaries.rs: checked that it only tightens the existingWriteconflict rule (adds the memo-block entry position) and thatmemo_blockis restored on exit from nested scopes.- Ruled out as pre-existing rather than regressions: optional-chain reads (
o?.y) before a reassignment are not tracked, matching main's behavior; the 51 remaining fuzz cases cited in the description are attributed to a separate PR. - Test coverage: 24 component fixtures rendered multiple times against a one-cache-per-component react stub, with compiled vs. plain output compared and a "not compiled" line asserting every component was memoized.
Extended reasoning...
Overview
The PR touches two passes in Bun's Rust port of React Compiler (src/react_compiler/inference/propagate_scope_dependencies_hir.rs, src/react_compiler/reactive_scopes/promote_used_temporaries.rs) and adds a parameterized test to test/bundler/transpiler/react-compiler.test.ts. The first change adds a LoadsOfReassignedLocals tracker that keeps a temporary out of the temporaries sidemap when it is used after its source local was reassigned, so the memo block depends on the temporary rather than the (already-mutated) local by name. The second introduces an Effect::Store variant so an unnamed assignment pending at a memo block entry is promoted and printed ahead of the block instead of inside it, where a cache hit would skip it.
Security risks
None. This is compile-time transform logic operating on the compiler's own HIR; it does not parse untrusted bytes, allocate based on external sizes, or touch I/O. The indexing style (env.identifiers[...], func.instructions[...]) matches the crate's existing invariant-based convention.
Level of scrutiny
High. The crate's own design doc treats inference/ and reactive_scopes/ as minimal-diff upstream ports so they can be re-synced hunk by hunk; this change adds a sizable non-upstream block and alters the semantics of an existing pass. The logic has several deliberate carve-outs (switch case tests, method-call properties, the Let-kind reliance with a fallback scan for parameters) whose correctness depends on lowering/codegen details a maintainer familiar with the port should confirm. The bug hunt ran dry without findings and the ruled-out candidates (optional chains, conditional-branch loads, Store ordering in the promote pass, parameter rescan coverage) all landed on either pre-existing behavior or behavior consistent with main, but the design question of whether this belongs in these passes vs. lowering/codegen is one a human should weigh.
Other factors
The test is substantive: 24 fixtures, both plain and compiled variants, multiple renders against a persistent memo cache, and a trailing "not compiled:" line that would surface any component the compiler bailed on. The PR description claims all existing upstream fixtures produce byte-identical output; I did not independently re-run that corpus. No CODEOWNERS entry covers these paths, and there are no outstanding third-party review objections in the timeline.
|
On the design question in the review: why these two passes, and not lowering or codegen. The dependency. For the repro, main already prints
The assignment. In the HIR the assignment is outside the memo block. It ends up inside only because codegen prints a temporary without a name at its use. Cost for a sync with upstream. The upstream part of On the optional chains that the review lists as not tracked: 12 hand-written shapes ( The |
This branch has not been deployed
1 participant
Problem
--react-compiler --target=browser,let i = p.n; const list = [i, (i = i >> 1), i];returns the array of the previous render whenp.ngoes from 4 to 5. The memo block tests$[0] !== i || $[1] !== t1, the values after the assignment, and not the first read (const t0 = i). With a property path a valid component throwsTypeError: undefined is not an object (evaluating 'o.y.z').collect_temporaries_sidemap_impl,src/react_compiler/inference/propagate_scope_dependencies_hir.rs:260) maps the result ofLoadLocal itoi, also wheniis reassigned before the use. Upstream does the same.[i, (i = G.v)]). A cache hit skips it, and a later read ofiis wrong.Fix
find_temporaries_used_outside_declaring_scopenow also keeps out of the sidemap a temporary that is used after a reassignment of its local. The block then depends on the temporary. A block that contains the load depends on the local by name.PromoteInterposedTemporariesnames an assignment to a local that is pending when a memo block starts. It then prints ahead of the block.test/bundler/transpiler/react-compiler.test.ts(24 new components, 22 wrong on main),react-compiler-fixtures.test.ts,bundler_jsx.test.ts. All upstream fixture outputs are byte-identical to main.Background
if ($[0] !== dep) {...}) runs again only when a dependency changed. Only client mode has them.t = LoadLocal i). Codegen prints a temporary without a name at its use. The sidemap lets the dependency pass see through it.PromoteUsedTemporariesnames a temporary (const t0 = i). It then prints as a statement.Notes
Origin. A differential fuzz run found this. There is no user report. Only client mode (
--target=browser) is affected, because ssr mode has no memo blocks. #42859 made it more visible: it names the assignment ahead of the block, so the wrong dependency always compares equal.Repro. A react stub with one memo cache per component (the test has it), then:
Main prints this for the component.
t0holds the value the block needs, and the block does not test it:This branch prints
if ($[0] !== i || $[1] !== t0 || $[2] !== t1).What counts as a reassignment.
StoreLocalandDestructurewith kindReassign,PrefixUpdateandPostfixUpdate. The test is conservative: a reassignment with an instruction id between the load and the use, on any path. A false positive costs one named temporary, which holds the right value. A local that a closure assigns is a context variable and is not in SSA form.LoadContexthas its own guard (is_load_context_mutable), and those shapes are right on main.Two uses that are not checked.
switchcase test. Lowering puts it ahead of the discriminant, and it prints behind it, where it runs.switch ((i = p.m)) { case i: }has to read the newiby name.PromoteInterposedTemporarieshas the same exemption.xs.joininxs.join(((xs = []), "-"))). Codegen prints it through the receiver, and the receiver is checked. With a name of its own, codegen rejects the function (MethodCall::property must be an unpromoted + unmemoized MemberExpression) and it stays uncompiled.The test has a component for each (
SwitchCaseTest,MethodOfReassignedReceiver), and a line that lists every component that did not compile.How the walk stays cheap.
rewrite_instruction_kinds_based_on_reassignmentleaves kindLetonly on a declaration that a later instruction reassigns. The walk records such a declaration, and from then on it records the loads of that local (the load id and the local). Nothing declares a parameter. So when the walk meets the first reassignment of a local it has no record of, it collects the loads of the instructions before it once.ReassignedParameterin the test takes that path. Correctness does not depend on theLetkind: without it the same fallback runs. A function with no recorded load skips the check of the operands.Measured. Debug builds (ASAN),
bun build --react-compilerof one file, median of 5 runs, main then this branch. The time differences are below the run-to-run noise (about 10%).The 17 MB of the second row include the larger output: each statement has two more named temporaries and two more cache slots.
Why not at the use. Swapping the dependency for the temporary in
visit_operandis not enough with nested blocks. InNestedBlocksthe load is inside the block ofouter, and the use is inside the block of the inner array. The temporary is not a valid dependency of the outer block (it is declared inside it), and because the load is a sidemap entry nothing visits it at the load. The outer block would lose its dependency oni. With the temporary out of the sidemap, the load is visited as a normal instruction.The assignment inside the block. The value of
i = G.vis not reactive, so no block depends on it and nothing names it. On main the block of[i, (i = G.v)]depends oniby name, which makes the block entry a read forPromoteInterposedTemporaries, and that names the pending assignment. With the dependency ont0that no longer happens. Main has the same defect wherever the block does not read the local by name:make((i = G.v))(a block with no dependency,iisundefinedfrom the second render on),[i + 1, (i = G.v)](the block depends on the temporary ofi + 1),[(i = G.v), k]with aconst k, and[((i = p.m), 0)]. The new effectStoreconflicts with a memo block that starts after the temporary.StoreLocal,StoreContextandDestructurewith kindReassignhave it, and the two updates. A call that is pending at a block still prints inside it, as before.The 24 components of the test. Wrong on main: plain and compound assignment, JSX attribute,
Mapentries, call arguments, object literal with a computed key,i++,--i, destructuring assignment, a property path (throws on main), a reassigned parameter, two locals, nested blocks, an assignment in a?:branch and in an&&operand, and seven with an assignment that main prints inside the block. Right on main, as guards: theswitchcase test and the method call.Other checks.
infermode, and 1,556 outputs with@compilationMode:"all"added to each fixture (973 of them memoized).i,jandoinside arrays, objects, calls, JSX,?:,&&and sequences, with reactive and non-reactive values: 122 wrong on main, 51 here, none newly wrong. The 51 are right with react_compiler: fix stale values for aletreassigned inside a memo block #42482 applied on top (it applies cleanly).switch,try, closures, hooks, parameters, optional chains, every expression kind) found the two unchecked uses above. With those in place they give the groups below and nothing else.Right on main by luck. #42482 fixes a different defect: a block that reassigns one of its own dependencies stores the new value in the cache slot, so the next comparison is wrong. In
make(j, ident((i = i - 1)), (j = i % 2), j)bothiandjare such dependencies on main. The slot ofjnever compares equal, so the block always runs again and the result is right. With the dependency ont0the block can hit, and the slot ofithen matches whenp.ngoes from 8 to 7. 8 of about 10,800 generated components in those corpora are like this. All of them are right with #42482 applied on top, so #42482 is best merged first.NestedBlocksin the test renders the right values, but its outer block never hits its cache until #42482.Not fixed here. These have other causes:
key. Lowering puts it ahead of the other attributes, and the runtime call evaluates it last.<b key={i} a={(i = p.m)} />and<b key={i++} b={i} />are wrong on main. For a non-reactiveithe props of the element alone were right on main, because everything printed inline. The named assignment now prints ahead, as it already does for a reactivei. The key, and a later read ofi, were wrong on main in every variant.p + ((p = p.n >> 1) + [p].length).PromoteInterposedTemporariestakes every parameter for a constant.[GL.one, (i = GL.two)]runstwofirst. Main does the same for[GL.one, (i = p.m)]: a property load on a global has no effect in that pass.[human-review] gate passed · iteration 0 · 3 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