Improve error messages and update tests

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

@@ -5,7 +5,15 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {CompilerError, Effect, ErrorSeverity, SourceLocation} from '..';
+import {
+  CompilerDiagnostic,
+  CompilerDiagnosticDetail,
+  CompilerError,
+  CompilerErrorDetail,
+  Effect,
+  ErrorSeverity,
+  SourceLocation,
+} from '..';
 import {
   ArrayExpression,
   BasicBlock,
@@ -46,8 +54,8 @@ type DerivationMetadata = {
 };
 
 type ErrorMetadata = {
-  errorType: TypeOfValue;
-  invalidDepInfo: string | undefined;
+  type: TypeOfValue;
+  description: string | undefined;
   loc: SourceLocation;
   setStateName: SetStateName;
 };
@@ -164,7 +172,18 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
 
   const throwableErrors = new CompilerError();
   for (const error of errors) {
-    let reason;
+    let compilerDiagnostic: CompilerDiagnostic | undefined = undefined;
+    let detailMessage = '';
+    switch (error.type) {
+      case 'fromProps':
+        detailMessage = 'This state value shadows a value passed as a prop.';
+        break;
+      case 'fromPropsOrState':
+        detailMessage =
+          'This state value shadows a value passed as a prop or a value from state.';
+        break;
+    }
+
     /*
      * If we use a setState from an invalid useEffect elsewhere then we probably have to
      * hoist state up, else we should calculate in render
@@ -172,21 +191,56 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     if (
       setStateCalls.get(error.setStateName)?.length !=
         effectSetStates.get(error.setStateName)?.length &&
-      error.errorType !== 'fromState'
+      error.type !== '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)';
+      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 {
-      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)';
+      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.`,
+        severity: ErrorSeverity.InvalidReact,
+      }).withDetail({
+        kind: 'error',
+        loc: error.loc,
+        message: detailMessage,
+      });
+
+      for (const [key, setStateCallArray] of effectSetStates) {
+        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),
+              )
+            ) {
+              compilerDiagnostic.withDetail({
+                kind: 'error',
+                loc: place.loc,
+                message:
+                  'this setState updates the shadowed state, but should call an onChange event from the parent',
+              });
+            }
+          }
+        }
+      }
     }
 
-    throwableErrors.push({
-      reason: reason,
-      description: `You are using invalid dependencies:\n\n${error.invalidDepInfo}`,
-      severity: ErrorSeverity.InvalidReact,
-      loc: error.loc,
-    });
+    if (compilerDiagnostic) {
+      throwableErrors.pushDiagnostic(compilerDiagnostic);
+    }
   }
 
   if (throwableErrors.hasErrors()) {
@@ -473,31 +527,31 @@ function validateEffect(
       .join(', ');
 
     let sourceNames = '';
-    let invalidDepInfo = '';
+    let errorDescription = '';
 
     if (call.invalidDeps.typeOfValue === 'fromProps') {
       sourceNames += `[${placeNames}], `;
       sourceNames = sourceNames.slice(0, -2);
-      invalidDepInfo = sourceNames
-        ? `Invalid deps from props ${sourceNames}`
+      errorDescription = sourceNames
+        ? `This setState() appears to derive a value from props ${sourceNames}.`
         : '';
     } else if (call.invalidDeps.typeOfValue === 'fromState') {
       sourceNames += `[${placeNames}], `;
       sourceNames = sourceNames.slice(0, -2);
-      invalidDepInfo = sourceNames
-        ? `Invalid deps from local state: ${sourceNames}`
+      errorDescription = sourceNames
+        ? `This setState() appears to derive a value local state ${sourceNames}.`
         : '';
     } else {
       sourceNames += `[${placeNames}], `;
       sourceNames = sourceNames.slice(0, -2);
-      invalidDepInfo = sourceNames
-        ? `Invalid deps from both props and local state: ${sourceNames}`
+      errorDescription = sourceNames
+        ? `This setState() appears to derive a value both props and local state ${sourceNames}.`
         : '';
     }
 
     errors.push({
-      errorType: call.invalidDeps.typeOfValue,
-      invalidDepInfo: invalidDepInfo,
+      type: call.invalidDeps.typeOfValue,
+      description: errorDescription,
       loc: call.loc,
       setStateName:
         call.loc !== GeneratedSource ? call.loc.identifierName : undefined,

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

@@ -34,17 +34,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from both props and local state: [prefix, name].
+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.
 
 error.bug-derived-state-from-mixed-deps.ts:9:4
    7 |
    8 |   useEffect(() => {
 >  9 |     setDisplayName(prefix + name);
-     |     ^^^^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^^^^ This state value shadows a value passed as a prop or a value from state.
   10 |   }, [prefix, name]);
   11 |
   12 |   return (

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

@@ -32,17 +32,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from props [value].
+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.
 
 error.derived-state-with-conditional.ts:9:6
    7 |   useEffect(() => {
    8 |     if (enabled) {
 >  9 |       setLocalValue(value);
-     |       ^^^^^^^^^^^^^ 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)
+     |       ^^^^^^^^^^^^^ This state value shadows a value passed as a prop.
   10 |     } else {
   11 |       setLocalValue('disabled');
   12 |     }

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

@@ -30,17 +30,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from props [value].
+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.
 
 error.derived-state-with-side-effects.ts:9:4
    7 |   useEffect(() => {
    8 |     console.log('Value changed:', value);
 >  9 |     setLocalValue(value);
-     |     ^^^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^^^ This state value shadows a value passed as a prop.
   10 |     document.title = `Value: ${value}`;
   11 |   }, [value]);
   12 |

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

@@ -24,17 +24,15 @@ function BadExample() {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from local state: [firstName, lastName].
+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.
 
 error.invalid-derived-computation-in-effect.ts:9:4
    7 |   const [fullName, setFullName] = useState('');
    8 |   useEffect(() => {
 >  9 |     setFullName(capitalize(firstName + ' ' + lastName));
-     |     ^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^
   10 |   }, [firstName, lastName]);
   11 |
   12 |   return <div>{fullName}</div>;

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

@@ -29,17 +29,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from props [props].
+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.
 
 error.invalid-derived-state-from-props-computed.ts:9:4
    7 |   useEffect(() => {
    8 |     const computed = props.prefix + props.value + props.suffix;
 >  9 |     setDisplayValue(computed);
-     |     ^^^^^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^^^^^ This state value shadows a value passed as a prop.
   10 |   }, [props.prefix, props.value, props.suffix]);
   11 |
   12 |   return <div>{displayValue}</div>;

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

@@ -28,17 +28,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from props [firstName, lastName].
+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.
 
 error.invalid-derived-state-from-props-destructured.ts:8:4
    6 |
    7 |   useEffect(() => {
 >  8 |     setFullName(firstName + ' ' + lastName);
-     |     ^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^ This state value shadows a value passed as a prop.
    9 |   }, [firstName, lastName]);
   10 |
   11 |   return <div>{fullName}</div>;

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

@@ -28,17 +28,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from props [firstName, lastName].
+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.
 
 error.invalid-derived-state-from-props-in-effect.ts:8:4
    6 |
    7 |   useEffect(() => {
 >  8 |     setFullName(firstName + ' ' + lastName);
-     |     ^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^ This state value shadows a value passed as a prop.
    9 |   }, [firstName, lastName]);
   10 |
   11 |   return <div>{fullName}</div>;

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

@@ -26,17 +26,15 @@ export default function InProductLobbyGeminiCard(
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from props [input].
+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.
 
 error.invalid-derived-state-from-props-with-default-value.ts:9:4
    7 |
    8 |   useEffect(() => {
 >  9 |     setCurrInput(input)
-     |     ^^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^^ This state value shadows a value passed as a prop.
   10 |   }, [input]);
   11 |
   12 |   return (

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

@@ -36,17 +36,15 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: 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)
+Error: Derive values in render, not effects.
 
-You are using invalid dependencies:
-
-Invalid deps from local state: [firstName, lastName].
+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.
 
 error.invalid-derived-state-from-state-in-effect.ts:10:4
    8 |
    9 |   useEffect(() => {
 > 10 |     setFullName(firstName + ' ' + lastName);
-     |     ^^^^^^^^^^^ 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)
+     |     ^^^^^^^^^^^
   11 |   }, [firstName, lastName]);
   12 |
   13 |   return (