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 5aafa19a2f..33adcc8aeb 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 @@ -20,6 +20,8 @@ import { isUseStateType, BasicBlock, isUseRefType, + GeneratedSource, + SourceLocation, } from '../HIR'; import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; @@ -38,6 +40,8 @@ type ValidationContext = { readonly errors: CompilerError; readonly derivationCache: DerivationCache; readonly effects: Set; + readonly setStateCache: Map>; + readonly effectSetStateCache: Map>; }; class DerivationCache { @@ -148,11 +152,19 @@ export function validateNoDerivedComputationsInEffects_exp( const errors = new CompilerError(); const effects: Set = new Set(); + const setStateCache: Map> = new Map(); + const effectSetStateCache: Map< + string | undefined | null, + Array + > = new Map(); + const context: ValidationContext = { functions, errors, derivationCache, effects, + setStateCache, + effectSetStateCache, }; if (fn.fnType === 'Hook') { @@ -178,13 +190,16 @@ export function validateNoDerivedComputationsInEffects_exp( } } + let isFirstPass = true; do { for (const block of fn.body.blocks.values()) { recordPhiDerivations(block, context); for (const instr of block.instructions) { - recordInstructionDerivations(instr, context); + recordInstructionDerivations(instr, context, isFirstPass); } } + + isFirstPass = false; } while (context.derivationCache.snapshot()); for (const effect of effects) { @@ -239,6 +254,7 @@ function joinValue( function recordInstructionDerivations( instr: Instruction, context: ValidationContext, + isFirstPass: boolean, ): void { let typeOfValue: TypeOfValue = 'ignored'; const sources: Set = new Set(); @@ -247,7 +263,7 @@ function recordInstructionDerivations( context.functions.set(lvalue.identifier.id, value); for (const [, block] of value.loweredFunc.func.body.blocks) { for (const instr of block.instructions) { - recordInstructionDerivations(instr, context); + recordInstructionDerivations(instr, context, isFirstPass); } } } else if (value.kind === 'CallExpression' || value.kind === 'MethodCall') { @@ -273,6 +289,18 @@ function recordInstructionDerivations( } for (const operand of eachInstructionOperand(instr)) { + if ( + isSetStateType(operand.identifier) && + operand.loc !== GeneratedSource && + isFirstPass + ) { + if (context.setStateCache.has(operand.loc.identifierName)) { + context.setStateCache.get(operand.loc.identifierName)!.push(operand); + } else { + context.setStateCache.set(operand.loc.identifierName, [operand]); + } + } + const operandMetadata = context.derivationCache.cache.get( operand.identifier.id, ); @@ -347,6 +375,7 @@ function validateEffect( const effectDerivedSetStateCalls: Array<{ value: CallExpression; + loc: SourceLocation; sourceIds: Set; }> = []; @@ -365,6 +394,23 @@ function validateEffect( return; } + for (const operand of eachInstructionOperand(instr)) { + if ( + isSetStateType(operand.identifier) && + operand.loc !== GeneratedSource + ) { + if (context.effectSetStateCache.has(operand.loc.identifierName)) { + context.effectSetStateCache + .get(operand.loc.identifierName)! + .push(operand); + } else { + context.effectSetStateCache.set(operand.loc.identifierName, [ + operand, + ]); + } + } + } + if ( instr.value.kind === 'CallExpression' && isSetStateType(instr.value.callee.identifier) && @@ -378,6 +424,7 @@ function validateEffect( if (argMetadata !== undefined) { effectDerivedSetStateCalls.push({ value: instr.value, + loc: instr.value.callee.loc, sourceIds: argMetadata.sourcesIds, }); } @@ -410,13 +457,24 @@ function validateEffect( } for (const derivedSetStateCall of effectDerivedSetStateCalls) { - context.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.callee.loc, - suggestions: null, - }); + if ( + derivedSetStateCall.loc !== GeneratedSource && + context.effectSetStateCache.has(derivedSetStateCall.loc.identifierName) && + context.setStateCache.has(derivedSetStateCall.loc.identifierName) && + context.effectSetStateCache.get(derivedSetStateCall.loc.identifierName)! + .length === + context.setStateCache.get(derivedSetStateCall.loc.identifierName)! + .length - + 1 + ) { + context.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.callee.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..ef817a3ebf --- /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,84 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp +import {useEffect, useState} from 'react'; + +function Component({initialName}) { + const [name, setName] = useState(''); + + useEffect(() => { + setName(initialName); + }, [initialName]); + + return ( +
+ setName(e.target.value)} /> +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{initialName: 'John'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +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 = ( +
+ +
+ ); + $[4] = name; + $[5] = t4; + } else { + t4 = $[5]; + } + return t4; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ initialName: "John" }], +}; + +``` + +### Eval output +(kind: ok)
\ 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..2924de0da6 --- /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_exp +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_exp +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 1b204093b0..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,47 +0,0 @@ - -## Input - -```javascript -// @validateNoDerivedComputationsInEffects_exp -import {useEffect, useState} from 'react'; - -function Component({initialName}) { - const [name, setName] = useState(''); - - useEffect(() => { - setName(initialName); - }, [initialName]); - - return ( -
- 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 f3f45a24ef..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_exp -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