From ba3cadffe9662e0988ec085097389760eeefc402 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Jun 2025 14:00:56 -0700 Subject: [PATCH] [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-poisoned] --- .../Inference/InferMutationAliasingEffects.ts | 135 +++++++++++++++--- ...backedge-phi-with-later-mutation.expect.md | 105 ++++++++++++++ ...apture-backedge-phi-with-later-mutation.js | 35 +++++ 3 files changed, 257 insertions(+), 18 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index 67f1e75a2368..ab5e76d22f60 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -63,6 +63,7 @@ import {FunctionSignature} from '../HIR/ObjectShape'; import {getWriteErrorReason} from './InferFunctionEffects'; import prettyFormat from 'pretty-format'; import {createTemporaryPlace} from '../HIR/HIRBuilder'; +import {pretty} from './InferMutationAliasingRanges'; export function inferMutationAliasingEffects( fn: HIRFunction, @@ -189,6 +190,7 @@ export function inferMutationAliasingEffects( } class Context { + internedEffects: Map = new Map(); instructionSignatureCache: Map = new Map(); effectInstructionValueCache: Map = new Map(); @@ -200,6 +202,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 +401,11 @@ function applySignature( function applyEffect( context: Context, state: InferenceState, - effect: AliasingEffect, + _effect: AliasingEffect, aliased: Set, effects: Array, ): void { + const effect = context.internEffect(_effect); if (DEBUG) { console.log(printAliasingEffect(effect)); } @@ -465,15 +479,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 +1374,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 +1397,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 +2229,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 +2343,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 = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.expect.md new file mode 100644 index 000000000000..e310d0d71e0c --- /dev/null +++ b/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 ; +} + +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 = ; + $[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') ]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/capture-backedge-phi-with-later-mutation.js new file mode 100644 index 000000000000..bda11b500191 --- /dev/null +++ b/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 ; +} + +// 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}, +// ], +// };