[compiler] More useMemo validation

Two additional validations for useMemo:
* Disallow reassigning to values declared outside the useMemo callback (always on)
* Disallow unused useMemo calls (part of the validateNoVoidUseMemo feature flag, which in turn is off by default)

We should probably enable this flag though!

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

@@ -458,9 +458,9 @@ export function dropManualMemoization(
                     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`,
+                        ? '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',

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

@@ -10,7 +10,16 @@ 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> {
@@ -18,8 +27,19 @@ export function validateUseMemo(fn: HIRFunction): Result<void, 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 +65,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 +122,73 @@ 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);
+          }
+          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()) {
+      errors.pushDiagnostic(
+        CompilerDiagnostic.create({
+          category: ErrorCategory.VoidUseMemo,
+          reason: 'Unused useMemo()',
+          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',
+        }),
+      );
+    }
+  }
+  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;
         }
       }
     }
   }
-  return errors.asResult();
 }

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/error.invalid-unused-usememo.expect.md

@@ -0,0 +1,35 @@
+
+## Input
+
+```javascript
+// @validateNoVoidUseMemo
+function Component() {
+  useMemo(() => {
+    return [];
+  }, []);
+  return <div />;
+}
+
+```
+
+
+## Error
+
+```
+Found 1 error:
+
+Error: Unused useMemo()
+
+This useMemo() value is unused. useMemo() is for computing and caching values, not for arbitrary side effects.
+
+error.invalid-unused-usememo.ts:3:2
+  1 | // @validateNoVoidUseMemo
+  2 | function Component() {
+> 3 |   useMemo(() => {
+    |   ^^^^^^^ useMemo() result is unused
+  4 |     return [];
+  5 |   }, []);
+  6 |   return <div />;
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-unused-usememo.js

@@ -0,0 +1,7 @@
+// @validateNoVoidUseMemo
+function Component() {
+  useMemo(() => {
+    return [];
+  }, []);
+  return <div />;
+}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useMemo-no-return-value.expect.md

@@ -28,7 +28,7 @@ Found 2 errors:
 
 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.
+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
@@ -45,7 +45,7 @@ error.useMemo-no-return-value.ts:3:16
 
 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.
+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');

compiler/packages/eslint-plugin-react-compiler/__tests__/PluginTest-test.ts

@@ -120,7 +120,7 @@ testRule('plugin-recommended', TestRecommendedRules, {
             
         return <Child x={state} />;
         }`,
-      errors: [makeTestCaseError('useMemo() callbacks must return a value')],
+      errors: [makeTestCaseError('Unused useMemo()')],
     },
     {
       name: 'Pipeline errors are reported',