[compiler] First functional disambiguated single line validation of no derived computations in effects

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

@@ -5,29 +5,217 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {CompilerError, SourceLocation} from '..';
+import {CompilerError, Effect, ErrorSeverity, SourceLocation} from '..';
 import {ErrorCategory} from '../CompilerError';
 import {
   ArrayExpression,
+  BasicBlock,
   BlockId,
   FunctionExpression,
   HIRFunction,
   IdentifierId,
+  Instruction,
   Place,
   isSetStateType,
   isUseEffectHookType,
+  isUseStateType,
+  GeneratedSource,
 } from '../HIR';
-import {printInstruction, printPlace} from '../HIR/PrintHIR';
 import {
-  eachInstructionValueOperand,
+  eachInstructionOperand,
   eachTerminalOperand,
+  eachInstructionLValue,
 } from '../HIR/visitors';
+import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
+import {assertExhaustive} from '../Utils/utils';
 
+// TODO: Maybe I can consolidate some types
 type SetStateCall = {
   loc: SourceLocation;
-  propsSource: Place | null; // null means state-derived, non-null means props-derived
+  invalidDeps: DerivationMetadata;
+  setStateId: IdentifierId;
 };
 
+type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState';
+
+type SetStateName = string | undefined | null;
+
+type DerivationMetadata = {
+  typeOfValue: TypeOfValue;
+  place: Place;
+  sources: Array<Place>;
+};
+
+type ErrorMetadata = {
+  errorType: TypeOfValue;
+  invalidDepInfo: string | undefined;
+  loc: SourceLocation;
+  setStateName: SetStateName;
+};
+
+function joinValue(
+  lvalueType: TypeOfValue,
+  valueType: TypeOfValue,
+): TypeOfValue {
+  if (lvalueType === 'ignored') return valueType;
+  if (valueType === 'ignored') return lvalueType;
+  if (lvalueType === valueType) return lvalueType;
+  return 'fromPropsOrState';
+}
+
+function updateDerivationMetadata(
+  target: Place,
+  sources: Array<DerivationMetadata>,
+  typeOfValue: TypeOfValue,
+  derivedTuple: Map<IdentifierId, DerivationMetadata>,
+): void {
+  let newValue: DerivationMetadata = {
+    place: 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') {
+      newValue.sources.push(target);
+    } else {
+      newValue.sources.push(...source.sources);
+    }
+  }
+  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.
@@ -55,96 +243,54 @@ 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 derivedFromProps: Map<IdentifierId, Place> = new Map();
+  const derivedTuple: Map<IdentifierId, DerivationMetadata> = new Map();
 
-  const errors = new CompilerError();
+  const effectSetStates: Map<SetStateName, Array<Place>> = new Map();
+  const setStateCalls: Map<SetStateName, Array<Place>> = new Map();
+
+  const errors: Array<ErrorMetadata> = [];
 
   if (fn.fnType === 'Hook') {
     for (const param of fn.params) {
       if (param.kind === 'Identifier') {
-        derivedFromProps.set(param.identifier.id, param);
+        derivedTuple.set(param.identifier.id, {
+          place: param,
+          sources: [param],
+          typeOfValue: 'fromProps',
+        });
       }
     }
   } else if (fn.fnType === 'Component') {
     const props = fn.params[0];
     if (props != null && props.kind === 'Identifier') {
-      derivedFromProps.set(props.identifier.id, props);
+      derivedTuple.set(props.identifier.id, {
+        place: props,
+        sources: [props],
+        typeOfValue: 'fromProps',
+      });
     }
   }
 
   for (const block of fn.body.blocks.values()) {
+    parseBlockPhi(block, derivedTuple);
+
     for (const instr of block.instructions) {
       const {lvalue, value} = instr;
 
-      // Track props derivation through instruction effects
-      if (instr.effects != null) {
-        for (const effect of instr.effects) {
-          switch (effect.kind) {
-            case 'Assign':
-            case 'Alias':
-            case 'MaybeAlias':
-            case 'Capture': {
-              const source = derivedFromProps.get(effect.from.identifier.id);
-              if (source != null) {
-                derivedFromProps.set(effect.into.identifier.id, source);
-              }
-              break;
-            }
-          }
-        }
-      }
+      parseInstr(instr, derivedTuple, setStateCalls);
 
-      /**
-       * TODO: figure out why property access off of props does not create an Assign or Alias/Maybe
-       * Alias
-       *
-       * import {useEffect, useState} from 'react'
-       *
-       *        function Component(props) {
-       *          const [displayValue, setDisplayValue] = useState('');
-       *
-       *          useEffect(() => {
-       *            const computed = props.prefix + props.value + props.suffix;
-       *                             ^^^^^^^^^^^^   ^^^^^^^^^^^   ^^^^^^^^^^^^
-       *                             we want to track that these are from props
-       *            setDisplayValue(computed);
-       *          }, [props.prefix, props.value, props.suffix]);
-       *
-       *          return <div>{displayValue}</div>;
-       *        }
+      /*
+       * 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) {
-            if (instr.effects != null) {
-              console.group(printInstruction(instr));
-              for (const effect of instr.effects) {
-                console.log(effect);
-                switch (effect.kind) {
-                  case 'Assign':
-                  case 'Alias':
-                  case 'MaybeAlias':
-                  case 'Capture': {
-                    const source = derivedFromProps.get(
-                      effect.from.identifier.id,
-                    );
-                    if (source != null) {
-                      derivedFromProps.set(effect.into.identifier.id, source);
-                    }
-                    break;
-                  }
-                }
-              }
-            }
-            console.groupEnd();
+            parseInstr(instr, derivedTuple, setStateCalls);
           }
         }
       }
 
-      for (const [, place] of derivedFromProps) {
-        console.log(printPlace(place));
-      }
-
       if (value.kind === 'LoadLocal') {
         locals.set(lvalue.identifier.id, value.place.identifier.id);
       } else if (value.kind === 'ArrayExpression') {
@@ -157,6 +303,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
       ) {
         const callee =
           value.kind === 'CallExpression' ? value.callee : value.property;
+
         if (
           isUseEffectHookType(callee.identifier) &&
           value.args.length === 2 &&
@@ -188,7 +335,8 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
             validateEffect(
               effectFunction.loweredFunc.func,
               dependencies,
-              derivedFromProps,
+              derivedTuple,
+              effectSetStates,
               errors,
             );
           }
@@ -196,57 +344,98 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
       }
     }
   }
-  if (errors.hasAnyErrors()) {
-    throw errors;
+
+  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;
   }
 }
 
 function validateEffect(
   effectFunction: HIRFunction,
   effectDeps: Array<IdentifierId>,
-  derivedFromProps: Map<IdentifierId, Place>,
-  errors: CompilerError,
+  derivedTuple: Map<IdentifierId, DerivationMetadata>,
+  effectSetStates: Map<SetStateName, Array<Place>>,
+  errors: Array<ErrorMetadata>,
 ): 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;
     } else if (effectDeps.find(dep => dep === operand.identifier.id) != null) {
       continue;
-    } else if (derivedFromProps.has(operand.identifier.id)) {
+    } else if (derivedTuple.has(operand.identifier.id)) {
       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
+  let hasInvalidDep = false;
   for (const dep of effectDeps) {
-    console.log({dep});
+    const depMetadata = derivedTuple.get(dep);
     if (
-      effectFunction.context.find(operand => operand.identifier.id === dep) ==
+      effectFunction.context.find(operand => operand.identifier.id === dep) !=
         null ||
-      derivedFromProps.has(dep) === false
+      (depMetadata !== undefined && depMetadata.typeOfValue !== 'ignored')
     ) {
-      console.log('early return 2');
-      // effect dep wasn't actually used in the function
-      return;
+      hasInvalidDep = true;
     }
   }
 
+  if (!hasInvalidDep) {
+    console.log('early return 2');
+    // effect dep wasn't actually used in the function
+    return;
+  }
+
   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 effectDerivedFromProps: Map<IdentifierId, Place> = new Map();
+  const effectInvalidlyDerived: Map<IdentifierId, DerivationMetadata> =
+    new Map();
 
   for (const dep of effectDeps) {
-    console.log({dep});
     values.set(dep, [dep]);
-    const propsSource = derivedFromProps.get(dep);
-    if (propsSource != null) {
-      effectDerivedFromProps.set(dep, propsSource);
+    const depMetadata = derivedTuple.get(dep);
+    if (depMetadata !== undefined) {
+      effectInvalidlyDerived.set(dep, depMetadata);
     }
   }
 
-  const setStateCalls: Array<SetStateCall> = [];
+  const setStateCallsInEffect: Array<SetStateCall> = [];
   for (const block of effectFunction.body.blocks.values()) {
     for (const pred of block.preds) {
       if (!seenBlocks.has(pred)) {
@@ -254,31 +443,29 @@ function validateEffect(
         return;
       }
     }
-    for (const phi of block.phis) {
-      const aggregateDeps: Set<IdentifierId> = new Set();
-      let propsSource: Place | null = null;
 
-      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 source = effectDerivedFromProps.get(operand.identifier.id);
-        if (source != null) {
-          propsSource = source;
-        }
-      }
+    parseBlockPhi(block, effectInvalidlyDerived);
 
-      if (aggregateDeps.size !== 0) {
-        values.set(phi.place.identifier.id, Array.from(aggregateDeps));
-      }
-      if (propsSource != null) {
-        effectDerivedFromProps.set(phi.place.identifier.id, propsSource);
-      }
-    }
     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,
+          ]);
+        }
+      }
       switch (instr.value.kind) {
         case 'Primitive':
         case 'JSXText':
@@ -299,7 +486,7 @@ function validateEffect(
         case 'CallExpression':
         case 'MethodCall': {
           const aggregateDeps: Set<IdentifierId> = new Set();
-          for (const operand of eachInstructionValueOperand(instr.value)) {
+          for (const operand of eachInstructionOperand(instr)) {
             const deps = values.get(operand.identifier.id);
             if (deps != null) {
               for (const dep of deps) {
@@ -317,80 +504,69 @@ function validateEffect(
             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) {
-              const propsSource = effectDerivedFromProps.get(
-                instr.value.args[0].identifier.id,
-              );
+            const invalidDeps = derivedTuple.get(
+              instr.value.args[0].identifier.id,
+            );
 
-              setStateCalls.push({
+            if (invalidDeps !== undefined) {
+              setStateCallsInEffect.push({
                 loc: instr.value.callee.loc,
-                propsSource: propsSource ?? null,
+                setStateId: instr.value.callee.identifier.id,
+                invalidDeps: invalidDeps,
               });
-            } else {
-              // doesn't depend on all deps
-              return;
             }
           }
           break;
         }
         default: {
+          console.log('early return 4');
           return;
         }
       }
-
-      // Track props derivation through instruction effects
-      if (instr.effects != null) {
-        for (const effect of instr.effects) {
-          switch (effect.kind) {
-            case 'Assign':
-            case 'Alias':
-            case 'MaybeAlias':
-            case 'Capture': {
-              const source = effectDerivedFromProps.get(
-                effect.from.identifier.id,
-              );
-              if (source != null) {
-                effectDerivedFromProps.set(effect.into.identifier.id, source);
-              }
-              break;
-            }
-          }
-        }
-      }
     }
+
     for (const operand of eachTerminalOperand(block.terminal)) {
       if (values.has(operand.identifier.id)) {
-        //
         return;
       }
     }
     seenBlocks.add(block.id);
   }
 
-  for (const call of setStateCalls) {
-    if (call.propsSource != null) {
-      const propName = call.propsSource.identifier.name?.value;
-      const propInfo = propName != null ? ` (from prop '${propName}')` : '';
+  for (const call of setStateCallsInEffect) {
+    const placeNames = call.invalidDeps.sources
+      .map(place => place.identifier.name?.value)
+      .join(', ');
 
-      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,
-      });
+    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}`
+        : '';
     } else {
-      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,
-      });
+      sourceNames += `[${placeNames}], `;
+      sourceNames = sourceNames.slice(0, -2);
+      invalidDepInfo = sourceNames
+        ? `Invalid deps from both props and local state: ${sourceNames}`
+        : '';
     }
+
+    errors.push({
+      errorType: call.invalidDeps.typeOfValue,
+      invalidDepInfo: invalidDepInfo,
+      loc: call.loc,
+      setStateName:
+        call.loc !== GeneratedSource ? call.loc.identifierName : undefined,
+    });
   }
 }