From 01f0449b1e3ad79a1af8213581fb07e7ea720b25 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 7 May 2025 17:30:03 -0700 Subject: [PATCH] [compiler][wip] Infer alias effects for function expressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: 32978ceda69caa434ba9ec37a1be09bbde7434b0 Pull Request resolved: https://github.com/facebook/react/pull/33151 --- .../src/HIR/HIR.ts | 4 + .../src/HIR/PrintHIR.ts | 23 +++- .../src/Inference/AnalyseFunctions.ts | 97 ++++++++++++- .../src/Inference/InferAlias.ts | 4 + .../InferAliasesForFunctionCaptureEffects.ts | 33 +++++ .../src/Inference/InferFunctionEffects.ts | 130 ++++++++++-------- .../src/Inference/InferMutableRanges.ts | 8 +- .../src/Inference/InferReferenceEffects.ts | 115 ++++++++-------- ...ay-map-captures-receiver-noAlias.expect.md | 26 +--- ...-use-effect-function-mutates-ref.expect.md | 48 ------- ...ctly-flagged-as-context-mutation.expect.md | 89 ++++++++++++ ...ncorrectly-flagged-as-context-mutation.js} | 3 + 12 files changed, 386 insertions(+), 194 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Inference/InferAliasesForFunctionCaptureEffects.ts delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-incorrectly-flagged-as-context-mutation.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.invalid-use-effect-function-mutates-ref.js => repro-local-mutation-incorrectly-flagged-as-context-mutation.js} (67%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 99b8c189ee..991ac9e72d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -300,6 +300,10 @@ export type FunctionEffect = places: ReadonlySet; effect: Effect; loc: SourceLocation; + } + | { + kind: 'CaptureEffect'; + places: ReadonlySet; }; /* 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 c8182c9e72..93acb4c944 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -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(', ') ?? ''; 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 a439b4cd01..b7322d901a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -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 { 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): 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, +): 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 = []; + 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, +): 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 = new Set( + [...set] + .map(identifier => contextIdentifiers.get(identifier)) + .filter(place => place != null) as Array, + ); + if (contextOperands.size !== 0) { + loweredFunc.func.effects ??= []; + loweredFunc.func.effects?.push({ + kind: 'CaptureEffect', + places: contextOperands, + }); + } + } } function isMutatedOrReassigned(id: Identifier): boolean { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAlias.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAlias.ts index 80422c8391..fd7ec6becd 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAlias.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAlias.ts @@ -60,6 +60,10 @@ function inferInstr( alias = instrValue.value; break; } + case 'IteratorNext': { + alias = instrValue.collection; + break; + } default: return; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAliasesForFunctionCaptureEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAliasesForFunctionCaptureEffects.ts new file mode 100644 index 0000000000..8bc7cee4f9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferAliasesForFunctionCaptureEffects.ts @@ -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, +): 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)); + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts index a58ae44021..ba09bcba1e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts @@ -95,45 +95,58 @@ function inheritFunctionEffects( return effects .flatMap(effect => { - if (effect.kind === 'GlobalMutation' || effect.kind === 'ReactMutation') { - return [effect]; - } else { - const effects: Array = []; - 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 = []; + 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, ): Array { - 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; } function isEffectSafeOutsideRender(effect: FunctionEffect): boolean { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts index 624c302fbf..27230b9e7f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRanges.ts @@ -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 { // 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(a: Map, b: Map): boolean { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index 3dc532c87d..01d78663d0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -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 = 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 = 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 { - 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, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-captures-receiver-noAlias.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-captures-receiver-noAlias.expect.md index 1680386c74..efd094c1a5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-captures-receiver-noAlias.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-captures-receiver-noAlias.expect.md @@ -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) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md deleted file mode 100644 index b3f1104239..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.expect.md +++ /dev/null @@ -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] -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-incorrectly-flagged-as-context-mutation.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-incorrectly-flagged-as-context-mutation.expect.md new file mode 100644 index 0000000000..cc3e810799 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-incorrectly-flagged-as-context-mutation.expect.md @@ -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 \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-incorrectly-flagged-as-context-mutation.js similarity index 67% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-incorrectly-flagged-as-context-mutation.js index 2e8b7827d0..5ba1eb1f64 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-effect-function-mutates-ref.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-local-mutation-incorrectly-flagged-as-context-mutation.js @@ -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); },