[compiler][wip] Infer alias effects for function expressions

This is a stab at addressing a pattern that mofeiz and I have both stumbled across. Today, FunctionExpression's context list describes values from the outer context that are accessed in the function, and with what effect they were accessed. This allows us to describe the fact that a value from the outer context is known to be mutated inside a function expression, or is known to be captured (aliased) into some other value in the function expression. However, the basic `Effect` kind is insufficient to describe the full semantics. Notably, it doesn't let us describe more complex aliasing relationships.

From an example mofeiz added:

```js
const x = {};
const y = {};
const f = () => {
  const a = [y];
  const b = x;
  // this sets y.x = x
  a[0].x = b;
}
f();
mutate(y.x);  // which means this mutates x!
```

Here, the Effect on the context operands are `[mutate y, read x]`. The `mutate y` is bc of the array push. But the `read x` is surprising — `x` is captured into `y`, but there is no subsequent mutation of y or x, so we consider this a read. But as the comments indicate, the final line mutates x! We need to reflect the fact that even though x isn't mutated inside the function, it is aliased into y, such that if y is subsequently mutated that this should count as a mutation of x too.

The idea of this PR is to extend the FunctionEffect type with a CaptureEffect variant which lists out the aliasing groups that occur inside the function expression. This allows us to bubble up the results of alias analysis from inside a function. The idea is to:

* Return the alias sets from InferMutableRanges
* Augment them with capturing of the form above, handling cases such as the `a[0].x = b`
* For each alias group, record a CaptureEffect for any group that contains 2+ context operands
* Extend the alias sets in the _outer_ function with the CaptureEffect sets from FunctionExpression/ObjectMethod instructions.

As part of this, I realized that our handling of PropertyStore's effect wasn't quite right. We used a store effect for the object, but only if it was a known object type — otherwise we recorded it as a mutation. But a PropertyStore really always is a store — it only mutates direct aliases of a value, not any interior objects that are captured. So I updated to always use store for known properties, and use mutate for computed properties. The latter is still also wrong, but i want to debug the change there separately.

ghstack-source-id: 32978ceda6
Pull Request resolved: https://github.com/facebook/react/pull/33151
This commit is contained in:
Joe Savona
2025-06-09 11:10:08 -07:00
parent ff77c0abe9
commit 01f0449b1e
12 changed files with 386 additions and 194 deletions
@@ -300,6 +300,10 @@ export type FunctionEffect =
places: ReadonlySet<Place>;
effect: Effect;
loc: SourceLocation;
}
| {
kind: 'CaptureEffect';
places: ReadonlySet<Place>;
};
/*
@@ -546,12 +546,23 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
const effects =
instrValue.loweredFunc.func.effects
?.map(effect => {
if (effect.kind === 'ContextMutation') {
return `ContextMutation places=[${[...effect.places]
.map(place => printPlace(place))
.join(', ')}] effect=${effect.effect}`;
} else {
return `GlobalMutation`;
switch (effect.kind) {
case 'ContextMutation': {
return `ContextMutation places=[${[...effect.places]
.map(place => printPlace(place))
.join(', ')}] effect=${effect.effect}`;
}
case 'GlobalMutation': {
return 'GlobalMutation';
}
case 'ReactMutation': {
return 'ReactMutation';
}
case 'CaptureEffect': {
return `CaptureEffect places=[${[...effect.places]
.map(place => printPlace(place))
.join(', ')}]`;
}
}
})
.join(', ') ?? '';
@@ -11,6 +11,7 @@ import {
HIRFunction,
Identifier,
LoweredFunction,
Place,
isRefOrRefValue,
makeInstructionId,
} from '../HIR';
@@ -19,6 +20,14 @@ import {inferReactiveScopeVariables} from '../ReactiveScopes';
import {rewriteInstructionKindsBasedOnReassignment} from '../SSA';
import {inferMutableRanges} from './InferMutableRanges';
import inferReferenceEffects from './InferReferenceEffects';
import DisjointSet from '../Utils/DisjointSet';
import {
eachInstructionLValue,
eachInstructionValueOperand,
} from '../HIR/visitors';
import prettyFormat from 'pretty-format';
import {printIdentifier} from '../HIR/PrintHIR';
import {Iterable_some} from '../Utils/utils';
export default function analyseFunctions(func: HIRFunction): void {
for (const [_, block] of func.body.blocks) {
@@ -26,8 +35,8 @@ export default function analyseFunctions(func: HIRFunction): void {
switch (instr.value.kind) {
case 'ObjectMethod':
case 'FunctionExpression': {
lower(instr.value.loweredFunc.func);
infer(instr.value.loweredFunc);
const aliases = lower(instr.value.loweredFunc.func);
infer(instr.value.loweredFunc, aliases);
/**
* Reset mutable range for outer inferReferenceEffects
@@ -44,11 +53,11 @@ export default function analyseFunctions(func: HIRFunction): void {
}
}
function lower(func: HIRFunction): void {
function lower(func: HIRFunction): DisjointSet<Identifier> {
analyseFunctions(func);
inferReferenceEffects(func, {isFunctionExpression: true});
deadCodeElimination(func);
inferMutableRanges(func);
const aliases = inferMutableRanges(func);
rewriteInstructionKindsBasedOnReassignment(func);
inferReactiveScopeVariables(func);
func.env.logger?.debugLogIRs?.({
@@ -56,9 +65,70 @@ function lower(func: HIRFunction): void {
name: 'AnalyseFunction (inner)',
value: func,
});
inferAliasesForCapturing(func, aliases);
return aliases;
}
function infer(loweredFunc: LoweredFunction): void {
export function debugAliases(aliases: DisjointSet<Identifier>): void {
console.log(
prettyFormat(
aliases
.buildSets()
.map(set => [...set].map(ident => printIdentifier(ident))),
),
);
}
/**
* The alias sets returned by InferMutableRanges() accounts only for aliases that
* are known to mutate together. Notably this skips cases where a value is captured
* into some other value, but neither is subsequently mutated. An example is pushing
* a mutable value onto an array, where neither the array or value are subsequently
* mutated.
*
* This function extends the aliases sets to account for such capturing, so that we
* can detect cases where one of the values in a set is mutated later (in an outer function)
* we can correctly infer them as mutating together.
*/
function inferAliasesForCapturing(
fn: HIRFunction,
aliases: DisjointSet<Identifier>,
): void {
for (const block of fn.body.blocks.values()) {
for (const instr of block.instructions) {
const {lvalue, value} = instr;
const hasStore =
lvalue.effect === Effect.Store ||
Iterable_some(
eachInstructionValueOperand(value),
operand => operand.effect === Effect.Store,
);
if (!hasStore) {
continue;
}
const operands: Array<Identifier> = [];
for (const lvalue of eachInstructionLValue(instr)) {
operands.push(lvalue.identifier);
}
for (const operand of eachInstructionValueOperand(instr.value)) {
if (
operand.effect === Effect.Store ||
operand.effect === Effect.Capture
) {
operands.push(operand.identifier);
}
}
if (operands.length > 1) {
aliases.union(operands);
}
}
}
}
function infer(
loweredFunc: LoweredFunction,
aliases: DisjointSet<Identifier>,
): void {
for (const operand of loweredFunc.func.context) {
const identifier = operand.identifier;
CompilerError.invariant(operand.effect === Effect.Unknown, {
@@ -85,6 +155,23 @@ function infer(loweredFunc: LoweredFunction): void {
operand.effect = Effect.Read;
}
}
const contextIdentifiers = new Map(
loweredFunc.func.context.map(place => [place.identifier, place]),
);
for (const set of aliases.buildSets()) {
const contextOperands: Set<Place> = new Set(
[...set]
.map(identifier => contextIdentifiers.get(identifier))
.filter(place => place != null) as Array<Place>,
);
if (contextOperands.size !== 0) {
loweredFunc.func.effects ??= [];
loweredFunc.func.effects?.push({
kind: 'CaptureEffect',
places: contextOperands,
});
}
}
}
function isMutatedOrReassigned(id: Identifier): boolean {
@@ -60,6 +60,10 @@ function inferInstr(
alias = instrValue.value;
break;
}
case 'IteratorNext': {
alias = instrValue.collection;
break;
}
default:
return;
}
@@ -0,0 +1,33 @@
/**
* 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 {HIRFunction, Identifier} from '../HIR/HIR';
import DisjointSet from '../Utils/DisjointSet';
export function inferAliasForFunctionCaptureEffects(
func: HIRFunction,
aliases: DisjointSet<Identifier>,
): void {
for (const [_, block] of func.body.blocks) {
for (const instr of block.instructions) {
const {value} = instr;
if (
value.kind !== 'FunctionExpression' &&
value.kind !== 'ObjectMethod'
) {
continue;
}
const loweredFunction = value.loweredFunc.func;
for (const effect of loweredFunction.effects ?? []) {
if (effect.kind !== 'CaptureEffect') {
continue;
}
aliases.union([...effect.places].map(place => place.identifier));
}
}
}
}
@@ -95,45 +95,58 @@ function inheritFunctionEffects(
return effects
.flatMap(effect => {
if (effect.kind === 'GlobalMutation' || effect.kind === 'ReactMutation') {
return [effect];
} else {
const effects: Array<FunctionEffect | null> = [];
CompilerError.invariant(effect.kind === 'ContextMutation', {
reason: 'Expected ContextMutation',
loc: null,
});
/**
* Contextual effects need to be replayed against the current inference
* state, which may know more about the value to which the effect applied.
* The main cases are:
* 1. The mutated context value is _still_ a context value in the current scope,
* so we have to continue propagating the original context mutation.
* 2. The mutated context value is a mutable value in the current scope,
* so the context mutation was fine and we can skip propagating the effect.
* 3. The mutated context value is an immutable value in the current scope,
* resulting in a non-ContextMutation FunctionEffect. We propagate that new,
* more detailed effect to the current function context.
*/
for (const place of effect.places) {
if (state.isDefined(place)) {
const replayedEffect = inferOperandEffect(state, {
...place,
loc: effect.loc,
effect: effect.effect,
});
if (replayedEffect != null) {
if (replayedEffect.kind === 'ContextMutation') {
// Case 1, still a context value so propagate the original effect
effects.push(effect);
} else {
// Case 3, immutable value so propagate the more precise effect
effects.push(replayedEffect);
}
} // else case 2, local mutable value so this effect was fine
switch (effect.kind) {
case 'GlobalMutation':
case 'ReactMutation': {
return [effect];
}
case 'ContextMutation': {
const effects: Array<FunctionEffect | null> = [];
CompilerError.invariant(effect.kind === 'ContextMutation', {
reason: 'Expected ContextMutation',
loc: null,
});
/**
* Contextual effects need to be replayed against the current inference
* state, which may know more about the value to which the effect applied.
* The main cases are:
* 1. The mutated context value is _still_ a context value in the current scope,
* so we have to continue propagating the original context mutation.
* 2. The mutated context value is a mutable value in the current scope,
* so the context mutation was fine and we can skip propagating the effect.
* 3. The mutated context value is an immutable value in the current scope,
* resulting in a non-ContextMutation FunctionEffect. We propagate that new,
* more detailed effect to the current function context.
*/
for (const place of effect.places) {
if (state.isDefined(place)) {
const replayedEffect = inferOperandEffect(state, {
...place,
loc: effect.loc,
effect: effect.effect,
});
if (replayedEffect != null) {
if (replayedEffect.kind === 'ContextMutation') {
// Case 1, still a context value so propagate the original effect
effects.push(effect);
} else {
// Case 3, immutable value so propagate the more precise effect
effects.push(replayedEffect);
}
} // else case 2, local mutable value so this effect was fine
}
}
return effects;
}
case 'CaptureEffect': {
return [];
}
default: {
assertExhaustive(
effect,
`Unexpected effect kind '${(effect as any).kind}'`,
);
}
return effects;
}
})
.filter((effect): effect is FunctionEffect => effect != null);
@@ -298,26 +311,31 @@ export function inferTerminalFunctionEffects(
export function transformFunctionEffectErrors(
functionEffects: Array<FunctionEffect>,
): Array<CompilerErrorDetailOptions> {
return functionEffects.map(eff => {
switch (eff.kind) {
case 'ReactMutation':
case 'GlobalMutation': {
return eff.error;
return functionEffects
.map(eff => {
switch (eff.kind) {
case 'ReactMutation':
case 'GlobalMutation': {
return eff.error;
}
case 'ContextMutation': {
return {
severity: ErrorSeverity.Invariant,
reason: `Unexpected ContextMutation in top-level function effects`,
loc: eff.loc,
};
}
case 'CaptureEffect': {
return null;
}
default:
assertExhaustive(
eff,
`Unexpected function effect kind \`${(eff as any).kind}\``,
);
}
case 'ContextMutation': {
return {
severity: ErrorSeverity.Invariant,
reason: `Unexpected ContextMutation in top-level function effects`,
loc: eff.loc,
};
}
default:
assertExhaustive(
eff,
`Unexpected function effect kind \`${(eff as any).kind}\``,
);
}
});
})
.filter(eff => eff != null) as Array<CompilerErrorDetailOptions>;
}
function isEffectSafeOutsideRender(effect: FunctionEffect): boolean {
@@ -6,15 +6,17 @@
*/
import {HIRFunction, Identifier} from '../HIR/HIR';
import DisjointSet from '../Utils/DisjointSet';
import {inferAliasForUncalledFunctions} from './InerAliasForUncalledFunctions';
import {inferAliases} from './InferAlias';
import {inferAliasForFunctionCaptureEffects} from './InferAliasesForFunctionCaptureEffects';
import {inferAliasForPhis} from './InferAliasForPhis';
import {inferAliasForStores} from './InferAliasForStores';
import {inferMutableLifetimes} from './InferMutableLifetimes';
import {inferMutableRangesForAlias} from './InferMutableRangesForAlias';
import {inferTryCatchAliases} from './InferTryCatchAliases';
export function inferMutableRanges(ir: HIRFunction): void {
export function inferMutableRanges(ir: HIRFunction): DisjointSet<Identifier> {
// Infer mutable ranges for non fields
inferMutableLifetimes(ir, false);
@@ -38,6 +40,8 @@ export function inferMutableRanges(ir: HIRFunction): void {
// Update aliasing information of fields
inferAliasForStores(ir, aliases);
inferAliasForFunctionCaptureEffects(ir, aliases);
// Update aliasing information of phis
inferAliasForPhis(ir, aliases);
@@ -84,6 +88,8 @@ export function inferMutableRanges(ir: HIRFunction): void {
}
prevAliases = nextAliases;
}
return aliases;
}
function areEqualMaps<T>(a: Map<T, T>, b: Map<T, T>): boolean {
@@ -31,7 +31,6 @@ import {
isArrayType,
isMapType,
isMutableEffect,
isObjectType,
isSetType,
} from '../HIR/HIR';
import {FunctionSignature} from '../HIR/ObjectShape';
@@ -48,7 +47,7 @@ import {
eachTerminalOperand,
eachTerminalSuccessor,
} from '../HIR/visitors';
import {assertExhaustive} from '../Utils/utils';
import {assertExhaustive, retainWhere} from '../Utils/utils';
import {
inferTerminalFunctionEffects,
inferInstructionFunctionEffects,
@@ -521,7 +520,7 @@ class InferenceState {
* `expected valueKind to be 'Mutable' but found to be \`${valueKind}\``
* );
*/
effect = isObjectType(place.identifier) ? Effect.Store : Effect.Mutate;
effect = Effect.Store;
break;
}
case Effect.Capture: {
@@ -670,11 +669,7 @@ class InferenceState {
for (const [value, kind] of this.#values) {
const id = identify(value);
result.values[id] = {
abstract: {
kind: kind.kind,
context: [...kind.context].map(printPlace),
reason: [...kind.reason],
},
abstract: this.debugAbstractValue(kind),
value: printInstructionValue(value),
};
}
@@ -684,6 +679,14 @@ class InferenceState {
return result;
}
debugAbstractValue(value: AbstractValue): any {
return {
kind: value.kind,
context: [...value.context].map(printPlace),
reason: [...value.reason],
};
}
inferPhi(phi: Phi): void {
const values: Set<InstructionValue> = new Set();
for (const [_, operand] of phi.operands) {
@@ -909,19 +912,11 @@ function inferBlock(
break;
}
case 'ArrayExpression': {
const contextRefOperands = getContextRefOperand(state, instrValue);
const valueKind: AbstractValue =
contextRefOperands.length > 0
? {
kind: ValueKind.Context,
reason: new Set([ValueReason.Other]),
context: new Set(contextRefOperands),
}
: {
kind: ValueKind.Mutable,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
const valueKind: AbstractValue = {
kind: ValueKind.Mutable,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
for (const element of instrValue.elements) {
if (element.kind === 'Spread') {
@@ -942,6 +937,7 @@ function inferBlock(
let _: 'Hole' = element.kind;
}
}
state.initialize(instrValue, valueKind);
state.define(instr.lvalue, instrValue);
instr.lvalue.effect = Effect.Store;
@@ -961,19 +957,11 @@ function inferBlock(
break;
}
case 'ObjectExpression': {
const contextRefOperands = getContextRefOperand(state, instrValue);
const valueKind: AbstractValue =
contextRefOperands.length > 0
? {
kind: ValueKind.Context,
reason: new Set([ValueReason.Other]),
context: new Set(contextRefOperands),
}
: {
kind: ValueKind.Mutable,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
const valueKind: AbstractValue = {
kind: ValueKind.Mutable,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
for (const property of instrValue.properties) {
switch (property.kind) {
@@ -1197,6 +1185,35 @@ function inferBlock(
);
hasMutableOperand ||= isMutableEffect(operand.effect, operand.loc);
}
/*
* Filter CaptureEffects to remove values that are immutable and don't
* need to be tracked for aliasing
*/
const effects = instrValue.loweredFunc.func.effects;
if (effects != null && effects.length !== 0) {
retainWhere(effects, effect => {
if (effect.kind !== 'CaptureEffect') {
return true;
}
const places: Set<Place> = new Set();
for (const place of effect.places) {
const kind = state.kind(place);
if (
kind.kind === ValueKind.Context ||
kind.kind === ValueKind.MaybeFrozen ||
kind.kind === ValueKind.Mutable
) {
places.add(place);
}
}
if (places.size === 0) {
return false;
}
effect.places = places;
return true;
});
}
/*
* If a closure did not capture any mutable values, then we can consider it to be
* frozen, which allows it to be independently memoized.
@@ -1287,20 +1304,18 @@ function inferBlock(
break;
}
case 'PropertyStore': {
const effect =
state.kind(instrValue.object).kind === ValueKind.Context
? Effect.ConditionallyMutate
: Effect.Capture;
state.referenceAndRecordEffects(
freezeActions,
instrValue.value,
effect,
Effect.Capture,
ValueReason.Other,
);
state.referenceAndRecordEffects(
freezeActions,
instrValue.object,
Effect.Store,
typeof instrValue.property === 'string'
? Effect.Store
: Effect.Mutate,
ValueReason.Other,
);
@@ -1818,7 +1833,9 @@ function inferBlock(
state.isDefined(operand) &&
((operand.identifier.type.kind === 'Function' &&
state.isFunctionExpression) ||
state.kind(operand).kind === ValueKind.Context)
state.kind(operand).kind === ValueKind.Context ||
(state.kind(operand).kind === ValueKind.Mutable &&
state.isFunctionExpression))
) {
/**
* Returned values should only be typed as 'frozen' if they are both (1)
@@ -1845,22 +1862,6 @@ function inferBlock(
);
}
function getContextRefOperand(
state: InferenceState,
instrValue: InstructionValue,
): Array<Place> {
const result = [];
for (const place of eachInstructionValueOperand(instrValue)) {
if (state.isDefined(place)) {
const kind = state.kind(place);
if (kind.kind === ValueKind.Context) {
result.push(...kind.context);
}
}
}
return result;
}
export function getFunctionCallSignature(
env: Environment,
type: Type,
@@ -23,34 +23,18 @@ export const FIXTURE_ENTRYPOINT = {
```javascript
import { c as _c } from "react/compiler-runtime";
function Component(props) {
const $ = _c(6);
const $ = _c(2);
let t0;
if ($[0] !== props.a) {
t0 = { a: props.a };
const item = { a: props.a };
const items = [item];
t0 = items.map(_temp);
$[0] = props.a;
$[1] = t0;
} else {
t0 = $[1];
}
const item = t0;
let t1;
if ($[2] !== item) {
t1 = [item];
$[2] = item;
$[3] = t1;
} else {
t1 = $[3];
}
const items = t1;
let t2;
if ($[4] !== items) {
t2 = items.map(_temp);
$[4] = items;
$[5] = t2;
} else {
t2 = $[5];
}
const mapped = t2;
const mapped = t0;
return mapped;
}
function _temp(item_0) {
@@ -1,48 +0,0 @@
## Input
```javascript
// @validateNoFreezingKnownMutableFunctions
import {useCallback, useEffect, useRef} from 'react';
import {useHook} from 'shared-runtime';
function Component() {
const params = useHook();
const update = useCallback(
partialParams => {
const nextParams = {
...params,
...partialParams,
};
nextParams.param = 'value';
console.log(nextParams);
},
[params]
);
const ref = useRef(null);
useEffect(() => {
if (ref.current === null) {
update();
}
}, [update]);
return 'ok';
}
```
## Error
```
12 | ...partialParams,
13 | };
> 14 | nextParams.param = 'value';
| ^^^^^^^^^^ InvalidReact: Mutating a value returned from a function whose return value should not be mutated. Found mutation of `params` (14:14)
15 | console.log(nextParams);
16 | },
17 | [params]
```
@@ -0,0 +1,89 @@
## Input
```javascript
// @validateNoFreezingKnownMutableFunctions
import {useCallback, useEffect, useRef} from 'react';
import {useHook} from 'shared-runtime';
function Component() {
const params = useHook();
const update = useCallback(
partialParams => {
const nextParams = {
...params,
...partialParams,
};
// Due to how we previously represented ObjectExpressions in InferReferenceEffects,
// this was recorded as a mutation of a context value (`params`) which then made
// the function appear ineligible for freezing when passing to useEffect below.
nextParams.param = 'value';
console.log(nextParams);
},
[params]
);
const ref = useRef(null);
useEffect(() => {
if (ref.current === null) {
update();
}
}, [update]);
return 'ok';
}
```
## Code
```javascript
import { c as _c } from "react/compiler-runtime"; // @validateNoFreezingKnownMutableFunctions
import { useCallback, useEffect, useRef } from "react";
import { useHook } from "shared-runtime";
function Component() {
const $ = _c(5);
const params = useHook();
let t0;
if ($[0] !== params) {
t0 = (partialParams) => {
const nextParams = { ...params, ...partialParams };
nextParams.param = "value";
console.log(nextParams);
};
$[0] = params;
$[1] = t0;
} else {
t0 = $[1];
}
const update = t0;
const ref = useRef(null);
let t1;
let t2;
if ($[2] !== update) {
t1 = () => {
if (ref.current === null) {
update();
}
};
t2 = [update];
$[2] = update;
$[3] = t1;
$[4] = t2;
} else {
t1 = $[3];
t2 = $[4];
}
useEffect(t1, t2);
return "ok";
}
```
### Eval output
(kind: exception) Fixture not implemented
@@ -11,6 +11,9 @@ function Component() {
...params,
...partialParams,
};
// Due to how we previously represented ObjectExpressions in InferReferenceEffects,
// this was recorded as a mutation of a context value (`params`) which then made
// the function appear ineligible for freezing when passing to useEffect below.
nextParams.param = 'value';
console.log(nextParams);
},