[compiler][wip] Extend ValidateNoDerivedComputationsInEffects for props derived effects

poteto · jorge-cab · commit 7b38acca0b3e · 2025-09-09T14:10:44.000-07:00
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
diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts
@@ -13,14 +13,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.
@@ -48,12 +55,96 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
   const candidateDependencies: Map<IdentifierId, ArrayExpression> = new Map();
   const functions: Map<IdentifierId, FunctionExpression> = new Map();
   const locals: Map<IdentifierId, IdentifierId> = new Map();
+  const derivedFromProps: Map<IdentifierId, Place> = 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 <div>{displayValue}</div>;
+       *        }
+       */
+      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') {
@@ -97,6 +188,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
             validateEffect(
               effectFunction.loweredFunc.func,
               dependencies,
+              derivedFromProps,
               errors,
             );
           }
@@ -112,35 +204,49 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
 function validateEffect(
   effectFunction: HIRFunction,
   effectDeps: Array<IdentifierId>,
+  derivedFromProps: Map<IdentifierId, Place>,
   errors: CompilerError,
 ): void {
   for (const operand of effectFunction.context) {
     if (isSetStateType(operand.identifier)) {
       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;
     }
   }
 
   const seenBlocks: Set<BlockId> = new Set();
   const values: Map<IdentifierId, Array<IdentifierId>> = new Map();
+  const effectDerivedFromProps: Map<IdentifierId, Place> = 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<SourceLocation> = [];
+  const setStateCalls: Array<SetStateCall> = [];
   for (const block of effectFunction.body.blocks.values()) {
     for (const pred of block.preds) {
       if (!seenBlocks.has(pred)) {
@@ -150,17 +256,27 @@ function validateEffect(
     }
     for (const phi of block.phis) {
       const aggregateDeps: Set<IdentifierId> = new Set();
+      let propsSource: Place | null = null;
+
       for (const operand of phi.operands.values()) {
         const deps = values.get(operand.identifier.id);
         if (deps != null) {
           for (const dep of deps) {
             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) {
@@ -203,9 +319,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;
             }
           }
@@ -215,6 +338,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)) {
@@ -225,14 +368,29 @@ function validateEffect(
     seenBlocks.add(block.id);
   }
 
-  for (const loc of setStateLocations) {
-    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,
-      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
@@ -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 <div>{fullName}</div>;
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
@@ -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
@@ -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 <div>{fullName}</div>;
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
@@ -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
@@ -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 <div>{fullName}</div>;
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
@@ -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 (
