[compiler] More complete validation against locals being reassigned after 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: b3fe541
Pull Request resolved: #30540

Author: mvitousek

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

@@ -8,6 +8,7 @@
 import {CompilerError, Effect} from '..';
 import {HIRFunction, IdentifierId, Place} from '../HIR';
 import {
+  eachInstructionLValue,
   eachInstructionValueOperand,
   eachTerminalOperand,
 } from '../HIR/visitors';
@@ -139,15 +140,18 @@ function getContextReassignment(
             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 {
+                for (const lval of eachInstructionLValue(instr)) {
+                  reassigningFunctions.set(lval.identifier.id, reassignment);
+                }
+              }
             }
           }
           break;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/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/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/error.context-variable-only-chained-assign.expect.md

@@ -0,0 +1,47 @@
+
+## Input
+
+```javascript
+import {useEffect, useState} from 'react';
+import {mutate} from 'shared-runtime';
+
+function Component(props) {
+  const x = [{...props.value}];
+  useEffect(() => {}, []);
+  const onClick = () => {
+    console.log(x.length);
+  };
+  let y;
+  return (
+    <div onClick={onClick}>
+      {x.map(item => {
+        y = item;
+        return <span key={item.id}>{item.text}</span>;
+      })}
+      {mutate(y)}
+    </div>
+  );
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{value: {id: 0, text: 'Hello!'}}],
+  isComponent: true,
+};
+
+```
+
+
+## Error
+
+```
+  12 |     <div onClick={onClick}>
+  13 |       {x.map(item => {
+> 14 |         y = item;
+     |         ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `y` cannot be reassigned after render (14:14)
+  15 |         return <span key={item.id}>{item.text}</span>;
+  16 |       })}
+  17 |       {mutate(y)}
+```
+          
+      

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.js renamed to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.js

@@ -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.declare-reassign-variable-in-function-declaration.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 = {};
+```
+          
+