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..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 @@ -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, @@ -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: @@ -487,6 +495,13 @@ type TreeNode = { children: Array; }; +type DataFlowInfo = { + rootSources: string; + trees: Array; + propsArr: Array; + stateArr: Array; +}; + function buildTreeNode( sourceId: IdentifierId, context: ValidationContext, @@ -512,6 +527,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, @@ -591,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 { @@ -755,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 @@ -820,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); } } } 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} +
+ ); +} 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} ```