[pull] main from facebook:main

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/Inference/DropManualMemoization.ts

@@ -438,40 +438,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,
@@ -629,17 +595,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/Validation/ValidateUseMemo.ts

@@ -10,16 +10,37 @@ import {
   CompilerError,
   ErrorCategory,
 } from '../CompilerError';
-import {FunctionExpression, HIRFunction, IdentifierId} from '../HIR';
+import {
+  FunctionExpression,
+  HIRFunction,
+  IdentifierId,
+  SourceLocation,
+} from '../HIR';
+import {
+  eachInstructionValueOperand,
+  eachTerminalOperand,
+} from '../HIR/visitors';
 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>();
+  const unusedUseMemos = new Map<IdentifierId, SourceLocation>();
   for (const [, block] of fn.body.blocks) {
     for (const {lvalue, value} of block.instructions) {
+      if (unusedUseMemos.size !== 0) {
+        /**
+         * Most of the time useMemo results are referenced immediately. Don't bother
+         * scanning instruction operands for useMemos unless there is an as-yet-unused
+         * useMemo.
+         */
+        for (const operand of eachInstructionValueOperand(value)) {
+          unusedUseMemos.delete(operand.identifier.id);
+        }
+      }
       switch (value.kind) {
         case 'LoadGlobal': {
           if (value.binding.name === 'useMemo') {
@@ -45,10 +66,8 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
         case 'CallExpression': {
           // Is the function being called useMemo, with at least 1 argument?
           const callee =
-            value.kind === 'CallExpression'
-              ? value.callee.identifier.id
-              : value.property.identifier.id;
-          const isUseMemo = useMemos.has(callee);
+            value.kind === 'CallExpression' ? value.callee : value.property;
+          const isUseMemo = useMemos.has(callee.identifier.id);
           if (!isUseMemo || value.args.length === 0) {
             continue;
           }
@@ -104,10 +123,103 @@ export function validateUseMemo(fn: HIRFunction): Result<void, CompilerError> {
             );
           }
 
+          validateNoContextVariableAssignment(body.loweredFunc.func, errors);
+
+          if (fn.env.config.validateNoVoidUseMemo) {
+            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;
         }
       }
     }
+    if (unusedUseMemos.size !== 0) {
+      for (const operand of eachTerminalOperand(block.terminal)) {
+        unusedUseMemos.delete(operand.identifier.id);
+      }
+    }
   }
+  if (unusedUseMemos.size !== 0) {
+    /**
+     * Basic check for unused memos, where the result of the call is never referenced. This runs
+     * before DCE so it's more of an AST-level check that something, _anything_, cares about the value.
+     *
+     * This is easy to defeat with e.g. `const _ = useMemo(...)` but it at least gives us something to teach.
+     * Even a DCE-based version could be bypassed with `noop(useMemo(...))`.
+     */
+    for (const loc of unusedUseMemos.values()) {
+      voidMemoErrors.pushDiagnostic(
+        CompilerDiagnostic.create({
+          category: ErrorCategory.VoidUseMemo,
+          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({
+          kind: 'error',
+          loc,
+          message: 'useMemo() result is unused',
+        }),
+      );
+    }
+  }
+  fn.env.logErrors(voidMemoErrors.asResult());
   return errors.asResult();
 }
+
+function validateNoContextVariableAssignment(
+  fn: HIRFunction,
+  errors: CompilerError,
+): void {
+  for (const block of fn.body.blocks.values()) {
+    for (const instr of block.instructions) {
+      const value = instr.value;
+      switch (value.kind) {
+        case 'StoreContext': {
+          errors.pushDiagnostic(
+            CompilerDiagnostic.create({
+              category: ErrorCategory.UseMemo,
+              reason:
+                'useMemo() callbacks may not reassign variables declared outside of the callback',
+              description:
+                'useMemo() callbacks must be pure functions and cannot reassign variables defined outside of the callback function',
+              suggestions: null,
+            }).withDetails({
+              kind: 'error',
+              loc: value.lvalue.place.loc,
+              message: 'Cannot reassign variable',
+            }),
+          );
+          break;
+        }
+      }
+    }
+  }
+}
+
+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.invalid-reassign-variable-in-usememo.expect.md

@@ -0,0 +1,38 @@
+
+## Input
+
+```javascript
+function Component() {
+  let x;
+  const y = useMemo(() => {
+    let z;
+    x = [];
+    z = true;
+    return z;
+  }, []);
+  return [x, y];
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: useMemo() callbacks may not reassign variables declared outside of the callback
+
+useMemo() callbacks must be pure functions and cannot reassign variables defined outside of the callback function.
+
+error.invalid-reassign-variable-in-usememo.ts:5:4
+  3 |   const y = useMemo(() => {
+  4 |     let z;
+> 5 |     x = [];
+    |     ^ Cannot reassign variable
+  6 |     z = true;
+  7 |     return z;
+  8 |   }, []);
+```
+
+

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-variable-in-usememo.js

@@ -0,0 +1,10 @@
+function Component() {
+  let x;
+  const y = useMemo(() => {
+    let z;
+    x = [];
+    z = true;
+    return z;
+  }, []);
+  return [x, y];
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-unused-usememo.expect.md

@@ -0,0 +1,41 @@
+
+## Input
+
+```javascript
+// @validateNoVoidUseMemo @loggerTestOnly
+function Component() {
+  useMemo(() => {
+    return [];
+  }, []);
+  return <div />;
+}
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @validateNoVoidUseMemo @loggerTestOnly
+function Component() {
+  const $ = _c(1);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = <div />;
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  return t0;
+}
+
+```
+
+## Logs
+
+```
+{"kind":"CompileError","detail":{"options":{"category":"VoidUseMemo","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,"details":[{"kind":"error","loc":{"start":{"line":3,"column":2,"index":67},"end":{"line":3,"column":9,"index":74},"filename":"invalid-unused-usememo.ts","identifierName":"useMemo"},"message":"useMemo() result is unused"}]}},"fnLoc":null}
+{"kind":"CompileSuccess","fnLoc":{"start":{"line":2,"column":0,"index":42},"end":{"line":7,"column":1,"index":127},"filename":"invalid-unused-usememo.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0}
+```
+
+### Eval output
+(kind: exception) Fixture not implemented