Repository navigation
Conversation
In a function inside a component, `const id = ref.current++` gave `id` the new value. The update lowers to a load, an add and a store, and its value is the temporary that holds the load. Codegen prints an unnamed temporary where it is read, and the store is a statement, so a read after the store printed `ref.current` again. PromoteInterposedTemporaries names that temporary for the component, but upstream does not run it on a nested function (react/react#35205). When the value of a postfix member update is used in a nested function, lower it to `(old = o.p, o.p = old + 1, old)`. `old` is a temporary that the builder declares with `let` at the top of the function, so the sequence can sit in a value block. The expression statement and the update of a `for` keep the previous lowering.
…t to the statement `(o.p = (old = o.p) + 1, old)` has no statement between the load and the store, so PromoteInterposedTemporaries finds nothing to promote inside the sequence when the compiler inlines the callback into the component (an IIFE or a `useMemo` callback). The `let` goes ahead of the current statement and only a value block defers it to the top of the function, so an inlined callback keeps its memo slot count.
…ated expressions Add an IIFE, a `useMemo` callback and a callback inside a `useMemo` callback to the forms. Add a seeded differential test: 48 generated expressions around member updates give the same values and the same order of side effects with and without the compiler.
…update The compiler inlines an IIFE and a `useMemo` callback into the component. There PromoteInterposedTemporaries names only some operands of an expression, so `pair(c.n++, c.n, c.n--)` gave `[0, 0, 1]`. A function whose lowering used the in-place form stays a function. The temporary goes back to the top of the function in every case. Run the forms with and without minify.syntax. The seeded test now also generates functions that capture nothing and functions that the compiler would inline.
A `try` at the start of a `useMemo` callback declares its catch binding first, also with a promoted temporary, so the callback of the upstream fixture repro-nested-try-catch-in-usememo stopped being inlined. Only a `let` declaration marks an in-place member update.
|
Status Reproduced on 1.4.3 and on main with the entry below and a stub import { useRef } from "react";
const Stub = () => null;
function Tabs() {
const newTabIndex = useRef(0);
const add = () => {
const id = newTabIndex.current++;
const key = `newTab${newTabIndex.current++}`;
return id + ":" + key + ":" + newTabIndex.current;
};
return <Stub run={add} />;
}
console.log(Tabs().p.run());bun build k15.jsx --outfile=plain.js && bun plain.js # 0:newTab1:2
bun build k15.jsx --react-compiler --outfile=rc.js && bun rc.js # 1:newTab2:2 before, 0:newTab1:2 with this PRTests: Self-reviewed in two rounds. Round one raised 4 blocking concerns, all addressed: a callback that the compiler inlines (an IIFE, a CI: every react-compiler test passes on all lanes (Linux, macOS, Windows, ASAN) in builds 115371 and 115380. Both builds are red for one test that this diff does not touch: |
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughThe compiler separates effect-only updates from value-producing updates. Nested-function postfix member updates use entry-declared temporaries. IIFE inlining detects this pattern, and tests cover update semantics across callbacks, loops, modules, and syntax minification. ChangesPostfix member update lowering
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No current merge-blocking risk was identified in the reviewed lowering and IIFE compatibility paths. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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. Because it's an intentional deviation from upstream React Compiler lowering with cross-pass coupling (the entry-block let shape doubles as a signal to inline_iifes) and a stated ordering dependency on #42451, a human look at the design tradeoffs would still be worthwhile.
What was reviewed:
lower_member_updateextraction — behavior of the non-nested / prefix / unused-value paths is unchanged from the original inlined code.push_entry_declarations— only runs whenentry_declarationsis non-empty, so top-level components and nested functions without a used postfix member update produce identical HIR.has_in_place_member_update— theLet+Promoted-name check on the entry block's first instruction matches only whatdeclare_temporary_at_entryprepends; I found no other lowering path that puts a promotedletat index 0 of a nested function's entry block.- Test matrix covers both
minifySyntaxstates, prefix/statement/for-update cheap paths, name collisions witht0/t1, IIFE/useMemoinlining, and evaluation-order againstnew/calls; the seeded differential test asserts every generated hook still compiles (no new bailouts).
Extended reasoning...
Overview
This PR fixes a React Compiler miscompilation (react/react#35205): a postfix member update o.p++ inside a nested function returned the post-update value because codegen re-printed o.p after the store. The fix threads a value_is_used flag into lower_update, and in a nested function lowers a used postfix member update to the sequence (o.p = (t0 = o.p) + 1, t0) with let t0 prepended to the function's entry block. Supporting machinery: lower_in_sequence_block (extracted from lower_sequence), lower_member_update (extracted so both paths share the load+add+store), lower_expression_for_effect (routes SExpr and for-update through the cheap path), and HirBuilder gains is_nested_function, entry_declarations, declare_temporary_at_entry, and push_entry_declarations. inline_iifes.rs gains has_in_place_member_update, which detects the prepended let and skips inlining that function so PromoteInterposedTemporaries in the outer body cannot reorder its operands. Tests add a 24-scenario itBundled matrix (× minifySyntax) plus a 48-case seeded differential test comparing compiled vs plain output.
Security risks
None. This is pure compile-time HIR lowering in the React Compiler pass; no untrusted input parsing, network, filesystem, auth, or crypto is touched. The added test fixtures use in-tree stub react/compiler-runtime modules — no external network.
Level of scrutiny
High. This is compiler correctness code that affects generated JS semantics for every user who enables --react-compiler, and it is an explicit deviation from upstream behavior (flagged with Intentional deviation: comments per what the PR says DESIGN.md requires). The fix couples two passes through a structural shape: HirBuilder::build guarantees the promoted let is the first instruction of the entry block, and inline_iifes reads exactly that position. That coupling is documented at both ends, but a maintainer should confirm no other path can produce a promoted let at entry-block index 0 (which would silently disable IIFE inlining for unrelated code) and that the memoization-granularity tradeoff — a useMemo/IIFE body containing such an update is now memoized as one call rather than statement-by-statement — is acceptable. The PR also states it should land after #42451 and that two known-remaining issues (#42628, #42629) reproduce on main; a human should confirm the merge order.
Other factors
Test coverage is thorough: the matrix asserts compiled: true for every case (guarding against silent bailouts), covers the cheap paths (Statement, ForUpdate, LogicalStatement), the ordering-sensitive shapes (EarlierOperands, EarlierSum, EarlierNewAndCall), the outlined-function path, and the IIFE/useMemo inlining interaction. The seeded differential test compares against a build without the compiler and asserts every hook took a memo cache. The Rust changes follow local conventions (arena HirVec/AstAlloc::vec, CompilerError propagation, no .unwrap() on the entry-block lookup). The refactored lower_sequence/lower_member_update are behavior-preserving on the pre-existing paths. No CODEOWNERS entry covers src/react_compiler/.
…ds for effect `[x, x += 1]` in a nested function gave `[1, 1]`: the store to `x` was a statement ahead of the load of the first operand. In a nested function the value of a compound assignment to a local is now the value of the store, as for `x = y`. An operand of a comma that is not the last one has no reader, so it takes the previous lowering of `o.p++`. `minify.syntax` joins `o.p++; o.q++;` into one comma expression. The tests check which callbacks are still inlined, the body of the component, and each generated hook on its own. The seeded test moves to the end of the file, after the tests that measure the memory of a child process.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 10:09 PM PT - Sep 13th, 2026
❌ @robobun, your commit 7e21328 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42630That installs a local version of the PR into your bun-42630 --bun |
robobun
changed the title
x.y++ in a nested functionx.y++ and x += 1 in place in a nested function
|
A note on the compound assignment hunk here ( The same forms are also wrong in the component body on main, and this PR leaves them so by design. For example #42809 makes that hunk unconditional, as a separate small change. It fixes those forms in the component body and in callbacks. The cost is the one you found: The two PRs conflict in that one hunk. If #42809 lands first, the gate on the compound assignment goes away in the rebase. The member update part of this PR is not affected. I also tried to close the component body gaps in the pass, by tracking an expression over a load. That makes forms wrong that main gets right, for example |
|
Follow-up to my note above, after a maintainer question on #42809 about the fixture count. The gate on the compound assignment hunk here ( function Foo() {
const getX = () => x;
let x = 4;
x += 5;
return <Stringify getX={getX} shouldInvokeFns={true} />;
}main already prints So the change needed here is small:
With the gate kept, two tests from #42809 fail: the recorded slot count in |
… store (#42809) ### Problem - With `bun build --react-compiler`, `o["k" + i] = i += 2` writes the key `k3`, not `k1`: the output is `i = i + 2; o["k" + i] = i;`. Any operand that reads `i` ahead of `i += 2` in one expression has this. 1.4.2 and main, both output modes. - As a statement, `x += 5` leaves a stray `x;` behind. In client mode it can land after the memo block that declares `x`: `ReferenceError: x is not defined` in a valid component. main and 1.4.3-canary only: 1.4.2 leaves such a component uncompiled. - `lower_compound_assignment_identifier` (`src/react_compiler/lowering/build_hir/expr.rs:904`) drops the result of the store and returns a new read of the local. Codegen prints a store that nothing reads as a statement, ahead of its expression. Upstream does the same. ### Fix - Return the result of the store, as `x = y` and the global arm already do. The store prints in place: `o["k" + i] = i = i + 2`. The extra read is gone. - Correct because the value of `x op= y` is the value that it stores. - The fixture `hoisting-invalid-tdz-let` goes from `_c(2)` to `_c(1)`. `_c(2)` is the count of the output with the stray `x;`. main already prints `_c(1)` for the same source with `x = x + 5` (Notes). The fixtures test records the new count. - Verified: `test/bundler/transpiler/react-compiler.test.ts` (2 new tests, 16 forms each, 14 fail on main in each mode). Also `react-compiler-fixtures.test.ts`, `bundler_jsx.test.ts`. ### Background - Codegen prints the result of an HIR instruction inline at its use. It prints an instruction whose result nothing reads as a statement. - In client mode, memo blocks (`if ($[0] === sentinel) {...}`) wrap groups of statements. `_c(N)` allocates their `N` cache slots. Adjacent blocks with the same dependencies merge. - #42630 has this change for callbacks only (behind `is_nested_function()`), to keep that fixture at `_c(2)`. It has to drop that gate when it rebases. <details><summary>Notes</summary> **The slot count of `hoisting-invalid-tdz-let`.** The fixture is `const getX = () => x; console.log(getX()); let x = 4; x += 5; return <Stringify getX={getX} shouldInvokeFns={true} />`. It calls `getX()` before `let x` on purpose, so it always throws (upstream's eval output: `Cannot access 'x' before initialization`). Upstream and main print this: ```js let $ = _c(2); let getX; if ($[0] === sentinel) { getX = () => x; console.log(getX()); let x = 4; x = x + 5; $[0] = getX; } else getX = $[0]; x; // the read that `x += 5` leaves behind. `x` is not in scope here. let t0; if ($[1] === sentinel) { t0 = <Stringify getX={getX} ... />; $[1] = t0; } else t0 = $[1]; ``` The stray `x;` never runs in the fixture, because `getX()` throws first. Remove the `console.log(getX())` line and the component is valid. Then, in client mode (`--target=browser`), main and 1.4.3-canary throw `ReferenceError: x is not defined` on every render, where the uncompiled component gives `getX() === 9`. This branch gives 9. 1.4.2 does not compile a component whose closure reads a `let` ahead of its declaration, so no release has this error yet. The new form `CapturedBeforeItsDeclaration` in the test is this case. The stray read is also the only thing that keeps the two memo blocks apart. Both have no dependencies, so without a statement between them they merge: one block, `_c(1)`. main already prints exactly that for the same fixture with `x = x + 5` in place of `x += 5`, and this branch prints the same text for both spellings. Memoization does not get coarser: with `<Stringify getX={getX} value={p.value} />` the second block depends on `p.value`, the blocks stay apart, and the count is `_c(3)` before and after. **Forms.** Each form is built without the compiler, in ssr mode (`--target=bun`) and in client mode (`--target=browser`), with `i = 1`. "main" is a debug build of 7e56b40 (it has #42386 and #42644). 1.4.2 prints the same as main for the first form. | form | expected | main | | --- | --- | --- | | `o["k" + i] = i += 2`, and with a template literal as the key | `{"k1":3}` | `{"k3":3}` | | ``list(`k${i}`, i++, "k" + i + "z", (i += 2), typeof i + i)`` | `["k1",1,"k2z",4,"number4"]` | `["k4",1,"k4z",4,"number4"]` | | `[[i, -i], { a: "k" + i }, (i += 2)]` | `[[1,-1],{"a":"k1"},3]` | ssr `[[3,-3],{"a":"k3"},3]`, client `[[3,-1],{"a":"k3"},3]` | | `<a title={"k" + i} id={(i += 2)} />` | `title: "k1"` | ssr `title: "k3"`, client right | | `out.push(list("k" + i, (i += x)))` in `for (const x of [1, 2])` | `[["k1",2],["k2",4]]` | `[["k2",2],["k4",4]]` | | `p.flag ? list("k" + i, (i += 2)) : null` | `["k1",3]` | `["k3",3]` | | in a callback: `o["k" + i] = i += 2; return list(o, "k" + i, (i *= 2))` | `[{"k1":3},"k3",6]` | `[{"k3":3},"k6",6]` | | `i` captured by a closure: `list("k" + i, (i += 2), read())` | `["k1",3,3]` | `["k3",3,3]` | | `const read = () => i; let i = p.n; i *= 3; return <div>{read}</div>`, then `read()` | `3` | ssr right, client `ReferenceError: i is not defined` | | `list(p.flag ? i : 0, (i += 2))`, `list(p.flag && i, (i += 2))` | `[1,3]` | `[3,3]` | | `list(p.items?.[i], (i += 2))` with `items = [5,6,7,8]` | `[6,3]` | `[8,3]` | | `list((0, "k" + i), (i += 2))` | `["k1",3]` | `["k3",3]` | | `list(p.flag ? i++ : 0, "k" + i, i, (j += 2))` | `[1,"k2",2,3]` | `[1,"k2",1,3]` | | `i += 2; for (let j = 0; j < 2; j += 1) i *= 2;` (control) | `12` | `12` | All of them match the uncompiled output on this branch, in both modes. **The last wrong row.** `list(p.flag ? i++ : 0, "k" + i, i, (j += 2))` has no operand that reads `j`. On main the statement `j = j + 2` makes `PromoteInterposedTemporaries` give the plain load of `i` a name, and the `const t0 = i` prints ahead of the ternary that updates `i`. With the store in place there is no statement, so nothing is promoted. **Why not the pass.** `PromoteInterposedTemporaries` (`src/react_compiler/reactive_scopes/promote_used_temporaries.rs`) is the upstream answer to a statement in the middle of an expression: it names the operands that come before the statement. It tracks only the results of loads, calls and stores, which is why `o[i] = i += 2` was right and `o["k" + i] = i += 2` was not. I first widened the pass to track an expression over a load. That fixed these forms, but it made forms wrong that main gets right, for example `list(p.flag ? i++ : 0, "k" + i, (j += 2))` gave `"k1"` and `list(p.bump?.(r), r.n + 1, (j += 2))` gave `[11,2,3]`. A name moves an operand ahead of an earlier operand that is a ternary, `&&`, `?.` or comma, because the pass never names those. Here the statement does not have to exist, so this PR removes it and leaves the pass alone. **Not fixed here.** A postfix update of a member prints its store as a statement, because the value of the expression is the old value. So `[r.n + 1, r.n++]` gives `[3,1]` in ssr mode, `list(o.n + 1, o.n += 2, o.n * 2, o.n++)` gives `[5,3,8,3]`, and `list(new A(), call(), r.n++)` runs `call()` first. These need either the form #42630 uses in callbacks or a pass that names every earlier operand. **Fixture text.** I compiled every upstream fixture that builds (1,585 in client mode, 1,622 in ssr mode) with main and with this branch and compared the files. Seven differ, the same seven in both modes: - `component`, `primitive-reassigned-loop-force-scopes-enabled`: `for (...; i = i + 1, i)` becomes `for (...; i = i + 1)`. - `hoisting-reassigned-let-declaration`, `hoisting-reassigned-twice-let-declaration`, `repro-scope-missing-mutable-range` and its copy under `propagate-scope-deps-hir-fork/`: a stray `x;` or `count;` statement after the assignment is gone. - `hoisting-invalid-tdz-let`: the source is `const getX = () => x; console.log(getX()); let x = 4; x += 5; return <Stringify getX={getX} />`. Upstream and main memoize `getX` in one block, then print the stray `x;` outside it, where `x` is not in scope, then memoize the JSX in a second block. Without the stray read one block holds both. </details>
…de their expression (#42859) ### Problem - With `bun build --react-compiler`, one expression can run its operands out of source order. The value is silently wrong: `list(-r.n, (() => { r.n = 10; return r.n; })())` gives `[-10,10]`, not `[-2,10]`. main and 1.4.3. - Codegen prints an instruction with an unnamed result inside the statement that uses it, and every other instruction as a statement at once (`codegen_instruction`, `src/react_compiler/codegen.rs:1630`). A statement in the middle of an expression then runs ahead of the earlier inline operands. - `PromoteInterposedTemporaries` (`src/react_compiler/reactive_scopes/promote_used_temporaries.rs`) names those operands. It knows only loads, calls and stores, and only statements with a side effect. It takes a store inside a comma operand for a statement. Upstream has the same gaps. ### Fix - Each temporary gets a position and the effect (read or write) of the whole expression it prints as. A use names it when a later statement conflicts: one writes, the other reads or writes. - A named temporary counts as a statement at its own position. Operands are checked last-defined first, so earlier operands get a name too. - A memo block that compares dependencies is a read. A statement inside a value block counts only inside it. A switch case test, and an operand that fbt wants inside its call, never get a name (Notes). - Verified: `test/bundler/transpiler/react-compiler.test.ts` (3 new tests fail on main), `react-compiler-fixtures.test.ts`, `bundler_jsx.test.ts`. ### Background - HIR is the flat instruction form. Each instruction writes a temporary. - A value block is a ternary, `&&`, `?.` or comma. It prints as one expression, so it cannot hold a declaration. - A memo block is `if ($[0] !== dep) {...} else {...}`. Only client mode has them. <details><summary>Notes</summary> **The three gaps, with the repro from the report** (`p = { n: 2, flag: true }`, `r = { n: p.n }`): | source | expected | main | | --- | --- | --- | | `<a title={++r.n && (i += 2)} id={[i, p.n]}>{p.flag ? r.n : (i += 2)}</a>` (client) | `id: [3,2]` | `id: [1,2]`: the memo block of the array reads `i` ahead of the inline `&&` | | ``<a title={[list(r.n), ++r.n]} id={i}>{new Box(`${p.flag ? r.n : r.n}:${m[i++ % 3]}`).v}</a>`` (client) | `children: "3:6"` | `"2:6"`: the template has a name (a memo block depends on it) and reads `r.n` ahead of the inline store | | `list(-r.n, new Box(++r.n).v, (--r.n, 2))` (both modes) | `[-2,3,2]` | `[-3,3,2]`: the store in the comma operand names `new Box(++r.n).v`, which moves ahead of `-r.n` | The third one now prints fully inline. The first two print `const t1 = (r.n = r.n + 1) && (i = 3);` and `const t1 = [list(r.n), r.n = r.n + 1];` ahead of the statement. **Why positions and not the upstream flag.** Upstream keeps one needs-promotion flag per tracked temporary and sets all of them at a statement with a side effect. A use by another temporary (`-r.n` uses the load of `r.n`) happens before the flag, so the load stays inline, and the unary was never tracked. With every kind tracked, the temporary that is still unused when the statement prints is the one that gets the name, together with the expression inside it. The position compare also drops the loop over all tracked temporaries at each statement, which was quadratic in the size of the function. **Effects.** Write: calls, `new`, tagged templates, `await`, stores, deletes, updates, every destructuring (an array pattern runs an iterator), iterator steps. Read: a load of a local that is reassigned, a property load that is not on a global, a spread, and an operator (`+`, unary minus, a template literal, `in`) over an operand whose inferred type is not primitive, because the conversion reads the object. A declaration writes a variable that nothing earlier can read. A temporary inherits the effects of the unnamed operands inside it. The model has no aliasing: any write conflicts with any read. That is what upstream does for the kinds it tracks. **Memo block entry.** It counts as a read only when a dependency has a property path or is a local that is reassigned. `$[0] !== lastname` on a constant cannot change, so a pending call is not named for it. **Value blocks.** A store with no result inside `(--r.n, 2)` prints in place inside that expression. It is a statement for the uses inside the same value block, where a name is a declaration that codegen rejects ("Cannot declare variables in a value block", the function stays uncompiled, as before). It is no longer a statement for the operands around the block. In 2,000 generated components with control flow, 94 that main leaves uncompiled for that reason now compile, for example `for (let k = 0; k < 2; k += (--r.n, 1))`. No generated component that compiles on main bails on this branch. **Not named on purpose.** - A switch case test. Lowering evaluates every case test ahead of the discriminant (upstream requires a "reorderable" expression there), and codegen prints it in place, behind the discriminant, which is the source order. A name would run `case bump(r):` ahead of the discriminant. The test has this form as a control. - `StartMemoize` and `FinishMemoize`. Codegen prints nothing for them, so they do not evaluate their operand. Treating `FinishMemoize` as a use named every `useMemo` result. - The `init` and `test` of a `for..of` print the collection once, so they count as one statement. - An operand that fbt wants inside its call. The fbt transform rejects a variable in place of a nested `fbt.param(...)` call (upstream fixture `fbt/repro-fbt-param-nested-fbt-jsx`). Upstream never names one there, because a JSX statement does not raise its flag. Here the first `fbt.param()` call is pending when the memo block of the second element reads `props.lastname`, and without an exemption the output is `const t2 = fbt.param("firstname", t1); ... fbt(["Name: ", t2, ...])`. `MemoizeFbtAndMacroOperandsInSameScope` now returns, next to `macroValues`, the operands it tags: the ones that have to print inside the macro call. Only the promotion check reads that set. A value given to `fbt.param()` is not in it and gets a name as before. When a tagged operand conflicts with a statement, the operands inside it get the names: `fbt(["a", fbt.param("n", r.n), fbt.param("m", (() => { r.n = r.n * 10; return r.n; })())], "d")` prints `const t1 = r.n; r.n = r.n * 10; fbt(["a", fbt.param("n", t1), ...])`. main hoists the whole `fbt.param("n", r.n)` call there. The pass tags nothing in ssr mode (it needs reactive scopes), so ssr output is as on main. **Differential runs.** A seeded generator of components whose expressions mix reads and writes of the same state (a `let`, a captured `let`, a member, an array): unary, binary, template, array, object, `new`, JSX, spread, `in`, ternary, `&&`, `||`, `??`, `?.`, comma, calls, prefix and postfix updates, assignments, destructuring assignment, `delete`, inside a declaration, JSX attributes, `if`, loops, `switch`, `try`, a labeled block, an early return, an inlined IIFE and a `useMemo` callback. Each component is built without the compiler and for both targets, and rendered four times with a fresh memo cache and four times with a kept one. The counts are results that differ from the build without the compiler. | cases | main | this branch | | --- | --- | --- | | 3,000, mixed | 73 | 1 | | 2,000, control flow | 21 | 0 | | 1,500, with `r.n++` | 44 | 0 | | 1,000, deeper expressions | 34 | 0 | The 1 is a `let` that is reassigned inside a memo block and is stale on a cache hit. main has it too, and #42482 is open for it. **Upstream fixtures.** I built every fixture that builds (1,585 in client mode, 1,622 in ssr mode), with and without `minify.syntax`, on main and on this branch, and compared the files. One differs, in ssr mode: `jsx-tag-evaluation-order-non-global` prints `jsxDEV(Tag, { children: [(Tag = props.alternateComponent, maybeMutate(maybeMutable)), jsxDEV(Tag, {})] })` where main prints `const T0 = Tag;` first. The store in the comma operand named the tag on main. The tag is the first argument, so it still reads the old `Tag`. No slot count changes. **Not fixed here.** - `{ ...r, a: (delete r.k) }` when the later operand has a name. HIR reads a spread when the object is created, after all operands. That needs a change in lowering. - Nested functions do not run this pass (#42630). - `++` on a string is lowered to `x + 1` (upstream desugaring). `IdMap::values_mut` had no other caller, and the crate denies dead code. </details>
|
A note for the rebase. #42859 is on main now, and this PR conflicts with it. The branch
I ran the tests of this PR against that branch. All 29 forms of What else the branch does:
I did not open a PR for the branch, because this PR and #42451 already own these two bugs. Take the branch if it helps, or ask me to open it. |
This branch has not been deployed
1 participant
Problem
--react-compiler, a postfix++or--on a member inside a callback yields the new value:const id = ref.current++givesold + 1.[x, x += 1]on a local gives[1, 1]. The component body is right. Upstream: facebook/react#35205.lower_update(src/react_compiler/lowering/build_hir/expr.rs) lowerso.p++to a load, an add and a store, and returns the temporary of the load. Codegen prints an unnamed temporary at each read, so a read after the store printso.pagain.PromoteInterposedTemporariesnames it in the component. Nested functions skip that pass.Fix
(o.p = (t0 = o.p) + 1, t0), withlet t0at the top of the function. It keeps its place among the other operands, even in a ternary or a loop test. A compound assignment to a local returns the value of its store, asx = ydoes.inline_iifesleaves a function with the new form a function (Notes).counts[i.current++] += 10prints the update twice.PostfixMemberUpdateInNestedFunction(29 forms, with and withoutminify.syntax) and a seeded differential test intest/bundler/transpiler/react-compiler.test.ts. Both fail on 1.4.3. Alsoreact-compiler-fixtures.test.ts.Background
const t0 = ....useMemocallback into the component.DESIGN.mdkeeps lowering 1:1 with upstream, so each new branch has anIntentional deviation:comment.Notes
Reach.
while (ref.current++ < 3)ran one turn short, anditems[i.current++]read the wrong slot. 20 of the 29 forms in the test are wrong on 1.4.3.Option taken. Not a bail:
const id = nextId.current++in a handler is common (the antd tabs demo has`newTab${newTabIndex.current++}`), and a bail leaves every such component without memoization. Not the eager promotion from the issue thread (const t0 = o.p; o.p = t0 + 1;): the two statements move ahead of the earlier operands of the same expression, so[ref.current, ref.current++]gives[1, 0].Why not the existing pass. I tried it first: phase 3 of
promote_used_temporariesincodegen_function_expression, with names for the promoted temporaries after RenameVariables. facebook/react#36737 does the same for outlined functions and says that other nested functions need a different fix. The pass tracks only the results of loads and calls.[r.current + 1, r.current++]gave[2, 0](plain[1, 0]).f(new A(), g(), r.current++)rang()beforenew A(), which 1.4.3 runs in order. The component body has these gaps today. That is upstream behavior and this change does not touch it. Remove the deviations when upstream fixes every nested function.IIFE and
useMemocallbacks. The compiler inlines them into the component, where that pass runs. With the new form inside,pair(c.n++, c.n, c.n--)gave[0, 0, 1]: the pass named the load ofc.nand moved it ahead of the first update.minify.syntaxproduces that shape fromconst first = seq.n++; return [first, seq.n]. So a function whose lowering used the form is not inlined.HirBuilder::buildputs theletof the temporary first in the entry block, andinline_iifeslooks for a promotedletthere (atryputs its catch binding there too, with another kind). Such a callback is then memoized as one call and not statement by statement. A callback with no such update is inlined as before, also underminify.syntax, which joinsc.n++; c.n++;into one comma expression. The test checks both.Compound assignment.
lower_compound_assignment_identifieremitted the store, dropped its temporary and returned a load ofx. In a nested function it now returns the temporary of the store, likelower_simple_assignment_identifier. The component keeps the upstream shape: with the new one everywhere, the fixturehoisting-invalid-tdz-letgoes from_c(2)to_c(1).#42451. A value that is lowered once and printed twice:
counts.current[i.current++] += 10becomescounts.current[(...)] = counts.current[(...)] + 10and advancesitwice. Before this change it advancedionce and used the wrong slot. An IIFE that stays a function and is the object of another update prints twice the same way. #42451 leaves such a function uncompiled. The two changes merge cleanly insrc/. Both append to the test file.Differential runs. A generator of random expressions around member updates (arrays, calls,
new, templates, ternaries,&&,??, optional calls, comma,try,switch,while), compared with the same file built without the compiler. The counts are cases whose output differs.useMemocallback in the component, 800 casesIn these runs no case that compiles on main bails on this branch. The 2 cases are #42628: a later
(x = x + 5)on a local prints as a statement ahead of an earlier one. The 1 case is #42629:ReferenceError: x is not defined, with a prefix update only. Both reproduce on main. The test generator leaves out assignments to locals for that reason.Upstream fixtures. I built all 1733 fixture inputs before and after, with and without
minify.syntax. Two outputs change and no slot count does.bug-ref-prefix-postfix-operator:repro-scope-missing-mutable-rangeloses a straycount;aftercount = count + ...in aforEachcallback.Not changed.
o.p = o.p + 1for++is the desugaring of upstream. It is wrong for a string ("5"++stores"51") and for a BigInt, in every position. A computed key with a side effect (o[key()]++) callskey()twice, which is #42451.for (;; a.b++, c.d++)does not compile before or after (a comma as the whole update of afor).[human-review] gate passed · iteration 1 · 7 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