[compiler] Bail out from validation if there is a ref in the effect

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

@@ -520,7 +520,8 @@ function printErrorSummary(category: ErrorCategory, message: string): string {
     case ErrorCategory.AutomaticEffectDependencies:
     case ErrorCategory.CapitalizedCalls:
     case ErrorCategory.Config:
-    case ErrorCategory.EffectDerivationsOfState:
+    case ErrorCategory.EffectDerivationDeriveInRender:
+    case ErrorCategory.EffectDerivationShadowingParentState:
     case ErrorCategory.EffectSetState:
     case ErrorCategory.ErrorBoundaries:
     case ErrorCategory.Factories:
@@ -614,7 +615,14 @@ export enum ErrorCategory {
    * Checks for no setState in effect bodies
    */
   EffectSetState = 'EffectSetState',
-  EffectDerivationsOfState = 'EffectDerivationsOfState',
+  /**
+   * Checks for derived state in effects that could be calculated in render
+   */
+  EffectDerivationDeriveInRender = 'EffectDerivationDeriveInRender',
+  /**
+   * Checks for derived state in effects that could be hoisted to parent
+   */
+  EffectDerivationShadowingParentState = 'EffectDerivationShadowingParentState',
   /**
    * Validates against try/catch in place of error boundaries
    */
@@ -751,13 +759,23 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule {
         recommended: false,
       };
     }
-    case ErrorCategory.EffectDerivationsOfState: {
+    case ErrorCategory.EffectDerivationDeriveInRender: {
       return {
         category,
         severity: ErrorSeverity.Error,
-        name: 'no-deriving-state-in-effects',
+        name: 'effect-derive-in-render',
         description:
-          'Validates against deriving values from state in an effect',
+          'Validates if a useEffect is deriving state from props and/or local state that could be calculated in render.',
+        recommended: false,
+      };
+    }
+    case ErrorCategory.EffectDerivationShadowingParentState: {
+      return {
+        category,
+        severity: ErrorSeverity.Error,
+        name: 'effect-shadow-parent-state',
+        description:
+          'Validates if a useEffect is deriving state from parent state and if the component is updating the shadowed state locally.',
         recommended: false,
       };
     }

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

@@ -5,13 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {
-  CompilerDiagnostic,
-  CompilerError,
-  Effect,
-  ErrorSeverity,
-  SourceLocation,
-} from '..';
+import {CompilerDiagnostic, CompilerError, Effect, SourceLocation} from '..';
 import {ErrorCategory} from '../CompilerError';
 import {
   ArrayExpression,
@@ -25,6 +19,7 @@ import {
   isSetStateType,
   isUseEffectHookType,
   isUseStateType,
+  isUseRefType,
   GeneratedSource,
 } from '../HIR';
 import {eachInstructionOperand, eachInstructionLValue} from '../HIR/visitors';
@@ -42,7 +37,7 @@ type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState';
 type DerivationMetadata = {
   typeOfValue: TypeOfValue;
   place: Place;
-  sources: Set<Place>;
+  sources: Array<Place>;
 };
 
 type ErrorMetadata = {
@@ -50,6 +45,7 @@ type ErrorMetadata = {
   description: string | undefined;
   loc: SourceLocation;
   setStateName: string | undefined | null;
+  derivedDepsNames: Array<string>;
 };
 
 /**
@@ -80,6 +76,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
   const functions: Map<IdentifierId, FunctionExpression> = new Map();
   const locals: Map<IdentifierId, IdentifierId> = new Map();
   const derivationCache: Map<IdentifierId, DerivationMetadata> = new Map();
+  const shadowingUseState: Map<string, Array<SourceLocation>> = new Map();
 
   const effectSetStates: Map<
     string | undefined | null,
@@ -94,7 +91,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
       if (param.kind === 'Identifier') {
         derivationCache.set(param.identifier.id, {
           place: param,
-          sources: new Set([param]),
+          sources: [param],
           typeOfValue: 'fromProps',
         });
       }
@@ -104,7 +101,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     if (props != null && props.kind === 'Identifier') {
       derivationCache.set(props.identifier.id, {
         place: props,
-        sources: new Set([props]),
+        sources: [props],
         typeOfValue: 'fromProps',
       });
     }
@@ -116,7 +113,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
     for (const instr of block.instructions) {
       const {lvalue, value} = instr;
 
-      parseInstr(instr, derivationCache, setStateCalls);
+      parseInstr(instr, derivationCache, setStateCalls, shadowingUseState);
 
       if (value.kind === 'LoadLocal') {
         locals.set(lvalue.identifier.id, value.place.identifier.id);
@@ -175,6 +172,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
   const compilerError = generateCompilerError(
     setStateCalls,
     effectSetStates,
+    shadowingUseState,
     errors,
   );
 
@@ -186,21 +184,12 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
 function generateCompilerError(
   setStateCalls: Map<string | undefined | null, Array<Place>>,
   effectSetStates: Map<string | undefined | null, Array<Place>>,
+  shadowingUseState: Map<string, Array<SourceLocation>>,
   errors: Array<ErrorMetadata>,
 ): CompilerError {
   const throwableErrors = new CompilerError();
   for (const error of errors) {
     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
@@ -212,15 +201,28 @@ function generateCompilerError(
       error.type !== 'fromState'
     ) {
       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: `Local state shadows parent state.`,
-        severity: ErrorSeverity.InvalidReact,
-      }).withDetail({
+        description: `The setState within a useEffect is deriving from ${error.description}. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render`,
+        category: ErrorCategory.EffectDerivationShadowingParentState,
+        reason:
+          'You might not need an effect. Local state shadows parent state.',
+      }).withDetails({
         kind: 'error',
         loc: error.loc,
-        message: 'this setState synchronizes the state',
+        message: `this derives values from props ${error.type === 'fromPropsOrState' ? 'and local state ' : ''}to synchronize state`,
       });
 
+      for (const derivedDep of error.derivedDepsNames) {
+        if (shadowingUseState.has(derivedDep)) {
+          for (const loc of shadowingUseState.get(derivedDep)!) {
+            compilerDiagnostic.withDetails({
+              kind: 'error',
+              loc: loc,
+              message: `this useState shadows ${derivedDep}`,
+            });
+          }
+        }
+      }
+
       for (const [key, setStateCallArray] of effectSetStates) {
         if (setStateCallArray.length === 0) {
           continue;
@@ -230,7 +232,7 @@ function generateCompilerError(
         if (nonUseEffectSetStateCalls) {
           for (const place of nonUseEffectSetStateCalls) {
             if (!setStateCallArray.includes(place)) {
-              compilerDiagnostic.withDetail({
+              compilerDiagnostic.withDetails({
                 kind: 'error',
                 loc: place.loc,
                 message:
@@ -242,10 +244,11 @@ function generateCompilerError(
       }
     } 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({
+        description: `${error.description ? error.description.charAt(0).toUpperCase() + error.description.slice(1) : ''}. 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: ErrorCategory.EffectDerivationDeriveInRender,
+        reason:
+          'You might not need an effect. Derive values in render, not effects.',
+      }).withDetails({
         kind: 'error',
         loc: error.loc,
         message: 'This should be computed during render, not in an effect',
@@ -278,7 +281,7 @@ function updateDerivationMetadata(
 ): void {
   let newValue: DerivationMetadata = {
     place: target,
-    sources: new Set(),
+    sources: [],
     typeOfValue: typeOfValue ?? 'ignored',
   };
 
@@ -293,9 +296,9 @@ function updateDerivationMetadata(
           place.identifier.name === null ||
           place.identifier.name?.kind === 'promoted'
         ) {
-          newValue.sources.add(target);
+          newValue.sources.push(target);
         } else {
-          newValue.sources.add(place);
+          newValue.sources.push(place);
         }
       }
     }
@@ -308,38 +311,19 @@ function parseInstr(
   instr: Instruction,
   derivationCache: Map<IdentifierId, DerivationMetadata>,
   setStateCalls: Map<string | undefined | null, Array<Place>>,
+  shadowingUseState: Map<string, Array<SourceLocation>>,
 ): void {
   // Recursively parse function expressions
+  let typeOfValue: TypeOfValue = 'ignored';
+
+  let sources: Array<DerivationMetadata> = [];
   if (instr.value.kind === 'FunctionExpression') {
     for (const [, block] of instr.value.loweredFunc.func.body.blocks) {
       for (const instr of block.instructions) {
-        parseInstr(instr, derivationCache, setStateCalls);
+        parseInstr(instr, derivationCache, setStateCalls, shadowingUseState);
       }
     }
-  }
-
-  let typeOfValue: TypeOfValue = 'ignored';
-
-  // Catch any useState hook calls
-  let sources: Array<DerivationMetadata> = [];
-  if (
-    instr.value.kind === 'Destructure' &&
-    instr.value.lvalue.pattern.kind === 'ArrayPattern' &&
-    isUseStateType(instr.value.value.identifier)
-  ) {
-    typeOfValue = 'fromState';
-
-    const stateValueSource = instr.value.lvalue.pattern.items[0];
-    if (stateValueSource.kind === 'Identifier') {
-      sources.push({
-        place: stateValueSource,
-        typeOfValue: typeOfValue,
-        sources: new Set([stateValueSource]),
-      });
-    }
-  }
-
-  if (
+  } else if (
     instr.value.kind === 'CallExpression' &&
     isSetStateType(instr.value.callee.identifier) &&
     instr.value.args.length === 1 &&
@@ -355,6 +339,22 @@ function parseInstr(
         instr.value.callee,
       ]);
     }
+  } else if (
+    (instr.value.kind === 'CallExpression' ||
+      instr.value.kind === 'MethodCall') &&
+    isUseStateType(instr.lvalue.identifier) &&
+    instr.value.args.length > 0
+  ) {
+    const stateValueSource = instr.value.args[0];
+    if (stateValueSource.kind === 'Identifier') {
+      sources.push({
+        place: stateValueSource,
+        typeOfValue: typeOfValue,
+        sources: [stateValueSource],
+      });
+    }
+
+    typeOfValue = joinValue(typeOfValue, 'fromState');
   }
 
   for (const operand of eachInstructionOperand(instr)) {
@@ -365,6 +365,27 @@ function parseInstr(
 
     typeOfValue = joinValue(typeOfValue, opSource.typeOfValue);
     sources.push(opSource);
+
+    if (
+      (instr.value.kind === 'CallExpression' ||
+        instr.value.kind === 'MethodCall') &&
+      opSource.typeOfValue === 'fromProps' &&
+      isUseStateType(instr.lvalue.identifier)
+    ) {
+      opSource.sources.forEach(source => {
+        if (source.identifier.name !== null) {
+          if (shadowingUseState.has(source.identifier.name.value)) {
+            shadowingUseState
+              .get(source.identifier.name.value)
+              ?.push(instr.lvalue.loc);
+          } else {
+            shadowingUseState.set(source.identifier.name.value, [
+              instr.lvalue.loc,
+            ]);
+          }
+        }
+      });
+    }
   }
 
   if (typeOfValue !== 'ignored') {
@@ -398,8 +419,13 @@ function parseInstr(
           CompilerError.invariant(false, {
             reason: 'Unexpected unknown effect',
             description: null,
-            loc: operand.loc,
-            suggestions: null,
+            details: [
+              {
+                kind: 'error',
+                loc: operand.loc,
+                message: 'Unexpected unknown effect',
+              },
+            ],
           });
         }
         default: {
@@ -418,16 +444,26 @@ function parseBlockPhi(
   derivationCache: Map<IdentifierId, DerivationMetadata>,
 ): void {
   for (const phi of block.phis) {
+    let typeOfValue: TypeOfValue = 'ignored';
+    let sources: Array<DerivationMetadata> = [];
     for (const operand of phi.operands.values()) {
-      const phiSource = derivationCache.get(operand.identifier.id);
-      if (phiSource !== undefined) {
-        updateDerivationMetadata(
-          phi.place,
-          [phiSource],
-          phiSource?.typeOfValue,
-          derivationCache,
-        );
+      const opSource = derivationCache.get(operand.identifier.id);
+
+      if (opSource === undefined) {
+        continue;
       }
+
+      typeOfValue = joinValue(typeOfValue, opSource?.typeOfValue ?? 'ignored');
+      sources.push(opSource);
+    }
+
+    if (typeOfValue !== 'ignored') {
+      updateDerivationMetadata(
+        phi.place,
+        sources,
+        typeOfValue,
+        derivationCache,
+      );
     }
   }
 }
@@ -470,6 +506,11 @@ function validateEffect(
     parseBlockPhi(block, derivationCache);
 
     for (const instr of block.instructions) {
+      // Early return if any instruction is deriving a value from a ref
+      if (isUseRefType(instr.lvalue.identifier)) {
+        return;
+      }
+
       if (
         instr.value.kind === 'CallExpression' &&
         isSetStateType(instr.value.callee.identifier) &&
@@ -531,7 +572,7 @@ function validateEffect(
   }
 
   for (const call of derivedSetStateCall) {
-    const placeNames = Array.from(call.derivedDep.sources)
+    const derivedDepsStr = Array.from(call.derivedDep.sources)
       .map(place => {
         return place.identifier.name?.value;
       })
@@ -541,19 +582,24 @@ function validateEffect(
     let errorDescription = '';
 
     if (call.derivedDep.typeOfValue === 'fromProps') {
-      errorDescription = `props [${placeNames}].`;
+      errorDescription = `props [${derivedDepsStr}]`;
     } else if (call.derivedDep.typeOfValue === 'fromState') {
-      errorDescription = `local state [${placeNames}].`;
+      errorDescription = `local state [${derivedDepsStr}]`;
     } else {
-      errorDescription = `both props and local state [${placeNames}].`;
+      errorDescription = `both props and local state [${derivedDepsStr}]`;
     }
 
     errors.push({
       type: call.derivedDep.typeOfValue,
-      description: `This setState() appears to derive a value from ${errorDescription}`,
+      description: `${errorDescription}`,
       loc: call.loc,
       setStateName:
         call.loc !== GeneratedSource ? call.loc.identifierName : undefined,
+      derivedDepsNames: Array.from(call.derivedDep.sources)
+        .map(place => {
+          return place.identifier.name?.value ?? '';
+        })
+        .filter(Boolean),
     });
   }
 }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-from-ref-and-state-no-error.expect.md

@@ -0,0 +1,73 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    setLocal(myRef.current + test);
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects
+import { useEffect, useState, useRef } from "react";
+
+export default function Component(t0) {
+  const $ = _c(5);
+  const { test } = t0;
+  const [local, setLocal] = useState("");
+
+  const myRef = useRef(null);
+  let t1;
+  let t2;
+  if ($[0] !== test) {
+    t1 = () => {
+      setLocal(myRef.current + test);
+    };
+    t2 = [test];
+    $[0] = test;
+    $[1] = t1;
+    $[2] = t2;
+  } else {
+    t1 = $[1];
+    t2 = $[2];
+  }
+  useEffect(t1, t2);
+  let t3;
+  if ($[3] !== local) {
+    t3 = <>{local}</>;
+    $[3] = local;
+    $[4] = t3;
+  } else {
+    t3 = $[4];
+  }
+  return t3;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ test: "testString" }],
+};
+
+```
+      
+### Eval output
+(kind: ok) nulltestString

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/derived-state-from-ref-and-state-no-error.js

@@ -0,0 +1,19 @@
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    setLocal(myRef.current + test);
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};

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

@@ -34,9 +34,9 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-This setState() appears to derive a value from 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.
+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

@@ -8,7 +8,9 @@ import {useState, useEffect} from 'react';
 function Component({props, number}) {
   const nothing = 0;
   const missDirection = number;
-  const [displayValue, setDisplayValue] = useState(props.prefix + missDirection + nothing);
+  const [displayValue, setDisplayValue] = useState(
+    props.prefix + missDirection + nothing
+  );
 
   useEffect(() => {
     setDisplayValue(props.prefix + missDirection + nothing);
@@ -32,27 +34,53 @@ function Component({props, number}) {
 ```
 Found 1 error:
 
-Error: Local state shadows parent state.
+Error: You might not need an effect. 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.
+The setState within a useEffect is deriving from props [props, number]. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render.
 
-error.derived-state-from-shadowed-props.ts:10:4
-   8 |
-   9 |   useEffect(() => {
-> 10 |     setDisplayValue(props.prefix + missDirection + nothing);
-     |     ^^^^^^^^^^^^^^^ this setState synchronizes the state
-  11 |   }, [props.prefix, missDirection, nothing]);
-  12 |
-  13 |   return (
+error.derived-state-from-shadowed-props.ts:12:4
+  10 |
+  11 |   useEffect(() => {
+> 12 |     setDisplayValue(props.prefix + missDirection + nothing);
+     |     ^^^^^^^^^^^^^^^ this derives values from props to synchronize state
+  13 |   }, [props.prefix, missDirection, nothing]);
+  14 |
+  15 |   return (
 
-error.derived-state-from-shadowed-props.ts:16:8
-  14 |     <div
-  15 |       onClick={() => {
-> 16 |         setDisplayValue('clicked');
+error.derived-state-from-shadowed-props.ts:7:42
+   5 |   const nothing = 0;
+   6 |   const missDirection = number;
+>  7 |   const [displayValue, setDisplayValue] = useState(
+     |                                           ^^^^^^^^^
+>  8 |     props.prefix + missDirection + nothing
+     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+>  9 |   );
+     | ^^^^ this useState shadows props
+  10 |
+  11 |   useEffect(() => {
+  12 |     setDisplayValue(props.prefix + missDirection + nothing);
+
+error.derived-state-from-shadowed-props.ts:7:42
+   5 |   const nothing = 0;
+   6 |   const missDirection = number;
+>  7 |   const [displayValue, setDisplayValue] = useState(
+     |                                           ^^^^^^^^^
+>  8 |     props.prefix + missDirection + nothing
+     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+>  9 |   );
+     | ^^^^ this useState shadows number
+  10 |
+  11 |   useEffect(() => {
+  12 |     setDisplayValue(props.prefix + missDirection + nothing);
+
+error.derived-state-from-shadowed-props.ts:18:8
+  16 |     <div
+  17 |       onClick={() => {
+> 18 |         setDisplayValue('clicked');
      |         ^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent
-  17 |       }}>
-  18 |       {displayValue}
-  19 |     </div>
+  19 |       }}>
+  20 |       {displayValue}
+  21 |     </div>
 ```
           
       

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

@@ -4,7 +4,9 @@ import {useState, useEffect} from 'react';
 function Component({props, number}) {
   const nothing = 0;
   const missDirection = number;
-  const [displayValue, setDisplayValue] = useState(props.prefix + missDirection + nothing);
+  const [displayValue, setDisplayValue] = useState(
+    props.prefix + missDirection + nothing
+  );
 
   useEffect(() => {
     setDisplayValue(props.prefix + missDirection + nothing);

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

@@ -32,9 +32,9 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-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.
+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

@@ -30,9 +30,9 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-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.
+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

@@ -24,9 +24,9 @@ function BadExample() {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-This setState() appears to derive a value from 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.
+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

@@ -29,9 +29,9 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-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.
+Props [props, 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

@@ -6,7 +6,9 @@
 import {useEffect, useState} from 'react';
 
 function Component({props}) {
-  const [fullName, setFullName] = useState(props.firstName + ' ' + props.lastName);
+  const [fullName, setFullName] = useState(
+    props.firstName + ' ' + props.lastName
+  );
 
   useEffect(() => {
     setFullName(props.firstName + ' ' + props.lastName);
@@ -28,18 +30,18 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-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.
+Props [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-destructured.ts:8:4
-   6 |
-   7 |   useEffect(() => {
->  8 |     setFullName(props.firstName + ' ' + props.lastName);
+error.invalid-derived-state-from-props-destructured.ts:10:4
+   8 |
+   9 |   useEffect(() => {
+> 10 |     setFullName(props.firstName + ' ' + props.lastName);
      |     ^^^^^^^^^^^ This should be computed during render, not in an effect
-   9 |   }, [props.firstName, props.lastName]);
-  10 |
-  11 |   return <div>{fullName}</div>;
+  11 |   }, [props.firstName, props.lastName]);
+  12 |
+  13 |   return <div>{fullName}</div>;
 ```
           
       

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

@@ -2,7 +2,9 @@
 import {useEffect, useState} from 'react';
 
 function Component({props}) {
-  const [fullName, setFullName] = useState(props.firstName + ' ' + props.lastName);
+  const [fullName, setFullName] = useState(
+    props.firstName + ' ' + props.lastName
+  );
 
   useEffect(() => {
     setFullName(props.firstName + ' ' + props.lastName);

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

@@ -28,9 +28,9 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-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.
+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

@@ -4,18 +4,14 @@
 ```javascript
 // @validateNoDerivedComputationsInEffects
 
-export default function InProductLobbyGeminiCard(
-  input = 'empty',
-) {
+export default function InProductLobbyGeminiCard(input = 'empty') {
   const [currInput, setCurrInput] = useState(input);
 
   useEffect(() => {
-    setCurrInput(input)
+    setCurrInput(input);
   }, [input]);
 
-  return (
-    <div>{currInput}</div>
-  )
+  return <div>{currInput}</div>;
 }
 
 ```
@@ -26,18 +22,18 @@ export default function InProductLobbyGeminiCard(
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-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.
+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 |
-   8 |   useEffect(() => {
->  9 |     setCurrInput(input)
+error.invalid-derived-state-from-props-with-default-value.ts:7:4
+   5 |
+   6 |   useEffect(() => {
+>  7 |     setCurrInput(input);
      |     ^^^^^^^^^^^^ This should be computed during render, not in an effect
-  10 |   }, [input]);
-  11 |
-  12 |   return (
+   8 |   }, [input]);
+   9 |
+  10 |   return <div>{currInput}</div>;
 ```
           
       

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

@@ -1,15 +1,11 @@
 // @validateNoDerivedComputationsInEffects
 
-export default function InProductLobbyGeminiCard(
-  input = 'empty',
-) {
+export default function InProductLobbyGeminiCard(input = 'empty') {
   const [currInput, setCurrInput] = useState(input);
 
   useEffect(() => {
-    setCurrInput(input)
+    setCurrInput(input);
   }, [input]);
 
-  return (
-    <div>{currInput}</div>
-  )
+  return <div>{currInput}</div>;
 }

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

@@ -36,9 +36,9 @@ export const FIXTURE_ENTRYPOINT = {
 ```
 Found 1 error:
 
-Error: Derive values in render, not effects.
+Error: You might not need an effect. Derive values in render, not effects.
 
-This setState() appears to derive a value from 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.
+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 |

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.expect.md

@@ -0,0 +1,63 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+
+function EndDate({startDate, endDate, onStartDateChange}) {
+  const [localStartDate, setLocalStartDate] = useState(startDate);
+
+  useEffect(() => {
+    setLocalStartDate(startDate);
+  }, [startDate]);
+
+  const onChange = date => {
+    setLocalStartDate(date);
+    onStartDateChange(date);
+  };
+  return (
+    <DateInput value={localStartDate} second={endDate} onChange={onChange} />
+  );
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: You might not need an effect. Local state shadows parent state.
+
+The setState within a useEffect is deriving from props [startDate]. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there. If you are purposefully initializing state with a prop, and want to update it when a prop changes, do so conditionally in render.
+
+error.shadowed-props-with-onchange.ts:7:4
+   5 |
+   6 |   useEffect(() => {
+>  7 |     setLocalStartDate(startDate);
+     |     ^^^^^^^^^^^^^^^^^ this derives values from props to synchronize state
+   8 |   }, [startDate]);
+   9 |
+  10 |   const onChange = date => {
+
+error.shadowed-props-with-onchange.ts:4:46
+  2 |
+  3 | function EndDate({startDate, endDate, onStartDateChange}) {
+> 4 |   const [localStartDate, setLocalStartDate] = useState(startDate);
+    |                                               ^^^^^^^^^^^^^^^^^^^ this useState shadows startDate
+  5 |
+  6 |   useEffect(() => {
+  7 |     setLocalStartDate(startDate);
+
+error.shadowed-props-with-onchange.ts:11:4
+   9 |
+  10 |   const onChange = date => {
+> 11 |     setLocalStartDate(date);
+     |     ^^^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent
+  12 |     onStartDateChange(date);
+  13 |   };
+  14 |   return (
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.shadowed-props-with-onchange.js

@@ -0,0 +1,17 @@
+// @validateNoDerivedComputationsInEffects
+
+function EndDate({startDate, endDate, onStartDateChange}) {
+  const [localStartDate, setLocalStartDate] = useState(startDate);
+
+  useEffect(() => {
+    setLocalStartDate(startDate);
+  }, [startDate]);
+
+  const onChange = date => {
+    setLocalStartDate(date);
+    onStartDateChange(date);
+  };
+  return (
+    <DateInput value={localStartDate} second={endDate} onChange={onChange} />
+  );
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/ref-conditional-in-effect-no-error.expect.md

@@ -0,0 +1,82 @@
+
+## Input
+
+```javascript
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    if (myRef.current) {
+      setLocal(test + 'Available');
+    } else {
+      setLocal(test + 'NotAvailable');
+    }
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects
+import { useEffect, useState, useRef } from "react";
+
+export default function Component(t0) {
+  const $ = _c(5);
+  const { test } = t0;
+  const [local, setLocal] = useState("");
+
+  const myRef = useRef(null);
+  let t1;
+  let t2;
+  if ($[0] !== test) {
+    t1 = () => {
+      if (myRef.current) {
+        setLocal(test + "Available");
+      } else {
+        setLocal(test + "NotAvailable");
+      }
+    };
+
+    t2 = [test];
+    $[0] = test;
+    $[1] = t1;
+    $[2] = t2;
+  } else {
+    t1 = $[1];
+    t2 = $[2];
+  }
+  useEffect(t1, t2);
+  let t3;
+  if ($[3] !== local) {
+    t3 = <>{local}</>;
+    $[3] = local;
+    $[4] = t3;
+  } else {
+    t3 = $[4];
+  }
+  return t3;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ test: "testString" }],
+};
+
+```
+      
+### Eval output
+(kind: ok) testStringNotAvailable

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/ref-conditional-in-effect-no-error.js

@@ -0,0 +1,23 @@
+// @validateNoDerivedComputationsInEffects
+import {useEffect, useState, useRef} from 'react';
+
+export default function Component({test}) {
+  const [local, setLocal] = useState('');
+
+  const myRef = useRef(null);
+
+  useEffect(() => {
+    if (myRef.current) {
+      setLocal(test + 'Available');
+    } else {
+      setLocal(test + 'NotAvailable');
+    }
+  }, [test]);
+
+  return <>{local}</>;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{test: 'testString'}],
+};

compiler/yarn.lock

@@ -10494,16 +10494,7 @@ string-length@^4.0.1:
     char-regex "^1.0.2"
     strip-ansi "^6.0.0"
 
-"string-width-cjs@npm:string-width@^4.2.0":
-  version "4.2.3"
-  resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz"
-  integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
-  dependencies:
-    emoji-regex "^8.0.0"
-    is-fullwidth-code-point "^3.0.0"
-    strip-ansi "^6.0.1"
-
-string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
+"string-width-cjs@npm:string-width@^4.2.0", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
   version "4.2.3"
   resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz"
   integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
@@ -10576,14 +10567,7 @@ string_decoder@~1.1.1:
   dependencies:
     safe-buffer "~5.1.0"
 
-"strip-ansi-cjs@npm:strip-ansi@^6.0.1":
-  version "6.0.1"
-  resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz"
-  integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
-  dependencies:
-    ansi-regex "^5.0.1"
-
-strip-ansi@^6.0.0, strip-ansi@^6.0.1:
+"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1:
   version "6.0.1"
   resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz"
   integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==
@@ -11360,7 +11344,7 @@ workerpool@^6.5.1:
   resolved "https://registry.npmjs.org/workerpool/-/workerpool-6.5.1.tgz"
   integrity sha512-Fs4dNYcsdpYSAfVxhnl1L5zTksjvOJxtC5hzMNl+1t9B8hTJTdKDyZ5ju7ztgPy+ft9tBFXoOlDNiOT9WUXZlA==
 
-"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0":
+"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0", wrap-ansi@^7.0.0:
   version "7.0.0"
   resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz"
   integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
@@ -11378,15 +11362,6 @@ wrap-ansi@^6.2.0:
     string-width "^4.1.0"
     strip-ansi "^6.0.0"
 
-wrap-ansi@^7.0.0:
-  version "7.0.0"
-  resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz"
-  integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==
-  dependencies:
-    ansi-styles "^4.0.0"
-    string-width "^4.1.0"
-    strip-ansi "^6.0.0"
-
 wrap-ansi@^8.1.0:
   version "8.1.0"
   resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-8.1.0.tgz"