From 4dbb857beadfdfc9104561601ca30f7be4e743e6 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Tue, 23 Sep 2025 14:08:48 -0700 Subject: [PATCH] [compiler] Don't throw calculate in render if the blamed setter is used outside of the effect --- .../ValidateNoDerivedComputationsInEffects.ts | 90 ++++++++++++++++--- ...ter-call-outside-effect-no-error.expect.md | 88 ++++++++++++++++++ ...op-setter-call-outside-effect-no-error.js} | 0 ...ter-used-outside-effect-no-error.expect.md | 85 ++++++++++++++++++ ...op-setter-used-outside-effect-no-error.js} | 0 ...ter-call-outside-effect-no-error.expect.md | 49 ---------- ...ter-used-outside-effect-no-error.expect.md | 46 ---------- 7 files changed, 249 insertions(+), 109 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{error.derived-state-from-prop-setter-call-outside-effect-no-error.js => derived-state-from-prop-setter-call-outside-effect-no-error.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/{error.derived-state-from-prop-setter-used-outside-effect-no-error.js => derived-state-from-prop-setter-used-outside-effect-no-error.js} (100%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts index e2449f70a6..82fdecf370 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -19,8 +19,9 @@ import { Instruction, isUseStateType, isUseRefType, + GeneratedSource, + SourceLocation, } from '../HIR'; -import {printInstruction} from '../HIR/PrintHIR'; import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; import {assertExhaustive} from '../Utils/utils'; @@ -60,6 +61,10 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const functions: Map = new Map(); const derivationCache: Map = new Map(); + const setStateCache: Map> = new Map(); + + const effects: Array = []; + if (fn.fnType === 'Hook') { for (const param of fn.params) { if (param.kind === 'Identifier') { @@ -128,11 +133,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { ) { const effectFunction = functions.get(value.args[0].identifier.id); if (effectFunction != null) { - validateEffect( - effectFunction.loweredFunc.func, - errors, - derivationCache, - ); + effects.push(effectFunction.loweredFunc.func); } } else if (isUseStateType(lvalue.identifier)) { const stateValueSource = value.args[0]; @@ -144,6 +145,25 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } for (const operand of eachInstructionOperand(instr)) { + // Record setState usages everywhere + switch (instr.value.kind) { + case 'JsxExpression': + case 'CallExpression': + case 'MethodCall': + if ( + isSetStateType(operand.identifier) && + operand.loc !== GeneratedSource + ) { + if (setStateCache.has(operand.loc.identifierName)) { + setStateCache.get(operand.loc.identifierName)!.push(operand); + } else { + setStateCache.set(operand.loc.identifierName, [operand]); + } + } + break; + default: + } + const operandMetadata = derivationCache.get(operand.identifier.id); if (operandMetadata === undefined) { @@ -212,6 +232,10 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } } + for (const effect of effects) { + validateEffect(effect, errors, derivationCache, setStateCache); + } + if (errors.hasAnyErrors()) { throw errors; } @@ -269,11 +293,17 @@ function validateEffect( effectFunction: HIRFunction, errors: CompilerError, derivationCache: Map, + setStateCache: Map>, ): void { + const effectSetStateCache: Map< + string | undefined | null, + Array + > = new Map(); const seenBlocks: Set = new Set(); const effectDerivedSetStateCalls: Array<{ value: CallExpression; + loc: SourceLocation; sourceIds: Set; }> = []; @@ -292,6 +322,28 @@ function validateEffect( return; } + for (const operand of eachInstructionOperand(instr)) { + switch (instr.value.kind) { + case 'JsxExpression': + case 'CallExpression': + case 'MethodCall': + if ( + isSetStateType(operand.identifier) && + operand.loc !== GeneratedSource + ) { + if (effectSetStateCache.has(operand.loc.identifierName)) { + effectSetStateCache + .get(operand.loc.identifierName)! + .push(operand); + } else { + effectSetStateCache.set(operand.loc.identifierName, [operand]); + } + } + break; + default: + } + } + if ( instr.value.kind === 'CallExpression' && isSetStateType(instr.value.callee.identifier) && @@ -305,6 +357,7 @@ function validateEffect( if (argMetadata !== undefined) { effectDerivedSetStateCalls.push({ value: instr.value, + loc: instr.value.callee.loc, sourceIds: argMetadata.sourcesIds, }); } @@ -337,13 +390,22 @@ function validateEffect( } for (const derivedSetStateCall of effectDerivedSetStateCalls) { - errors.push({ - category: ErrorCategory.EffectDerivationsOfState, - reason: - 'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)', - description: null, - loc: derivedSetStateCall.value.loc, - suggestions: null, - }); + if ( + derivedSetStateCall.loc !== GeneratedSource && + effectSetStateCache.has(derivedSetStateCall.loc.identifierName) && + setStateCache.has(derivedSetStateCall.loc.identifierName) && + effectSetStateCache.get(derivedSetStateCall.loc.identifierName)! + .length === + setStateCache.get(derivedSetStateCall.loc.identifierName)!.length + ) { + errors.push({ + category: ErrorCategory.EffectDerivationsOfState, + reason: + 'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)', + description: null, + loc: derivedSetStateCall.value.loc, + suggestions: null, + }); + } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.expect.md new file mode 100644 index 0000000000..5d8aab0679 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.expect.md @@ -0,0 +1,88 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, [initialName]); + + return ( +
+ // 🟡 If the is also called outside of the effect, it's still wrong but + // should be solved by hoisting state + setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects +import { useEffect, useState } from "react"; + +function Component(t0) { + const $ = _c(6); + const { initialName } = t0; + const [name, setName] = useState(""); + let t1; + let t2; + if ($[0] !== initialName) { + t1 = () => { + setName(initialName); + }; + t2 = [initialName]; + $[0] = initialName; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t3 = (e) => setName(e.target.value); + $[3] = t3; + } else { + t3 = $[3]; + } + let t4; + if ($[4] !== name) { + t4 = ( +
+ // 🟡 If the is also called outside of the effect, it's still wrong but + // should be solved by hoisting state + +
+ ); + $[4] = name; + $[5] = t4; + } else { + t4 = $[5]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ initialName: "John" }], +}; + +``` + +### Eval output +(kind: ok)
// 🟡 If the is also called outside of the effect, it's still wrong but // should be solved by hoisting state
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-call-outside-effect-no-error.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.expect.md new file mode 100644 index 0000000000..57aee86a69 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.expect.md @@ -0,0 +1,85 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects +import {useEffect, useState} from 'react'; + +function MockComponent({onSet}) { + return
onSet('clicked')}>Mock Component
; +} + +function Component({propValue}) { + const [value, setValue] = useState(null); + useEffect(() => { + setValue(propValue); + }, [propValue]); + + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{propValue: 'test'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects +import { useEffect, useState } from "react"; + +function MockComponent(t0) { + const $ = _c(2); + const { onSet } = t0; + let t1; + if ($[0] !== onSet) { + t1 =
onSet("clicked")}>Mock Component
; + $[0] = onSet; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +function Component(t0) { + const $ = _c(4); + const { propValue } = t0; + const [, setValue] = useState(null); + let t1; + let t2; + if ($[0] !== propValue) { + t1 = () => { + setValue(propValue); + }; + t2 = [propValue]; + $[0] = propValue; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t3 = ; + $[3] = t3; + } else { + t3 = $[3]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ propValue: "test" }], +}; + +``` + +### Eval output +(kind: ok)
Mock Component
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/derived-state-from-prop-setter-used-outside-effect-no-error.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md deleted file mode 100644 index 5cc82027de..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-call-outside-effect-no-error.expect.md +++ /dev/null @@ -1,49 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects -import {useEffect, useState} from 'react'; - -function Component({initialName}) { - const [name, setName] = useState(''); - - useEffect(() => { - setName(initialName); - }, [initialName]); - - return ( -
- // 🟡 If the is also called outside of the effect, it's still wrong but - // should be solved by hoisting state - setName(e.target.value)} /> -
- ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{initialName: 'John'}], -}; - -``` - - -## Error - -``` -Found 1 error: - -Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) - -error.derived-state-from-prop-setter-call-outside-effect-no-error.ts:8:4 - 6 | - 7 | useEffect(() => { -> 8 | setName(initialName); - | ^^^^^^^^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) - 9 | }, [initialName]); - 10 | - 11 | return ( -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md deleted file mode 100644 index 34f2384f03..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-prop-setter-used-outside-effect-no-error.expect.md +++ /dev/null @@ -1,46 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects -import {useEffect, useState} from 'react'; - -function MockComponent({onSet}) { - return
onSet('clicked')}>Mock Component
; -} - -function Component({propValue}) { - const [value, setValue] = useState(null); - useEffect(() => { - setValue(propValue); - }, [propValue]); - - return ; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{propValue: 'test'}], -}; - -``` - - -## Error - -``` -Found 1 error: - -Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) - -error.derived-state-from-prop-setter-used-outside-effect-no-error.ts:11:4 - 9 | const [value, setValue] = useState(null); - 10 | useEffect(() => { -> 11 | setValue(propValue); - | ^^^^^^^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) - 12 | }, [propValue]); - 13 | - 14 | return ; -``` - - \ No newline at end of file