[compiler] Bail out from validation if there is a ref in the effect

compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts

@@ -520,7 +520,8 @@ function printErrorSummary(category: ErrorCategory, message: string): string {
     case ErrorCategory.AutomaticEffectDependencies:
     case ErrorCategory.CapitalizedCalls:
     case ErrorCategory.Config:
-    case ErrorCategory.EffectDerivationsOfState:
+    case ErrorCategory.EffectDerivationDeriveInRender:
+    case ErrorCategory.EffectDerivationShadowingParentState:
     case ErrorCategory.EffectSetState:
     case ErrorCategory.ErrorBoundaries:
     case ErrorCategory.Factories:
@@ -614,7 +615,14 @@ export enum ErrorCategory {
    * Checks for no setState in effect bodies
    */
   EffectSetState = 'EffectSetState',
-  EffectDerivationsOfState = 'EffectDerivationsOfState',
+  /**
+   * Checks for derived state in effects that could be calculated in render
+   */
+  EffectDerivationDeriveInRender = 'EffectDerivationDeriveInRender',
+  /**
+   * Checks for derived state in effects that could be hoisted to parent
+   */
+  EffectDerivationShadowingParentState = 'EffectDerivationShadowingParentState',
   /**
    * Validates against try/catch in place of error boundaries
    */
@@ -751,13 +759,23 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule {
         recommended: false,
       };
     }
-    case ErrorCategory.EffectDerivationsOfState: {
+    case ErrorCategory.EffectDerivationDeriveInRender: {
       return {
         category,
         severity: ErrorSeverity.Error,
-        name: 'no-deriving-state-in-effects',
+        name: 'effect-derive-in-render',
         description:
-          'Validates against deriving values from state in an effect',
+          'Validates if a useEffect is deriving state from props and/or local state that could be calculated in render.',
+        recommended: false,
+      };
+    }
+    case ErrorCategory.EffectDerivationShadowingParentState: {
+      return {
+        category,
+        severity: ErrorSeverity.Error,
+        name: 'effect-shadow-parent-state',
+        description:
+          'Validates if a useEffect is deriving state from parent state and if the component is updating the shadowed state locally.',
         recommended: false,
       };
     }

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts

@@ -5,14 +5,12 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {effect} from 'zod';
-import {CompilerError, Effect, ErrorSeverity, SourceLocation} from '..';
+import {CompilerDiagnostic, CompilerError, Effect, SourceLocation} from '..';
 import {ErrorCategory} from '../CompilerError';
 import {
   ArrayExpression,
   BasicBlock,
   BlockId,
-  Identifier,
   FunctionExpression,
   HIRFunction,
   IdentifierId,
@@ -21,208 +19,35 @@ import {
   isSetStateType,
   isUseEffectHookType,
   isUseStateType,
-  IdentifierName,
+  isUseRefType,
   GeneratedSource,
 } from '../HIR';
-import {printInstruction} from '../HIR/PrintHIR';
-import {
-  eachInstructionOperand,
-  eachTerminalOperand,
-  eachInstructionLValue,
-  eachPatternOperand,
-} from '../HIR/visitors';
+import {eachInstructionOperand, eachInstructionLValue} from '../HIR/visitors';
 import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
 import {assertExhaustive} from '../Utils/utils';
 
-// TODO: Maybe I can consolidate some types
 type SetStateCall = {
   loc: SourceLocation;
-  invalidDeps: DerivationMetadata;
+  derivedDep: DerivationMetadata;
   setStateId: IdentifierId;
 };
 
 type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState';
 
-type SetStateName = string | undefined | null;
-
 type DerivationMetadata = {
   typeOfValue: TypeOfValue;
-  // TODO: Rename to place
-  identifierPlace: Place;
-  sources: Place[];
+  place: Place;
+  sources: Array<Place>;
 };
 
-// TODO: This needs refining
 type ErrorMetadata = {
-  errorType: TypeOfValue;
-  invalidDepInfo: string | undefined;
+  type: TypeOfValue;
+  description: string | undefined;
   loc: SourceLocation;
-  setStateName: SetStateName;
+  setStateName: string | undefined | null;
+  derivedDepsNames: Array<string>;
 };
 
-function joinValue(
-  lvalueType: TypeOfValue,
-  valueType: TypeOfValue,
-): TypeOfValue {
-  if (lvalueType === 'ignored') return valueType;
-  if (valueType === 'ignored') return lvalueType;
-  if (lvalueType === valueType) return lvalueType;
-  return 'fromPropsOrState';
-}
-
-function updateDerivationMetadata(
-  target: Place,
-  sources: DerivationMetadata[],
-  typeOfValue: TypeOfValue,
-  derivedTuple: Map<IdentifierId, DerivationMetadata>,
-): void {
-  let newValue: DerivationMetadata = {
-    identifierPlace: target,
-    sources: [],
-    typeOfValue: typeOfValue,
-  };
-
-  for (const source of sources) {
-    // If the identifier of the source is a promoted identifier, then
-    // we should set the target as the source.
-    if (source.identifierPlace.identifier.name?.kind === 'promoted') {
-      newValue.sources.push(target);
-    } else {
-      newValue.sources.push(...source.sources);
-    }
-  }
-  derivedTuple.set(target.identifier.id, newValue);
-}
-
-function parseInstr(
-  instr: Instruction,
-  derivedTuple: Map<IdentifierId, DerivationMetadata>,
-  setStateCalls: Map<SetStateName, Place[]>,
-) {
-  // console.log(printInstruction(instr));
-  // console.log(instr);
-  let typeOfValue: TypeOfValue = 'ignored';
-
-  // TODO: Not sure if this will catch every time we create a new useState
-  if (
-    instr.value.kind === 'Destructure' &&
-    instr.value.lvalue.pattern.kind === 'ArrayPattern' &&
-    isUseStateType(instr.value.value.identifier)
-  ) {
-    const value = instr.value.lvalue.pattern.items[0];
-    if (value.kind === 'Identifier') {
-      derivedTuple.set(value.identifier.id, {
-        identifierPlace: value,
-        sources: [value],
-        typeOfValue: 'fromState',
-      });
-    }
-  }
-
-  if (
-    instr.value.kind === 'CallExpression' &&
-    isSetStateType(instr.value.callee.identifier) &&
-    instr.value.args.length === 1 &&
-    instr.value.args[0].kind === 'Identifier' &&
-    instr.value.callee.loc !== GeneratedSource
-  ) {
-    if (setStateCalls.has(instr.value.callee.loc.identifierName)) {
-      setStateCalls
-        .get(instr.value.callee.loc.identifierName)!
-        .push(instr.value.callee);
-    } else {
-      setStateCalls.set(instr.value.callee.loc.identifierName, [
-        instr.value.callee,
-      ]);
-    }
-  }
-
-  let sources: DerivationMetadata[] = [];
-  for (const operand of eachInstructionOperand(instr)) {
-    const opSource = derivedTuple.get(operand.identifier.id);
-    if (opSource === undefined) {
-      continue;
-    }
-
-    typeOfValue = joinValue(typeOfValue, opSource.typeOfValue);
-    sources.push(opSource);
-  }
-
-  if (typeOfValue !== 'ignored') {
-    for (const lvalue of eachInstructionLValue(instr)) {
-      updateDerivationMetadata(lvalue, sources, typeOfValue, derivedTuple);
-    }
-
-    for (const operand of eachInstructionOperand(instr)) {
-      switch (operand.effect) {
-        case Effect.Capture:
-        case Effect.Store:
-        case Effect.ConditionallyMutate:
-        case Effect.ConditionallyMutateIterator:
-        case Effect.Mutate: {
-          if (isMutable(instr, operand)) {
-            updateDerivationMetadata(
-              operand,
-              sources,
-              typeOfValue,
-              derivedTuple,
-            );
-          }
-          break;
-        }
-        case Effect.Freeze:
-        case Effect.Read: {
-          // no-op
-          break;
-        }
-        case Effect.Unknown: {
-          CompilerError.invariant(false, {
-            reason: 'Unexpected unknown effect',
-            description: null,
-            loc: operand.loc,
-            suggestions: null,
-          });
-        }
-        default: {
-          assertExhaustive(
-            operand.effect,
-            `Unexpected effect kind \`${operand.effect}\``,
-          );
-        }
-      }
-    }
-  }
-}
-
-function parseBlockPhi(
-  block: BasicBlock,
-  derivedTuple: Map<IdentifierId, DerivationMetadata>,
-) {
-  for (const phi of block.phis) {
-    for (const operand of phi.operands.values()) {
-      const source = derivedTuple.get(operand.identifier.id);
-      if (source !== undefined && source.typeOfValue === 'fromProps') {
-        if (
-          source.identifierPlace.identifier.name === null ||
-          source.identifierPlace.identifier.name?.kind === 'promoted'
-        ) {
-          derivedTuple.set(phi.place.identifier.id, {
-            identifierPlace: phi.place,
-            sources: [phi.place],
-            typeOfValue: 'fromProps',
-          });
-        } else {
-          derivedTuple.set(phi.place.identifier.id, {
-            identifierPlace: phi.place,
-            sources: source.sources,
-            typeOfValue: 'fromProps',
-          });
-        }
-      }
-    }
-  }
-}
-
 /**
  * Validates that useEffect is not used for derived computations which could/should
  * be performed in render.
@@ -250,18 +75,22 @@ 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 derivedTuple: Map<IdentifierId, DerivationMetadata> = new Map();
+  const derivationCache: Map<IdentifierId, DerivationMetadata> = new Map();
+  const shadowingUseState: Map<string, Array<SourceLocation>> = new Map();
 
-  const effectSetStates: Map<SetStateName, Place[]> = new Map();
-  const setStateCalls: Map<SetStateName, Place[]> = new Map();
+  const effectSetStates: Map<
+    string | undefined | null,
+    Array<Place>
+  > = new Map();
+  const setStateCalls: Map<string | undefined | null, Array<Place>> = new Map();
 
-  const errors: ErrorMetadata[] = [];
+  const errors: Array<ErrorMetadata> = [];
 
   if (fn.fnType === 'Hook') {
     for (const param of fn.params) {
       if (param.kind === 'Identifier') {
-        derivedTuple.set(param.identifier.id, {
-          identifierPlace: param,
+        derivationCache.set(param.identifier.id, {
+          place: param,
           sources: [param],
           typeOfValue: 'fromProps',
         });
@@ -270,8 +99,8 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
   } else if (fn.fnType === 'Component') {
     const props = fn.params[0];
     if (props != null && props.kind === 'Identifier') {
-      derivedTuple.set(props.identifier.id, {
-        identifierPlace: props,
+      derivationCache.set(props.identifier.id, {
+        place: props,
         sources: [props],
         typeOfValue: 'fromProps',
       });
@@ -279,24 +108,12 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
   }
 
   for (const block of fn.body.blocks.values()) {
-    parseBlockPhi(block, derivedTuple);
+    parseBlockPhi(block, derivationCache);
 
     for (const instr of block.instructions) {
       const {lvalue, value} = instr;
 
-      parseInstr(instr, derivedTuple, setStateCalls);
-
-      /*
-       * Special case for function expressions, we need to parse nested instructions
-       * TODO: Can there be more recursive levels?
-       */
-      if (value.kind === 'FunctionExpression') {
-        for (const [, block] of value.loweredFunc.func.body.blocks) {
-          for (const instr of block.instructions) {
-            parseInstr(instr, derivedTuple, setStateCalls);
-          }
-        }
-      }
+      parseInstr(instr, derivationCache, setStateCalls, shadowingUseState);
 
       if (value.kind === 'LoadLocal') {
         locals.set(lvalue.identifier.id, value.place.identifier.id);
@@ -342,7 +159,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
             validateEffect(
               effectFunction.loweredFunc.func,
               dependencies,
-              derivedTuple,
+              derivationCache,
               effectSetStates,
               errors,
             );
@@ -352,11 +169,28 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     }
   }
 
+  const compilerError = generateCompilerError(
+    setStateCalls,
+    effectSetStates,
+    shadowingUseState,
+    errors,
+  );
+
+  if (compilerError.hasErrors()) {
+    throw compilerError;
+  }
+}
+
+function generateCompilerError(
+  setStateCalls: Map<string | undefined | null, Array<Place>>,
+  effectSetStates: Map<string | undefined | null, Array<Place>>,
+  shadowingUseState: Map<string, Array<SourceLocation>>,
+  errors: Array<ErrorMetadata>,
+): CompilerError {
   const throwableErrors = new CompilerError();
   for (const error of errors) {
-    let reason;
-    let description = '';
-    // TODO: Not sure if this is robust enough.
+    let compilerDiagnostic: CompilerDiagnostic | undefined = undefined;
+
     /*
      * If we use a setState from an invalid useEffect elsewhere then we probably have to
      * hoist state up, else we should calculate in render
@@ -364,86 +198,303 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     if (
       setStateCalls.get(error.setStateName)?.length !=
         effectSetStates.get(error.setStateName)?.length &&
-      error.errorType !== 'fromState'
+      error.type !== 'fromState'
     ) {
-      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)';
+      compilerDiagnostic = CompilerDiagnostic.create({
+        description: `The setState within a useEffect is deriving from ${error.description}. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render`,
+        category: ErrorCategory.EffectDerivationShadowingParentState,
+        reason:
+          'You might not need an effect. Local state shadows parent state.',
+      }).withDetails({
+        kind: 'error',
+        loc: error.loc,
+        message: `this derives values from props ${error.type === 'fromPropsOrState' ? 'and local state ' : ''}to synchronize state`,
+      });
+
+      for (const derivedDep of error.derivedDepsNames) {
+        if (shadowingUseState.has(derivedDep)) {
+          for (const loc of shadowingUseState.get(derivedDep)!) {
+            compilerDiagnostic.withDetails({
+              kind: 'error',
+              loc: loc,
+              message: `this useState shadows ${derivedDep}`,
+            });
+          }
+        }
+      }
+
+      for (const [key, setStateCallArray] of effectSetStates) {
+        if (setStateCallArray.length === 0) {
+          continue;
+        }
+
+        const nonUseEffectSetStateCalls = setStateCalls.get(key);
+        if (nonUseEffectSetStateCalls) {
+          for (const place of nonUseEffectSetStateCalls) {
+            if (!setStateCallArray.includes(place)) {
+              compilerDiagnostic.withDetails({
+                kind: 'error',
+                loc: place.loc,
+                message:
+                  'this setState updates the shadowed state, but should call an onChange event from the parent',
+              });
+            }
+          }
+        }
+      }
     } else {
-      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)';
+      compilerDiagnostic = CompilerDiagnostic.create({
+        description: `${error.description ? error.description.charAt(0).toUpperCase() + error.description.slice(1) : ''}. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user`,
+        category: ErrorCategory.EffectDerivationDeriveInRender,
+        reason:
+          'You might not need an effect. Derive values in render, not effects.',
+      }).withDetails({
+        kind: 'error',
+        loc: error.loc,
+        message: 'This should be computed during render, not in an effect',
+      });
     }
 
-    throwableErrors.push({
-      reason: reason,
-      description: `You are using invalid dependencies: \n\n${error.invalidDepInfo}`,
-      severity: ErrorSeverity.InvalidReact,
-      loc: error.loc,
-    });
+    if (compilerDiagnostic) {
+      throwableErrors.pushDiagnostic(compilerDiagnostic);
+    }
   }
 
-  if (throwableErrors.hasAnyErrors()) {
-    throw throwableErrors;
+  return throwableErrors;
+}
+
+function joinValue(
+  lvalueType: TypeOfValue,
+  valueType: TypeOfValue,
+): TypeOfValue {
+  if (lvalueType === 'ignored') return valueType;
+  if (valueType === 'ignored') return lvalueType;
+  if (lvalueType === valueType) return lvalueType;
+  return 'fromPropsOrState';
+}
+
+function updateDerivationMetadata(
+  target: Place,
+  sources: Array<DerivationMetadata> | undefined,
+  typeOfValue: TypeOfValue | undefined,
+  derivationCache: Map<IdentifierId, DerivationMetadata>,
+): void {
+  let newValue: DerivationMetadata = {
+    place: target,
+    sources: [],
+    typeOfValue: typeOfValue ?? 'ignored',
+  };
+
+  if (sources !== undefined) {
+    for (const source of sources) {
+      /*
+       * If the identifier of the source is a promoted identifier, then
+       *  we should set the target as the source.
+       */
+      for (const place of source.sources) {
+        if (
+          place.identifier.name === null ||
+          place.identifier.name?.kind === 'promoted'
+        ) {
+          newValue.sources.push(target);
+        } else {
+          newValue.sources.push(place);
+        }
+      }
+    }
+  }
+
+  derivationCache.set(target.identifier.id, newValue);
+}
+
+function parseInstr(
+  instr: Instruction,
+  derivationCache: Map<IdentifierId, DerivationMetadata>,
+  setStateCalls: Map<string | undefined | null, Array<Place>>,
+  shadowingUseState: Map<string, Array<SourceLocation>>,
+): void {
+  // Recursively parse function expressions
+  let typeOfValue: TypeOfValue = 'ignored';
+
+  let sources: Array<DerivationMetadata> = [];
+  if (instr.value.kind === 'FunctionExpression') {
+    for (const [, block] of instr.value.loweredFunc.func.body.blocks) {
+      for (const instr of block.instructions) {
+        parseInstr(instr, derivationCache, setStateCalls, shadowingUseState);
+      }
+    }
+  } else if (
+    instr.value.kind === 'CallExpression' &&
+    isSetStateType(instr.value.callee.identifier) &&
+    instr.value.args.length === 1 &&
+    instr.value.args[0].kind === 'Identifier' &&
+    instr.value.callee.loc !== GeneratedSource
+  ) {
+    if (setStateCalls.has(instr.value.callee.loc.identifierName)) {
+      setStateCalls
+        .get(instr.value.callee.loc.identifierName)!
+        .push(instr.value.callee);
+    } else {
+      setStateCalls.set(instr.value.callee.loc.identifierName, [
+        instr.value.callee,
+      ]);
+    }
+  } else if (
+    (instr.value.kind === 'CallExpression' ||
+      instr.value.kind === 'MethodCall') &&
+    isUseStateType(instr.lvalue.identifier) &&
+    instr.value.args.length > 0
+  ) {
+    const stateValueSource = instr.value.args[0];
+    if (stateValueSource.kind === 'Identifier') {
+      sources.push({
+        place: stateValueSource,
+        typeOfValue: typeOfValue,
+        sources: [stateValueSource],
+      });
+    }
+
+    typeOfValue = joinValue(typeOfValue, 'fromState');
+  }
+
+  for (const operand of eachInstructionOperand(instr)) {
+    const opSource = derivationCache.get(operand.identifier.id);
+    if (opSource === undefined) {
+      continue;
+    }
+
+    typeOfValue = joinValue(typeOfValue, opSource.typeOfValue);
+    sources.push(opSource);
+
+    if (
+      (instr.value.kind === 'CallExpression' ||
+        instr.value.kind === 'MethodCall') &&
+      opSource.typeOfValue === 'fromProps' &&
+      isUseStateType(instr.lvalue.identifier)
+    ) {
+      opSource.sources.forEach(source => {
+        if (source.identifier.name !== null) {
+          if (shadowingUseState.has(source.identifier.name.value)) {
+            shadowingUseState
+              .get(source.identifier.name.value)
+              ?.push(instr.lvalue.loc);
+          } else {
+            shadowingUseState.set(source.identifier.name.value, [
+              instr.lvalue.loc,
+            ]);
+          }
+        }
+      });
+    }
+  }
+
+  if (typeOfValue !== 'ignored') {
+    for (const lvalue of eachInstructionLValue(instr)) {
+      updateDerivationMetadata(lvalue, sources, typeOfValue, derivationCache);
+    }
+
+    for (const operand of eachInstructionOperand(instr)) {
+      switch (operand.effect) {
+        case Effect.Capture:
+        case Effect.Store:
+        case Effect.ConditionallyMutate:
+        case Effect.ConditionallyMutateIterator:
+        case Effect.Mutate: {
+          if (isMutable(instr, operand)) {
+            updateDerivationMetadata(
+              operand,
+              sources,
+              typeOfValue,
+              derivationCache,
+            );
+          }
+          break;
+        }
+        case Effect.Freeze:
+        case Effect.Read: {
+          // no-op
+          break;
+        }
+        case Effect.Unknown: {
+          CompilerError.invariant(false, {
+            reason: 'Unexpected unknown effect',
+            description: null,
+            details: [
+              {
+                kind: 'error',
+                loc: operand.loc,
+                message: 'Unexpected unknown effect',
+              },
+            ],
+          });
+        }
+        default: {
+          assertExhaustive(
+            operand.effect,
+            `Unexpected effect kind \`${operand.effect}\``,
+          );
+        }
+      }
+    }
+  }
+}
+
+function parseBlockPhi(
+  block: BasicBlock,
+  derivationCache: Map<IdentifierId, DerivationMetadata>,
+): void {
+  for (const phi of block.phis) {
+    let typeOfValue: TypeOfValue = 'ignored';
+    let sources: Array<DerivationMetadata> = [];
+    for (const operand of phi.operands.values()) {
+      const opSource = derivationCache.get(operand.identifier.id);
+
+      if (opSource === undefined) {
+        continue;
+      }
+
+      typeOfValue = joinValue(typeOfValue, opSource?.typeOfValue ?? 'ignored');
+      sources.push(opSource);
+    }
+
+    if (typeOfValue !== 'ignored') {
+      updateDerivationMetadata(
+        phi.place,
+        sources,
+        typeOfValue,
+        derivationCache,
+      );
+    }
   }
 }
 
 function validateEffect(
   effectFunction: HIRFunction,
   effectDeps: Array<IdentifierId>,
-  derivedTuple: Map<IdentifierId, DerivationMetadata>,
-  effectSetStates: Map<SetStateName, Place[]>,
-  errors: ErrorMetadata[],
+  derivationCache: Map<IdentifierId, DerivationMetadata>,
+  effectSetStates: Map<string | undefined | null, Array<Place>>,
+  errors: Array<ErrorMetadata>,
 ): void {
-  /*
-   * TODO: This makes it so we only capture single line useEffects.
-   * We should be able to capture multiline as well
-   */
-  for (const operand of effectFunction.context) {
-    if (isSetStateType(operand.identifier)) {
-      continue;
-    } else if (effectDeps.find(dep => dep === operand.identifier.id) != null) {
-      continue;
-    } else if (derivedTuple.has(operand.identifier.id)) {
-      continue;
-    } else {
-      // Captured something other than the effect dep or setState
-      return;
-    }
-  }
-
-  // TODO: This might be wrong gotta double check
-  let hasInvalidDep = false;
+  let isUsingDerivedDeps = false;
   for (const dep of effectDeps) {
-    const depMetadata = derivedTuple.get(dep);
+    const depMetadata = derivationCache.get(dep);
     if (
       effectFunction.context.find(operand => operand.identifier.id === dep) !=
         null ||
       (depMetadata !== undefined && depMetadata.typeOfValue !== 'ignored')
     ) {
-      hasInvalidDep = true;
+      isUsingDerivedDeps = true;
     }
   }
 
-  if (!hasInvalidDep) {
-    console.log('early return 2');
-    // effect dep wasn't actually used in the function
+  if (!isUsingDerivedDeps) {
+    // no prop/state derived deps were used in the body of the effect
     return;
   }
 
   const seenBlocks: Set<BlockId> = new Set();
-  // This variable is suspicious maybe we don't need it?
-  const values: Map<IdentifierId, Array<IdentifierId>> = new Map();
-  const effectInvalidlyDerived: Map<IdentifierId, DerivationMetadata> =
-    new Map();
 
-  for (const dep of effectDeps) {
-    values.set(dep, [dep]);
-    const depMetadata = derivedTuple.get(dep);
-    if (depMetadata !== undefined) {
-      effectInvalidlyDerived.set(dep, depMetadata);
-    }
-  }
-
-  const setStateCallsInEffect: Array<SetStateCall> = [];
+  const derivedSetStateCall: Array<SetStateCall> = [];
   for (const block of effectFunction.body.blocks.values()) {
     for (const pred of block.preds) {
       if (!seenBlocks.has(pred)) {
@@ -452,9 +503,14 @@ function validateEffect(
       }
     }
 
-    parseBlockPhi(block, effectInvalidlyDerived);
+    parseBlockPhi(block, derivationCache);
 
     for (const instr of block.instructions) {
+      // Early return if any instruction is deriving a value from a ref
+      if (isUseRefType(instr.lvalue.identifier)) {
+        return;
+      }
+
       if (
         instr.value.kind === 'CallExpression' &&
         isSetStateType(instr.value.callee.identifier) &&
@@ -481,10 +537,6 @@ function validateEffect(
           break;
         }
         case 'LoadLocal': {
-          const deps = values.get(instr.value.place.identifier.id);
-          if (deps != null) {
-            values.set(instr.lvalue.identifier.id, deps);
-          }
           break;
         }
         case 'ComputedLoad':
@@ -493,82 +545,61 @@ function validateEffect(
         case 'TemplateLiteral':
         case 'CallExpression':
         case 'MethodCall': {
-          const aggregateDeps: Set<IdentifierId> = new Set();
-          for (const operand of eachInstructionOperand(instr)) {
-            const deps = values.get(operand.identifier.id);
-            if (deps != null) {
-              for (const dep of deps) {
-                aggregateDeps.add(dep);
-              }
-            }
-          }
-          if (aggregateDeps.size !== 0) {
-            values.set(instr.lvalue.identifier.id, Array.from(aggregateDeps));
-          }
-
           if (
             instr.value.kind === 'CallExpression' &&
             isSetStateType(instr.value.callee.identifier) &&
             instr.value.args.length === 1 &&
             instr.value.args[0].kind === 'Identifier'
           ) {
-            const invalidDeps = derivedTuple.get(
+            const derivedDep = derivationCache.get(
               instr.value.args[0].identifier.id,
             );
 
-            if (invalidDeps !== undefined) {
-              setStateCallsInEffect.push({
+            if (derivedDep !== undefined) {
+              derivedSetStateCall.push({
                 loc: instr.value.callee.loc,
                 setStateId: instr.value.callee.identifier.id,
-                invalidDeps: invalidDeps,
+                derivedDep: derivedDep,
               });
             }
           }
           break;
         }
-        default: {
-          console.log('early return 4');
-          return;
-        }
       }
     }
 
-    for (const operand of eachTerminalOperand(block.terminal)) {
-      if (values.has(operand.identifier.id)) {
-        return;
-      }
-    }
     seenBlocks.add(block.id);
   }
 
-  for (const call of setStateCallsInEffect) {
-    const placeNames = call.invalidDeps.sources
-      .map(place => place.identifier.name?.value)
+  for (const call of derivedSetStateCall) {
+    const derivedDepsStr = Array.from(call.derivedDep.sources)
+      .map(place => {
+        return place.identifier.name?.value;
+      })
+      .filter(Boolean)
       .join(', ');
 
-    let sourceNames = '';
-    let invalidDepInfo = '';
-    console.log(call.invalidDeps);
-    if (call.invalidDeps.typeOfValue === 'fromProps') {
-      sourceNames += `[${placeNames}], `;
-      sourceNames = sourceNames.slice(0, -2);
-      invalidDepInfo = sourceNames
-        ? `Invalid deps from props ${sourceNames}`
-        : '';
-    } else if (call.invalidDeps.typeOfValue === 'fromState') {
-      sourceNames += `[${placeNames}], `;
-      sourceNames = sourceNames.slice(0, -2);
-      invalidDepInfo = sourceNames
-        ? `Invalid deps from local state: ${sourceNames}`
-        : '';
+    let errorDescription = '';
+
+    if (call.derivedDep.typeOfValue === 'fromProps') {
+      errorDescription = `props [${derivedDepsStr}]`;
+    } else if (call.derivedDep.typeOfValue === 'fromState') {
+      errorDescription = `local state [${derivedDepsStr}]`;
+    } else {
+      errorDescription = `both props and local state [${derivedDepsStr}]`;
     }
 
     errors.push({
-      errorType: call.invalidDeps.typeOfValue,
-      invalidDepInfo: invalidDepInfo,
+      type: call.derivedDep.typeOfValue,
+      description: `${errorDescription}`,
       loc: call.loc,
       setStateName:
         call.loc !== GeneratedSource ? call.loc.identifierName : undefined,
+      derivedDepsNames: Array.from(call.derivedDep.sources)
+        .map(place => {
+          return place.identifier.name?.value ?? '';
+        })
+        .filter(Boolean),
     });
   }
 }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-from-ref-and-state-no-error.expect.md

@@ -0,0 +1,73 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    setLocal(myRef.current + test);
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects
+import { useEffect, useState, useRef } from "react";
+
+export default function Component(t0) {
+  const $ = _c(5);
+  const { test } = t0;
+  const [local, setLocal] = useState("");
+
+  const myRef = useRef(null);
+  let t1;
+  let t2;
+  if ($[0] !== test) {
+    t1 = () => {
+      setLocal(myRef.current + test);
+    };
+    t2 = [test];
+    $[0] = test;
+    $[1] = t1;
+    $[2] = t2;
+  } else {
+    t1 = $[1];
+    t2 = $[2];
+  }
+  useEffect(t1, t2);
+  let t3;
+  if ($[3] !== local) {
+    t3 = <>{local}</>;
+    $[3] = local;
+    $[4] = t3;
+  } else {
+    t3 = $[4];
+  }
+  return t3;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ test: "testString" }],
+};
+
+```
+      
+### Eval output
+(kind: ok) nulltestString

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-from-ref-and-state-no-error.js

@@ -0,0 +1,19 @@
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    setLocal(myRef.current + test);
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-conditional-no-error.expect.md

@@ -1,79 +0,0 @@
-
-## Input
-
-```javascript
-// @validateNoDerivedComputationsInEffects
-import {useEffect, useState} from 'react';
-
-function Component({value, enabled}) {
-  const [localValue, setLocalValue] = useState('');
-
-  useEffect(() => {
-    if (enabled) {
-      setLocalValue(value);
-    } else {
-      setLocalValue('disabled');
-    }
-  }, [value, enabled]);
-
-  return <div>{localValue}</div>;
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: Component,
-  params: [{value: 'test', enabled: true}],
-};
-
-```
-
-## Code
-
-```javascript
-import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects
-import { useEffect, useState } from "react";
-
-function Component(t0) {
-  const $ = _c(6);
-  const { value, enabled } = t0;
-  const [localValue, setLocalValue] = useState("");
-  let t1;
-  let t2;
-  if ($[0] !== enabled || $[1] !== value) {
-    t1 = () => {
-      if (enabled) {
-        setLocalValue(value);
-      } else {
-        setLocalValue("disabled");
-      }
-    };
-
-    t2 = [value, enabled];
-    $[0] = enabled;
-    $[1] = value;
-    $[2] = t1;
-    $[3] = t2;
-  } else {
-    t1 = $[2];
-    t2 = $[3];
-  }
-  useEffect(t1, t2);
-  let t3;
-  if ($[4] !== localValue) {
-    t3 = <div>{localValue}</div>;
-    $[4] = localValue;
-    $[5] = t3;
-  } else {
-    t3 = $[5];
-  }
-  return t3;
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: Component,
-  params: [{ value: "test", enabled: true }],
-};
-
-```
-      
-### Eval output
-(kind: ok) <div>test</div>

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-with-side-effects-no-error.expect.md

@@ -1,74 +0,0 @@
-
-## Input
-
-```javascript
-// @validateNoDerivedComputationsInEffects
-import {useEffect, useState} from 'react';
-
-function Component({value}) {
-  const [localValue, setLocalValue] = useState('');
-
-  useEffect(() => {
-    console.log('Value changed:', value);
-    setLocalValue(value);
-    document.title = `Value: ${value}`;
-  }, [value]);
-
-  return <div>{localValue}</div>;
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: Component,
-  params: [{value: 'test'}],
-};
-
-```
-
-## Code
-
-```javascript
-import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects
-import { useEffect, useState } from "react";
-
-function Component(t0) {
-  const $ = _c(5);
-  const { value } = t0;
-  const [localValue, setLocalValue] = useState("");
-  let t1;
-  let t2;
-  if ($[0] !== value) {
-    t1 = () => {
-      console.log("Value changed:", value);
-      setLocalValue(value);
-      document.title = `Value: ${value}`;
-    };
-    t2 = [value];
-    $[0] = value;
-    $[1] = t1;
-    $[2] = t2;
-  } else {
-    t1 = $[1];
-    t2 = $[2];
-  }
-  useEffect(t1, t2);
-  let t3;
-  if ($[3] !== localValue) {
-    t3 = <div>{localValue}</div>;
-    $[3] = localValue;
-    $[4] = t3;
-  } else {
-    t3 = $[4];
-  }
-  return t3;
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: Component,
-  params: [{ value: "test" }],
-};
-
-```
-      
-### Eval output
-(kind: ok) <div>test</div>
-logs: ['Value changed:','test']

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md

@@ -34,15 +34,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-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: You might not need an effect. Derive values in render, not effects.
 
-This effect updates state based on other state values. Consider calculating this value directly during render.
+Both props and local state [prefix, name]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
 
 error.bug-derived-state-from-mixed-deps.ts:9:4
    7 |
    8 |   useEffect(() => {
 >  9 |     setDisplayName(prefix + name);
-     |     ^^^^^^^^^^^^^^ 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 should be computed during render, not in an effect
   10 |   }, [prefix, name]);
   11 |
   12 |   return (

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md

@@ -0,0 +1,86 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useState, useEffect} from 'react';
+
+function Component({props, number}) {
+  const nothing = 0;
+  const missDirection = number;
+  const [displayValue, setDisplayValue] = useState(
+    props.prefix + missDirection + nothing
+  );
+
+  useEffect(() => {
+    setDisplayValue(props.prefix + missDirection + nothing);
+  }, [props.prefix, missDirection, nothing]);
+
+  return (
+    <div
+      onClick={() => {
+        setDisplayValue('clicked');
+      }}>
+      {displayValue}
+    </div>
+  );
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Local state shadows parent state.
+
+The setState within a useEffect is deriving from props [props, number]. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render.
+
+error.derived-state-from-shadowed-props.ts:12:4
+  10 |
+  11 |   useEffect(() => {
+> 12 |     setDisplayValue(props.prefix + missDirection + nothing);
+     |     ^^^^^^^^^^^^^^^ this derives values from props to synchronize state
+  13 |   }, [props.prefix, missDirection, nothing]);
+  14 |
+  15 |   return (
+
+error.derived-state-from-shadowed-props.ts:7:42
+   5 |   const nothing = 0;
+   6 |   const missDirection = number;
+>  7 |   const [displayValue, setDisplayValue] = useState(
+     |                                           ^^^^^^^^^
+>  8 |     props.prefix + missDirection + nothing
+     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+>  9 |   );
+     | ^^^^ this useState shadows props
+  10 |
+  11 |   useEffect(() => {
+  12 |     setDisplayValue(props.prefix + missDirection + nothing);
+
+error.derived-state-from-shadowed-props.ts:7:42
+   5 |   const nothing = 0;
+   6 |   const missDirection = number;
+>  7 |   const [displayValue, setDisplayValue] = useState(
+     |                                           ^^^^^^^^^
+>  8 |     props.prefix + missDirection + nothing
+     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+>  9 |   );
+     | ^^^^ this useState shadows number
+  10 |
+  11 |   useEffect(() => {
+  12 |     setDisplayValue(props.prefix + missDirection + nothing);
+
+error.derived-state-from-shadowed-props.ts:18:8
+  16 |     <div
+  17 |       onClick={() => {
+> 18 |         setDisplayValue('clicked');
+     |         ^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent
+  19 |       }}>
+  20 |       {displayValue}
+  21 |     </div>
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.js

@@ -0,0 +1,23 @@
+// @validateNoDerivedComputationsInEffects
+import {useState, useEffect} from 'react';
+
+function Component({props, number}) {
+  const nothing = 0;
+  const missDirection = number;
+  const [displayValue, setDisplayValue] = useState(
+    props.prefix + missDirection + nothing
+  );
+
+  useEffect(() => {
+    setDisplayValue(props.prefix + missDirection + nothing);
+  }, [props.prefix, missDirection, nothing]);
+
+  return (
+    <div
+      onClick={() => {
+        setDisplayValue('clicked');
+      }}>
+      {displayValue}
+    </div>
+  );
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-conditional.expect.md

@@ -0,0 +1,49 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState} from 'react';
+
+function Component({value, enabled}) {
+  const [localValue, setLocalValue] = useState('');
+
+  useEffect(() => {
+    if (enabled) {
+      setLocalValue(value);
+    } else {
+      setLocalValue('disabled');
+    }
+  }, [value, enabled]);
+
+  return <div>{localValue}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{value: 'test', enabled: true}],
+};
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Derive values in render, not effects.
+
+Props [value]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+
+error.derived-state-with-conditional.ts:9:6
+   7 |   useEffect(() => {
+   8 |     if (enabled) {
+>  9 |       setLocalValue(value);
+     |       ^^^^^^^^^^^^^ This should be computed during render, not in an effect
+  10 |     } else {
+  11 |       setLocalValue('disabled');
+  12 |     }
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-side-effects.expect.md

@@ -0,0 +1,47 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState} from 'react';
+
+function Component({value}) {
+  const [localValue, setLocalValue] = useState('');
+
+  useEffect(() => {
+    console.log('Value changed:', value);
+    setLocalValue(value);
+    document.title = `Value: ${value}`;
+  }, [value]);
+
+  return <div>{localValue}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{value: 'test'}],
+};
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Derive values in render, not effects.
+
+Props [value]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+
+error.derived-state-with-side-effects.ts:9:4
+   7 |   useEffect(() => {
+   8 |     console.log('Value changed:', value);
+>  9 |     setLocalValue(value);
+     |     ^^^^^^^^^^^^^ This should be computed during render, not in an effect
+  10 |     document.title = `Value: ${value}`;
+  11 |   }, [value]);
+  12 |
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md

@@ -24,15 +24,15 @@ function BadExample() {
 ```
 Found 1 error:
 
-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: You might not need an effect. Derive values in render, not effects.
 
-This effect updates state based on other state values. Consider calculating this value directly during render.
+Local state [firstName, lastName]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
 
 error.invalid-derived-computation-in-effect.ts:9:4
    7 |   const [fullName, setFullName] = useState('');
    8 |   useEffect(() => {
 >  9 |     setFullName(capitalize(firstName + ' ' + lastName));
-     |     ^^^^^^^^^^^ 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 should be computed during render, not in an effect
   10 |   }, [firstName, lastName]);
   11 |
   12 |   return <div>{fullName}</div>;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-computed.expect.md

@@ -0,0 +1,46 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState} from 'react';
+
+function Component(props) {
+  const [displayValue, setDisplayValue] = useState('');
+
+  useEffect(() => {
+    const computed = props.prefix + props.value + props.suffix;
+    setDisplayValue(computed);
+  }, [props.prefix, props.value, props.suffix]);
+
+  return <div>{displayValue}</div>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{prefix: '[', value: 'test', suffix: ']'}],
+};
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Derive values in render, not effects.
+
+Props [props, props, props]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+
+error.invalid-derived-state-from-props-computed.ts:9:4
+   7 |   useEffect(() => {
+   8 |     const computed = props.prefix + props.value + props.suffix;
+>  9 |     setDisplayValue(computed);
+     |     ^^^^^^^^^^^^^^^ This should be computed during render, not in an effect
+  10 |   }, [props.prefix, props.value, props.suffix]);
+  11 |
+  12 |   return <div>{displayValue}</div>;
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md

@@ -5,19 +5,21 @@
 // @validateNoDerivedComputationsInEffects
 import {useEffect, useState} from 'react';
 
-function Component({user: {firstName, lastName}}) {
-  const [fullName, setFullName] = useState('');
+function Component({props}) {
+  const [fullName, setFullName] = useState(
+    props.firstName + ' ' + props.lastName
+  );
 
   useEffect(() => {
-    setFullName(firstName + ' ' + lastName);
-  }, [firstName, lastName]);
+    setFullName(props.firstName + ' ' + props.lastName);
+  }, [props.firstName, props.lastName]);
 
   return <div>{fullName}</div>;
 }
 
 export const FIXTURE_ENTRYPOINT = {
   fn: Component,
-  params: [{user: {firstName: 'John', lastName: 'Doe'}}],
+  params: [{firstName: 'John', lastName: 'Doe'}],
 };
 
 ```
@@ -28,18 +30,18 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-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: You might not need an effect. Derive values in render, not effects.
 
-This effect updates state based on other state values. Consider calculating this value directly during render.
+Props [props, props]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
 
-error.invalid-derived-state-from-props-destructured.ts:8:4
-   6 |
-   7 |   useEffect(() => {
->  8 |     setFullName(firstName + ' ' + lastName);
-     |     ^^^^^^^^^^^ 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>;
+error.invalid-derived-state-from-props-destructured.ts:10:4
+   8 |
+   9 |   useEffect(() => {
+> 10 |     setFullName(props.firstName + ' ' + props.lastName);
+     |     ^^^^^^^^^^^ This should be computed during render, not in an effect
+  11 |   }, [props.firstName, props.lastName]);
+  12 |
+  13 |   return <div>{fullName}</div>;
 ```
           
       

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.js

@@ -1,12 +1,14 @@
 // @validateNoDerivedComputationsInEffects
 import {useEffect, useState} from 'react';
 
-function Component({firstName, lastName}) {
-  const [fullName, setFullName] = useState('');
+function Component({props}) {
+  const [fullName, setFullName] = useState(
+    props.firstName + ' ' + props.lastName
+  );
 
   useEffect(() => {
-    setFullName(firstName + ' ' + lastName);
-  }, [firstName, lastName]);
+    setFullName(props.firstName + ' ' + props.lastName);
+  }, [props.firstName, props.lastName]);
 
   return <div>{fullName}</div>;
 }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md

@@ -28,15 +28,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-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: You might not need an effect. Derive values in render, not effects.
 
-This effect updates state based on other state values. Consider calculating this value directly during render.
+Props [firstName, lastName]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
 
 error.invalid-derived-state-from-props-in-effect.ts:8:4
    6 |
    7 |   useEffect(() => {
 >  8 |     setFullName(firstName + ' ' + lastName);
-     |     ^^^^^^^^^^^ 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 should be computed during render, not in an effect
    9 |   }, [firstName, lastName]);
   10 |
   11 |   return <div>{fullName}</div>;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-with-default-value.expect.md

@@ -0,0 +1,39 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+
+export default function InProductLobbyGeminiCard(input = 'empty') {
+  const [currInput, setCurrInput] = useState(input);
+
+  useEffect(() => {
+    setCurrInput(input);
+  }, [input]);
+
+  return <div>{currInput}</div>;
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Derive values in render, not effects.
+
+Props [input]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
+
+error.invalid-derived-state-from-props-with-default-value.ts:7:4
+   5 |
+   6 |   useEffect(() => {
+>  7 |     setCurrInput(input);
+     |     ^^^^^^^^^^^^ This should be computed during render, not in an effect
+   8 |   }, [input]);
+   9 |
+  10 |   return <div>{currInput}</div>;
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-with-default-value.js

@@ -0,0 +1,11 @@
+// @validateNoDerivedComputationsInEffects
+
+export default function InProductLobbyGeminiCard(input = 'empty') {
+  const [currInput, setCurrInput] = useState(input);
+
+  useEffect(() => {
+    setCurrInput(input);
+  }, [input]);
+
+  return <div>{currInput}</div>;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md

@@ -36,15 +36,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-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: You might not need an effect. Derive values in render, not effects.
 
-This effect updates state based on other state values. Consider calculating this value directly during render.
+Local state [firstName, lastName]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user.
 
 error.invalid-derived-state-from-state-in-effect.ts:10:4
    8 |
    9 |   useEffect(() => {
 > 10 |     setFullName(firstName + ' ' + lastName);
-     |     ^^^^^^^^^^^ 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 should be computed during render, not in an effect
   11 |   }, [firstName, lastName]);
   12 |
   13 |   return (

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.expect.md

@@ -0,0 +1,63 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+
+function EndDate({startDate, endDate, onStartDateChange}) {
+  const [localStartDate, setLocalStartDate] = useState(startDate);
+
+  useEffect(() => {
+    setLocalStartDate(startDate);
+  }, [startDate]);
+
+  const onChange = date => {
+    setLocalStartDate(date);
+    onStartDateChange(date);
+  };
+  return (
+    <DateInput value={localStartDate} second={endDate} onChange={onChange} />
+  );
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Local state shadows parent state.
+
+The setState within a useEffect is deriving from props [startDate]. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render.
+
+error.shadowed-props-with-onchange.ts:7:4
+   5 |
+   6 |   useEffect(() => {
+>  7 |     setLocalStartDate(startDate);
+     |     ^^^^^^^^^^^^^^^^^ this derives values from props to synchronize state
+   8 |   }, [startDate]);
+   9 |
+  10 |   const onChange = date => {
+
+error.shadowed-props-with-onchange.ts:4:46
+  2 |
+  3 | function EndDate({startDate, endDate, onStartDateChange}) {
+> 4 |   const [localStartDate, setLocalStartDate] = useState(startDate);
+    |                                               ^^^^^^^^^^^^^^^^^^^ this useState shadows startDate
+  5 |
+  6 |   useEffect(() => {
+  7 |     setLocalStartDate(startDate);
+
+error.shadowed-props-with-onchange.ts:11:4
+   9 |
+  10 |   const onChange = date => {
+> 11 |     setLocalStartDate(date);
+     |     ^^^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent
+  12 |     onStartDateChange(date);
+  13 |   };
+  14 |   return (
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.js

@@ -0,0 +1,17 @@
+// @validateNoDerivedComputationsInEffects
+
+function EndDate({startDate, endDate, onStartDateChange}) {
+  const [localStartDate, setLocalStartDate] = useState(startDate);
+
+  useEffect(() => {
+    setLocalStartDate(startDate);
+  }, [startDate]);
+
+  const onChange = date => {
+    setLocalStartDate(date);
+    onStartDateChange(date);
+  };
+  return (
+    <DateInput value={localStartDate} second={endDate} onChange={onChange} />
+  );
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.expect.md

@@ -1,72 +0,0 @@
-
-## Input
-
-```javascript
-// @validateNoDerivedComputationsInEffects
-import {useEffect, useState} from 'react';
-
-function Component(props) {
-  const [displayValue, setDisplayValue] = useState('');
-
-  useEffect(() => {
-    const computed = props.prefix + props.value + props.suffix;
-    setDisplayValue(computed);
-  }, [props.prefix, props.value, props.suffix]);
-
-  return <div>{displayValue}</div>;
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: Component,
-  params: [{prefix: '[', value: 'test', suffix: ']'}],
-};
-
-```
-
-## Code
-
-```javascript
-import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects
-import { useEffect, useState } from "react";
-
-function Component(props) {
-  const $ = _c(7);
-  const [displayValue, setDisplayValue] = useState("");
-  let t0;
-  let t1;
-  if ($[0] !== props.prefix || $[1] !== props.suffix || $[2] !== props.value) {
-    t0 = () => {
-      const computed = props.prefix + props.value + props.suffix;
-      setDisplayValue(computed);
-    };
-    t1 = [props.prefix, props.value, props.suffix];
-    $[0] = props.prefix;
-    $[1] = props.suffix;
-    $[2] = props.value;
-    $[3] = t0;
-    $[4] = t1;
-  } else {
-    t0 = $[3];
-    t1 = $[4];
-  }
-  useEffect(t0, t1);
-  let t2;
-  if ($[5] !== displayValue) {
-    t2 = <div>{displayValue}</div>;
-    $[5] = displayValue;
-    $[6] = t2;
-  } else {
-    t2 = $[6];
-  }
-  return t2;
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: Component,
-  params: [{ prefix: "[", value: "test", suffix: "]" }],
-};
-
-```
-      
-### Eval output
-(kind: ok) <div>[test]</div>

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/ref-conditional-in-effect-no-error.expect.md

@@ -0,0 +1,82 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    if (myRef.current) {
+      setLocal(test + 'Available');
+    } else {
+      setLocal(test + 'NotAvailable');
+    }
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects
+import { useEffect, useState, useRef } from "react";
+
+export default function Component(t0) {
+  const $ = _c(5);
+  const { test } = t0;
+  const [local, setLocal] = useState("");
+
+  const myRef = useRef(null);
+  let t1;
+  let t2;
+  if ($[0] !== test) {
+    t1 = () => {
+      if (myRef.current) {
+        setLocal(test + "Available");
+      } else {
+        setLocal(test + "NotAvailable");
+      }
+    };
+
+    t2 = [test];
+    $[0] = test;
+    $[1] = t1;
+    $[2] = t2;
+  } else {
+    t1 = $[1];
+    t2 = $[2];
+  }
+  useEffect(t1, t2);
+  let t3;
+  if ($[3] !== local) {
+    t3 = <>{local}</>;
+    $[3] = local;
+    $[4] = t3;
+  } else {
+    t3 = $[4];
+  }
+  return t3;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ test: "testString" }],
+};
+
+```
+      
+### Eval output
+(kind: ok) testStringNotAvailable

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/ref-conditional-in-effect-no-error.js

@@ -0,0 +1,23 @@
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    if (myRef.current) {
+      setLocal(test + 'Available');
+    } else {
+      setLocal(test + 'NotAvailable');
+    }
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};

compiler/yarn.lock

@@ -10494,16 +10494,7 @@ string-length@^4.0.1:
     char-regex "^1.0.2"
     strip-ansi "^6.0.0"
 
-"string-width-cjs@npm:string-width@^4.2.0":
-  version "4.2.3"
-  resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz"
-  integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
-  dependencies:
-    emoji-regex "^8.0.0"
-    is-fullwidth-code-point "^3.0.0"
-    strip-ansi "^6.0.1"
-
-string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
+"string-width-cjs@npm:string-width@^4.2.0", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
   version "4.2.3"
   resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz"
   integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
@@ -10576,14 +10567,7 @@ string_decoder@~1.1.1:
   dependencies:
     safe-buffer "~5.1.0"
 
-"strip-ansi-cjs@npm:strip-ansi@^6.0.1":
-  version "6.0.1"
-  resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz"
-  integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
-  dependencies:
-    ansi-regex "^5.0.1"
-
-strip-ansi@^6.0.0, strip-ansi@^6.0.1:
+"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1:
   version "6.0.1"
   resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz"
   integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
@@ -11360,7 +11344,7 @@ workerpool@^6.5.1:
   resolved "https://registry.npmjs.org/workerpool/-/workerpool-6.5.1.tgz"
   integrity sha512-Fs4dNYcsdpYSAfVxhnl1L5zTksjvOJxtC5hzMNl+1t9B8hTJTdKDyZ5ju7ztgPy+ft9tBFXoOlDNiOT9WUXZlA==
 
-"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0":
+"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0", wrap-ansi@^7.0.0:
   version "7.0.0"
   resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz"
   integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
@@ -11378,15 +11362,6 @@ wrap-ansi@^6.2.0:
     string-width "^4.1.0"
     strip-ansi "^6.0.0"
 
-wrap-ansi@^7.0.0:
-  version "7.0.0"
-  resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz"
-  integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
-  dependencies:
-    ansi-styles "^4.0.0"
-    string-width "^4.1.0"
-    strip-ansi "^6.0.0"
-
 wrap-ansi@^8.1.0:
   version "8.1.0"
   resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-8.1.0.tgz"