[compiler] Added validation for local state and refined error messages

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

@@ -34,13 +34,17 @@ import {
 import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
 import {assertExhaustive} from '../Utils/utils';
 
+// TODO: Maybe I can consolidate some types
 type SetStateCall = {
   loc: SourceLocation;
-  invalidDeps: Map<Identifier, Place[]> | undefined;
+  invalidDeps: DerivationMetadata;
   setStateId: IdentifierId;
 };
+
 type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState';
 
+type SetStateName = string | undefined | null;
+
 type DerivationMetadata = {
   typeOfValue: TypeOfValue;
   // TODO: Rename to place
@@ -50,10 +54,10 @@ type DerivationMetadata = {
 
 // TODO: This needs refining
 type ErrorMetadata = {
-  errorType: 'HoistState' | 'CalculateInRender';
-  propInfo: string | undefined;
+  errorType: TypeOfValue;
+  invalidDepInfo: string | undefined;
   loc: SourceLocation;
-  setStateId: IdentifierId;
+  setStateName: SetStateName;
 };
 
 function joinValue(
@@ -93,13 +97,13 @@ function updateDerivationMetadata(
 function parseInstr(
   instr: Instruction,
   derivedTuple: Map<IdentifierId, DerivationMetadata>,
-  setStateCalls: Map<string, Place>,
+  setStateCalls: Map<SetStateName, Place[]>,
 ) {
   // console.log(printInstruction(instr));
   // console.log(instr);
   let typeOfValue: TypeOfValue = 'ignored';
 
-  // If the instruction is destructuring a useState hook call
+  // 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' &&
@@ -115,20 +119,22 @@ function parseInstr(
     }
   }
 
-  // If the instruction is calling a setState
   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
+    instr.value.callee.loc !== GeneratedSource
   ) {
-    setStateCalls.set(
-      instr.value.callee.loc.identifierName,
-      instr.value.callee,
-    );
+    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: DerivationMetadata[] = [];
@@ -246,11 +252,8 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
   const locals: Map<IdentifierId, IdentifierId> = new Map();
   const derivedTuple: Map<IdentifierId, DerivationMetadata> = new Map();
 
-  // Investigating
-  const effectSetStates: Map<string, Place> = new Map();
-  const setStateCalls: Map<string, Place> = new Map();
-
-  // let shouldCalculateInRender: boolean = true;
+  const effectSetStates: Map<SetStateName, Place[]> = new Map();
+  const setStateCalls: Map<SetStateName, Place[]> = new Map();
 
   const errors: ErrorMetadata[] = [];
 
@@ -295,7 +298,6 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
         }
       }
 
-      // Maybe this should run for every instruction being parsed
       if (value.kind === 'LoadLocal') {
         locals.set(lvalue.identifier.id, value.place.identifier.id);
       } else if (value.kind === 'ArrayExpression') {
@@ -350,13 +352,30 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     }
   }
 
-  console.log('setStateCalls: ', setStateCalls);
-  console.log('effectSetStates: ', effectSetStates);
   const throwableErrors = new CompilerError();
   for (const error of errors) {
+    let reason;
+    let description = '';
+    // 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: `You may not need an effect. Values derived from state should be calculated in render, not in an effect. `,
-      description: `You are using a value derived from props${error.propInfo} to update local state in an effect.`,
+      reason: reason,
+      description: `You are using invalid dependencies: \n\n${error.invalidDepInfo}`,
       severity: ErrorSeverity.InvalidReact,
       loc: error.loc,
     });
@@ -371,7 +390,7 @@ function validateEffect(
   effectFunction: HIRFunction,
   effectDeps: Array<IdentifierId>,
   derivedTuple: Map<IdentifierId, DerivationMetadata>,
-  effectSetStates: Map<string, Place>,
+  effectSetStates: Map<SetStateName, Place[]>,
   errors: ErrorMetadata[],
 ): void {
   /*
@@ -391,7 +410,7 @@ function validateEffect(
     }
   }
 
-  // This might be wrong gotta double check
+  // TODO: This might be wrong gotta double check
   let hasInvalidDep = false;
   for (const dep of effectDeps) {
     const depMetadata = derivedTuple.get(dep);
@@ -445,10 +464,15 @@ function validateEffect(
         instr.value.callee.loc.identifierName !== undefined &&
         instr.value.callee.loc.identifierName !== null
       ) {
-        effectSetStates.set(
-          instr.value.callee.loc.identifierName,
-          instr.value.callee,
-        );
+        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':
@@ -488,23 +512,15 @@ function validateEffect(
             instr.value.args.length === 1 &&
             instr.value.args[0].kind === 'Identifier'
           ) {
-            const propSources = derivedTuple.get(
+            const invalidDeps = derivedTuple.get(
               instr.value.args[0].identifier.id,
             );
 
-            if (propSources !== undefined) {
+            if (invalidDeps !== undefined) {
               setStateCallsInEffect.push({
                 loc: instr.value.callee.loc,
                 setStateId: instr.value.callee.identifier.id,
-                invalidDeps: new Map([
-                  [instr.value.args[0].identifier, propSources.sources],
-                ]),
-              });
-            } else {
-              setStateCallsInEffect.push({
-                loc: instr.value.callee.loc,
-                setStateId: instr.value.callee.identifier.id,
-                invalidDeps: undefined,
+                invalidDeps: invalidDeps,
               });
             }
           }
@@ -525,37 +541,34 @@ function validateEffect(
     seenBlocks.add(block.id);
   }
 
-  // need to track if the setState call has been used elsewhere
-  // if it is then the solution should be to lift the state up to the parent component
-  // if not the solution should be to calculate the value in rende
-  //
-  // If the same setState is used both inside and outside the effect
-
   for (const call of setStateCallsInEffect) {
-    if (call.invalidDeps != null) {
-      let propNames = '';
-      for (const [, places] of call.invalidDeps.entries()) {
-        const placeNames = places
-          .map(place => place.identifier.name?.value)
-          .join(', ');
-        propNames += `[${placeNames}], `;
-      }
-      propNames = propNames.slice(0, -2);
-      const propInfo = propNames ? ` (from props '${propNames}')` : '';
+    const placeNames = call.invalidDeps.sources
+      .map(place => place.identifier.name?.value)
+      .join(', ');
 
-      errors.push({
-        errorType: 'HoistState',
-        propInfo: propInfo,
-        loc: call.loc,
-        setStateId: call.setStateId,
-      });
-    } else {
-      errors.push({
-        errorType: 'CalculateInRender',
-        propInfo: undefined,
-        loc: call.loc,
-        setStateId: call.setStateId,
-      });
+    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}`
+        : '';
     }
+
+    errors.push({
+      errorType: call.invalidDeps.typeOfValue,
+      invalidDepInfo: invalidDepInfo,
+      loc: call.loc,
+      setStateName:
+        call.loc !== GeneratedSource ? call.loc.identifierName : undefined,
+    });
   }
 }