[Compiler] Don't count a setState in the dependency array of the effect it is called on as a usage

Author: jorge-cab

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

@@ -22,6 +22,7 @@ import {
   BasicBlock,
   isUseRefType,
   SourceLocation,
+  ArrayExpression,
 } from '../HIR';
 import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors';
 import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
@@ -36,11 +37,17 @@ type DerivationMetadata = {
   isStateSource: boolean;
 };
 
+type EffectMetadata = {
+  effect: HIRFunction;
+  dependencies: ArrayExpression;
+};
+
 type ValidationContext = {
   readonly functions: Map<IdentifierId, FunctionExpression>;
+  readonly candidateDependencies: Map<IdentifierId, ArrayExpression>;
   readonly errors: CompilerError;
   readonly derivationCache: DerivationCache;
-  readonly effects: Set<HIRFunction>;
+  readonly effectsCache: Map<IdentifierId, EffectMetadata>;
   readonly setStateLoads: Map<IdentifierId, IdentifierId | null>;
   readonly setStateUsages: Map<IdentifierId, Set<SourceLocation>>;
 };
@@ -175,18 +182,20 @@ export function validateNoDerivedComputationsInEffects_exp(
   fn: HIRFunction,
 ): Result<void, CompilerError> {
   const functions: Map<IdentifierId, FunctionExpression> = new Map();
+  const candidateDependencies: Map<IdentifierId, ArrayExpression> = new Map();
   const derivationCache = new DerivationCache();
   const errors = new CompilerError();
-  const effects: Set<HIRFunction> = new Set();
+  const effectsCache: Map<IdentifierId, EffectMetadata> = new Map();
 
   const setStateLoads: Map<IdentifierId, IdentifierId> = new Map();
   const setStateUsages: Map<IdentifierId, Set<SourceLocation>> = new Map();
 
   const context: ValidationContext = {
     functions,
+    candidateDependencies,
     errors,
     derivationCache,
-    effects,
+    effectsCache,
     setStateLoads,
     setStateUsages,
   };
@@ -229,8 +238,8 @@ export function validateNoDerivedComputationsInEffects_exp(
     isFirstPass = false;
   } while (context.derivationCache.snapshot());
 
-  for (const effect of effects) {
-    validateEffect(effect, context);
+  for (const [, effect] of effectsCache) {
+    validateEffect(effect.effect, effect.dependencies, context);
   }
 
   return errors.asResult();
@@ -354,8 +363,14 @@ function recordInstructionDerivations(
       value.args[1].kind === 'Identifier'
     ) {
       const effectFunction = context.functions.get(value.args[0].identifier.id);
-      if (effectFunction != null) {
-        context.effects.add(effectFunction.loweredFunc.func);
+      const deps = context.candidateDependencies.get(
+        value.args[1].identifier.id,
+      );
+      if (effectFunction != null && deps != null) {
+        context.effectsCache.set(value.args[0].identifier.id, {
+          effect: effectFunction.loweredFunc.func,
+          dependencies: deps,
+        });
       }
     } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) {
       typeOfValue = 'fromState';
@@ -367,6 +382,8 @@ function recordInstructionDerivations(
       );
       return;
     }
+  } else if (value.kind === 'ArrayExpression') {
+    context.candidateDependencies.set(lvalue.identifier.id, value);
   }
 
   for (const operand of eachInstructionOperand(instr)) {
@@ -596,6 +613,7 @@ function getFnLocalDeps(
 
 function validateEffect(
   effectFunction: HIRFunction,
+  dependencies: ArrayExpression,
   context: ValidationContext,
 ): void {
   const seenBlocks: Set<BlockId> = new Set();
@@ -612,6 +630,16 @@ function validateEffect(
     Set<SourceLocation>
   > = new Map();
 
+  // Consider setStates in the effect's dependency array as being part of effectSetStateUsages
+  for (const dep of dependencies.elements) {
+    if (dep.kind === 'Identifier') {
+      const usage = context.setStateUsages.get(dep.identifier.id);
+      if (usage !== undefined) {
+        effectSetStateUsages.set(dep.identifier.id, usage);
+      }
+    }
+  }
+
   let cleanUpFunctionDeps: Set<IdentifierId> | undefined;
 
   const globals: Set<IdentifierId> = new Set();

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md

@@ -0,0 +1,58 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects_exp
+
+function Component({ prop }) {
+
+  const [s, setS] = useState(0)
+  useEffect(() => {
+    setS(prop)
+  }, [prop, setS]);
+
+  return (<div>{prop}</div>)
+}
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp
+
+function Component(t0) {
+  const $ = _c(5);
+  const { prop } = t0;
+
+  const [, setS] = useState(0);
+  let t1;
+  let t2;
+  if ($[0] !== prop) {
+    t1 = () => {
+      setS(prop);
+    };
+    t2 = [prop, setS];
+    $[0] = prop;
+    $[1] = t1;
+    $[2] = t2;
+  } else {
+    t1 = $[1];
+    t2 = $[2];
+  }
+  useEffect(t1, t2);
+  let t3;
+  if ($[3] !== prop) {
+    t3 = <div>{prop}</div>;
+    $[3] = prop;
+    $[4] = t3;
+  } else {
+    t3 = $[4];
+  }
+  return t3;
+}
+
+```
+      
+### Eval output
+(kind: exception) Fixture not implemented

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.js

@@ -0,0 +1,11 @@
+// @validateNoDerivedComputationsInEffects_exp
+
+function Component({ prop }) {
+
+  const [s, setS] = useState(0)
+  useEffect(() => {
+    setS(prop)
+  }, [prop, setS]);
+
+  return (<div>{prop}</div>)
+}