Repository navigation
react-compiler: a compound assignment to a local has the value of its store - #42809
dylan-conway merged 3 commits into
Conversation
… store `i += 2` on a local was lowered to a store whose result is dropped, followed by a new read of `i`. Nothing read the result of the store, so codegen printed the store as a statement, ahead of the expression it is part of. An earlier operand of that expression that reads `i` then printed after it: `o["k" + i] = i += 2` became `i = i + 2; o["k" + i] = i;` and wrote the key "k3". Return the result of the store, as a simple assignment and a compound assignment to a global already do. The store prints in place. The upstream fixture hoisting-invalid-tdz-let goes from _c(2) to _c(1): the stray read of `x` after `x += 5` no longer splits the scope.
|
Status: fix pushed, waiting for CI. Reproduction, with a stub
// k.jsx
function App(p) {
const o = {};
let i = p.n;
o["k" + i] = i += 2;
return <div>{JSON.stringify(o)}</div>;
}
console.log(App({ n: 1 }).props.children);
function Foo() {
const getX = () => x;
let x = 4;
x += 5;
return <Stringify getX={getX} shouldInvokeFns={true} />;
}
console.log(Foo().props.getX());The uncompiled build prints With this branch the first prints |
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughChangesThe compiler now returns the stored value for compound assignments to context and local identifiers. Regression tests cover evaluation order across targets and expression contexts. Fixture validation supports a known memo-slot count divergence. Compound assignment lowering
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The compound-assignment regression is covered across the affected output targets, with no unresolved actionable issue identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@robobun so this pr changes the test from _c(2) to _c(1), then the other pr changes it back to _c(2). is _c(2) the correct result? we cannot merge this if it introduces a bug like this. combine the prs? |
…er its memo block
|
@dylan-conway No. What if ($[0] === sentinel) { getX = () => x; console.log(getX()); let x = 4; x = x + 5; $[0] = getX; } else getX = $[0];
x; // not in scope here
if ($[1] === sentinel) { t0 = <Stringify getX={getX} .../>; $[1] = t0; } else t0 = $[1];The fixture never reaches that line. It calls function Foo() {
const getX = () => x;
let x = 4;
x += 5;
return <Stringify getX={getX} shouldInvokeFns={true} />;
}
1.4.2 leaves this component uncompiled, so no release has this error yet. It is in 1.4.3-canary. Why it becomes The other PR. #42630 does not change the count back. It has the same hunk, but only for callbacks ( Combine? I would keep them apart and remove the overlap. This PR is the whole compound assignment change (about 10 lines of |
Upstream: - WebKit 65513e295c73: DFG LiveCatchVariablePreservationPhase flushes the right handler's live locals when two try ranges are adjacent (oven-sh/WebKit#629) - deps: mimalloc to the head of bun-dev3-v2 (oven-sh/mimalloc#47/49/50/51: recently used large pages are left alone for a while) - Bun.serve: send the completed responses of a pipelined read before JavaScript runs again (oven-sh#42796) - Remove dead code from the SQL and node:http native classes, 13 internal-field cells, Http2Response and the WebView ObjC wrappers (oven-sh#42816); .typos.toml and the bun-error images go with it - ast: keep a class with a static accessor initializer in place (oven-sh#40887) - react-compiler: a compound assignment to a local has the value of its store (oven-sh#42809) Conflicts: none. The 4 files both sides changed (webkit.ts, the two test files above plus serve.test.ts) auto-merged. Impact assessment (complete, not sampled): - all 48 files the merge touches checked for lost `ohos` references: none - every line the OHOS side added since 7e56b40 is still present in the 4 files both sides changed - docs/ohos-adaptation-checklist.md markers all verified present - no deleted file is referenced by the OHOS build or tests - upstream moved the WebKit and mimalloc pins; the OHOS build was rebuilt from both and smoked (see the build log)
Problem
bun build --react-compiler,o["k" + i] = i += 2writes the keyk3, notk1: the output isi = i + 2; o["k" + i] = i;. Any operand that readsiahead ofi += 2in one expression has this. 1.4.2 and main, both output modes.x += 5leaves a strayx;behind. In client mode it can land after the memo block that declaresx:ReferenceError: x is not definedin 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
x = yand the global arm already do. The store prints in place:o["k" + i] = i = i + 2. The extra read is gone.x op= yis the value that it stores.hoisting-invalid-tdz-letgoes from_c(2)to_c(1)._c(2)is the count of the output with the strayx;. main already prints_c(1)for the same source withx = x + 5(Notes). The fixtures test records the new count.test/bundler/transpiler/react-compiler.test.ts(2 new tests, 16 forms each, 14 fail on main in each mode). Alsoreact-compiler-fixtures.test.ts,bundler_jsx.test.ts.Background
if ($[0] === sentinel) {...}) wrap groups of statements._c(N)allocates theirNcache slots. Adjacent blocks with the same dependencies merge.x.y++andx += 1in place in a nested function #42630 has this change for callbacks only (behindis_nested_function()), to keep that fixture at_c(2). It has to drop that gate when it rebases.Notes
The slot count of
hoisting-invalid-tdz-let. The fixture isconst getX = () => x; console.log(getX()); let x = 4; x += 5; return <Stringify getX={getX} shouldInvokeFns={true} />. It callsgetX()beforelet xon purpose, so it always throws (upstream's eval output:Cannot access 'x' before initialization). Upstream and main print this:The stray
x;never runs in the fixture, becausegetX()throws first. Remove theconsole.log(getX())line and the component is valid. Then, in client mode (--target=browser), main and 1.4.3-canary throwReferenceError: x is not definedon every render, where the uncompiled component givesgetX() === 9. This branch gives 9. 1.4.2 does not compile a component whose closure reads aletahead of its declaration, so no release has this error yet. The new formCapturedBeforeItsDeclarationin 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 withx = x + 5in place ofx += 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 onp.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), withi = 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.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][[3,-3],{"a":"k3"},3], client[[3,-1],{"a":"k3"},3]<a title={"k" + i} id={(i += 2)} />title: "k1"title: "k3", client rightout.push(list("k" + i, (i += x)))infor (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]o["k" + i] = i += 2; return list(o, "k" + i, (i *= 2))[{"k1":3},"k3",6][{"k3":3},"k6",6]icaptured 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>, thenread()3ReferenceError: i is not definedlist(p.flag ? i : 0, (i += 2)),list(p.flag && i, (i += 2))[1,3][3,3]list(p.items?.[i], (i += 2))withitems = [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)1212All 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 readsj. On main the statementj = j + 2makesPromoteInterposedTemporariesgive the plain load ofia name, and theconst t0 = iprints ahead of the ternary that updatesi. 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 whyo[i] = i += 2was right ando["k" + i] = i += 2was 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 examplelist(p.flag ? i++ : 0, "k" + i, (j += 2))gave"k1"andlist(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], andlist(new A(), call(), r.n++)runscall()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)becomesfor (...; i = i + 1).hoisting-reassigned-let-declaration,hoisting-reassigned-twice-let-declaration,repro-scope-missing-mutable-rangeand its copy underpropagate-scope-deps-hir-fork/: a strayx;orcount;statement after the assignment is gone.hoisting-invalid-tdz-let: the source isconst getX = () => x; console.log(getX()); let x = 4; x += 5; return <Stringify getX={getX} />. Upstream and main memoizegetXin one block, then print the strayx;outside it, wherexis not in scope, then memoize the JSX in a second block. Without the stray read one block holds both.