[compiler] Validate against deriving data in an effect instead of render

Validates against cases described in https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state

ghstack-source-id: 3f34ddf
Pull Request resolved: #32285

Author: josephsavona

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

@@ -91,7 +91,6 @@ import {
 } from '../Validation';
 import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLocalsNotReassignedAfterRender';
 import {outlineFunctions} from '../Optimization/OutlineFunctions';
-import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes';
 import {lowerContextAccess} from '../Optimization/LowerContextAccess';
 import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects';
 import {validateNoJSXInTryStatement} from '../Validation/ValidateNoJSXInTryStatement';
@@ -100,6 +99,7 @@ import {outlineJSX} from '../Optimization/OutlineJsx';
 import {optimizePropsMethodCalls} from '../Optimization/OptimizePropsMethodCalls';
 import {transformFire} from '../Transform';
 import {validateNoImpureFunctionsInRender} from '../Validation/ValiateNoImpureFunctionsInRender';
+import {validateNoDerivedComputationsInEffects} from '../Validation/ValidateNoDerivedComputationsInEffects';
 
 export type CompilerPipelineValue =
   | {kind: 'ast'; name: string; value: CodegenFunction}
@@ -251,6 +251,7 @@ function runWithEnvironment(
     validateNoSetStateInRender(hir);
   }
 
+  validateNoDerivedComputationsInEffects(hir);
   if (env.config.validateNoSetStateInPassiveEffects) {
     validateNoSetStateInPassiveEffects(hir);
   }

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

@@ -0,0 +1,213 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import {CompilerError, ErrorSeverity, SourceLocation} from '..';
+import {
+  ArrayExpression,
+  BlockId,
+  FunctionExpression,
+  HIRFunction,
+  IdentifierId,
+  isSetStateType,
+  isUseEffectHookType,
+} from '../HIR';
+import {
+  eachInstructionValueOperand,
+  eachTerminalOperand,
+} from '../HIR/visitors';
+
+/**
+ * Validates that useEffect is not used for derived computations which could/should
+ * be performed in render.
+ *
+ * See https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state
+ *
+ * Example:
+ *
+ * ```
+ * // 🔴 Avoid: redundant state and unnecessary Effect
+ * const [fullName, setFullName] = useState('');
+ * useEffect(() => {
+ *   setFullName(firstName + ' ' + lastName);
+ * }, [firstName, lastName]);
+ * ```
+ *
+ * Instead use:
+ *
+ * ```
+ * // ✅ Good: calculated during rendering
+ * const fullName = firstName + ' ' + lastName;
+ * ```
+ */
+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 errors = new CompilerError();
+
+  for (const block of fn.body.blocks.values()) {
+    for (const instr of block.instructions) {
+      const {lvalue, value} = instr;
+      if (value.kind === 'LoadLocal') {
+        locals.set(lvalue.identifier.id, value.place.identifier.id);
+      } else if (value.kind === 'ArrayExpression') {
+        candidateDependencies.set(lvalue.identifier.id, value);
+      } else if (value.kind === 'FunctionExpression') {
+        functions.set(lvalue.identifier.id, value);
+      } else if (
+        value.kind === 'CallExpression' ||
+        value.kind === 'MethodCall'
+      ) {
+        const callee =
+          value.kind === 'CallExpression' ? value.callee : value.property;
+        if (
+          isUseEffectHookType(callee.identifier) &&
+          value.args.length === 2 &&
+          value.args[0].kind === 'Identifier' &&
+          value.args[1].kind === 'Identifier'
+        ) {
+          const effectFunction = functions.get(value.args[0].identifier.id);
+          const deps = candidateDependencies.get(value.args[1].identifier.id);
+          if (
+            effectFunction != null &&
+            deps != null &&
+            deps.elements.length !== 0 &&
+            deps.elements.every(element => element.kind === 'Identifier')
+          ) {
+            const dependencies: Array<IdentifierId> = deps.elements.map(dep => {
+              CompilerError.invariant(dep.kind === 'Identifier', {
+                reason: `Dependency is checked as a place above`,
+                loc: value.loc,
+              });
+              return locals.get(dep.identifier.id) ?? dep.identifier.id;
+            });
+            validateEffect(
+              effectFunction.loweredFunc.func,
+              dependencies,
+              errors,
+            );
+          }
+        }
+      }
+    }
+  }
+  if (errors.hasErrors()) {
+    throw errors;
+  }
+}
+
+function validateEffect(
+  effectFunction: HIRFunction,
+  effectDeps: Array<IdentifierId>,
+  errors: CompilerError,
+): void {
+  for (const operand of effectFunction.context) {
+    if (isSetStateType(operand.identifier)) {
+      continue;
+    } else if (effectDeps.find(dep => dep === operand.identifier.id) != null) {
+      continue;
+    } else {
+      // Captured something other than the effect dep or setState
+      return;
+    }
+  }
+  for (const dep of effectDeps) {
+    if (
+      effectFunction.context.find(operand => operand.identifier.id === dep) ==
+      null
+    ) {
+      // effect dep wasn't actually used in the function
+      return;
+    }
+  }
+
+  const seenBlocks: Set<BlockId> = new Set();
+  const values: Map<IdentifierId, Array<IdentifierId>> = new Map();
+  for (const dep of effectDeps) {
+    values.set(dep, [dep]);
+  }
+
+  const setStateLocations: Array<SourceLocation> = [];
+  for (const block of effectFunction.body.blocks.values()) {
+    for (const pred of block.preds) {
+      if (!seenBlocks.has(pred)) {
+        // skip if block has a back edge
+        return;
+      }
+    }
+    for (const instr of block.instructions) {
+      switch (instr.value.kind) {
+        case 'LoadLocal': {
+          const deps = values.get(instr.value.place.identifier.id);
+          if (deps != null) {
+            values.set(instr.lvalue.identifier.id, deps);
+          }
+          break;
+        }
+        case 'Primitive':
+        case 'JSXText':
+        case 'LoadGlobal':
+        case 'BinaryExpression':
+        case 'TemplateLiteral': {
+          const aggregateDeps: Set<IdentifierId> = new Set();
+          for (const operand of eachInstructionValueOperand(instr.value)) {
+            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));
+          }
+          break;
+        }
+        case 'CallExpression': {
+          if (
+            isSetStateType(instr.value.callee.identifier) &&
+            instr.value.args.length === 1 &&
+            instr.value.args[0].kind === 'Identifier'
+          ) {
+            const deps = values.get(instr.value.args[0].identifier.id);
+            if (deps != null && new Set(deps).size === effectDeps.length) {
+              setStateLocations.push(instr.value.callee.loc);
+            } else {
+              // doesn't depend on any deps
+              return;
+            }
+          } else {
+            // some other call
+            return;
+          }
+          break;
+        }
+        default: {
+          return;
+        }
+      }
+    }
+    for (const operand of eachTerminalOperand(block.terminal)) {
+      if (values.has(operand.identifier.id)) {
+        return;
+      }
+    }
+    seenBlocks.add(block.id);
+  }
+
+  for (const loc of setStateLocations) {
+    errors.push({
+      reason:
+        'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)',
+      description: null,
+      severity: ErrorSeverity.InvalidReact,
+      loc,
+      suggestions: null,
+    });
+  }
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.expect.md

@@ -0,0 +1,33 @@
+
+## Input
+
+```javascript
+function Form() {
+  const [firstName, setFirstName] = useState('Taylor');
+  const [lastName, setLastName] = useState('Swift');
+
+  // 🔴 Avoid: redundant state and unnecessary Effect
+  const [fullName, setFullName] = useState('');
+  useEffect(() => {
+    setFullName(firstName + ' ' + lastName);
+  }, [firstName, lastName]);
+
+  return <div>{fullName}</div>;
+}
+
+```
+
+
+## Error
+
+```
+   6 |   const [fullName, setFullName] = useState('');
+   7 |   useEffect(() => {
+>  8 |     setFullName(firstName + ' ' + lastName);
+     |     ^^^^^^^^^^^ InvalidReact: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state) (8:8)
+   9 |   }, [firstName, lastName]);
+  10 |
+  11 |   return <div>{fullName}</div>;
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-validate-no-derived-computations-in-effects.js

@@ -0,0 +1,12 @@
+function Form() {
+  const [firstName, setFirstName] = useState('Taylor');
+  const [lastName, setLastName] = useState('Swift');
+
+  // 🔴 Avoid: redundant state and unnecessary Effect
+  const [fullName, setFullName] = useState('');
+  useEffect(() => {
+    setFullName(firstName + ' ' + lastName);
+  }, [firstName, lastName]);
+
+  return <div>{fullName}</div>;
+}