[compiler][newinference] Fix for phi types, extracting primitives from objects

First a quick fix: if we have a known type for the lvalue of CreateFrom, we can drop the effect. This is a bit awkward generally because the types and abstract values overlap a bit, and i'd prefer to only look at types during one phase. So maybe move all the type-checking to where we generate effects, and then after that applyEffect() doesn't have to consider types.

The larger fix is for InferMutationAliasingRanges. When processing a mutation we have to walk the graph in both forward and backwards directions. Consider `alias a -> b, mutate(b)`. We have to walk back the alias chain from b to a, and mark a as mutated too. But for `alias a -> b, mutate(a)`, we also have to mark b as mutated — walking forwards along the alias chain.

But phis are a bit different. You can have a mutation of one of the phi operands, such that you have `a, b -> phi c, mutate(a)`. Here, we do need to mark c as mutated, but we should not walk back to b — it's a different operand! a and b don't alias together.

There are now about 150 fixtures failing, but they're in a few categories and all of them are addressable:
* Infinite loops. `applyEffect()` creates new un-memoized effect values which means that any input with a backedge (loop) will spin until my infinite loop detection throws. This is somewhat tedious to address but it's a pragmatic concern and not a flaw in the model. I also need to convince myself that the approach in InferMutationAliasingRanges is safe for loops, but first i have to get inputs w loops to even reach that phase.
* LoadContext/StoreContext - i currently treat these too similarly to regular load/store, ie assuming the mutations happen in order. One idea is to treat LoadContext as a mutate instead of an alias, just to make sure all instances get grouped together.
* InvalidReact detection. We already synthesize MutateFrozen/MutateGlobal effects but we don't throw these as errors yet. This is probably the largest category of failing tests, which means overall this is actually "pretty close" (so, 50% of the way there).

ghstack-source-id: 7d6374c
Pull Request resolved: #33440

Author: josephsavona

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

@@ -25,6 +25,7 @@ import {
   InstructionValue,
   isArrayType,
   isMapType,
+  isPrimitiveType,
   isSetType,
   makeIdentifierId,
   ObjectMethod,
@@ -464,12 +465,15 @@ function applyEffect(
           break;
         }
         default: {
-          effects.push({
-            // OK: recording information flow
-            kind: 'CreateFrom', // prev Alias
-            from: effect.from,
-            into: effect.into,
-          });
+          // Even if the source is non-primitive, the destination may be. skip primitives.
+          if (!isPrimitiveType(effect.into.identifier)) {
+            effects.push({
+              // OK: recording information flow
+              kind: 'CreateFrom', // prev Alias
+              from: effect.from,
+              into: effect.into,
+            });
+          }
         }
       }
       break;
@@ -1622,6 +1626,7 @@ function computeSignatureForInstruction(
           suggestions: null,
         },
       });
+      effects.push({kind: 'Assign', from: value.value, into: lvalue});
       break;
     }
     case 'TypeCastExpression': {

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

@@ -28,12 +28,13 @@ import {printIdentifier, printPlace} from '../HIR/PrintHIR';
 import {MutationKind} from './InferMutationAliasingFunctionEffects';
 
 const DEBUG = false;
+const VERBOSE = false;
 
 /**
  * Infers mutable ranges for all values.
  */
 export function inferMutationAliasingRanges(fn: HIRFunction): void {
-  if (DEBUG) {
+  if (VERBOSE) {
     console.log();
     console.log();
     console.log();
@@ -81,7 +82,7 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
   const seenBlocks = new Set<BlockId>();
   for (const block of fn.body.blocks.values()) {
     for (const phi of block.phis) {
-      state.create(phi.place);
+      state.create(phi.place, true);
       for (const [pred, operand] of phi.operands) {
         if (!seenBlocks.has(pred)) {
           // NOTE: annotation required to actually typecheck and not silently infer `any`
@@ -172,7 +173,7 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
     }
   }
 
-  if (DEBUG) {
+  if (VERBOSE) {
     console.log(state.debug());
     console.log(pretty(mutations));
   }
@@ -185,7 +186,7 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
       mutation.kind,
     );
   }
-  if (DEBUG) {
+  if (VERBOSE) {
     console.log(state.debug());
   }
   fn.aliasingEffects ??= [];
@@ -364,7 +365,7 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void {
     }
   }
 
-  if (DEBUG) {
+  if (VERBOSE) {
     console.log(printFunction(fn));
   }
 }
@@ -377,11 +378,12 @@ type Node = {
   edges: Array<{index: number; node: Identifier; kind: 'capture' | 'alias'}>;
   transitive: MutationKind;
   local: MutationKind;
+  phi: boolean;
 };
 class AliasingState {
   nodes: Map<Identifier, Node> = new Map();
 
-  create(place: Place): void {
+  create(place: Place, phi: boolean = false): void {
     this.nodes.set(place.identifier, {
       id: place.identifier,
       createdFrom: new Map(),
@@ -390,6 +392,7 @@ class AliasingState {
       edges: [],
       transitive: MutationKind.None,
       local: MutationKind.None,
+      phi,
     });
   }
 
@@ -398,7 +401,7 @@ class AliasingState {
     const fromNode = this.nodes.get(from.identifier);
     const toNode = this.nodes.get(into.identifier);
     if (fromNode == null || toNode == null) {
-      if (DEBUG) {
+      if (VERBOSE) {
         console.log(
           `skip: createFrom ${printPlace(from)}${!!fromNode} -> ${printPlace(into)}${!!toNode}`,
         );
@@ -415,7 +418,7 @@ class AliasingState {
     const fromNode = this.nodes.get(from.identifier);
     const toNode = this.nodes.get(into.identifier);
     if (fromNode == null || toNode == null) {
-      if (DEBUG) {
+      if (VERBOSE) {
         console.log(
           `skip: capture ${printPlace(from)}${!!fromNode} -> ${printPlace(into)}${!!toNode}`,
         );
@@ -432,7 +435,7 @@ class AliasingState {
     const fromNode = this.nodes.get(from.identifier);
     const toNode = this.nodes.get(into.identifier);
     if (fromNode == null || toNode == null) {
-      if (DEBUG) {
+      if (VERBOSE) {
         console.log(
           `skip: assign ${printPlace(from)}${!!fromNode} -> ${printPlace(into)}${!!toNode}`,
         );
@@ -453,9 +456,13 @@ class AliasingState {
     kind: MutationKind,
   ): void {
     const seen = new Set<Identifier>();
-    const queue = [{place: start, transitive}];
+    const queue: Array<{
+      place: Identifier;
+      transitive: boolean;
+      direction: 'backwards' | 'forwards';
+    }> = [{place: start, transitive, direction: 'backwards'}];
     while (queue.length !== 0) {
-      const {place: current, transitive} = queue.pop()!;
+      const {place: current, transitive, direction} = queue.pop()!;
       if (seen.has(current)) {
         continue;
       }
@@ -471,7 +478,7 @@ class AliasingState {
       }
       if (DEBUG) {
         console.log(
-          `mutate $${node.id.id} via ${printIdentifier(start)} at [${end}]`,
+          `[${end}] mutate index=${index} ${printIdentifier(start)}: ${printIdentifier(node.id)}`,
         );
       }
       node.id.mutableRange.end = makeInstructionId(
@@ -491,24 +498,31 @@ class AliasingState {
         if (edge.index >= index) {
           break;
         }
-        queue.push({place: edge.node, transitive});
+        queue.push({place: edge.node, transitive, direction: 'forwards'});
       }
       for (const [alias, when] of node.createdFrom) {
         if (when >= index) {
           continue;
         }
-        queue.push({place: alias, transitive: true});
+        queue.push({place: alias, transitive: true, direction: 'backwards'});
       }
-      /**
-       * all mutations affect backward alias edges by the rules:
-       * - Alias a -> b, mutate(b) => mutate(a)
-       * - Alias a -> b, mutateTransitive(b) => mutate(a)
-       */
-      for (const [alias, when] of node.aliases) {
-        if (when >= index) {
-          continue;
+      if (direction === 'backwards' || !node.phi) {
+        /**
+         * all mutations affect backward alias edges by the rules:
+         * - Alias a -> b, mutate(b) => mutate(a)
+         * - Alias a -> b, mutateTransitive(b) => mutate(a)
+         *
+         * However, if we reached a phi because one of its inputs was mutated
+         * (and we're advancing "forwards" through that node's edges), then
+         * we know we've already processed the mutation at its source. The
+         * phi's other inputs can't be affected.
+         */
+        for (const [alias, when] of node.aliases) {
+          if (when >= index) {
+            continue;
+          }
+          queue.push({place: alias, transitive, direction: 'backwards'});
         }
-        queue.push({place: alias, transitive});
       }
       /**
        * but only transitive mutations affect captures
@@ -518,10 +532,18 @@ class AliasingState {
           if (when >= index) {
             continue;
           }
-          queue.push({place: capture, transitive});
+          queue.push({place: capture, transitive, direction: 'backwards'});
         }
       }
     }
+    if (DEBUG) {
+      const nodes = new Map();
+      for (const id of seen) {
+        const node = this.nodes.get(id);
+        nodes.set(id.id, node);
+      }
+      console.log(pretty(nodes));
+    }
   }
 
   debug(): string {

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts

@@ -44,7 +44,7 @@ import {
   getHookKind,
   makeIdentifierName,
 } from '../HIR/HIR';
-import {printIdentifier, printPlace} from '../HIR/PrintHIR';
+import {printIdentifier, printInstruction, printPlace} from '../HIR/PrintHIR';
 import {eachPatternOperand} from '../HIR/visitors';
 import {Err, Ok, Result} from '../Utils/Result';
 import {GuardKind} from '../Utils/RuntimeDiagnosticConstants';
@@ -1310,7 +1310,7 @@ function codegenInstructionNullable(
         });
         CompilerError.invariant(value?.type === 'FunctionExpression', {
           reason: 'Expected a function as a function declaration value',
-          description: null,
+          description: `Got ${value == null ? String(value) : value.type} at ${printInstruction(instr)}`,
           loc: instr.value.loc,
           suggestions: null,
         });

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/mutate-through-boxing-unboxing-function-call-indirections-2.expect.md

@@ -0,0 +1,67 @@
+
+## Input
+
+```javascript
+// @enableNewMutationAliasingModel
+import {Stringify} from 'shared-runtime';
+
+function Component({a, b}) {
+  const x = {a, b};
+  const f = () => {
+    const y = [x];
+    return y[0];
+  };
+  const x0 = f();
+  const z = [x0];
+  const x1 = z[0];
+  x1.key = 'value';
+  return <Stringify x={x} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{a: 0, b: 1}],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel
+import { Stringify } from "shared-runtime";
+
+function Component(t0) {
+  const $ = _c(3);
+  const { a, b } = t0;
+  let t1;
+  if ($[0] !== a || $[1] !== b) {
+    const x = { a, b };
+    const f = () => {
+      const y = [x];
+      return y[0];
+    };
+
+    const x0 = f();
+    const z = [x0];
+    const x1 = z[0];
+    x1.key = "value";
+    t1 = <Stringify x={x} />;
+    $[0] = a;
+    $[1] = b;
+    $[2] = t1;
+  } else {
+    t1 = $[2];
+  }
+  return t1;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ a: 0, b: 1 }],
+};
+
+```
+      
+### Eval output
+(kind: ok) <div>{"x":{"a":0,"b":1,"key":"value"}}</div>

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/mutate-through-boxing-unboxing-function-call-indirections-2.js

@@ -0,0 +1,20 @@
+// @enableNewMutationAliasingModel
+import {Stringify} from 'shared-runtime';
+
+function Component({a, b}) {
+  const x = {a, b};
+  const f = () => {
+    const y = [x];
+    return y[0];
+  };
+  const x0 = f();
+  const z = [x0];
+  const x1 = z[0];
+  x1.key = 'value';
+  return <Stringify x={x} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{a: 0, b: 1}],
+};