From 532192cb29006320b8bbd58bb65a907822ae6931 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Wed, 19 Nov 2025 12:58:20 -0800 Subject: [PATCH] [compiler] Prevent innaccurate derivation recording on FunctionExpressions on no-derived-computation-in-effects Summary: The operands of a function expression are the elements passed as context. This means that it doesn't make sense to record mutations for them. The relevant mutations will happen in the function body, so we need to prevent FunctionExpression type instruction from running the logic for effect mutations. This was also causing some values to depend on themselves in some cases triggering an infinite loop. Also added n invariant to prevent this issue Test Plan: Added fixture test --- ...idateNoDerivedComputationsInEffects_exp.ts | 37 ++++++ ...on-expression-mutation-edge-case.expect.md | 115 ++++++++++++++++++ .../function-expression-mutation-edge-case.js | 32 +++++ 3 files changed, 184 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts index af5927548af52..114af9603624b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -52,6 +52,8 @@ type ValidationContext = { readonly setStateUsages: Map>; }; +const MAX_FIXPOINT_ITERATIONS = 100; + class DerivationCache { hasChanges: boolean = false; cache: Map = new Map(); @@ -224,6 +226,7 @@ export function validateNoDerivedComputationsInEffects_exp( } let isFirstPass = true; + let iterationCount = 0; do { context.derivationCache.takeSnapshot(); @@ -236,6 +239,19 @@ export function validateNoDerivedComputationsInEffects_exp( context.derivationCache.checkForChanges(); isFirstPass = false; + iterationCount++; + CompilerError.invariant(iterationCount < MAX_FIXPOINT_ITERATIONS, { + reason: + '[ValidateNoDerivedComputationsInEffects] Fixpoint iteration failed to converge', + description: `Fixpoint iteration exceeded ${MAX_FIXPOINT_ITERATIONS} iterations while tracking derivations. This suggests a cyclic dependency in the derivation cache.`, + details: [ + { + kind: 'error', + loc: fn.loc, + message: `Exceeded ${MAX_FIXPOINT_ITERATIONS} iterations in ValidateNoDerivedComputationsInEffects`, + }, + ], + }); } while (context.derivationCache.snapshot()); for (const [, effect] of effectsCache) { @@ -422,6 +438,14 @@ function recordInstructionDerivations( ); } + if (value.kind === 'FunctionExpression') { + /* + * We don't want to record effect mutations of FunctionExpressions the mutations will happen in the + * function body and we will record them there. + */ + return; + } + for (const operand of eachInstructionOperand(instr)) { switch (operand.effect) { case Effect.Capture: @@ -512,6 +536,19 @@ function buildTreeNode( const namedSiblings: Set = new Set(); for (const childId of sourceMetadata.sourcesIds) { + CompilerError.invariant(childId !== sourceId, { + reason: + 'Unexpected self-reference: a value should not have itself as a source', + description: null, + details: [ + { + kind: 'error', + loc: sourceMetadata.place.loc, + message: null, + }, + ], + }); + const childNodes = buildTreeNode( childId, context, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.expect.md new file mode 100644 index 0000000000000..4e0ff4e2394d1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.expect.md @@ -0,0 +1,115 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component() { + const [foo, setFoo] = useState({}); + const [bar, setBar] = useState(new Set()); + + /* + * isChanged is considered context of the effect's function expression, + * if we don't bail out of effect mutation derivation tracking, isChanged + * will inherit the sources of the effect's function expression. + * + * This is innacurate and with the multiple passes ends up causing an infinite loop. + */ + useEffect(() => { + let isChanged = false; + + const newData = foo.map(val => { + bar.someMethod(val); + isChanged = true; + }); + + if (isChanged) { + setFoo(newData); + } + }, [foo, bar]); + + return ( +
+ {foo}, {bar} +
+ ); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component() { + const $ = _c(9); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = {}; + $[0] = t0; + } else { + t0 = $[0]; + } + const [foo, setFoo] = useState(t0); + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = new Set(); + $[1] = t1; + } else { + t1 = $[1]; + } + const [bar] = useState(t1); + let t2; + let t3; + if ($[2] !== bar || $[3] !== foo) { + t2 = () => { + let isChanged = false; + + const newData = foo.map((val) => { + bar.someMethod(val); + isChanged = true; + }); + + if (isChanged) { + setFoo(newData); + } + }; + + t3 = [foo, bar]; + $[2] = bar; + $[3] = foo; + $[4] = t2; + $[5] = t3; + } else { + t2 = $[4]; + t3 = $[5]; + } + useEffect(t2, t3); + let t4; + if ($[6] !== bar || $[7] !== foo) { + t4 = ( +
+ {foo}, {bar} +
+ ); + $[6] = bar; + $[7] = foo; + $[8] = t4; + } else { + t4 = $[8]; + } + return t4; +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [foo, bar]\n\nData Flow Tree:\n└── newData\n ├── foo (State)\n └── bar (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":23,"column":6,"index":663},"end":{"line":23,"column":12,"index":669},"filename":"function-expression-mutation-edge-case.ts","identifierName":"setFoo"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":32,"column":1,"index":762},"filename":"function-expression-mutation-edge-case.ts"},"fnName":"Component","memoSlots":9,"memoBlocks":4,"memoValues":5,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.js new file mode 100644 index 0000000000000..ab0bd70f363fc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/function-expression-mutation-edge-case.js @@ -0,0 +1,32 @@ +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component() { + const [foo, setFoo] = useState({}); + const [bar, setBar] = useState(new Set()); + + /* + * isChanged is considered context of the effect's function expression, + * if we don't bail out of effect mutation derivation tracking, isChanged + * will inherit the sources of the effect's function expression. + * + * This is innacurate and with the multiple passes ends up causing an infinite loop. + */ + useEffect(() => { + let isChanged = false; + + const newData = foo.map(val => { + bar.someMethod(val); + isChanged = true; + }); + + if (isChanged) { + setFoo(newData); + } + }, [foo, bar]); + + return ( +
+ {foo}, {bar} +
+ ); +}