[compiler][wip] Fix for ValidatePreserveMemo edge case w multiple return in useMemo

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

@@ -988,7 +988,7 @@ function getRuleForCategoryImpl(category: ErrorCategory): LintRule {
         severity: ErrorSeverity.Error,
         name: 'void-use-memo',
         description:
-          'Validates that useMemos always return a value. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.',
+          'Validates that useMemos always return a value and that the result of the useMemo is used by the component/hook. See [`useMemo()` docs](https://react.dev/reference/react/useMemo) for more information.',
         preset: LintRulePreset.RecommendedLatest,
       };
     }

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

@@ -659,7 +659,7 @@ export const EnvironmentConfigSchema = z.object({
    * Invalid:
    *   useMemo(() => { ... }, [...]);
    */
-  validateNoVoidUseMemo: z.boolean().default(false),
+  validateNoVoidUseMemo: z.boolean().default(true),
 
   /**
    * Validates that Components/Hooks are always defined at module level. This prevents scope

compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts

@@ -988,7 +988,7 @@ export function createTemporaryPlace(
     identifier: makeTemporaryIdentifier(env.nextIdentifierId, loc),
     reactive: false,
     effect: Effect.Unknown,
-    loc: GeneratedSource,
+    loc,
   };
 }
 

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

@@ -11,7 +11,6 @@ import {
   CallExpression,
   Effect,
   Environment,
-  FinishMemoize,
   FunctionExpression,
   HIRFunction,
   IdentifierId,
@@ -25,7 +24,6 @@ import {
   Place,
   PropertyLoad,
   SpreadPattern,
-  StartMemoize,
   TInstruction,
   getHookKindForType,
   makeInstructionId,
@@ -184,36 +182,52 @@ function makeManualMemoizationMarkers(
   depsList: Array<ManualMemoDependency> | null,
   memoDecl: Place,
   manualMemoId: number,
-): [TInstruction<StartMemoize>, TInstruction<FinishMemoize>] {
+): [Array<Instruction>, Array<Instruction>] {
+  const temp = createTemporaryPlace(env, memoDecl.loc);
   return [
-    {
-      id: makeInstructionId(0),
-      lvalue: createTemporaryPlace(env, fnExpr.loc),
-      value: {
-        kind: 'StartMemoize',
-        manualMemoId,
-        /*
-         * Use deps list from source instead of inferred deps
-         * as dependencies
-         */
-        deps: depsList,
+    [
+      {
+        id: makeInstructionId(0),
+        lvalue: createTemporaryPlace(env, fnExpr.loc),
+        value: {
+          kind: 'StartMemoize',
+          manualMemoId,
+          /*
+           * Use deps list from source instead of inferred deps
+           * as dependencies
+           */
+          deps: depsList,
+          loc: fnExpr.loc,
+        },
+        effects: null,
         loc: fnExpr.loc,
       },
-      effects: null,
-      loc: fnExpr.loc,
-    },
-    {
-      id: makeInstructionId(0),
-      lvalue: createTemporaryPlace(env, fnExpr.loc),
-      value: {
-        kind: 'FinishMemoize',
-        manualMemoId,
-        decl: {...memoDecl},
-        loc: fnExpr.loc,
+    ],
+    [
+      {
+        id: makeInstructionId(0),
+        lvalue: {...temp},
+        value: {
+          kind: 'LoadLocal',
+          place: {...memoDecl},
+          loc: memoDecl.loc,
+        },
+        effects: null,
+        loc: memoDecl.loc,
       },
-      effects: null,
-      loc: fnExpr.loc,
-    },
+      {
+        id: makeInstructionId(0),
+        lvalue: createTemporaryPlace(env, memoDecl.loc),
+        value: {
+          kind: 'FinishMemoize',
+          manualMemoId,
+          decl: {...temp},
+          loc: memoDecl.loc,
+        },
+        effects: null,
+        loc: memoDecl.loc,
+      },
+    ],
   ];
 }
 
@@ -409,10 +423,7 @@ export function dropManualMemoization(
    *   LoadLocal fnArg
    * - (if validation is enabled) collect manual memoization markers
    */
-  const queuedInserts: Map<
-    InstructionId,
-    TInstruction<StartMemoize> | TInstruction<FinishMemoize>
-  > = new Map();
+  const queuedInserts: Map<InstructionId, Array<Instruction>> = new Map();
   for (const [_, block] of func.body.blocks) {
     for (let i = 0; i < block.instructions.length; i++) {
       const instr = block.instructions[i]!;
@@ -438,40 +449,6 @@ export function dropManualMemoization(
             continue;
           }
 
-          /**
-           * 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({
-                    category: ErrorCategory.VoidUseMemo,
-                    reason: '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,
-                  }).withDetails({
-                    kind: 'error',
-                    loc: instr.value.loc,
-                    message: 'useMemo() callbacks must return a value',
-                  }),
-                );
-              }
-            }
-          }
-
           instr.value = getManualMemoizationReplacement(
             fnPlace,
             instr.value.loc,
@@ -557,11 +534,11 @@ export function dropManualMemoization(
       let nextInstructions: Array<Instruction> | null = null;
       for (let i = 0; i < block.instructions.length; i++) {
         const instr = block.instructions[i];
-        const insertInstr = queuedInserts.get(instr.id);
-        if (insertInstr != null) {
+        const insertInstructions = queuedInserts.get(instr.id);
+        if (insertInstructions != null) {
           nextInstructions = nextInstructions ?? block.instructions.slice(0, i);
           nextInstructions.push(instr);
-          nextInstructions.push(insertInstr);
+          nextInstructions.push(...insertInstructions);
         } else if (nextInstructions != null) {
           nextInstructions.push(instr);
         }
@@ -629,17 +606,3 @@ 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/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts

@@ -77,6 +77,15 @@ class Transform extends ReactiveFunctionTransform<boolean> {
         }
         break;
       }
+      case 'FinishMemoize': {
+        if (
+          !withinScope &&
+          this.alwaysInvalidatingValues.has(value.decl.identifier)
+        ) {
+          value.pruned = true;
+        }
+        break;
+      }
     }
     return {kind: 'keep'};
   }

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

@@ -24,6 +24,7 @@ import {Result} from '../Utils/Result';
 
 export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
   const errors = new CompilerError();
+  const voidMemoErrors = new CompilerError();
   const useMemos = new Set<IdentifierId>();
   const react = new Set<IdentifierId>();
   const functions = new Map<IdentifierId, FunctionExpression>();
@@ -125,7 +126,22 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
           validateNoContextVariableAssignment(body.loweredFunc.func, errors);
 
           if (fn.env.config.validateNoVoidUseMemo) {
-            unusedUseMemos.set(lvalue.identifier.id, callee.loc);
+            if (!hasNonVoidReturn(body.loweredFunc.func)) {
+              voidMemoErrors.pushDiagnostic(
+                CompilerDiagnostic.create({
+                  category: ErrorCategory.VoidUseMemo,
+                  reason: 'useMemo() callbacks must return a value',
+                  description: `This useMemo() callback doesn't return a value. useMemo() is for computing and caching values, not for arbitrary side effects`,
+                  suggestions: null,
+                }).withDetails({
+                  kind: 'error',
+                  loc: body.loc,
+                  message: 'useMemo() callbacks must return a value',
+                }),
+              );
+            } else {
+              unusedUseMemos.set(lvalue.identifier.id, callee.loc);
+            }
           }
           break;
         }
@@ -146,10 +162,10 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
      * Even a DCE-based version could be bypassed with `noop(useMemo(...))`.
      */
     for (const loc of unusedUseMemos.values()) {
-      errors.pushDiagnostic(
+      voidMemoErrors.pushDiagnostic(
         CompilerDiagnostic.create({
           category: ErrorCategory.VoidUseMemo,
-          reason: 'Unused useMemo()',
+          reason: 'useMemo() result is unused',
           description: `This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects`,
           suggestions: null,
         }).withDetails({
@@ -160,6 +176,7 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
       );
     }
   }
+  fn.env.logErrors(voidMemoErrors.asResult());
   return errors.asResult();
 }
 
@@ -192,3 +209,17 @@ function validateNoContextVariableAssignment(
     }
   }
 }
+
+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/__tests__/fixtures/compiler/error.bug-invariant-expected-consistent-destructuring.expect.md

@@ -29,7 +29,7 @@ Found 1 error:
 
 Invariant: Expected consistent kind for destructuring
 
-Other places were `Reassign` but 'mutate? #t8$46[7:9]{reactive}' is const.
+Other places were `Reassign` but 'mutate? #t8$47[7:9]{reactive}' is const.
 
 error.bug-invariant-expected-consistent-destructuring.ts:9:9
    7 |