[compiler] Reuse DropManualMemoization for ValidateNoVoidUseMemo

Much of the logic in the new validation pass is already implemented in DropManualMemoization, so let's combine them. I opted to keep the environment flag so we can more precisely control the rollout.

Author: poteto

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

@@ -82,7 +82,6 @@ import {
 import {inferTypes} from '../TypeInference';
 import {
   validateContextVariableLValues,
-  validateNoVoidUseMemo,
   validateHooksUsage,
   validateMemoizedEffectDependencies,
   validateNoCapitalizedCalls,
@@ -168,17 +167,14 @@ function runWithEnvironment(
 
   validateContextVariableLValues(hir);
   validateUseMemo(hir).unwrap();
-  if (env.config.enableValidateNoVoidUseMemo) {
-    validateNoVoidUseMemo(hir).unwrap();
-  }
 
   if (
     env.isInferredMemoEnabled &&
     !env.config.enablePreserveExistingManualUseMemo &&
     !env.config.disableMemoizationForDebugging &&
     !env.config.enableChangeDetectionForDebugging
   ) {
-    dropManualMemoization(hir);
+    dropManualMemoization(hir).unwrap();
     log({kind: 'hir', name: 'DropManualMemoization', value: hir});
   }
 

compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts

@@ -5,7 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {CompilerError, SourceLocation} from '..';
+import {CompilerError, ErrorSeverity, SourceLocation} from '..';
 import {
   CallExpression,
   Effect,
@@ -30,6 +30,7 @@ import {
   makeInstructionId,
 } from '../HIR';
 import {createTemporaryPlace, markInstructionIds} from '../HIR/HIRBuilder';
+import {Result} from '../Utils/Result';
 
 type ManualMemoCallee = {
   kind: 'useMemo' | 'useCallback';
@@ -341,8 +342,14 @@ function extractManualMemoizationArgs(
  * rely on type inference to find useMemo/useCallback invocations, and instead does basic tracking
  * of globals and property loads to find both direct calls as well as usage via the React namespace,
  * eg `React.useMemo()`.
+ *
+ * This pass also validates that useMemo callbacks return a value (not void), ensuring that useMemo
+ * is only used for memoizing values and not for running arbitrary side effects.
  */
-export function dropManualMemoization(func: HIRFunction): void {
+export function dropManualMemoization(
+  func: HIRFunction,
+): Result<void, CompilerError> {
+  const errors = new CompilerError();
   const isValidationEnabled =
     func.env.config.validatePreserveExistingMemoizationGuarantees ||
     func.env.config.validateNoSetStateInRender ||
@@ -390,6 +397,36 @@ export function dropManualMemoization(func: HIRFunction): void {
             manualMemo.kind,
             sidemap,
           );
+
+          /**
+           * Bailout on void return useMemos. This is an anti-pattern where code might be using
+           * useMemo like useEffect: running arbirtary side-effects synced to changes in specific
+           * values.
+           */
+          if (
+            func.env.config.enableValidateNoVoidUseMemo &&
+            manualMemo.kind === 'useMemo'
+          ) {
+            const funcToCheck = sidemap.functions.get(
+              fnPlace.identifier.id,
+            )?.value;
+            if (funcToCheck !== undefined && funcToCheck.loweredFunc.func) {
+              if (doesFunctionHaveVoidReturn(funcToCheck.loweredFunc.func)) {
+                errors.push({
+                  severity: ErrorSeverity.InvalidReact,
+                  reason: `React Compiler has skipped optimizing this component because ${
+                    manualMemo.loadInstr.value.kind === 'PropertyLoad'
+                      ? 'React.useMemo'
+                      : 'useMemo'
+                  } doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.`,
+                  loc: instr.value.loc,
+                  suggestions: null,
+                  description: null,
+                });
+              }
+            }
+          }
+
           instr.value = getManualMemoizationReplacement(
             fnPlace,
             instr.value.loc,
@@ -486,6 +523,8 @@ export function dropManualMemoization(func: HIRFunction): void {
       markInstructionIds(func.body);
     }
   }
+
+  return errors.asResult();
 }
 
 function findOptionalPlaces(fn: HIRFunction): Set<IdentifierId> {
@@ -530,3 +569,17 @@ function findOptionalPlaces(fn: HIRFunction): Set<IdentifierId> {
   }
   return optionals;
 }
+
+function doesFunctionHaveVoidReturn(func: HIRFunction): boolean {
+  for (const [, block] of func.body.blocks) {
+    if (block.terminal.kind === 'return') {
+      if (
+        block.terminal.returnVariant === 'Explicit' ||
+        block.terminal.returnVariant === 'Implicit'
+      ) {
+        return false;
+      }
+    }
+  }
+  return true;
+}

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

@@ -6,7 +6,6 @@
  */
 
 export {validateContextVariableLValues} from './ValidateContextVariableLValues';
-export {validateNoVoidUseMemo} from './ValidateNoVoidUseMemo';
 export {validateHooksUsage} from './ValidateHooksUsage';
 export {validateMemoizedEffectDependencies} from './ValidateMemoizedEffectDependencies';
 export {validateNoCapitalizedCalls} from './ValidateNoCapitalizedCalls';

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

@@ -24,18 +24,37 @@ function Component() {
 ## Error
 
 ```
-Found 1 error:
+Found 2 errors:
 
 Error: React Compiler has skipped optimizing this component because useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.
 
 error.useMemo-no-return-value.ts:3:16
   1 | // @enableValidateNoVoidUseMemo
   2 | function Component() {
 > 3 |   const value = useMemo(() => {
-    |                 ^^^^^^^ React Compiler has skipped optimizing this component because useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.
-  4 |     console.log('computing');
-  5 |   }, []);
+    |                 ^^^^^^^^^^^^^^^
+> 4 |     console.log('computing');
+    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+> 5 |   }, []);
+    | ^^^^^^^^^ React Compiler has skipped optimizing this component because useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.
   6 |   const value2 = React.useMemo(() => {
+  7 |     console.log('computing');
+  8 |   }, []);
+
+Error: React Compiler has skipped optimizing this component because React.useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.
+
+error.useMemo-no-return-value.ts:6:17
+   4 |     console.log('computing');
+   5 |   }, []);
+>  6 |   const value2 = React.useMemo(() => {
+     |                  ^^^^^^^^^^^^^^^^^^^^^
+>  7 |     console.log('computing');
+     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+>  8 |   }, []);
+     | ^^^^^^^^^ React Compiler has skipped optimizing this component because React.useMemo doesn't return a value. useMemo should only be used for memoizing values, not running arbitrary side effects.
+   9 |   return (
+  10 |     <div>
+  11 |       {value}
 ```