[compiler] Remove redundant InferMutableContextVariables (#32097)

This removes special casing for `PropertyStore` mutability inference
within FunctionExpressions.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32097).
* #32287
* #32104
* #32098
* __->__ #32097

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

@@ -10,7 +10,6 @@ import {
   Effect,
   HIRFunction,
   Identifier,
-  IdentifierId,
   LoweredFunction,
   isRefOrRefValue,
   makeInstructionId,
@@ -18,27 +17,9 @@ import {
 import {deadCodeElimination} from '../Optimization';
 import {inferReactiveScopeVariables} from '../ReactiveScopes';
 import {rewriteInstructionKindsBasedOnReassignment} from '../SSA';
-import {inferMutableContextVariables} from './InferMutableContextVariables';
 import {inferMutableRanges} from './InferMutableRanges';
 import inferReferenceEffects from './InferReferenceEffects';
 
-// Helper class to track indirections such as LoadLocal and PropertyLoad.
-export class IdentifierState {
-  properties: Map<IdentifierId, Identifier> = new Map();
-
-  resolve(identifier: Identifier): Identifier {
-    const resolved = this.properties.get(identifier.id);
-    if (resolved !== undefined) {
-      return resolved;
-    }
-    return identifier;
-  }
-
-  alias(lvalue: Identifier, value: Identifier): void {
-    this.properties.set(lvalue.id, this.properties.get(value.id) ?? value);
-  }
-}
-
 export default function analyseFunctions(func: HIRFunction): void {
   for (const [_, block] of func.body.blocks) {
     for (const instr of block.instructions) {
@@ -78,7 +59,6 @@ function lower(func: HIRFunction): void {
 }
 
 function infer(loweredFunc: LoweredFunction): void {
-  const knownMutated = inferMutableContextVariables(loweredFunc.func);
   for (const operand of loweredFunc.func.context) {
     const identifier = operand.identifier;
     CompilerError.invariant(operand.effect === Effect.Unknown, {
@@ -95,10 +75,11 @@ function infer(loweredFunc: LoweredFunction): void {
        * render
        */
       operand.effect = Effect.Capture;
-    } else if (knownMutated.has(operand)) {
-      operand.effect = Effect.Mutate;
     } else if (isMutatedOrReassigned(identifier)) {
-      // Note that this also reflects if identifier is ConditionallyMutated
+      /**
+       * Reflects direct reassignments, PropertyStores, and ConditionallyMutate
+       * (directly or through maybe-aliases)
+       */
       operand.effect = Effect.Capture;
     } else {
       operand.effect = Effect.Read;

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

@@ -1,105 +0,0 @@
-/**
- * Copyright (c) Meta Platforms, Inc. and affiliates.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- */
-
-import {Effect, HIRFunction, Identifier, Place} from '../HIR';
-import {
-  eachInstructionValueOperand,
-  eachTerminalOperand,
-} from '../HIR/visitors';
-import {IdentifierState} from './AnalyseFunctions';
-
-/*
- * This pass infers which of the given function's context (free) variables
- * are definitively mutated by the function. This analysis is *partial*,
- * and only annotates provable mutations, and may miss mutations via indirections.
- * The intent of this pass is to drive validations, rejecting known-bad code
- * while avoiding false negatives, and the inference should *not* be used to
- * drive changes in output.
- *
- * Note that a complete analysis is possible but would have too many false negatives.
- * The approach would be to run LeaveSSA and InferReactiveScopeVariables in order to
- * find all possible aliases of a context variable which may be mutated. However, this
- * can lead to false negatives:
- *
- * ```
- * const [x, setX] = useState(null); // x is frozen
- * const fn = () => { // context=[x]
- *    const z = {}; // z is mutable
- *    foo(z, x); // potentially mutate z and x
- *    z.a = true; // definitively mutate z
- * }
- * fn();
- * ```
- *
- * When we analyze function expressions we assume that context variables are mutable,
- * so we assume that `x` is mutable. We infer that `foo(z, x)` could be mutating the
- * two variables to alias each other, such that `z.a = true` could be mutating `x`,
- * and we would infer that `x` is definitively mutated. Then when we run
- * InferReferenceEffects on the outer code we'd reject it, since there is a definitive
- * mutation of a frozen value.
- *
- * Thus the actual implementation looks at only basic aliasing. The above example would
- * pass, since z does not directly alias `x`. However, mutations through trivial aliases
- * are detected:
- *
- * ```
- * const [x, setX] = useState(null); // x is frozen
- * const fn = () => { // context=[x]
- *    const z = x;
- *    z.a = true; // ERROR: mutates x
- * }
- * fn();
- * ```
- */
-export function inferMutableContextVariables(fn: HIRFunction): Set<Place> {
-  const state = new IdentifierState();
-  const knownMutatedIdentifiers = new Set<Identifier>();
-  for (const [, block] of fn.body.blocks) {
-    for (const instr of block.instructions) {
-      switch (instr.value.kind) {
-        case 'PropertyLoad':
-        case 'ComputedLoad': {
-          state.alias(instr.lvalue.identifier, instr.value.object.identifier);
-          break;
-        }
-        case 'LoadLocal':
-        case 'LoadContext': {
-          if (instr.lvalue.identifier.name === null) {
-            state.alias(instr.lvalue.identifier, instr.value.place.identifier);
-          }
-          break;
-        }
-        default: {
-          for (const operand of eachInstructionValueOperand(instr.value)) {
-            visitOperand(state, knownMutatedIdentifiers, operand);
-          }
-        }
-      }
-    }
-    for (const operand of eachTerminalOperand(block.terminal)) {
-      visitOperand(state, knownMutatedIdentifiers, operand);
-    }
-  }
-  const results = new Set<Place>();
-  for (const operand of fn.context) {
-    if (knownMutatedIdentifiers.has(operand.identifier)) {
-      results.add(operand);
-    }
-  }
-  return results;
-}
-
-function visitOperand(
-  state: IdentifierState,
-  knownMutatedIdentifiers: Set<Identifier>,
-  operand: Place,
-): void {
-  const resolved = state.resolve(operand.identifier);
-  if (operand.effect === Effect.Mutate || operand.effect === Effect.Store) {
-    knownMutatedIdentifiers.add(resolved);
-  }
-}

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md

@@ -19,9 +19,14 @@ function Component() {
 
   // capture into a separate variable that is not a context variable.
   const y = x;
+  /**
+   * Note that this fixture currently produces a stale effect closure if `y = x
+   * = someGlobal` changes between renders. Under current compiler assumptions,
+   * that would be a rule of react violation.
+   */
   useEffect(() => {
     y.value = 'hello';
-  }, []);
+  });
 
   useEffect(() => {
     setState(someGlobal.value);
@@ -46,57 +51,50 @@ import { useEffect, useState } from "react";
 let someGlobal = { value: null };
 
 function Component() {
-  const $ = _c(7);
+  const $ = _c(5);
   const [state, setState] = useState(someGlobal);
-  let t0;
-  let t1;
-  let t2;
-  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
-    let x = someGlobal;
-    while (x == null) {
-      x = someGlobal;
-    }
 
-    const y = x;
-    t0 = useEffect;
-    t1 = () => {
+  let x = someGlobal;
+  while (x == null) {
+    x = someGlobal;
+  }
+
+  const y = x;
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = () => {
       y.value = "hello";
     };
-    t2 = [];
     $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  useEffect(t0);
+  let t1;
+  let t2;
+  if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
+    t1 = () => {
+      setState(someGlobal.value);
+    };
+    t2 = [someGlobal];
     $[1] = t1;
     $[2] = t2;
   } else {
-    t0 = $[0];
     t1 = $[1];
     t2 = $[2];
   }
-  t0(t1, t2);
-  let t3;
+  useEffect(t1, t2);
+
+  const t3 = String(state);
   let t4;
-  if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
-    t3 = () => {
-      setState(someGlobal.value);
-    };
-    t4 = [someGlobal];
+  if ($[3] !== t3) {
+    t4 = <div>{t3}</div>;
     $[3] = t3;
     $[4] = t4;
   } else {
-    t3 = $[3];
     t4 = $[4];
   }
-  useEffect(t3, t4);
-
-  const t5 = String(state);
-  let t6;
-  if ($[5] !== t5) {
-    t6 = <div>{t5}</div>;
-    $[5] = t5;
-    $[6] = t6;
-  } else {
-    t6 = $[6];
-  }
-  return t6;
+  return t4;
 }
 
 export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js

@@ -15,9 +15,14 @@ function Component() {
 
   // capture into a separate variable that is not a context variable.
   const y = x;
+  /**
+   * Note that this fixture currently produces a stale effect closure if `y = x
+   * = someGlobal` changes between renders. Under current compiler assumptions,
+   * that would be a rule of react violation.
+   */
   useEffect(() => {
     y.value = 'hello';
-  }, []);
+  });
 
   useEffect(() => {
     setState(someGlobal.value);

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md

@@ -36,21 +36,22 @@ import { useSharedValue } from "react-native-reanimated";
  * of render
  */
 function SomeComponent() {
-  const $ = _c(3);
+  const $ = _c(2);
   const sharedVal = useSharedValue(0);
-
-  const T0 = Button;
-  const t0 = () => (sharedVal.value = Math.random());
-  let t1;
-  if ($[0] !== T0 || $[1] !== t0) {
-    t1 = <T0 onPress={t0} title="Randomize" />;
-    $[0] = T0;
+  let t0;
+  if ($[0] !== sharedVal) {
+    t0 = (
+      <Button
+        onPress={() => (sharedVal.value = Math.random())}
+        title="Randomize"
+      />
+    );
+    $[0] = sharedVal;
     $[1] = t0;
-    $[2] = t1;
   } else {
-    t1 = $[2];
+    t0 = $[1];
   }
-  return t1;
+  return t0;
 }
 
 ```