[compiler] More complete validation against locals being reassigned a…

…fter render

Summary:
This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns.

This causes a number of new bailouts in test cases, all of which appear to me to be legit.

ghstack-source-id: 770bf02d079ea2480be243a49caa6f69573d8092
Pull Request resolved: #30540

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 = {};
+```
+          
+