[compiler] Add catching useStates that shadow a reactive value

Author: jorge-cab

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

@@ -575,7 +575,9 @@ export enum ErrorCategory {
   // Checks for no setState in effect bodies
   EffectSetState = 'EffectSetState',
 
-  EffectDerivationsOfState = 'EffectDerivationsOfState',
+  EffectDerivationDeriveInRender = 'EffectDerivationDeriveInRender',
+
+  EffectDerivationShadowingParentState = 'EffectDerivationShadowingParentState',
 
   // Validates against try/catch in place of error boundaries
   ErrorBoundaries = 'ErrorBoundaries',
@@ -692,12 +694,21 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule {
         recommended: false,
       };
     }
-    case ErrorCategory.EffectDerivationsOfState: {
+    case ErrorCategory.EffectDerivationDeriveInRender: {
+      return {
+        category,
+        name: 'no-deriving-state-in-effects',
+        description:
+          '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,
         name: 'no-deriving-state-in-effects',
         description:
-          'Validates against deriving values from state in an effect',
+          '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

@@ -42,14 +42,15 @@ type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState';
 type DerivationMetadata = {
   typeOfValue: TypeOfValue;
   place: Place;
-  sources: Set<Place>;
+  sources: Array<Place>;
 };
 
 type ErrorMetadata = {
   type: TypeOfValue;
   description: string | undefined;
   loc: SourceLocation;
   setStateName: string | undefined | null;
+  derivedDepsNames: Array<string>;
 };
 
 /**
@@ -80,6 +81,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 +96,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 +106,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 +118,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);
@@ -168,6 +170,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
   const compilerError = generateCompilerError(
     setStateCalls,
     effectSetStates,
+    shadowingUseState,
     errors,
   );
 
@@ -179,21 +182,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
@@ -205,15 +199,29 @@ 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.`,
+        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,
         severity: ErrorSeverity.InvalidReact,
+        reason:
+          'You might not need an effect. Local state shadows parent state.',
       }).withDetail({
         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.withDetail({
+              kind: 'error',
+              loc: loc,
+              message: `this useState shadows ${derivedDep}`,
+            });
+          }
+        }
+      }
+
       for (const [key, setStateCallArray] of effectSetStates) {
         if (setStateCallArray.length === 0) {
           continue;
@@ -235,9 +243,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.`,
+        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,
         severity: ErrorSeverity.InvalidReact,
+        reason:
+          'You might not need an effect. Derive values in render, not effects.',
       }).withDetail({
         kind: 'error',
         loc: error.loc,
@@ -271,7 +281,7 @@ function updateDerivationMetadata(
 ): void {
   let newValue: DerivationMetadata = {
     place: target,
-    sources: new Set(),
+    sources: [],
     typeOfValue: typeOfValue ?? 'ignored',
   };
 
@@ -286,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);
         }
       }
     }
@@ -301,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 &&
@@ -348,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)) {
@@ -358,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') {
@@ -411,16 +439,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,
+      );
     }
   }
 }
@@ -524,7 +562,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;
       })
@@ -534,19 +572,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/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 |