[compiler] Reuse DropManualMemoization for ValidateNoVoidUseMemo (#34001)

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.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34001).
* #34022
* #34002
* __->__ #34001

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.validateNoVoidUseMemo) {
-    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,12 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {CompilerError, SourceLocation} from '..';
+import {
+  CompilerDiagnostic,
+  CompilerError,
+  ErrorSeverity,
+  SourceLocation,
+} from '..';
 import {
   CallExpression,
   Effect,
@@ -30,6 +35,7 @@ import {
   makeInstructionId,
 } from '../HIR';
 import {createTemporaryPlace, markInstructionIds} from '../HIR/HIRBuilder';
+import {Result} from '../Utils/Result';
 
 type ManualMemoCallee = {
   kind: 'useMemo' | 'useCallback';
@@ -341,8 +347,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 +402,41 @@ 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.validateNoVoidUseMemo &&
+            manualMemo.kind === 'useMemo'
+          ) {
+            const funcToCheck = sidemap.functions.get(
+              fnPlace.identifier.id,
+            )?.value;
+            if (funcToCheck !== undefined && funcToCheck.loweredFunc.func) {
+              if (!hasNonVoidReturn(funcToCheck.loweredFunc.func)) {
+                errors.pushDiagnostic(
+                  CompilerDiagnostic.create({
+                    severity: ErrorSeverity.InvalidReact,
+                    category: 'useMemo() callbacks must return a value',
+                    description: `This ${
+                      manualMemo.loadInstr.value.kind === 'PropertyLoad'
+                        ? 'React.useMemo'
+                        : 'useMemo'
+                    } callback doesn't return a value. useMemo is for computing and caching values, not for arbitrary side effects.`,
+                    suggestions: null,
+                  }).withDetail({
+                    kind: 'error',
+                    loc: instr.value.loc,
+                    message: 'useMemo() callbacks must return a value',
+                  }),
+                );
+              }
+            }
+          }
+
           instr.value = getManualMemoizationReplacement(
             fnPlace,
             instr.value.loc,
@@ -486,6 +533,8 @@ export function dropManualMemoization(func: HIRFunction): void {
       markInstructionIds(func.body);
     }
   }
+
+  return errors.asResult();
 }
 
 function findOptionalPlaces(fn: HIRFunction): Set<IdentifierId> {
@@ -530,3 +579,17 @@ function findOptionalPlaces(fn: HIRFunction): Set<IdentifierId> {
   }
   return optionals;
 }
+
+function hasNonVoidReturn(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 true;
+      }
+    }
+  }
+  return false;
+}

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,41 @@ 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() callbacks must return a value
+
+This useMemo callback doesn't return a value. useMemo is for computing and caching values, not for arbitrary side effects.
 
 error.useMemo-no-return-value.ts:3:16
   1 | // @validateNoVoidUseMemo
   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 |   }, []);
+    | ^^^^^^^^^ useMemo() callbacks must return a value
   6 |   const value2 = React.useMemo(() => {
+  7 |     console.log('computing');
+  8 |   }, []);
+
+Error: useMemo() callbacks must return a value
+
+This React.useMemo callback doesn't return a value. useMemo is for computing and caching values, not for 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 |   }, []);
+     | ^^^^^^^^^ useMemo() callbacks must return a value
+   9 |   return (
+  10 |     <div>
+  11 |       {value}
 ```