[compiler] Fix false negatives and add data flow tree to compiler error for no-deriving-state-in-effects

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

@@ -47,6 +47,43 @@ type ValidationContext = {
 class DerivationCache {
   hasChanges: boolean = false;
   cache: Map<IdentifierId, DerivationMetadata> = new Map();
+  private previousCache: Map<IdentifierId, DerivationMetadata> | null = null;
+
+  takeSnapshot(): void {
+    this.previousCache = new Map();
+    for (const [key, value] of this.cache.entries()) {
+      this.previousCache.set(key, {
+        place: value.place,
+        sourcesIds: new Set(value.sourcesIds),
+        typeOfValue: value.typeOfValue,
+      });
+    }
+  }
+
+  checkForChanges(): void {
+    if (this.previousCache === null) {
+      this.hasChanges = true;
+      return;
+    }
+
+    for (const [key, value] of this.cache.entries()) {
+      const previousValue = this.previousCache.get(key);
+      if (
+        previousValue === undefined ||
+        !this.isDerivationEqual(previousValue, value)
+      ) {
+        this.hasChanges = true;
+        return;
+      }
+    }
+
+    if (this.cache.size !== this.previousCache.size) {
+      this.hasChanges = true;
+      return;
+    }
+
+    this.hasChanges = false;
+  }
 
   snapshot(): boolean {
     const hasChanges = this.hasChanges;
@@ -65,41 +102,27 @@ class DerivationCache {
       typeOfValue: typeOfValue ?? 'ignored',
     };
 
-    if (sourcesIds !== undefined) {
-      for (const id of sourcesIds) {
-        const sourcePlace = this.cache.get(id)?.place;
+    if (isNamedIdentifier(derivedVar)) {
+      newValue.sourcesIds.add(derivedVar.identifier.id);
+    }
 
-        if (sourcePlace === undefined) {
-          continue;
-        }
+    for (const id of sourcesIds) {
+      const sourceMetadata = this.cache.get(id);
 
-        /*
-         * If the identifier of the source is a promoted identifier, then
-         *  we should set the target as the source.
-         */
-        if (
-          sourcePlace.identifier.name === null ||
-          sourcePlace.identifier.name?.kind === 'promoted'
-        ) {
-          newValue.sourcesIds.add(derivedVar.identifier.id);
-        } else {
-          newValue.sourcesIds.add(sourcePlace.identifier.id);
-        }
+      if (sourceMetadata === undefined) {
+        continue;
       }
-    }
 
-    if (newValue.sourcesIds.size === 0) {
-      newValue.sourcesIds.add(derivedVar.identifier.id);
+      if (isNamedIdentifier(sourceMetadata.place)) {
+        newValue.sourcesIds.add(sourceMetadata.place.identifier.id);
+      } else {
+        for (const sourcesSourceId of sourceMetadata.sourcesIds) {
+          newValue.sourcesIds.add(sourcesSourceId);
+        }
+      }
     }
 
-    const existingValue = this.cache.get(derivedVar.identifier.id);
-    if (
-      existingValue === undefined ||
-      !this.isDerivationEqual(existingValue, newValue)
-    ) {
-      this.cache.set(derivedVar.identifier.id, newValue);
-      this.hasChanges = true;
-    }
+    this.cache.set(derivedVar.identifier.id, newValue);
   }
 
   private isDerivationEqual(
@@ -121,6 +144,12 @@ class DerivationCache {
   }
 }
 
+function isNamedIdentifier(place: Place): boolean {
+  return (
+    place.identifier.name !== null && place.identifier.name?.kind === 'named'
+  );
+}
+
 /**
  * Validates that useEffect is not used for derived computations which could/should
  * be performed in render.
@@ -172,33 +201,38 @@ export function validateNoDerivedComputationsInEffects_exp(
       if (param.kind === 'Identifier') {
         context.derivationCache.cache.set(param.identifier.id, {
           place: param,
-          sourcesIds: new Set([param.identifier.id]),
+          sourcesIds: new Set(
+            isNamedIdentifier(param) ? [param.identifier.id] : [],
+          ),
           typeOfValue: 'fromProps',
         });
-        context.derivationCache.hasChanges = true;
       }
     }
   } else if (fn.fnType === 'Component') {
     const props = fn.params[0];
     if (props != null && props.kind === 'Identifier') {
       context.derivationCache.cache.set(props.identifier.id, {
         place: props,
-        sourcesIds: new Set([props.identifier.id]),
+        sourcesIds: new Set(
+          isNamedIdentifier(props) ? [props.identifier.id] : [],
+        ),
         typeOfValue: 'fromProps',
       });
-      context.derivationCache.hasChanges = true;
     }
   }
 
   let isFirstPass = true;
   do {
+    context.derivationCache.takeSnapshot();
+
     for (const block of fn.body.blocks.values()) {
       recordPhiDerivations(block, context);
       for (const instr of block.instructions) {
         recordInstructionDerivations(instr, context, isFirstPass);
       }
     }
 
+    context.derivationCache.checkForChanges();
     isFirstPass = false;
   } while (context.derivationCache.snapshot());
 
@@ -262,6 +296,7 @@ function recordInstructionDerivations(
   if (value.kind === 'FunctionExpression') {
     context.functions.set(lvalue.identifier.id, value);
     for (const [, block] of value.loweredFunc.func.body.blocks) {
+      recordPhiDerivations(block, context);
       for (const instr of block.instructions) {
         recordInstructionDerivations(instr, context, isFirstPass);
       }
@@ -310,9 +345,7 @@ function recordInstructionDerivations(
     }
 
     typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue);
-    for (const id of operandMetadata.sourcesIds) {
-      sources.add(id);
-    }
+    sources.add(operand.identifier.id);
   }
 
   if (typeOfValue === 'ignored') {
@@ -331,11 +364,24 @@ function recordInstructionDerivations(
       case Effect.ConditionallyMutateIterator:
       case Effect.Mutate: {
         if (isMutable(instr, operand)) {
-          context.derivationCache.addDerivationEntry(
-            operand,
-            sources,
-            typeOfValue,
-          );
+          if (context.derivationCache.cache.has(operand.identifier.id)) {
+            const operandMetadata = context.derivationCache.cache.get(
+              operand.identifier.id,
+            );
+
+            if (operandMetadata !== undefined) {
+              operandMetadata.typeOfValue = joinValue(
+                typeOfValue,
+                operandMetadata.typeOfValue,
+              );
+            }
+          } else {
+            context.derivationCache.addDerivationEntry(
+              operand,
+              sources,
+              typeOfValue,
+            );
+          }
         }
         break;
       }
@@ -367,6 +413,68 @@ function recordInstructionDerivations(
   }
 }
 
+function buildDataFlowTree(
+  sourceId: IdentifierId,
+  indent: string = '',
+  isLast: boolean = true,
+  context: ValidationContext,
+  propsSet: Set<string>,
+  stateSet: Set<string>,
+): string {
+  const sourceMetadata = context.derivationCache.cache.get(sourceId);
+  if (!sourceMetadata || !sourceMetadata.place.identifier.name?.value) {
+    return '';
+  }
+
+  const sourceName = sourceMetadata.place.identifier.name.value;
+  const prefix = indent + (isLast ? '└── ' : '├── ');
+  const childIndent = indent + (isLast ? '    ' : '│   ');
+
+  const childSourceIds = Array.from(sourceMetadata.sourcesIds).filter(
+    id => id !== sourceId,
+  );
+
+  const isOriginal = childSourceIds.length === 0;
+
+  let result = `${prefix}${sourceName}`;
+
+  if (isOriginal) {
+    let typeLabel: string;
+    if (sourceMetadata.typeOfValue === 'fromProps') {
+      propsSet.add(sourceMetadata.place.identifier.name?.value);
+      typeLabel = 'Prop';
+    } else if (sourceMetadata.typeOfValue === 'fromState') {
+      stateSet.add(sourceMetadata.place.identifier.name?.value);
+      typeLabel = 'State';
+    } else {
+      propsSet.add(sourceMetadata.place.identifier.name?.value);
+      stateSet.add(sourceMetadata.place.identifier.name?.value);
+      typeLabel = 'Prop and State';
+    }
+    result += ` (${typeLabel})`;
+  }
+
+  if (childSourceIds.length > 0) {
+    result += '\n';
+    childSourceIds.forEach((childId, index) => {
+      const childTree = buildDataFlowTree(
+        childId,
+        childIndent,
+        index === childSourceIds.length - 1,
+        context,
+        propsSet,
+        stateSet,
+      );
+      if (childTree) {
+        result += childTree + '\n';
+      }
+    });
+    result = result.slice(0, -1);
+  }
+
+  return result;
+}
+
 function validateEffect(
   effectFunction: HIRFunction,
   context: ValidationContext,
@@ -469,27 +577,49 @@ function validateEffect(
           .length -
           1
     ) {
-      const derivedDepsStr = Array.from(derivedSetStateCall.sourceIds)
-        .map(sourceId => {
-          const sourceMetadata = context.derivationCache.cache.get(sourceId);
-          return sourceMetadata?.place.identifier.name?.value;
-        })
-        .filter(Boolean)
-        .join(', ');
-
-      let description;
-
-      if (derivedSetStateCall.typeOfValue === 'fromProps') {
-        description = `From props: [${derivedDepsStr}]`;
-      } else if (derivedSetStateCall.typeOfValue === 'fromState') {
-        description = `From local state: [${derivedDepsStr}]`;
-      } else {
-        description = `From props and local state: [${derivedDepsStr}]`;
+      const allSourceIds = Array.from(derivedSetStateCall.sourceIds);
+      const propsSet = new Set<string>();
+      const stateSet = new Set<string>();
+
+      const trees = allSourceIds
+        .map((id, index) =>
+          buildDataFlowTree(
+            id,
+            '',
+            index === allSourceIds.length - 1,
+            context,
+            propsSet,
+            stateSet,
+          ),
+        )
+        .filter(Boolean);
+
+      const propsArr = Array.from(propsSet);
+      const stateArr = Array.from(stateSet);
+
+      let rootSources = '';
+      if (propsArr.length > 0) {
+        rootSources += `Props: [${propsArr.join(', ')}]`;
+      }
+      if (stateArr.length > 0) {
+        if (rootSources) rootSources += '\n';
+        rootSources += `State: [${stateArr.join(', ')}]`;
       }
 
+      const description = `Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+${rootSources}
+
+Data Flow Tree:
+${trees.join('\n')}
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state`;
+
       context.errors.pushDiagnostic(
         CompilerDiagnostic.create({
-          description: `Derived values (${description}) 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`,
+          description: description,
           category: ErrorCategory.EffectDerivationsOfState,
           reason:
             'You might not need an effect. Derive values in render, not effects.',

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-conditionally-in-effect.expect.md

@@ -34,7 +34,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [value]) 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.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [value]
+
+Data Flow Tree:
+└── value (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-conditionally-in-effect.ts:9:6
    7 |   useEffect(() => {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/error.derived-state-from-default-props.expect.md

@@ -31,7 +31,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From props: [input]) 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.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+Props: [input]
+
+Data Flow Tree:
+└── input (Prop)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-from-default-props.ts:9:4
    7 |

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

@@ -28,7 +28,16 @@ Found 1 error:
 
 Error: You might not need an effect. Derive values in render, not effects.
 
-Derived values (From local state: [count]) 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.
+Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user
+
+This setState call is setting a derived value that depends on the following reactive sources:
+
+State: [count]
+
+Data Flow Tree:
+└── count (State)
+
+See: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state.
 
 error.derived-state-from-local-state-in-effect.ts:10:6
    8 |   useEffect(() => {