[compiler] Implement ValidateNoDerivedComputationsInEffects for calculate in render solvable cases

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

@@ -520,7 +520,7 @@ function printErrorSummary(category: ErrorCategory, message: string): string {
     case ErrorCategory.AutomaticEffectDependencies:
     case ErrorCategory.CapitalizedCalls:
     case ErrorCategory.Config:
-    case ErrorCategory.EffectDerivationsOfState:
+    case ErrorCategory.EffectStateDerivationCalculateInRender:
     case ErrorCategory.EffectSetState:
     case ErrorCategory.ErrorBoundaries:
     case ErrorCategory.Factories:
@@ -614,7 +614,10 @@ export enum ErrorCategory {
    * Checks for no setState in effect bodies
    */
   EffectSetState = 'EffectSetState',
-  EffectDerivationsOfState = 'EffectDerivationsOfState',
+  /**
+   * Checks for no deriving state in effects, solved by calculate in render
+   */
+  EffectStateDerivationCalculateInRender = 'EffectStateDerivationCalculateInRender',
   /**
    * Validates against try/catch in place of error boundaries
    */
@@ -751,11 +754,11 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule {
         recommended: false,
       };
     }
-    case ErrorCategory.EffectDerivationsOfState: {
+    case ErrorCategory.EffectStateDerivationCalculateInRender: {
       return {
         category,
         severity: ErrorSeverity.Error,
-        name: 'no-deriving-state-in-effects',
+        name: 'no-deriving-state-in-effects-calculate-in-render',
         description:
           'Validates against deriving values from state in an effect',
         recommended: false,

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

@@ -5,142 +5,296 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {CompilerError, SourceLocation} from '..';
-import {ErrorCategory} from '../CompilerError';
 import {
-  ArrayExpression,
-  BlockId,
-  FunctionExpression,
+  BasicBlock,
+  GeneratedSource,
   HIRFunction,
   IdentifierId,
+  Instruction,
   isSetStateType,
+  Place,
+  isUseStateType,
+  Effect,
   isUseEffectHookType,
+  FunctionExpression,
+  BlockId,
+  SourceLocation,
+  CallExpression,
 } from '../HIR';
+import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors';
+import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
 import {
-  eachInstructionValueOperand,
-  eachTerminalOperand,
-} from '../HIR/visitors';
+  CompilerDiagnostic,
+  CompilerError,
+  ErrorCategory,
+} from '../CompilerError';
+import {assertExhaustive} from '../Utils/utils';
+
+type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsAndState';
+
+type DerivationMetadata = {
+  typeOfValue: TypeOfValue;
+  place: Place;
+  sourcesIds: Set<IdentifierId>;
+};
+
+type DerivationCache = Map<IdentifierId, DerivationMetadata>;
+
+type SetStateCallCache = Map<string | undefined | null, Array<Place>>;
+
+type FunctionExpressionsCache = Map<IdentifierId, FunctionExpression>;
+
+type DerivedSetStateCall = {
+  value: CallExpression;
+  sourceIds: Set<IdentifierId>;
+};
+
+type ErrorMetadata = {
+  derivedComputationDetails: string;
+  loc: SourceLocation;
+};
+
+const DERIVE_IN_RENDER_REASON =
+  'You might net need an effect. Derive values in render, not effects.';
+
+const DERIVE_IN_RENDER_DETAIL_MESSAGE =
+  'This should be computed during render, not in an effect';
+
+const DERIVE_IN_RENDER_DESCRIPTION =
+  'State 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';
 
 /**
  * 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 derivationCache: DerivationCache = new Map();
+  const setStateCallCache: SetStateCallCache = new Map();
+  const effectSetStateCache: SetStateCallCache = new Map();
+  const functionExpressionsCache: FunctionExpressionsCache = new Map();
 
-  const errors = new CompilerError();
+  const stateDerivationErrors: Array<ErrorMetadata> = [];
+
+  parseFNParameters(fn, derivationCache);
 
   for (const block of fn.body.blocks.values()) {
+    parseBlockPhi(block, derivationCache);
+
     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'
+      parseInstr(
+        instr,
+        derivationCache,
+        setStateCallCache,
+        effectSetStateCache,
+        functionExpressionsCache,
+        stateDerivationErrors,
+      );
+    }
+  }
+
+  const compilerError = generateCompilerErrors(stateDerivationErrors);
+
+  if (compilerError.hasErrors()) {
+    throw compilerError;
+  }
+}
+
+function parseFNParameters(fn: HIRFunction, derivationCache: DerivationCache) {
+  if (fn.fnType === 'Hook') {
+    for (const param of fn.params) {
+      if (param.kind === 'Identifier') {
+        derivationCache.set(param.identifier.id, {
+          place: param,
+          sourcesIds: new Set([param.identifier.id]),
+          typeOfValue: 'fromProps',
+        });
+      }
+    }
+  } else if (fn.fnType === 'Component') {
+    const props = fn.params[0];
+    if (props != null && props.kind === 'Identifier') {
+      derivationCache.set(props.identifier.id, {
+        place: props,
+        sourcesIds: new Set([props.identifier.id]),
+        typeOfValue: 'fromProps',
+      });
+    }
+  }
+}
+
+function parseBlockPhi(
+  block: BasicBlock,
+  derivationCache: DerivationCache,
+): void {
+  for (const phi of block.phis) {
+    let typeOfValue: TypeOfValue = 'ignored';
+    let sourcesIds: Set<IdentifierId> = new Set();
+    for (const operand of phi.operands.values()) {
+      const operandMetadata = derivationCache.get(operand.identifier.id);
+
+      if (operandMetadata === undefined) {
+        continue;
+      }
+
+      typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue);
+      sourcesIds.add(operand.identifier.id);
+    }
+
+    if (typeOfValue !== 'ignored') {
+      addDerivationEntry(phi.place, sourcesIds, typeOfValue, derivationCache);
+    }
+  }
+}
+
+function joinValue(
+  lvalueType: TypeOfValue,
+  valueType: TypeOfValue,
+): TypeOfValue {
+  if (lvalueType === 'ignored') return valueType;
+  if (valueType === 'ignored') return lvalueType;
+  if (lvalueType === valueType) return lvalueType;
+  return 'fromPropsAndState';
+}
+
+function addDerivationEntry(
+  derivedVar: Place,
+  sourcesIds: Set<IdentifierId>,
+  typeOfValue: TypeOfValue,
+  derivationCache: DerivationCache,
+): void {
+  let newValue: DerivationMetadata = {
+    place: derivedVar,
+    sourcesIds: new Set(),
+    typeOfValue: typeOfValue ?? 'ignored',
+  };
+
+  if (sourcesIds !== undefined) {
+    for (const id of sourcesIds) {
+      const sourcePlace = derivationCache.get(id)?.place;
+
+      if (sourcePlace === undefined) {
+        continue;
+      }
+
+      /*
+       * 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'
       ) {
-        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`,
-                description: null,
-                details: [
-                  {
-                    kind: 'error',
-                    loc: value.loc,
-                    message: 'this is checked as a place above',
-                  },
-                ],
-              });
-              return locals.get(dep.identifier.id) ?? dep.identifier.id;
-            });
-            validateEffect(
-              effectFunction.loweredFunc.func,
-              dependencies,
-              errors,
-            );
-          }
-        }
+        newValue.sourcesIds.add(derivedVar.identifier.id);
+      } else {
+        newValue.sourcesIds.add(sourcePlace.identifier.id);
       }
     }
   }
-  if (errors.hasAnyErrors()) {
-    throw errors;
+
+  derivationCache.set(derivedVar.identifier.id, newValue);
+}
+
+function parseInstr(
+  instr: Instruction,
+  derivationCache: DerivationCache,
+  setStateCallCache: SetStateCallCache,
+  effectSetStateCache: SetStateCallCache,
+  functionExpressionsCache: FunctionExpressionsCache,
+  stateDerivationErrors: Array<ErrorMetadata>,
+): void {
+  const {value, lvalue} = instr;
+
+  let typeOfValue: TypeOfValue = 'ignored';
+  const sources: Set<IdentifierId> = new Set();
+
+  // Recursively parse function expressions
+  if (value.kind === 'FunctionExpression') {
+    for (const [, block] of value.loweredFunc.func.body.blocks) {
+      for (const instr of block.instructions) {
+        functionExpressionsCache.set(lvalue.identifier.id, value);
+
+        parseInstr(
+          instr,
+          derivationCache,
+          setStateCallCache,
+          effectSetStateCache,
+          functionExpressionsCache,
+          stateDerivationErrors,
+        );
+      }
+    }
+  }
+  // Record setState calls
+  else if (
+    value.kind === 'CallExpression' &&
+    isSetStateType(value.callee.identifier)
+  ) {
+    addSetStateCallEntry(value.callee, setStateCallCache);
+  } else if (value.kind === 'CallExpression' || value.kind === 'MethodCall') {
+    const callee =
+      value.kind === 'CallExpression' ? value.callee : value.property;
+
+    // Handle values derived from useState calls
+    if (isUseStateType(lvalue.identifier)) {
+      const stateValueSource = value.args[0];
+      if (stateValueSource.kind === 'Identifier') {
+        sources.add(stateValueSource.identifier.id);
+      }
+      typeOfValue = joinValue(typeOfValue, 'fromState');
+    }
+    // Validate useEffect calls
+    else if (
+      isUseEffectHookType(callee.identifier) &&
+      value.args.length === 2 &&
+      value.args[0].kind === 'Identifier' &&
+      value.args[1].kind === 'Identifier'
+    ) {
+      const effectFunction = functionExpressionsCache.get(
+        value.args[0].identifier.id,
+      );
+
+      validateEffect(
+        effectFunction?.loweredFunc.func,
+        effectSetStateCache,
+        derivationCache,
+        stateDerivationErrors,
+      );
+    }
+  }
+
+  parseOperands(instr, derivationCache, typeOfValue, sources);
+}
+
+function addSetStateCallEntry(
+  callee: Place,
+  setStateCallCache: SetStateCallCache,
+) {
+  if (callee.loc === GeneratedSource) {
+    return;
+  }
+
+  if (setStateCallCache.has(callee.loc.identifierName)) {
+    setStateCallCache.get(callee.loc.identifierName)!.push(callee);
+  } else {
+    setStateCallCache.set(callee.loc.identifierName, [callee]);
   }
 }
 
 function validateEffect(
-  effectFunction: HIRFunction,
-  effectDeps: Array<IdentifierId>,
-  errors: CompilerError,
+  effectFunction: HIRFunction | undefined,
+  effectSetStateCache: SetStateCallCache,
+  derivationCache: DerivationCache,
+  stateDerivationErrors: Array<ErrorMetadata>,
 ): 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;
-    }
+  if (effectFunction === undefined) {
+    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 effectDerivedSetStateCalls: Array<DerivedSetStateCall> = [];
 
-  const setStateLocations: Array<SourceLocation> = [];
   for (const block of effectFunction.body.blocks.values()) {
     for (const pred of block.preds) {
       if (!seenBlocks.has(pred)) {
@@ -148,91 +302,183 @@ function validateEffect(
         return;
       }
     }
-    for (const phi of block.phis) {
-      const aggregateDeps: Set<IdentifierId> = new Set();
-      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);
-          }
-        }
-      }
-      if (aggregateDeps.size !== 0) {
-        values.set(phi.place.identifier.id, Array.from(aggregateDeps));
-      }
-    }
     for (const instr of block.instructions) {
-      switch (instr.value.kind) {
-        case 'Primitive':
-        case 'JSXText':
-        case 'LoadGlobal': {
-          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':
-        case 'PropertyLoad':
-        case 'BinaryExpression':
-        case 'TemplateLiteral':
-        case 'CallExpression':
-        case 'MethodCall': {
-          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));
-          }
+      const {value} = instr;
+      if (
+        value.kind === 'CallExpression' &&
+        isSetStateType(value.callee.identifier) &&
+        value.args.length === 1 &&
+        value.args[0].kind === 'Identifier'
+      ) {
+        addSetStateCallEntry(value.callee, effectSetStateCache);
+        const argMetadata = derivationCache.get(value.args[0].identifier.id);
 
-          if (
-            instr.value.kind === 'CallExpression' &&
-            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;
-            }
-          }
-          break;
-        }
-        default: {
-          return;
+        if (argMetadata !== undefined) {
+          effectDerivedSetStateCalls.push({
+            value: value,
+            sourceIds: argMetadata.sourcesIds,
+          });
         }
       }
     }
-    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({
-      category: ErrorCategory.EffectDerivationsOfState,
-      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,
-      loc,
-      suggestions: null,
-    });
+  generateDerivedComputationDetails(
+    effectDerivedSetStateCalls,
+    derivationCache,
+    stateDerivationErrors,
+  );
+}
+
+function generateDerivedComputationDetails(
+  effectDerivedSetStateCalls: Array<DerivedSetStateCall>,
+  derivationCache: DerivationCache,
+  stateDerivationErrors: Array<ErrorMetadata>,
+) {
+  console.log(derivationCache);
+  for (const derivedCall of effectDerivedSetStateCalls) {
+    const arg = derivedCall.value.args[0];
+    if (arg.kind === 'Identifier') {
+      const argMetadata = derivationCache.get(arg.identifier.id);
+      if (argMetadata !== undefined) {
+        const derivationSources: Array<string> = [];
+
+        for (const sourceId of argMetadata.sourcesIds) {
+          const sourceMetadata = derivationCache.get(sourceId);
+          if (sourceMetadata !== undefined) {
+            const sourceName =
+              sourceMetadata.place.identifier.name?.value ||
+              `identifier_${sourceId}`;
+            derivationSources.push(sourceName);
+          }
+        }
+
+        let derivationType: string;
+        switch (argMetadata.typeOfValue) {
+          case 'fromProps':
+            derivationType = 'props';
+            break;
+          case 'fromState':
+            derivationType = 'local state';
+            break;
+          case 'fromPropsAndState':
+            derivationType = 'local state and props';
+            break;
+          default:
+            derivationType = 'unknown source';
+            break;
+        }
+
+        const sourcesList =
+          derivationSources.length > 0
+            ? ` [${derivationSources.join(', ')}]`
+            : '';
+
+        const formattedDetails = `State is being derived from ${derivationType}${sourcesList}`;
+
+        stateDerivationErrors.push({
+          derivedComputationDetails: formattedDetails,
+          loc: derivedCall.value.loc,
+        });
+      }
+    }
   }
 }
+
+function parseOperands(
+  instr: Instruction,
+  derivationCache: DerivationCache,
+  typeOfValue: TypeOfValue,
+  sourceIds: Set<IdentifierId>,
+) {
+  for (const operand of eachInstructionOperand(instr)) {
+    const operandMetadata = derivationCache.get(operand.identifier.id);
+
+    if (operandMetadata === undefined) {
+      continue;
+    }
+
+    typeOfValue = joinValue(typeOfValue, operandMetadata.typeOfValue);
+    for (const id of operandMetadata.sourcesIds) {
+      sourceIds.add(id);
+    }
+  }
+
+  if (typeOfValue === 'ignored') {
+    return;
+  }
+
+  propagateTypeOfValue(instr, sourceIds, typeOfValue, derivationCache);
+}
+
+function propagateTypeOfValue(
+  instr: Instruction,
+  sourceIds: Set<IdentifierId>,
+  typeOfValue: TypeOfValue,
+  derivationCache: DerivationCache,
+): void {
+  for (const lvalue of eachInstructionLValue(instr)) {
+    addDerivationEntry(lvalue, sourceIds, 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)) {
+          addDerivationEntry(operand, sourceIds, 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 generateCompilerErrors(stateDerivationErrors: Array<ErrorMetadata>) {
+  const throwableErrors = new CompilerError();
+  for (const e of stateDerivationErrors) {
+    throwableErrors.pushDiagnostic(
+      CompilerDiagnostic.create({
+        description:
+          DERIVE_IN_RENDER_DESCRIPTION + `\n\n${e.derivedComputationDetails}`,
+        category: ErrorCategory.EffectStateDerivationCalculateInRender,
+        reason: DERIVE_IN_RENDER_REASON,
+      }).withDetails({
+        kind: 'error',
+        loc: e.loc,
+        message: DERIVE_IN_RENDER_DETAIL_MESSAGE,
+      }),
+    );
+  }
+
+  return throwableErrors;
+}