[compiler] More complete validation against locals being reassigned after render

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

@@ -8,9 +8,11 @@
 import {CompilerError, Effect} from '..';
 import {HIRFunction, IdentifierId, Place} from '../HIR';
 import {
+  eachInstructionLValue,
   eachInstructionValueOperand,
   eachTerminalOperand,
 } from '../HIR/visitors';
+import {getFunctionCallSignature} from '../Inference/InferReferenceEffects';
 
 /**
  * Validates that local variables cannot be reassigned after render.
@@ -131,23 +133,49 @@ function getContextReassignment(
           break;
         }
         default: {
-          for (const operand of eachInstructionValueOperand(value)) {
+          let operands = eachInstructionValueOperand(value);
+          // If we're calling a function that doesn't let its arguments escape, only test the callee
+          if (value.kind === 'CallExpression') {
+            const signature = getFunctionCallSignature(
+              fn.env,
+              value.callee.identifier.type,
+            );
+            if (signature?.noAlias) {
+              operands = [value.callee];
+            }
+          } else if (value.kind === 'MethodCall') {
+            const signature = getFunctionCallSignature(
+              fn.env,
+              value.property.identifier.type,
+            );
+            if (signature?.noAlias) {
+              operands = [value.receiver, value.property];
+            }
+          }
+          for (const operand of operands) {
             CompilerError.invariant(operand.effect !== Effect.Unknown, {
               reason: `Expected effects to be inferred prior to ValidateLocalsNotReassignedAfterRender`,
               loc: operand.loc,
             });
             const reassignment = reassigningFunctions.get(
               operand.identifier.id,
             );
-            if (
-              reassignment !== undefined &&
-              operand.effect === Effect.Freeze
-            ) {
+            if (reassignment !== undefined) {
               /*
                * Functions that reassign local variables are inherently mutable and are unsafe to pass
                * to a place that expects a frozen value. Propagate the reassignment upward.
                */
-              return reassignment;
+              if (operand.effect === Effect.Freeze) {
+                return reassignment;
+              } else {
+                /*
+                 * If the operand is not frozen but it does reassign, then the lvalues
+                 * of the instruction could also be reassigning
+                 */
+                for (const lval of eachInstructionLValue(instr)) {
+                  reassigningFunctions.set(lval.identifier.id, reassignment);
+                }
+              }
             }
           }
           break;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.expect.md

@@ -0,0 +1,40 @@
+
+## Input
+
+```javascript
+import {identity, invoke} from 'shared-runtime';
+
+function foo() {
+  let x = 2;
+  const fn1 = () => {
+    const copy1 = (x = 3);
+    return identity(copy1);
+  };
+  const fn2 = () => {
+    const copy2 = (x = 4);
+    return [invoke(fn1), copy2, identity(copy2)];
+  };
+  return invoke(fn2);
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: foo,
+  params: [],
+};
+
+```
+
+
+## Error
+
+```
+   8 |   };
+   9 |   const fn2 = () => {
+> 10 |     const copy2 = (x = 4);
+     |                    ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `x` cannot be reassigned after render (10:10)
+  11 |     return [invoke(fn1), copy2, identity(copy2)];
+  12 |   };
+  13 |   return invoke(fn2);
+```
+
+

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.expect.md

@@ -0,0 +1,29 @@
+
+## Input
+
+```javascript
+function Component() {
+  let x = null;
+  function foo() {
+    x = 9;
+  }
+  const y = bar(foo);
+  return <Child y={y} />;
+}
+
+```
+
+
+## Error
+
+```
+  2 |   let x = null;
+  3 |   function foo() {
+> 4 |     x = 9;
+    |     ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `x` cannot be reassigned after render (4:4)
+  5 |   }
+  6 |   const y = bar(foo);
+  7 |   return <Child y={y} />;
+```
+
+

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md → compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.expect.md

@@ -42,60 +42,17 @@ function Component() {
 
 ```
 
-## Code
 
-```javascript
-import { c as _c } from "react/compiler-runtime";
-import { useEffect } from "react";
-function Component() {
-  const $ = _c(4);
-  let local;
-  let t0;
-  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
-    const mk_reassignlocal = () => {
-      const reassignLocal = (newValue) => {
-        local = newValue;
-      };
-      return reassignLocal;
-    };
-
-    t0 = mk_reassignlocal();
-    $[0] = t0;
-  } else {
-    t0 = $[0];
-  }
-  const reassignLocal_0 = t0;
-  let t1;
-  if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
-    t1 = (newValue_0) => {
-      reassignLocal_0("hello");
-      if (local === newValue_0) {
-        console.log("`local` was updated!");
-      } else {
-        throw new Error("`local` not updated!");
-      }
-    };
-    $[1] = t1;
-  } else {
-    t1 = $[1];
-  }
-  const onMount = t1;
-  let t2;
-  let t3;
-  if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
-    t2 = () => {
-      onMount();
-    };
-    t3 = [onMount];
-    $[2] = t2;
-    $[3] = t3;
-  } else {
-    t2 = $[2];
-    t3 = $[3];
-  }
-  useEffect(t2, t3);
-  return "ok";
-}
+## Error
 
 ```
+   5 |     // Create the reassignment function inside another function, then return it
+   6 |     const reassignLocal = newValue => {
+>  7 |       local = newValue;
+     |       ^^^^^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `local` cannot be reassigned after render (7:7)
+   8 |     };
+   9 |     return reassignLocal;
+  10 |   };
+```
+
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.expect.md

@@ -0,0 +1,44 @@
+
+## Input
+
+```javascript
+// @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions
+let cond = true;
+function Component(props) {
+  let a;
+  let b;
+  const f = () => {
+    if (cond) {
+      a = {};
+      b = [];
+    } else {
+      a = {};
+      b = [];
+    }
+    a.property = true;
+    b.push(false);
+  };
+  return <div onClick={f()} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{}],
+};
+
+```
+
+
+## Error
+
+```
+   6 |   const f = () => {
+   7 |     if (cond) {
+>  8 |       a = {};
+     |       ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `a` cannot be reassigned after render (8:8)
+   9 |       b = [];
+  10 |     } else {
+  11 |       a = {};
+```
+
+