From b3f1831e23a9a5a7766d57f2eff2e5e03c8c1909 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Wed, 19 Nov 2025 12:58:20 -0800 Subject: [PATCH 1/3] [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 | 21 ++++ ...on-expression-mutation-edge-case.expect.md | 115 ++++++++++++++++++ .../function-expression-mutation-edge-case.js | 32 +++++ 3 files changed, 168 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..b1f0edfe1e9b9 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 @@ -422,6 +422,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 +520,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} +
+ ); +} From 338e6cd114eafbf831ea21028cb1b885be6099bb Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Wed, 19 Nov 2025 13:20:53 -0800 Subject: [PATCH 2/3] [compiler] Remove useState argument constraint. no-derived-computations-in-effects Summary: I missed this conditional messing things up for undefined useState() calls. We should be tracking them. I also missed a test that expect an error was not throwing. Test Plan: Update broken test --- .../Validation/ValidateNoDerivedComputationsInEffects_exp.ts | 2 +- ...estate-derived-from-prop-no-show-in-data-flow-tree.expect.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) 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 b1f0edfe1e9b9..6b022762a7e60 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 @@ -372,7 +372,7 @@ function recordInstructionDerivations( dependencies: deps, }); } - } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) { + } else if (isUseStateType(lvalue.identifier)) { typeOfValue = 'fromState'; context.derivationCache.addDerivationEntry( lvalue, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md index 602fb9fff3a6c..87cf7722da353 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md @@ -64,6 +64,7 @@ function Component(t0) { ## 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: [second]\n\nData Flow Tree:\n└── second (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":14,"column":4,"index":443},"end":{"line":14,"column":8,"index":447},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts","identifierName":"setS"},"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":18,"column":1,"index":500},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} ``` From 291a6c3850089303db83f4214fb3aff89791a4e1 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Wed, 19 Nov 2025 14:52:50 -0800 Subject: [PATCH 3/3] [compiler] Hoisting State Up draft Summary: Just a draft for now I'm testing a few things --- ...idateNoDerivedComputationsInEffects_exp.ts | 165 ++++++++++++++---- 1 file changed, 130 insertions(+), 35 deletions(-) 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 6b022762a7e60..d6c7668c41bb9 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 @@ -495,6 +495,13 @@ type TreeNode = { children: Array; }; +type DataFlowInfo = { + rootSources: string; + trees: Array; + propsArr: Array; + stateArr: Array; +}; + function buildTreeNode( sourceId: IdentifierId, context: ValidationContext, @@ -612,6 +619,51 @@ function renderTree( return result; } +/** + * Builds the data flow information including trees and source categorization + */ +function buildDataFlowInfo( + sourceIds: Set, + context: ValidationContext, +): DataFlowInfo { + const propsSet = new Set(); + const stateSet = new Set(); + + const rootNodesMap = new Map(); + for (const id of sourceIds) { + const nodes = buildTreeNode(id, context); + for (const node of nodes) { + if (!rootNodesMap.has(node.name)) { + rootNodesMap.set(node.name, node); + } + } + } + const rootNodes = Array.from(rootNodesMap.values()); + + const trees = rootNodes.map((node, index) => + renderTree(node, '', index === rootNodes.length - 1, propsSet, stateSet), + ); + + const propsArr = Array.from(propsSet); + const stateArr = Array.from(stateSet); + + let rootSources = ''; + if (propsArr.length > 0) { + rootSources += `Props: [${propsArr.join(', ')}]`; + } + if (stateArr.length > 0) { + if (rootSources) rootSources += '\n'; + rootSources += `State: [${stateArr.join(', ')}]`; + } + + return { + rootSources, + trees, + propsArr, + stateArr, + }; +} + function getFnLocalDeps( fn: FunctionExpression | undefined, ): Set | undefined { @@ -776,47 +828,16 @@ function validateEffect( effectSetStateUsages.get(rootSetStateCall)!.size === context.setStateUsages.get(rootSetStateCall)!.size - 1 ) { - const propsSet = new Set(); - const stateSet = new Set(); - - const rootNodesMap = new Map(); - for (const id of derivedSetStateCall.sourceIds) { - const nodes = buildTreeNode(id, context); - for (const node of nodes) { - if (!rootNodesMap.has(node.name)) { - rootNodesMap.set(node.name, node); - } - } - } - const rootNodes = Array.from(rootNodesMap.values()); - - const trees = rootNodes.map((node, index) => - renderTree( - node, - '', - index === rootNodes.length - 1, - propsSet, - stateSet, - ), - ); - for (const dep of derivedSetStateCall.sourceIds) { if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) { return; } } - const propsArr = Array.from(propsSet); - const stateArr = Array.from(stateSet); - - let rootSources = ''; - if (propsArr.length > 0) { - rootSources += `Props: [${propsArr.join(', ')}]`; - } - if (stateArr.length > 0) { - if (rootSources) rootSources += '\n'; - rootSources += `State: [${stateArr.join(', ')}]`; - } + const {rootSources, trees} = buildDataFlowInfo( + derivedSetStateCall.sourceIds, + context, + ); const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user @@ -841,6 +862,80 @@ See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-o message: 'This should be computed during render, not in an effect', }), ); + } else if ( + rootSetStateCall !== null && + effectSetStateUsages.has(rootSetStateCall) && + context.setStateUsages.has(rootSetStateCall) && + effectSetStateUsages.get(rootSetStateCall)!.size < + context.setStateUsages.get(rootSetStateCall)!.size + ) { + for (const dep of derivedSetStateCall.sourceIds) { + if (cleanUpFunctionDeps !== undefined && cleanUpFunctionDeps.has(dep)) { + return; + } + } + + const {rootSources, trees} = buildDataFlowInfo( + derivedSetStateCall.sourceIds, + context, + ); + + // Find setState calls outside the effect + const allSetStateUsages = context.setStateUsages.get(rootSetStateCall)!; + const effectUsages = effectSetStateUsages.get(rootSetStateCall)!; + const outsideEffectUsages: Array = []; + + for (const usage of allSetStateUsages) { + if (!effectUsages.has(usage)) { + outsideEffectUsages.push(usage); + } + } + + const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user + +This setState call is setting a derived value that depends on the following reactive sources: + +${rootSources} + +Data Flow Tree: +${trees.join('\n')} + +This state is also being set outside of the effect. Consider hoisting the state up to a parent component and making this a controlled component. + +See: https://react.dev/learn/sharing-state-between-components`; + + const diagnosticDetails: Array<{ + kind: 'error'; + loc: SourceLocation; + message: string; + }> = [ + { + kind: 'error', + loc: derivedSetStateCall.value.callee.loc, + message: 'setState in effect', + }, + ]; + + for (const usage of outsideEffectUsages) { + diagnosticDetails.push({ + kind: 'error', + loc: usage, + message: 'setState outside effect', + }); + } + + let diagnostic = CompilerDiagnostic.create({ + description: description, + category: ErrorCategory.EffectDerivationsOfState, + reason: + 'Consider hoisting state to parent and making this a controlled component', + }); + + for (const detail of diagnosticDetails) { + diagnostic = diagnostic.withDetails(detail); + } + + context.errors.pushDiagnostic(diagnostic); } } }