Merge branch 'main' into pr34442

[compiler] Fix false positive hook return mutation error (#34424 )
This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call _may_ alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context. That change was sufficient for the original case like ```js const frozen = useContext(); useEffect(() => { frozen.method().property = true; }, [...]); ``` But it wasn't sufficient for cases where the aliasing occured between operands: ```js const dispatch = useDispatch(); <div onClick={(e) => { dispatch(...e.target.value) e.target.value = ...; }} /> ``` Here we would record a `Capture dispatch <- e.target` effect. Then during processing of the `event.target.value = ...` assignment we'd eventually _forward_ from `event` to `dispatch` (along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional. In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think: * If the value is frozen, it's an ImmutableCapture edge * If the values are mutable, it's a Capture * If it's a context->context, context->mutable, or mutable->context, count it as MaybeAlias.
2025-09-09 16:29:58 -07:00 · 2025-09-09 14:07:47 -07:00
5 changed files with 383 additions and 301 deletions
--- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts
+++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts
@@ -748,10 +748,14 @@ function applyEffect(
    case 'Alias':
    case 'Capture': {
      CompilerError.invariant(
-        effect.kind === 'Capture' || initialized.has(effect.into.identifier.id),
+        effect.kind === 'Capture' ||
+          effect.kind === 'MaybeAlias' ||
+          initialized.has(effect.into.identifier.id),
        {
-          reason: `Expected destination value to already be initialized within this instruction for Alias effect`,
-          description: `Destination ${printPlace(effect.into)} is not initialized in this instruction`,
+          reason: `Expected destination to already be initialized within this instruction`,
+          description:
+            `Destination ${printPlace(effect.into)} is not initialized in this ` +
+            `instruction for effect ${printAliasingEffect(effect)}`,
          details: [
            {
              kind: 'error',
@@ -767,49 +771,67 @@ function applyEffect(
       * copy-on-write semantics, then we can prune the effect
       */
      const intoKind = state.kind(effect.into).kind;
-      let isMutableDesination: boolean;
+      let destinationType: 'context' | 'mutable' | null = null;
      switch (intoKind) {
-        case ValueKind.Context:
-        case ValueKind.Mutable:
-        case ValueKind.MaybeFrozen: {
-          isMutableDesination = true;
+        case ValueKind.Context: {
+          destinationType = 'context';
          break;
        }
-        default: {
-          isMutableDesination = false;
+        case ValueKind.Mutable:
+        case ValueKind.MaybeFrozen: {
+          destinationType = 'mutable';
          break;
        }
      }
      const fromKind = state.kind(effect.from).kind;
-      let isMutableReferenceType: boolean;
+      let sourceType: 'context' | 'mutable' | 'frozen' | null = null;
      switch (fromKind) {
+        case ValueKind.Context: {
+          sourceType = 'context';
+          break;
+        }
        case ValueKind.Global:
        case ValueKind.Primitive: {
-          isMutableReferenceType = false;
          break;
        }
        case ValueKind.Frozen: {
-          isMutableReferenceType = false;
-          applyEffect(
-            context,
-            state,
-            {
-              kind: 'ImmutableCapture',
-              from: effect.from,
-              into: effect.into,
-            },
-            initialized,
-            effects,
-          );
+          sourceType = 'frozen';
          break;
        }
        default: {
-          isMutableReferenceType = true;
+          sourceType = 'mutable';
          break;
        }
      }
-      if (isMutableDesination && isMutableReferenceType) {
+
+      if (sourceType === 'frozen') {
+        applyEffect(
+          context,
+          state,
+          {
+            kind: 'ImmutableCapture',
+            from: effect.from,
+            into: effect.into,
+          },
+          initialized,
+          effects,
+        );
+      } else if (
+        (sourceType === 'mutable' && destinationType === 'mutable') ||
+        effect.kind === 'MaybeAlias'
+      ) {
        effects.push(effect);
+      } else if (
+        (sourceType === 'context' && destinationType != null) ||
+        (sourceType === 'mutable' && destinationType === 'context')
+      ) {
+        applyEffect(
+          context,
+          state,
+          {kind: 'MaybeAlias', from: effect.from, into: effect.into},
+          initialized,
+          effects,
+        );
      }
      break;
    }
--- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts
+++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts
@@ -779,7 +779,13 @@ class AliasingState {
        if (edge.index >= index) {
          break;
        }
-        queue.push({place: edge.node, transitive, direction: 'forwards', kind});
+        queue.push({
+          place: edge.node,
+          transitive,
+          direction: 'forwards',
+          // Traversing a maybeAlias edge always downgrades to conditional mutation
+          kind: edge.kind === 'maybeAlias' ? MutationKind.Conditional : kind,
+        });
      }
      for (const [alias, when] of node.createdFrom) {
        if (when >= index) {
@@ -807,7 +813,12 @@ class AliasingState {
          if (when >= index) {
            continue;
          }
-          queue.push({place: alias, transitive, direction: 'backwards', kind});
+          queue.push({
+            place: alias,
+            transitive,
+            direction: 'backwards',
+            kind,
+          });
        }
        /**
         * MaybeAlias indicates potential data flow from unknown function calls,
--- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts
+++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts
@@ -5,23 +5,23 @@
 * LICENSE file in the root directory of this source tree.
 */

+import {TypeOf} from 'zod';
 import {CompilerError, Effect, ErrorSeverity, SourceLocation} from '..';
 import {ErrorCategory} from '../CompilerError';
 import {
  ArrayExpression,
-  BasicBlock,
  BlockId,
  FunctionExpression,
  HIRFunction,
  IdentifierId,
-  Instruction,
+  InstructionValue,
  Place,
  isSetStateType,
  isUseEffectHookType,
-  isUseStateType,
-  GeneratedSource,
 } from '../HIR';
+import {printInstruction, printPlace} from '../HIR/PrintHIR';
 import {
+  eachInstructionValueOperand,
  eachInstructionOperand,
  eachTerminalOperand,
  eachInstructionLValue,
@@ -29,28 +29,16 @@ import {
 import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
 import {assertExhaustive} from '../Utils/utils';

-// TODO: Maybe I can consolidate some types
 type SetStateCall = {
  loc: SourceLocation;
-  invalidDeps: DerivationMetadata;
-  setStateId: IdentifierId;
+  propsSources: Place[] | undefined; // undefined means state-derived, defined means props-derived
 };
-
 type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState';

-type SetStateName = string | undefined | null;
-
 type DerivationMetadata = {
+  identifierPlace: Place;
+  sources: Place[];
  typeOfValue: TypeOfValue;
-  place: Place;
-  sources: Array<Place>;
-};
-
-type ErrorMetadata = {
-  errorType: TypeOfValue;
-  invalidDepInfo: string | undefined;
-  loc: SourceLocation;
-  setStateName: SetStateName;
 };

 function joinValue(
@@ -63,24 +51,38 @@ function joinValue(
  return 'fromPropsOrState';
 }

+function propagateDerivation(
+  dest: Place,
+  source: Place | undefined,
+  derivedFromProps: Map<IdentifierId, Place>,
+) {
+  if (source === undefined) {
+    return;
+  }
+
+  if (source.identifier.name?.kind === 'promoted') {
+    derivedFromProps.set(dest.identifier.id, dest);
+  } else {
+    derivedFromProps.set(dest.identifier.id, source);
+  }
+}
+
 function updateDerivationMetadata(
  target: Place,
-  sources: Array<DerivationMetadata>,
+  sources: DerivationMetadata[],
  typeOfValue: TypeOfValue,
  derivedTuple: Map<IdentifierId, DerivationMetadata>,
 ): void {
  let newValue: DerivationMetadata = {
-    place: target,
+    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.place.identifier.name?.kind === 'promoted') {
+    // If the identifier of the source is a promoted identifier, then
+    // we should set the source as the first named identifier.
+    if (source.identifierPlace.identifier.name?.kind === 'promoted') {
      newValue.sources.push(target);
    } else {
      newValue.sources.push(...source.sources);
@@ -89,133 +91,6 @@ function updateDerivationMetadata(
  derivedTuple.set(target.identifier.id, newValue);
 }

-function parseInstr(
-  instr: Instruction,
-  derivedTuple: Map<IdentifierId, DerivationMetadata>,
-  setStateCalls: Map<SetStateName, Array<Place>>,
-): void {
-  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, {
-        place: 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: Array<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>,
-): void {
-  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.place.identifier.name === null ||
-          source.place.identifier.name?.kind === 'promoted'
-        ) {
-          derivedTuple.set(phi.place.identifier.id, {
-            place: phi.place,
-            sources: [phi.place],
-            typeOfValue: 'fromProps',
-          });
-        } else {
-          derivedTuple.set(phi.place.identifier.id, {
-            place: phi.place,
-            sources: source.sources,
-            typeOfValue: 'fromProps',
-          });
-        }
-      }
-    }
-  }
-}
-
 /**
 * Validates that useEffect is not used for derived computations which could/should
 * be performed in render.
@@ -243,18 +118,23 @@ 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();
+
+  // MY take on this
+  const valueToType: Map<IdentifierId, TypeOfValue> = new Map();
+  const valueToSourceProps: Map<IdentifierId, Set<Place>> = new Map();
+  const valueToSourceStates: Map<IdentifierId, Set<Place>> = new Map();
+  const valueToSources: Map<IdentifierId, Set<Place>> = new Map();
+
+  // Sources are still probably not correct
  const derivedTuple: Map<IdentifierId, DerivationMetadata> = new Map();

-  const effectSetStates: Map<SetStateName, Array<Place>> = new Map();
-  const setStateCalls: Map<SetStateName, Array<Place>> = new Map();
-
-  const errors: Array<ErrorMetadata> = [];
+  const errors = new CompilerError();

  if (fn.fnType === 'Hook') {
    for (const param of fn.params) {
      if (param.kind === 'Identifier') {
        derivedTuple.set(param.identifier.id, {
-          place: param,
+          identifierPlace: param,
          sources: [param],
          typeOfValue: 'fromProps',
        });
@@ -264,7 +144,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
    const props = fn.params[0];
    if (props != null && props.kind === 'Identifier') {
      derivedTuple.set(props.identifier.id, {
-        place: props,
+        identifierPlace: props,
        sources: [props],
        typeOfValue: 'fromProps',
      });
@@ -272,25 +152,104 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
  }

  for (const block of fn.body.blocks.values()) {
-    parseBlockPhi(block, derivedTuple);
+    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',
+            });
+          }
+        }
+      }
+    }

    for (const instr of block.instructions) {
      const {lvalue, value} = instr;

-      parseInstr(instr, derivedTuple, setStateCalls);
+      // This needs to be repeated "recursively" on FunctionExpressions
+      // HERE >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
+      // DERIVATION LOGIC-----------------------------------------------------
+      console.log('instr', printInstruction(instr));
+      console.log('instr', instr);
+      // console.log('instr lValue', instr.lvalue);

-      /*
-       * 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);
+      let typeOfValue: TypeOfValue = 'ignored';
+
+      // TODO: Add handling for state derived props
+      let sources: DerivationMetadata[] = [];
+      for (const operand of eachInstructionValueOperand(value)) {
+        const opSource = derivedTuple.get(operand.identifier.id);
+        if (opSource === undefined) {
+          continue;
+        }
+
+        typeOfValue = joinValue(typeOfValue, opSource.typeOfValue);
+        sources.push(opSource);
+      }
+
+      // TODO: Add handling for state derived props
+      if (typeOfValue !== 'ignored') {
+        for (const lvalue of eachInstructionLValue(instr)) {
+          updateDerivationMetadata(lvalue, sources, typeOfValue, derivedTuple);
+        }
+
+        for (const operand of eachInstructionValueOperand(value)) {
+          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}\``,
+              );
+            }
          }
        }
      }
+      console.log('derivedTuple', derivedTuple);
+      // HERE >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

+      // console.log('derivedTuple', derivedTuple);
+      // DERIVATION LOGIC-----------------------------------------------------
      if (value.kind === 'LoadLocal') {
        locals.set(lvalue.identifier.id, value.place.identifier.id);
      } else if (value.kind === 'ArrayExpression') {
@@ -304,6 +263,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
        const callee =
          value.kind === 'CallExpression' ? value.callee : value.property;

+        // This is a useEffect hook
        if (
          isUseEffectHookType(callee.identifier) &&
          value.args.length === 2 &&
@@ -336,7 +296,6 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
              effectFunction.loweredFunc.func,
              dependencies,
              derivedTuple,
-              effectSetStates,
              errors,
            );
          }
@@ -344,37 +303,8 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
      }
    }
  }
-
-  const throwableErrors = new CompilerError();
-  for (const error of errors) {
-    let reason;
-    // TODO: Not sure if this is robust enough.
-    /*
-     * If we use a setState from an invalid useEffect elsewhere then we probably have to
-     * hoist state up, else we should calculate in render
-     */
-    if (
-      setStateCalls.get(error.setStateName)?.length !=
-        effectSetStates.get(error.setStateName)?.length &&
-      error.errorType !== '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)';
-    } 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)';
-    }
-
-    throwableErrors.push({
-      reason: reason,
-      description: `You are using invalid dependencies: \n\n${error.invalidDepInfo}`,
-      severity: ErrorSeverity.InvalidReact,
-      loc: error.loc,
-    });
-  }
-
-  if (throwableErrors.hasAnyErrors()) {
-    throw throwableErrors;
+  if (errors.hasAnyErrors()) {
+    throw errors;
  }
 }

@@ -382,13 +312,8 @@ function validateEffect(
  effectFunction: HIRFunction,
  effectDeps: Array<IdentifierId>,
  derivedTuple: Map<IdentifierId, DerivationMetadata>,
-  effectSetStates: Map<SetStateName, Array<Place>>,
-  errors: Array<ErrorMetadata>,
+  errors: CompilerError,
 ): 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;
@@ -398,11 +323,12 @@ function validateEffect(
      continue;
    } else {
      // Captured something other than the effect dep or setState
+      console.log('early return 1');
      return;
    }
  }

-  // TODO: This might be wrong gotta double check
+  // This might be wrong gotta double check
  let hasInvalidDep = false;
  for (const dep of effectDeps) {
    const depMetadata = derivedTuple.get(dep);
@@ -424,18 +350,17 @@ function validateEffect(
  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();
+  const effectInvalidlyDerived: Map<IdentifierId, Place[]> = new Map();

  for (const dep of effectDeps) {
    values.set(dep, [dep]);
    const depMetadata = derivedTuple.get(dep);
    if (depMetadata !== undefined) {
-      effectInvalidlyDerived.set(dep, depMetadata);
+      effectInvalidlyDerived.set(dep, depMetadata.sources);
    }
  }

-  const setStateCallsInEffect: Array<SetStateCall> = [];
+  const setStateCalls: Array<SetStateCall> = [];
  for (const block of effectFunction.body.blocks.values()) {
    for (const pred of block.preds) {
      if (!seenBlocks.has(pred)) {
@@ -444,28 +369,33 @@ function validateEffect(
      }
    }

-    parseBlockPhi(block, effectInvalidlyDerived);
+    // TODO: This might need editing
+    for (const phi of block.phis) {
+      const aggregateDeps: Set<IdentifierId> = new Set();
+      let propsSources: Place[] | null = null;

-    for (const instr of block.instructions) {
-      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 &&
-        instr.value.callee.loc.identifierName !== undefined &&
-        instr.value.callee.loc.identifierName !== null
-      ) {
-        if (effectSetStates.has(instr.value.callee.loc.identifierName)) {
-          effectSetStates
-            .get(instr.value.callee.loc.identifierName)!
-            .push(instr.value.callee);
-        } else {
-          effectSetStates.set(instr.value.callee.loc.identifierName, [
-            instr.value.callee,
-          ]);
+      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 sources = effectInvalidlyDerived.get(operand.identifier.id);
+        if (sources != null) {
+          propsSources = sources;
        }
      }
+
+      if (aggregateDeps.size !== 0) {
+        values.set(phi.place.identifier.id, Array.from(aggregateDeps));
+      }
+      if (propsSources != null) {
+        effectInvalidlyDerived.set(phi.place.identifier.id, propsSources);
+      }
+    }
+
+    for (const instr of block.instructions) {
      switch (instr.value.kind) {
        case 'Primitive':
        case 'JSXText':
@@ -504,16 +434,32 @@ function validateEffect(
            instr.value.args.length === 1 &&
            instr.value.args[0].kind === 'Identifier'
          ) {
-            const invalidDeps = derivedTuple.get(
-              instr.value.args[0].identifier.id,
-            );
+            const deps = values.get(instr.value.args[0].identifier.id);
+            console.log('deps', deps);
+            if (deps != null && new Set(deps).size === effectDeps.length) {
+              // console.log('setState arg', instr.value.args[0].identifier.id);
+              // console.log('effectInvalidlyDerived', effectInvalidlyDerived);
+              // console.log('derivedTuple', derivedTuple);
+              const propSources = derivedTuple.get(
+                instr.value.args[0].identifier.id,
+              );

-            if (invalidDeps !== undefined) {
-              setStateCallsInEffect.push({
-                loc: instr.value.callee.loc,
-                setStateId: instr.value.callee.identifier.id,
-                invalidDeps: invalidDeps,
-              });
+              console.log('Final reference', propSources);
+              if (propSources !== undefined) {
+                setStateCalls.push({
+                  loc: instr.value.callee.loc,
+                  propsSources: propSources.sources,
+                });
+              } else {
+                setStateCalls.push({
+                  loc: instr.value.callee.loc,
+                  propsSources: undefined,
+                });
+              }
+            } else {
+              // doesn't depend on all deps
+              console.log('early return 3');
+              return;
            }
          }
          break;
@@ -524,7 +470,6 @@ function validateEffect(
        }
      }
    }
-
    for (const operand of eachTerminalOperand(block.terminal)) {
      if (values.has(operand.identifier.id)) {
        return;
@@ -533,40 +478,32 @@ function validateEffect(
    seenBlocks.add(block.id);
  }

-  for (const call of setStateCallsInEffect) {
-    const placeNames = call.invalidDeps.sources
-      .map(place => place.identifier.name?.value)
-      .join(', ');
+  console.log('setStateCalls', setStateCalls);
+  for (const call of setStateCalls) {
+    if (call.propsSources != null) {
+      const propNames = call.propsSources
+        .map(place => place.identifier.name?.value)
+        .join(', ');
+      const propInfo = propNames != null ? ` (from props '${propNames}')` : '';

-    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}`
-        : '';
+      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 {
-      sourceNames += `[${placeNames}], `;
-      sourceNames = sourceNames.slice(0, -2);
-      invalidDepInfo = sourceNames
-        ? `Invalid deps from both props and local state: ${sourceNames}`
-        : '';
+      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,
+      });
    }
-
-    errors.push({
-      errorType: call.invalidDeps.typeOfValue,
-      invalidDepInfo: invalidDepInfo,
-      loc: call.loc,
-      setStateName:
-        call.loc !== GeneratedSource ? call.loc.identifierName : undefined,
-    });
  }
 }
--- a/compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.expect.md
+++ b/compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.expect.md
@@ -0,0 +1,82 @@
+
+## Input
+
+```javascript
+// @compilationMode:"infer"
+function Component() {
+  const dispatch = useDispatch();
+  // const [state, setState] = useState(0);
+
+  return (
+    <div>
+      <input
+        type="file"
+        onChange={event => {
+          dispatch(...event.target);
+          event.target.value = '';
+        }}
+      />
+    </div>
+  );
+}
+
+function useDispatch() {
+  'use no memo';
+  // skip compilation to make it easier to debug the above function
+  return (...values) => {
+    console.log(...values);
+  };
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @compilationMode:"infer"
+function Component() {
+  const $ = _c(2);
+  const dispatch = useDispatch();
+  let t0;
+  if ($[0] !== dispatch) {
+    t0 = (
+      <div>
+        <input
+          type="file"
+          onChange={(event) => {
+            dispatch(...event.target);
+            event.target.value = "";
+          }}
+        />
+      </div>
+    );
+    $[0] = dispatch;
+    $[1] = t0;
+  } else {
+    t0 = $[1];
+  }
+  return t0;
+}
+
+function useDispatch() {
+  "use no memo";
+  // skip compilation to make it easier to debug the above function
+  return (...values) => {
+    console.log(...values);
+  };
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+      
+### Eval output
+(kind: ok) <div><input type="file"></div>
--- a/compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.js
+++ b/compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.js
@@ -0,0 +1,30 @@
+// @compilationMode:"infer"
+function Component() {
+  const dispatch = useDispatch();
+  // const [state, setState] = useState(0);
+
+  return (
+    <div>
+      <input
+        type="file"
+        onChange={event => {
+          dispatch(...event.target);
+          event.target.value = '';
+        }}
+      />
+    </div>
+  );
+}
+
+function useDispatch() {
+  'use no memo';
+  // skip compilation to make it easier to debug the above function
+  return (...values) => {
+    console.log(...values);
+  };
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};