[compiler][newinference] ensure fixpoint converges for loops w backedges

The fixpoint converges based on the abstract values being equal, but we synthesize new values cached based on the effects and new effects can be synthesized on the fly. So here we intern the effect objects by "value" (cache key computed from the value) to ensure that effects are stable, values cached based on them are stable, and that the fixpoint can converge.

ghstack-source-id: 9ca53d7
Pull Request resolved: #33449

Author: josephsavona

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

@@ -189,6 +189,7 @@ export function inferMutationAliasingEffects(
 }
 
 class Context {
+  internedEffects: Map<string, AliasingEffect> = new Map();
   instructionSignatureCache: Map<Instruction, InstructionSignature> = new Map();
   effectInstructionValueCache: Map<AliasingEffect, InstructionValue> =
     new Map();
@@ -200,6 +201,17 @@ class Context {
     this.isFuctionExpression = isFunctionExpression;
     this.fn = fn;
   }
+
+  internEffect(effect: AliasingEffect): AliasingEffect {
+    const hash = hashEffect(effect);
+    let interned = this.internedEffects.get(hash);
+    if (interned == null) {
+      console.log(`intern: ${hash}`);
+      this.internedEffects.set(hash, effect);
+      interned = effect;
+    }
+    return interned;
+  }
 }
 
 function inferParam(
@@ -388,10 +400,11 @@ function applySignature(
 function applyEffect(
   context: Context,
   state: InferenceState,
-  effect: AliasingEffect,
+  _effect: AliasingEffect,
   aliased: Set<IdentifierId>,
   effects: Array<AliasingEffect>,
 ): void {
+  const effect = context.internEffect(_effect);
   if (DEBUG) {
     console.log(printAliasingEffect(effect));
   }
@@ -465,15 +478,12 @@ function applyEffect(
           break;
         }
         default: {
-          // 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,
-            });
-          }
+          effects.push({
+            // OK: recording information flow
+            kind: 'CreateFrom', // prev Alias
+            from: effect.from,
+            into: effect.into,
+          });
         }
       }
       break;
@@ -1363,11 +1373,19 @@ function computeSignatureForInstruction(
     }
     case 'PropertyLoad':
     case 'ComputedLoad': {
-      effects.push({
-        kind: 'CreateFrom',
-        from: value.object,
-        into: lvalue,
-      });
+      if (isPrimitiveType(lvalue.identifier)) {
+        effects.push({
+          kind: 'Create',
+          into: lvalue,
+          value: ValueKind.Primitive,
+        });
+      } else {
+        effects.push({
+          kind: 'CreateFrom',
+          from: value.object,
+          into: lvalue,
+        });
+      }
       break;
     }
     case 'PropertyStore':
@@ -1378,8 +1396,11 @@ function computeSignatureForInstruction(
         from: value.value,
         into: value.object,
       });
-      // OK: lvalues are assigned to
-      effects.push({kind: 'Assign', from: value.value, into: lvalue});
+      effects.push({
+        kind: 'Create',
+        into: lvalue,
+        value: ValueKind.Primitive,
+      });
       break;
     }
     case 'ObjectMethod':
@@ -2207,7 +2228,23 @@ export type AliasedPlace = {place: Place; kind: 'alias' | 'capture'};
 
 export type AliasingEffect =
   /**
-   * Marks the given value, its aliases, and indirect captures, as frozen.
+   * Marks the given value and its direct aliases as frozen.
+   *
+   * Captured values are *not* considered frozen, because we cannot be sure that a previously
+   * captured value will still be captured at the point of the freeze.
+   *
+   * For example:
+   * const x = {};
+   * const y = [x];
+   * y.pop(); // y dosn't contain x anymore!
+   * freeze(y);
+   * mutate(x); // safe to mutate!
+   *
+   * The exception to this is FunctionExpressions - since it is impossible to change which
+   * value a function closes over[1] we can transitively freeze functions and their captures.
+   *
+   * [1] Except for `let` values that are reassigned and closed over by a function, but we
+   * handle this explicitly with StoreContext/LoadContext.
    */
   | {kind: 'Freeze'; value: Place; reason: ValueReason}
   /**
@@ -2305,6 +2342,67 @@ export type AliasingEffect =
       error: CompilerErrorDetailOptions;
     };
 
+function hashEffect(effect: AliasingEffect): string {
+  switch (effect.kind) {
+    case 'Apply': {
+      return [
+        effect.kind,
+        effect.receiver.identifier.id,
+        effect.function.identifier.id,
+        effect.mutatesFunction,
+        effect.args
+          .map(a => {
+            if (a.kind === 'Hole') {
+              return '';
+            } else if (a.kind === 'Identifier') {
+              return a.identifier.id;
+            } else {
+              return `...${a.place.identifier.id}`;
+            }
+          })
+          .join(','),
+        effect.into.identifier.id,
+      ].join(':');
+    }
+    case 'CreateFrom':
+    case 'ImmutableCapture':
+    case 'Assign':
+    case 'Alias':
+    case 'Capture': {
+      return [
+        effect.kind,
+        effect.from.identifier.id,
+        effect.into.identifier.id,
+      ].join(':');
+    }
+    case 'Create': {
+      return [effect.kind, effect.into.identifier.id].join(':');
+    }
+    case 'Freeze': {
+      return [effect.kind, effect.value.identifier.id, effect.reason].join(':');
+    }
+    case 'MutateFrozen':
+    case 'MutateGlobal': {
+      return [effect.kind, effect.place.identifier.id].join(':');
+    }
+    case 'Mutate':
+    case 'MutateConditionally':
+    case 'MutateTransitive':
+    case 'MutateTransitiveConditionally': {
+      return [effect.kind, effect.value.identifier.id].join(':');
+    }
+    case 'CreateFunction': {
+      return [
+        effect.kind,
+        effect.into.identifier.id,
+        // return places are a unique way to identify functions themselves
+        effect.function.loweredFunc.func.returns.identifier.id,
+        effect.captures.map(p => p.identifier.id).join(','),
+      ].join(':');
+    }
+  }
+}
+
 export type AliasingSignatureEffect = AliasingEffect;
 
 export type AliasingSignature = {

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

@@ -160,9 +160,6 @@ export function inferMutationAliasingFunctionEffects(
     });
   }
 
-  // console.log(pretty(tracked));
-  // console.log(pretty(dataFlow));
-  // console.log('FunctionEffects', effects.map(printAliasingEffect).join('\n'));
   return effects;
 }
 

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md

@@ -0,0 +1,105 @@
+
+## Input
+
+```javascript
+// @enableNewMutationAliasingModel
+import {arrayPush, Stringify} from 'shared-runtime';
+
+function Component({prop1, prop2}) {
+  'use memo';
+
+  // we'll ultimately extract the item from this array as z, and mutate later
+  let x = [{value: prop1}];
+  let z;
+  while (x.length < 2) {
+    // there's a phi here for x (value before the loop and the reassignment later)
+
+    // this mutation occurs before the reassigned value
+    arrayPush(x, {value: prop2});
+
+    // this condition will never be true, so x doesn't get reassigned
+    if (x[0].value === null) {
+      x = [{value: prop2}];
+      const y = x;
+      z = y[0];
+    }
+  }
+  // the code is set up so that z will always be the value from the original x
+  z.other = true;
+  return <Stringify z={z} />;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{prop1: 0, prop2: 0}],
+  sequentialRenders: [
+    {prop1: 0, prop2: 0},
+    {prop1: 1, prop2: 0},
+    {prop1: 1, prop2: 1},
+    {prop1: 0, prop2: 1},
+    {prop1: 0, prop2: 0},
+  ],
+};
+
+```
+
+## Code
+
+```javascript
+import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel
+import { arrayPush, Stringify } from "shared-runtime";
+
+function Component(t0) {
+  "use memo";
+  const $ = _c(5);
+  const { prop1, prop2 } = t0;
+  let z;
+  if ($[0] !== prop1 || $[1] !== prop2) {
+    let x = [{ value: prop1 }];
+    while (x.length < 2) {
+      arrayPush(x, { value: prop2 });
+      if (x[0].value === null) {
+        x = [{ value: prop2 }];
+        const y = x;
+        z = y[0];
+      }
+    }
+
+    z.other = true;
+    $[0] = prop1;
+    $[1] = prop2;
+    $[2] = z;
+  } else {
+    z = $[2];
+  }
+  let t1;
+  if ($[3] !== z) {
+    t1 = <Stringify z={z} />;
+    $[3] = z;
+    $[4] = t1;
+  } else {
+    t1 = $[4];
+  }
+  return t1;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: Component,
+  params: [{ prop1: 0, prop2: 0 }],
+  sequentialRenders: [
+    { prop1: 0, prop2: 0 },
+    { prop1: 1, prop2: 0 },
+    { prop1: 1, prop2: 1 },
+    { prop1: 0, prop2: 1 },
+    { prop1: 0, prop2: 0 },
+  ],
+};
+
+```
+      
+### Eval output
+(kind: ok) [[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
+[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
+[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
+[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
+[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js

@@ -0,0 +1,35 @@
+// @enableNewMutationAliasingModel
+import {arrayPush, Stringify} from 'shared-runtime';
+
+function Component({prop1, prop2}) {
+  'use memo';
+
+  let x = [{value: prop1}];
+  let z;
+  while (x.length < 2) {
+    // there's a phi here for x (value before the loop and the reassignment later)
+
+    // this mutation occurs before the reassigned value
+    arrayPush(x, {value: prop2});
+
+    if (x[0].value === prop1) {
+      x = [{value: prop2}];
+      const y = x;
+      z = y[0];
+    }
+  }
+  z.other = true;
+  return <Stringify z={z} />;
+}
+
+// export const FIXTURE_ENTRYPOINT = {
+//   fn: Component,
+//   params: [{prop1: 0, prop2: 0}],
+//   sequentialRenders: [
+//     {prop1: 0, prop2: 0},
+//     {prop1: 1, prop2: 0},
+//     {prop1: 1, prop2: 1},
+//     {prop1: 0, prop2: 1},
+//     {prop1: 0, prop2: 0},
+//   ],
+// };