From 2d90be783fd92e6f6f3cfabdaa8dcbba00cf2de1 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 8 Aug 2025 15:55:18 -0400 Subject: [PATCH] [compiler][wip] Extend ValidateNoDerivedComputationsInEffects for props derived effects This PR adds infra to disambiguate between two types of derived state in effects: 1. State derived from props 2. State derived from other state TODO: - [ ] Props tracking through destructuring and property access does not seem to be propagated correctly inside of Functions' instructions (or i might be misunderstanding how we track aliasing effects) - [ ] compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js should be failing - [ ] Handle "mixed" case where deps flow from at least one prop AND state. Should probably have a different error reason, to aid with categorization --- .../ValidateNoDerivedComputationsInEffects.ts | 184 ++++++++++++++++-- ...id-derived-computation-in-effect.expect.md | 6 +- ...ug-derived-state-from-mixed-deps.expect.md | 8 +- ...ed-state-from-props-destructured.expect.md | 6 +- ...d-derived-state-from-props-destructured.js | 4 +- ...rived-state-from-props-in-effect.expect.md | 6 +- ...rived-state-from-state-in-effect.expect.md | 6 +- 7 files changed, 194 insertions(+), 26 deletions(-) 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 d026a94ed4..d45e18c448 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -12,14 +12,21 @@ import { FunctionExpression, HIRFunction, IdentifierId, + Place, isSetStateType, isUseEffectHookType, } from '../HIR'; +import {printInstruction, printPlace} from '../HIR/PrintHIR'; import { eachInstructionValueOperand, eachTerminalOperand, } from '../HIR/visitors'; +type SetStateCall = { + loc: SourceLocation; + propsSource: Place | null; // null means state-derived, non-null means props-derived +}; + /** * Validates that useEffect is not used for derived computations which could/should * be performed in render. @@ -47,12 +54,96 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const candidateDependencies: Map = new Map(); const functions: Map = new Map(); const locals: Map = new Map(); + const derivedFromProps: Map = new Map(); const errors = new CompilerError(); + if (fn.fnType === 'Hook') { + for (const param of fn.params) { + if (param.kind === 'Identifier') { + derivedFromProps.set(param.identifier.id, param); + } + } + } else if (fn.fnType === 'Component') { + const props = fn.params[0]; + if (props != null && props.kind === 'Identifier') { + derivedFromProps.set(props.identifier.id, props); + } + } + for (const block of fn.body.blocks.values()) { for (const instr of block.instructions) { const {lvalue, value} = instr; + + // Track props derivation through instruction effects + if (instr.effects != null) { + for (const effect of instr.effects) { + switch (effect.kind) { + case 'Assign': + case 'Alias': + case 'MaybeAlias': + case 'Capture': { + const source = derivedFromProps.get(effect.from.identifier.id); + if (source != null) { + derivedFromProps.set(effect.into.identifier.id, source); + } + break; + } + } + } + } + + /** + * TODO: figure out why property access off of props does not create an Assign or Alias/Maybe + * Alias + * + * import {useEffect, useState} from 'react' + * + * function Component(props) { + * const [displayValue, setDisplayValue] = useState(''); + * + * useEffect(() => { + * const computed = props.prefix + props.value + props.suffix; + * ^^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^^ + * we want to track that these are from props + * setDisplayValue(computed); + * }, [props.prefix, props.value, props.suffix]); + * + * return
{displayValue}
; + * } + */ + if (value.kind === 'FunctionExpression') { + for (const [, block] of value.loweredFunc.func.body.blocks) { + for (const instr of block.instructions) { + if (instr.effects != null) { + console.group(printInstruction(instr)); + for (const effect of instr.effects) { + console.log(effect); + switch (effect.kind) { + case 'Assign': + case 'Alias': + case 'MaybeAlias': + case 'Capture': { + const source = derivedFromProps.get( + effect.from.identifier.id, + ); + if (source != null) { + derivedFromProps.set(effect.into.identifier.id, source); + } + break; + } + } + } + } + console.groupEnd(); + } + } + } + + for (const [, place] of derivedFromProps) { + console.log(printPlace(place)); + } + if (value.kind === 'LoadLocal') { locals.set(lvalue.identifier.id, value.place.identifier.id); } else if (value.kind === 'ArrayExpression') { @@ -89,6 +180,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { validateEffect( effectFunction.loweredFunc.func, dependencies, + derivedFromProps, errors, ); } @@ -104,6 +196,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { function validateEffect( effectFunction: HIRFunction, effectDeps: Array, + derivedFromProps: Map, errors: CompilerError, ): void { for (const operand of effectFunction.context) { @@ -111,16 +204,22 @@ function validateEffect( continue; } else if (effectDeps.find(dep => dep === operand.identifier.id) != null) { continue; + } else if (derivedFromProps.has(operand.identifier.id)) { + continue; } else { // Captured something other than the effect dep or setState + console.log('early return 1'); return; } } for (const dep of effectDeps) { + console.log({dep}); if ( effectFunction.context.find(operand => operand.identifier.id === dep) == - null + null || + derivedFromProps.has(dep) === false ) { + console.log('early return 2'); // effect dep wasn't actually used in the function return; } @@ -128,11 +227,18 @@ function validateEffect( const seenBlocks: Set = new Set(); const values: Map> = new Map(); + const effectDerivedFromProps: Map = new Map(); + for (const dep of effectDeps) { + console.log({dep}); values.set(dep, [dep]); + const propsSource = derivedFromProps.get(dep); + if (propsSource != null) { + effectDerivedFromProps.set(dep, propsSource); + } } - const setStateLocations: Array = []; + const setStateCalls: Array = []; for (const block of effectFunction.body.blocks.values()) { for (const pred of block.preds) { if (!seenBlocks.has(pred)) { @@ -142,6 +248,8 @@ function validateEffect( } for (const phi of block.phis) { const aggregateDeps: Set = new Set(); + let propsSource: Place | null = null; + for (const operand of phi.operands.values()) { const deps = values.get(operand.identifier.id); if (deps != null) { @@ -149,10 +257,18 @@ function validateEffect( aggregateDeps.add(dep); } } + const source = effectDerivedFromProps.get(operand.identifier.id); + if (source != null) { + propsSource = source; + } } + if (aggregateDeps.size !== 0) { values.set(phi.place.identifier.id, Array.from(aggregateDeps)); } + if (propsSource != null) { + effectDerivedFromProps.set(phi.place.identifier.id, propsSource); + } } for (const instr of block.instructions) { switch (instr.value.kind) { @@ -195,9 +311,16 @@ function validateEffect( ) { const deps = values.get(instr.value.args[0].identifier.id); if (deps != null && new Set(deps).size === effectDeps.length) { - setStateLocations.push(instr.value.callee.loc); + const propsSource = effectDerivedFromProps.get( + instr.value.args[0].identifier.id, + ); + + setStateCalls.push({ + loc: instr.value.callee.loc, + propsSource: propsSource ?? null, + }); } else { - // doesn't depend on any deps + // doesn't depend on all deps return; } } @@ -207,6 +330,26 @@ function validateEffect( return; } } + + // Track props derivation through instruction effects + if (instr.effects != null) { + for (const effect of instr.effects) { + switch (effect.kind) { + case 'Assign': + case 'Alias': + case 'MaybeAlias': + case 'Capture': { + const source = effectDerivedFromProps.get( + effect.from.identifier.id, + ); + if (source != null) { + effectDerivedFromProps.set(effect.into.identifier.id, source); + } + break; + } + } + } + } } for (const operand of eachTerminalOperand(block.terminal)) { if (values.has(operand.identifier.id)) { @@ -217,14 +360,29 @@ function validateEffect( seenBlocks.add(block.id); } - for (const loc of setStateLocations) { - errors.push({ - 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, - severity: ErrorSeverity.InvalidReact, - loc, - suggestions: null, - }); + for (const call of setStateCalls) { + if (call.propsSource != null) { + const propName = call.propsSource.identifier.name?.value; + const propInfo = propName != null ? ` (from prop '${propName}')` : ''; + + errors.push({ + reason: `Consider lifting state up to the parent component to make this a controlled component. (https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes)`, + description: `You are using props${propInfo} to update local state in an effect.`, + severity: ErrorSeverity.InvalidReact, + loc: call.loc, + suggestions: null, + }); + } else { + errors.push({ + reason: + 'You may not need this effect. Values derived from 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: + 'This effect updates state based on other state values. ' + + 'Consider calculating this value directly during render', + severity: ErrorSeverity.InvalidReact, + loc: call.loc, + suggestions: null, + }); + } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md index d97a665ae6..1d7e24b3ef 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-derived-computation-in-effect.expect.md @@ -24,13 +24,15 @@ function BadExample() { ``` 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: You may not need this effect. Values derived from 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) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-computation-in-effect.ts:9:4 7 | const [fullName, setFullName] = useState(''); 8 | useEffect(() => { > 9 | setFullName(capitalize(firstName + ' ' + lastName)); - | ^^^^^^^^^^^ 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) + | ^^^^^^^^^^^ You may not need this effect. Values derived from 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) 10 | }, [firstName, lastName]); 11 | 12 | return
{fullName}
; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md index 54c95d68e3..8124f4b3f3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md @@ -34,13 +34,15 @@ export const FIXTURE_ENTRYPOINT = { ``` 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: You may not need this effect. Values derived from 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-mixed-deps-no-error.ts:9:4 +This effect updates state based on other state values. Consider calculating this value directly during render. + +error.bug-derived-state-from-mixed-deps.ts:9:4 7 | 8 | useEffect(() => { > 9 | setDisplayName(prefix + name); - | ^^^^^^^^^^^^^^ 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) + | ^^^^^^^^^^^^^^ You may not need this effect. Values derived from 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) 10 | }, [prefix, name]); 11 | 12 | return ( diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md index cb18bd12a3..26b8b7930b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md @@ -28,13 +28,15 @@ export const FIXTURE_ENTRYPOINT = { ``` 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: You may not need this effect. Values derived from 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) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-state-from-props-destructured.ts:8:4 6 | 7 | useEffect(() => { > 8 | setFullName(firstName + ' ' + lastName); - | ^^^^^^^^^^^ 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) + | ^^^^^^^^^^^ You may not need this effect. Values derived from 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 | }, [firstName, lastName]); 10 | 11 | return
{fullName}
; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js index 130d31c11a..966f09ea89 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js @@ -1,7 +1,7 @@ // @validateNoDerivedComputationsInEffects import {useEffect, useState} from 'react'; -function Component({user: {firstName, lastName}}) { +function Component({firstName, lastName}) { const [fullName, setFullName] = useState(''); useEffect(() => { @@ -13,5 +13,5 @@ function Component({user: {firstName, lastName}}) { export const FIXTURE_ENTRYPOINT = { fn: Component, - params: [{user: {firstName: 'John', lastName: 'Doe'}}], + params: [{firstName: 'John', lastName: 'Doe'}], }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md index 15d94c39ad..1f7ff8dc5d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md @@ -28,13 +28,15 @@ export const FIXTURE_ENTRYPOINT = { ``` 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: You may not need this effect. Values derived from 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) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-state-from-props-in-effect.ts:8:4 6 | 7 | useEffect(() => { > 8 | setFullName(firstName + ' ' + lastName); - | ^^^^^^^^^^^ 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) + | ^^^^^^^^^^^ You may not need this effect. Values derived from 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 | }, [firstName, lastName]); 10 | 11 | return
{fullName}
; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md index 7466edb3c5..c5548c970b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md @@ -36,13 +36,15 @@ export const FIXTURE_ENTRYPOINT = { ``` 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: You may not need this effect. Values derived from 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) + +This effect updates state based on other state values. Consider calculating this value directly during render. error.invalid-derived-state-from-state-in-effect.ts:10:4 8 | 9 | useEffect(() => { > 10 | setFullName(firstName + ' ' + lastName); - | ^^^^^^^^^^^ 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) + | ^^^^^^^^^^^ You may not need this effect. Values derived from 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) 11 | }, [firstName, lastName]); 12 | 13 | return (