Further refine validation error messages and add tests

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

@@ -28,6 +28,7 @@ import {
   isUseStateType,
   GeneratedSource,
 } from '../HIR';
+import {printInstruction} from '../HIR/PrintHIR';
 import {
   eachInstructionOperand,
   eachTerminalOperand,
@@ -170,6 +171,22 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     }
   }
 
+  const compilerError = generateCompilerError(
+    setStateCalls,
+    effectSetStates,
+    errors,
+  );
+
+  if (compilerError.hasErrors()) {
+    throw compilerError;
+  }
+}
+
+function generateCompilerError(
+  setStateCalls: Map<SetStateName, Array<Place>>,
+  effectSetStates: Map<SetStateName, Array<Place>>,
+  errors: Array<ErrorMetadata>,
+): CompilerError {
   const throwableErrors = new CompilerError();
   for (const error of errors) {
     let compilerDiagnostic: CompilerDiagnostic | undefined = undefined;
@@ -193,20 +210,9 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
         effectSetStates.get(error.setStateName)?.length &&
       error.type !== 'fromState'
     ) {
-      compilerDiagnostic = CompilerDiagnostic.create({
-        description: `${error.description} 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.`,
-        category: `Local state shadows parent state.`,
-        severity: ErrorSeverity.InvalidReact,
-      }).withDetail({
-        kind: 'error',
-        loc: error.loc,
-        message: detailMessage,
-      });
-    } else {
       compilerDiagnostic = CompilerDiagnostic.create({
         description: `${error.description} This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.`,
-        category: `Derive values in render, not effects.`,
+        category: `Local state shadows parent state.`,
         severity: ErrorSeverity.InvalidReact,
       }).withDetail({
         kind: 'error',
@@ -218,14 +224,11 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
         if (setStateCallArray.length === 0) {
           continue;
         }
-        const otherCalls = setStateCalls.get(key);
-        if (otherCalls && otherCalls.length > 1) {
-          for (const place of otherCalls) {
-            if (
-              !setStateCallArray.some(
-                existing => JSON.stringify(existing) === JSON.stringify(place),
-              )
-            ) {
+
+        const nonUseEffectSetStateCalls = setStateCalls.get(key);
+        if (nonUseEffectSetStateCalls) {
+          for (const place of nonUseEffectSetStateCalls) {
+            if (!setStateCallArray.includes(place)) {
               compilerDiagnostic.withDetail({
                 kind: 'error',
                 loc: place.loc,
@@ -236,6 +239,16 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
           }
         }
       }
+    } else {
+      compilerDiagnostic = CompilerDiagnostic.create({
+        description: `${error.description} 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.`,
+        category: `Derive values in render, not effects.`,
+        severity: ErrorSeverity.InvalidReact,
+      }).withDetail({
+        kind: 'error',
+        loc: error.loc,
+        message: detailMessage,
+      });
     }
 
     if (compilerDiagnostic) {
@@ -243,9 +256,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     }
   }
 
-  if (throwableErrors.hasErrors()) {
-    throw throwableErrors;
-  }
+  return throwableErrors;
 }
 
 function joinValue(
@@ -440,7 +451,7 @@ function validateEffect(
   }
 
   if (!isUsingDerivedDeps) {
-    // no effect prop/state derived deps were used in the body
+    // no prop/state derived deps were used in the body of the effect
     return;
   }
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md

@@ -36,7 +36,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value both props and local state [prefix, name]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value both props and local state [prefix, name]. 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.
 
 error.bug-derived-state-from-mixed-deps.ts:9:4
    7 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md

@@ -0,0 +1,58 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useState, useEffect} from 'react';
+
+function Component({props, number}) {
+  const nothing = 0;
+  const missDirection = number;
+  const [displayValue, setDisplayValue] = useState('');
+
+  useEffect(() => {
+    setDisplayValue(props.prefix + missDirection + nothing);
+  }, [props.prefix, missDirection, nothing]);
+
+  return (
+    <div
+      onClick={() => {
+        setDisplayValue('clicked');
+      }}>
+      {displayValue}
+    </div>
+  );
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: Local state shadows parent state.
+
+This setState() appears to derive a value from props [props, number]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+
+error.derived-state-from-shadowed-props.ts:10:4
+   8 |
+   9 |   useEffect(() => {
+> 10 |     setDisplayValue(props.prefix + missDirection + nothing);
+     |     ^^^^^^^^^^^^^^^ This state value shadows a value passed as a prop.
+  11 |   }, [props.prefix, missDirection, nothing]);
+  12 |
+  13 |   return (
+
+error.derived-state-from-shadowed-props.ts:16:8
+  14 |     <div
+  15 |       onClick={() => {
+> 16 |         setDisplayValue('clicked');
+     |         ^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent
+  17 |       }}>
+  18 |       {displayValue}
+  19 |     </div>
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.js

@@ -0,0 +1,21 @@
+// @validateNoDerivedComputationsInEffects
+import {useState, useEffect} from 'react';
+
+function Component({props, number}) {
+  const nothing = 0;
+  const missDirection = number;
+  const [displayValue, setDisplayValue] = useState('');
+
+  useEffect(() => {
+    setDisplayValue(props.prefix + missDirection + nothing);
+  }, [props.prefix, missDirection, nothing]);
+
+  return (
+    <div
+      onClick={() => {
+        setDisplayValue('clicked');
+      }}>
+      {displayValue}
+    </div>
+  );
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-conditional.expect.md

@@ -34,7 +34,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value from props [value]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value from props [value]. 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.
 
 error.derived-state-with-conditional.ts:9:6
    7 |   useEffect(() => {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-with-side-effects.expect.md

@@ -32,7 +32,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value from props [value]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value from props [value]. 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.
 
 error.derived-state-with-side-effects.ts:9:4
    7 |   useEffect(() => {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md

@@ -26,7 +26,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value local state [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value local state [firstName, lastName]. 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.
 
 error.invalid-derived-computation-in-effect.ts:9:4
    7 |   const [fullName, setFullName] = useState('');

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-computed.expect.md

@@ -31,7 +31,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value from props [props]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value from props [props]. 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.
 
 error.invalid-derived-state-from-props-computed.ts:9:4
    7 |   useEffect(() => {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-destructured.expect.md

@@ -30,7 +30,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value from props [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value from props [firstName, lastName]. 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.
 
 error.invalid-derived-state-from-props-destructured.ts:8:4
    6 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-in-effect.expect.md

@@ -30,7 +30,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value from props [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value from props [firstName, lastName]. 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.
 
 error.invalid-derived-state-from-props-in-effect.ts:8:4
    6 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-props-with-default-value.expect.md

@@ -28,7 +28,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value from props [input]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value from props [input]. 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.
 
 error.invalid-derived-state-from-props-with-default-value.ts:9:4
    7 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md

@@ -38,7 +38,7 @@ Found 1 error:
 
 Error: Derive values in render, not effects.
 
-This setState() appears to derive a value local state [firstName, lastName]. This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.
+This setState() appears to derive a value local state [firstName, lastName]. 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.
 
 error.invalid-derived-state-from-state-in-effect.ts:10:4
    8 |