From 74c236a99d17318002b52515155867dcff5266f7 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 28 May 2025 15:42:38 -0700 Subject: [PATCH] [compiler] Add ImmutableCapture effect, CreateFrom no longer needs Capture ImmutableCapture allows us to record information flow for escape analysis purposes, without impacting mutable range analysis. All of Alias, Capture, and CreateFrom now downgrade to ImmutableCapture if the `from` value is frozen. Globals and primitives are conceptually copy types and don't record any information flow at all. I guess we could add a `Copy` effect if we wanted but i haven't seen a need for it yet. Related, previously CreateFrom was always paired with Capture when constructing effects. But I had also coded up the inference to sort of treat them the same. So this diff makes that official, and means CreateFrom is a valid third way to represent data flow and not just creation. When applied, CreateFrom will turn into: ImmutableCapture for frozen values, Capture for mutable values, and disappear for globals/primitives. A final tweak is to change range inference to ensure that range.start is non-zero if the value is mutated. [ghstack-poisoned] --- .../src/HIR/PrintHIR.ts | 7 +- .../src/Inference/AnalyseFunctions.ts | 7 +- .../Inference/InferMutationAliasingEffects.ts | 136 +++++++++++------- .../Inference/InferMutationAliasingRanges.ts | 25 +++- .../InferReactiveScopeVariables.ts | 4 +- ...on-with-shadowed-local-same-name.expect.md | 2 +- ...mutation-via-function-expression.expect.md | 58 ++++++++ .../basic-mutation-via-function-expression.js | 11 ++ .../new-mutability/basic-mutation.expect.md | 42 ++++++ .../compiler/new-mutability/basic-mutation.js | 8 ++ ...-mutation-in-function-expression.expect.md | 59 ++++++++ ...tential-mutation-in-function-expression.js | 10 ++ 12 files changed, 308 insertions(+), 61 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 2cd2cfe63305..3bedd76ba8d1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -949,10 +949,13 @@ function getFunctionName( export function printAliasingEffect(effect: AliasingEffect): string { switch (effect.kind) { case 'Alias': { - return `Alias ${printPlaceForAliasEffect(effect.from)} -> ${printPlaceForAliasEffect(effect.into)}`; + return `Alias ${printPlaceForAliasEffect(effect.into)} = ${printPlaceForAliasEffect(effect.from)}`; } case 'Capture': { - return `Capture ${printPlaceForAliasEffect(effect.from)} -> ${printPlaceForAliasEffect(effect.into)}`; + return `Capture ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; + } + case 'ImmutableCapture': { + return `ImmutableCapture ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; } case 'Create': { return `Create ${printPlaceForAliasEffect(effect.into)} = ${effect.value}`; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts index 23cbf27a8d83..12b99b8b157b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -14,6 +14,7 @@ import { Place, isRefOrRefValue, makeInstructionId, + printFunction, } from '../HIR'; import {deadCodeElimination} from '../Optimization'; import {inferReactiveScopeVariables} from '../ReactiveScopes'; @@ -26,10 +27,7 @@ import { eachInstructionValueOperand, } from '../HIR/visitors'; import {Iterable_some} from '../Utils/utils'; -import { - AliasingEffect, - inferMutationAliasingEffects, -} from './InferMutationAliasingEffects'; +import {inferMutationAliasingEffects} from './InferMutationAliasingEffects'; import {inferMutationAliasingFunctionEffects} from './InferMutationAliasingFunctionEffects'; import {inferMutationAliasingRanges} from './InferMutationAliasingRanges'; @@ -44,7 +42,6 @@ export default function analyseFunctions(func: HIRFunction): void { infer(instr.value.loweredFunc, aliases); } else { lowerWithMutationAliasing(instr.value.loweredFunc.func); - infer(instr.value.loweredFunc, new DisjointSet()); } /** 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 a222dd59b121..ee19dbc03b3b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -38,7 +38,9 @@ import { Set_isSuperset, } from '../Utils/utils'; import { + printAliasingEffect, printIdentifier, + printInstruction, printInstructionValue, printPlace, printSourceLocation, @@ -258,6 +260,23 @@ function applySignature( state.define(effect.into, value); break; } + case 'ImmutableCapture': { + let value = effectInstructionValueCache.get(effect); + if (value == null) { + value = { + kind: 'ObjectExpression', + properties: [], + loc: effect.into.loc, + }; + effectInstructionValueCache.set(effect, value); + } + state.initialize(value, { + kind: ValueKind.Frozen, + reason: new Set([ValueReason.Other]), + }); + state.define(effect.into, value); + break; + } case 'CreateFrom': { const kind = state.kind(effect.from).kind; let value = effectInstructionValueCache.get(effect); @@ -274,6 +293,29 @@ function applySignature( reason: new Set([ValueReason.Other]), }); state.define(effect.into, value); + switch (kind) { + case ValueKind.Primitive: + case ValueKind.Global: { + // no need to track this data flow + break; + } + case ValueKind.Frozen: { + effects.push({ + kind: 'ImmutableCapture', + from: effect.from, + into: effect.into, + }); + break; + } + default: { + // TODO: should this be Alias?? + effects.push({ + kind: 'Capture', + from: effect.from, + into: effect.into, + }); + } + } break; } case 'Capture': { @@ -297,27 +339,28 @@ function applySignature( } } const fromKind = state.kind(effect.from).kind; - let isCopyByReferenceValue: boolean; + let isMutableReferenceType: boolean; switch (fromKind) { case ValueKind.Global: case ValueKind.Primitive: { - isCopyByReferenceValue = false; + isMutableReferenceType = false; break; } case ValueKind.Frozen: { - /* - * TODO: add a separate "ImmutableAlias" effect to downgrade to, that doesn't impact mutable ranges - * We want to remember that the data flow occurred for PruneNonEscapingScopes - */ - isCopyByReferenceValue = false; + isMutableReferenceType = false; + effects.push({ + kind: 'ImmutableCapture', + from: effect.from, + into: effect.into, + }); break; } default: { - isCopyByReferenceValue = true; + isMutableReferenceType = true; break; } } - if (isMutableDesination && isCopyByReferenceValue) { + if (isMutableDesination && isMutableReferenceType) { effects.push(effect); } break; @@ -329,12 +372,25 @@ function applySignature( */ const fromKind = state.kind(effect.from).kind; switch (fromKind) { - /* - * TODO: add a separate "ImmutableAlias" effect to downgrade to, that doesn't impact mutable ranges - * We want to remember that the data flow occurred for PruneNonEscapingScopes - * (use this to replace the ValueKind.Frozen case) - */ - case ValueKind.Frozen: + case ValueKind.Frozen: { + effects.push({ + kind: 'ImmutableCapture', + from: effect.from, + into: effect.into, + }); + let value = effectInstructionValueCache.get(effect); + if (value == null) { + value = { + kind: 'Primitive', + value: undefined, + loc: effect.from.loc, + }; + effectInstructionValueCache.set(effect, value); + } + state.initialize(value, {kind: fromKind, reason: new Set([])}); + state.define(effect.into, value); + break; + } case ValueKind.Global: case ValueKind.Primitive: { let value = effectInstructionValueCache.get(effect); @@ -385,24 +441,7 @@ function applySignature( case 'MutateTransitiveConditionally': { const didMutate = state.mutate(effect.kind, effect.value); if (didMutate) { - switch (effect.kind) { - case 'Mutate': { - effects.push(effect); - break; - } - case 'MutateConditionally': { - effects.push({kind: 'Mutate', value: effect.value}); - break; - } - case 'MutateTransitive': { - effects.push(effect); - break; - } - case 'MutateTransitiveConditionally': { - effects.push({kind: 'MutateTransitive', value: effect.value}); - break; - } - } + effects.push(effect); } break; } @@ -414,13 +453,19 @@ function applySignature( } } } - CompilerError.invariant( - state.isDefined(instruction.lvalue) && state.kind(instruction.lvalue), - { + if ( + !(state.isDefined(instruction.lvalue) && state.kind(instruction.lvalue)) + ) { + console.log(printInstruction(instruction)); + console.log('input effects'); + console.log(signature.effects.map(printAliasingEffect).join('\n')); + console.log('refined effects'); + console.log(effects.map(printAliasingEffect).join('\n')); + CompilerError.invariant(false, { reason: `Expected instruction lvalue to be initialized`, loc: instruction.loc, - }, - ); + }); + } return effects.length !== 0 ? effects : null; } @@ -961,11 +1006,6 @@ function computeSignatureForInstruction( from: value.object, into: lvalue, }); - effects.push({ - kind: 'Capture', - from: value.object, - into: lvalue, - }); break; } case 'PropertyStore': @@ -1106,11 +1146,6 @@ function computeSignatureForInstruction( from: value.value, into: patternLValue, }); - effects.push({ - kind: 'Capture', - from: value.value, - into: patternLValue, - }); } effects.push({kind: 'Alias', from: value.value, into: lvalue}); break; @@ -1267,6 +1302,7 @@ function computeEffectsForSignature( } break; } + case 'ImmutableCapture': case 'CreateFrom': case 'Apply': case 'Mutate': @@ -1413,6 +1449,10 @@ export type AliasingEffect = * Creates a new value with the same kind as the starting value. */ | {kind: 'CreateFrom'; from: Place; into: Place} + /** + * Immutable data flow, used for escape analysis. Does not influence mutable range analysis: + */ + | {kind: 'ImmutableCapture'; from: Place; into: Place} /** * Calls the function at the given place with the given arguments either captured or aliased, * and captures/aliases the result into the given place. diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts index e9f2bce050ae..e7224be8066b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -54,7 +54,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { effect.kind === 'Capture' ) { captures.union([effect.from.identifier, effect.into.identifier]); - } else if (effect.kind === 'MutateTransitive') { + } else if ( + effect.kind === 'MutateTransitive' || + effect.kind === 'MutateTransitiveConditionally' + ) { const value = effect.value; value.identifier.mutableRange.end = makeInstructionId(instr.id + 1); } @@ -83,7 +86,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { for (const effect of instr.effects) { if (effect.kind === 'Alias' || effect.kind === 'CreateFrom') { aliases.union([effect.from.identifier, effect.into.identifier]); - } else if (effect.kind === 'Mutate') { + } else if ( + effect.kind === 'Mutate' || + effect.kind === 'MutateConditionally' + ) { const value = effect.value; value.identifier.mutableRange.end = makeInstructionId(instr.id + 1); } @@ -94,7 +100,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { /** * Part 3 - * Add legacy operand-specific effects based on instruction effects and mutable ranges + * Add legacy operand-specific effects based on instruction effects and mutable ranges. + * Also fixes up operand mutable ranges, making sure that start is non-zero if the value + * is mutated (depended on by later passes like InferReactiveScopeVariables which uses this + * to filter spurious mutations of globals, which we now guard against more precisely) */ for (const block of fn.body.blocks.values()) { for (const phi of block.phis) { @@ -136,6 +145,10 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { } break; } + case 'ImmutableCapture': { + operandEffects.set(effect.from.identifier.id, Effect.Read); + break; + } case 'Create': { break; } @@ -178,6 +191,12 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { lvalue.effect = effect; } for (const operand of eachInstructionValueOperand(instr.value)) { + if ( + operand.identifier.mutableRange.end > instr.id && + operand.identifier.mutableRange.start === 0 + ) { + operand.identifier.mutableRange.start = instr.id; + } const effect = operandEffects.get(operand.identifier.id) ?? Effect.Read; operand.effect = effect; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index 82fd4bdece6e..0c1fd759bd5b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -376,12 +376,12 @@ export function findDisjointMutableValues( } else { for (const operand of eachInstructionOperand(instr)) { if ( - isMutable(instr, operand) + isMutable(instr, operand) && /* * exclude global variables from being added to scopes, we can't recreate them! * TODO: improve handling of module-scoped variables and globals */ - // && operand.identifier.mutableRange.start > 0 + operand.identifier.mutableRange.start > 0 ) { if ( instr.value.kind === 'FunctionExpression' || diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md index f66b970f00dd..2a935256d7a0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md @@ -22,7 +22,7 @@ function Component(props) { 7 | return hasErrors; 8 | } > 9 | return hasErrors(); - | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$14 (9:9) + | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$15 (9:9) 10 | } 11 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.expect.md new file mode 100644 index 000000000000..85ebf65a1fed --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.expect.md @@ -0,0 +1,58 @@ + +## Input + +```javascript +// @enableNewMutationAliasingModel +function Component({a, b}) { + const x = {a}; + const y = [b]; + const f = () => { + y.x = x; + mutate(y); + }; + return
{x}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel +function Component(t0) { + const $ = _c(7); + const { a, b } = t0; + let t1; + let x; + if ($[0] !== a || $[1] !== b) { + x = { a }; + const y = [b]; + t1 = () => { + y.x = x; + mutate(y); + }; + $[0] = a; + $[1] = b; + $[2] = t1; + $[3] = x; + } else { + t1 = $[2]; + x = $[3]; + } + const f = t1; + let t2; + if ($[4] !== f || $[5] !== x) { + t2 =
{x}
; + $[4] = f; + $[5] = x; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.js new file mode 100644 index 000000000000..8d4bb23742b2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation-via-function-expression.js @@ -0,0 +1,11 @@ +// @enableNewMutationAliasingModel +function Component({a, b}) { + const x = {a}; + const y = [b]; + const f = () => { + y.x = x; + mutate(y); + }; + f(); + return
{x}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.expect.md new file mode 100644 index 000000000000..0753f007b7b3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +// @enableNewMutationAliasingModel +function Component({a, b}) { + const x = {a}; + const y = [b]; + y.x = x; + mutate(y); + return
{x}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel +function Component(t0) { + const $ = _c(3); + const { a, b } = t0; + let t1; + if ($[0] !== a || $[1] !== b) { + const x = { a }; + const y = [b]; + y.x = x; + mutate(y); + t1 =
{x}
; + $[0] = a; + $[1] = b; + $[2] = t1; + } else { + t1 = $[2]; + } + return t1; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.js new file mode 100644 index 000000000000..480221fef4da --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/basic-mutation.js @@ -0,0 +1,8 @@ +// @enableNewMutationAliasingModel +function Component({a, b}) { + const x = {a}; + const y = [b]; + y.x = x; + mutate(y); + return
{x}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.expect.md new file mode 100644 index 000000000000..9e6fa024e3da --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.expect.md @@ -0,0 +1,59 @@ + +## Input + +```javascript +// @enableNewMutationAliasingModel +function Component({a, b, c}) { + const x = [a, b]; + const f = () => { + maybeMutate(x); + // different dependency to force this not to merge with x's scope + console.log(c); + }; + return ; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel +function Component(t0) { + const $ = _c(8); + const { a, b, c } = t0; + let t1; + let x; + if ($[0] !== a || $[1] !== b || $[2] !== c) { + x = [a, b]; + t1 = () => { + maybeMutate(x); + + console.log(c); + }; + $[0] = a; + $[1] = b; + $[2] = c; + $[3] = t1; + $[4] = x; + } else { + t1 = $[3]; + x = $[4]; + } + const f = t1; + let t2; + if ($[5] !== f || $[6] !== x) { + t2 = ; + $[5] = f; + $[6] = x; + $[7] = t2; + } else { + t2 = $[7]; + } + return t2; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.js new file mode 100644 index 000000000000..096f4f17ea54 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/potential-mutation-in-function-expression.js @@ -0,0 +1,10 @@ +// @enableNewMutationAliasingModel +function Component({a, b, c}) { + const x = [a, b]; + const f = () => { + maybeMutate(x); + // different dependency to force this not to merge with x's scope + console.log(c); + }; + return ; +}