From fbd39a7238fa3cea7a91f27984ca125a793a7962 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 5 Jun 2025 22:13:52 -0700 Subject: [PATCH] [compiler][newinference] Error handling and related fixes Lots of small fixes related to error handling. InferMutationAliasRanges now tracks transitive calls that may mutate frozen or global values. We properly populate and track the reason each value has the kind it has, to use when throwing errors for invalid mutations (can't mutate state vs can't mutate a captured jsx value, etc). When we infer mutation effects for inner functions, we populate the location of mutations as the location where the mutation occurred, not the declaration of the captured value (aside: this was quite involved to do in the old inference, it's trivial here). A bunch of other small fixes that make sense in context. And some of our "bug-*" fixtures output changes...becasue the new inference fixes the bugs. One example included here. [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 9 +- .../src/HIR/Environment.ts | 2 +- .../src/HIR/ObjectShape.ts | 3 + .../src/Inference/AnalyseFunctions.ts | 2 +- .../Inference/InferMutationAliasingEffects.ts | 96 ++++++---- .../InferMutationAliasingFunctionEffects.ts | 3 +- .../Inference/InferMutationAliasingRanges.ts | 167 ++++++++++++------ ...ValidateNoFreezingKnownMutableFunctions.ts | 25 +++ ...-func-maybealias-captured-mutate.expect.md | 125 +++++++++++++ ...pturing-func-maybealias-captured-mutate.ts | 49 +++++ 10 files changed, 392 insertions(+), 89 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 7524f4fff5090..2363858012928 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -266,8 +266,15 @@ function runWithEnvironment( inferMutableRanges(hir); log({kind: 'hir', name: 'InferMutableRanges', value: hir}); } else { - inferMutationAliasingRanges(hir); + const mutabilityAliasingErrors = inferMutationAliasingRanges(hir, { + isFunctionExpression: false, + }); log({kind: 'hir', name: 'InferMutationAliasingRanges', value: hir}); + if (env.isInferredMemoEnabled) { + if (mutabilityAliasingErrors.isErr()) { + throw mutabilityAliasingErrors.unwrapErr(); + } + } } if (env.isInferredMemoEnabled) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 8d2e72b22e4e9..5f063ad2f9995 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -246,7 +246,7 @@ export const EnvironmentConfigSchema = z.object({ /** * Enable a new model for mutability and aliasing inference */ - enableNewMutationAliasingModel: z.boolean().default(false), + enableNewMutationAliasingModel: z.boolean().default(true), /** * Enables inference of optional dependency chains. Without this flag diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index aa3865097f0e7..10c45ccb3f5e4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -336,6 +336,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ kind: 'Create', into: signatureArgument(2), value: ValueKind.Primitive, + reason: ValueReason.KnownReturnSignature, }, ], }, @@ -391,6 +392,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ kind: 'Create', into: signatureArgument(2), value: ValueKind.Mutable, + reason: ValueReason.KnownReturnSignature, }, // The first arg to the callback is an item extracted from the receiver array { @@ -403,6 +405,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [ kind: 'Create', into: signatureArgument(5), value: ValueKind.Primitive, + reason: ValueReason.KnownReturnSignature, }, // calls the callback, returning the result into a temporary { 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 30675e2820d11..91f99395b03e6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -63,7 +63,7 @@ function lowerWithMutationAliasing(fn: HIRFunction): void { analyseFunctions(fn); inferMutationAliasingEffects(fn, {isFunctionExpression: true}); deadCodeElimination(fn); - inferMutationAliasingRanges(fn); + inferMutationAliasingRanges(fn, {isFunctionExpression: true}); rewriteInstructionKindsBasedOnReassignment(fn); inferReactiveScopeVariables(fn); const effects = inferMutationAliasingFunctionEffects(fn); 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 1842b8fe36456..b0f406568ac5c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -26,6 +26,7 @@ import { isArrayType, isMapType, isPrimitiveType, + isRefOrRefValue, isSetType, makeIdentifierId, ObjectMethod, @@ -206,7 +207,6 @@ class Context { const hash = hashEffect(effect); let interned = this.internedEffects.get(hash); if (interned == null) { - console.log(`intern: ${hash}`); this.internedEffects.set(hash, effect); interned = effect; } @@ -334,7 +334,6 @@ function applySignature( } const value = state.kind(effect.value); switch (value.kind) { - case ValueKind.Global: case ValueKind.Frozen: { const reason = getWriteErrorReason({ kind: value.kind, @@ -353,7 +352,7 @@ function applySignature( description: effect.value.identifier.name !== null && effect.value.identifier.name.kind === 'named' - ? `Found mutation of \`${effect.value.identifier.name}\`` + ? `Found mutation of \`${effect.value.identifier.name.value}\`` : null, loc: effect.value.loc, suggestions: null, @@ -428,7 +427,7 @@ function applyEffect( } state.initialize(value, { kind: effect.value, - reason: new Set([ValueReason.Other]), + reason: new Set([effect.reason]), }); state.define(effect.into, value); break; @@ -448,7 +447,7 @@ function applyEffect( break; } case 'CreateFrom': { - const kind = state.kind(effect.from).kind; + const fromValue = state.kind(effect.from); let value = context.effectInstructionValueCache.get(effect); if (value == null) { value = { @@ -459,11 +458,11 @@ function applyEffect( context.effectInstructionValueCache.set(effect, value); } state.initialize(value, { - kind, - reason: new Set([ValueReason.Other]), + kind: fromValue.kind, + reason: new Set(fromValue.reason), }); state.define(effect.into, value); - switch (kind) { + switch (fromValue.kind) { case ValueKind.Primitive: case ValueKind.Global: { // no need to track this data flow @@ -587,7 +586,8 @@ function applyEffect( * Alias represents potential pointer aliasing. If the type is a global, * a primitive (copy-on-write semantics) then we can prune the effect */ - const fromKind = state.kind(effect.from).kind; + const fromValue = state.kind(effect.from); + const fromKind = fromValue.kind; switch (fromKind) { case ValueKind.Frozen: { effects.push({ @@ -604,7 +604,10 @@ function applyEffect( }; context.effectInstructionValueCache.set(effect, value); } - state.initialize(value, {kind: fromKind, reason: new Set([])}); + state.initialize(value, { + kind: fromKind, + reason: new Set(fromValue.reason), + }); state.define(effect.into, value); break; } @@ -619,7 +622,10 @@ function applyEffect( }; context.effectInstructionValueCache.set(effect, value); } - state.initialize(value, {kind: fromKind, reason: new Set([])}); + state.initialize(value, { + kind: fromKind, + reason: new Set(fromValue.reason), + }); state.define(effect.into, value); break; } @@ -722,6 +728,7 @@ function applyEffect( kind: 'Create', into: effect.into, value: ValueKind.Mutable, + reason: ValueReason.Other, }, aliased, effects, @@ -798,6 +805,8 @@ function applyEffect( const mutationKind = state.mutate(effect.kind, effect.value); if (mutationKind === 'mutate') { effects.push(effect); + } else if (mutationKind === 'mutate-ref') { + // no-op } else if ( mutationKind !== 'none' && (effect.kind === 'Mutate' || effect.kind === 'MutateTransitive') @@ -823,7 +832,7 @@ function applyEffect( description: effect.value.identifier.name !== null && effect.value.identifier.name.kind === 'named' - ? `Found mutation of \`${effect.value.identifier.name}\`` + ? `Found mutation of \`${effect.value.identifier.name.value}\`` : null, loc: effect.value.loc, suggestions: null, @@ -1010,6 +1019,9 @@ class InferenceState { kind: ValueKind.Frozen, reason: new Set([reason]), }); + if (DEBUG) { + console.log(`freeze value: ${printInstructionValue(value)} ${reason}`); + } if ( value.kind === 'FunctionExpression' && (this.env.config.enablePreserveExistingMemoizationGuarantees || @@ -1028,7 +1040,10 @@ class InferenceState { | 'MutateTransitive' | 'MutateTransitiveConditionally', place: Place, - ): 'none' | 'mutate' | 'mutate-frozen' | 'mutate-global' { + ): 'none' | 'mutate' | 'mutate-frozen' | 'mutate-global' | 'mutate-ref' { + if (isRefOrRefValue(place.identifier)) { + return 'mutate-ref'; + } const kind = this.kind(place).kind; switch (variant) { case 'MutateConditionally': @@ -1261,6 +1276,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Mutable, + reason: ValueReason.Other, }); // All elements are captured into part of the output value for (const element of value.elements) { @@ -1287,6 +1303,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Mutable, + reason: ValueReason.Other, }); for (const property of value.properties) { if (property.kind === 'ObjectProperty') { @@ -1310,6 +1327,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Mutable, + reason: ValueReason.Other, }); // Potentially mutates the receiver (awaiting it changes its state and can run side effects) effects.push({kind: 'MutateTransitiveConditionally', value: value.value}); @@ -1366,6 +1384,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); // Mutates the object by removing the property, no aliasing effects.push({kind: 'Mutate', value: value.object}); @@ -1378,6 +1397,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); } else { effects.push({ @@ -1400,6 +1420,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); break; } @@ -1458,6 +1479,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Mutable, + reason: ValueReason.Other, }); if ( isArrayType(value.collection.identifier) || @@ -1508,6 +1530,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); break; } @@ -1517,6 +1540,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Frozen, + reason: ValueReason.JsxCaptured, }); for (const operand of eachInstructionValueOperand(value)) { effects.push({ @@ -1538,12 +1562,14 @@ function computeSignatureForInstruction( kind: 'Create', into: value.lvalue.place, value: ValueKind.Mutable, + reason: ValueReason.Other, }); effects.push({ kind: 'Create', into: lvalue, // The result can't be referenced so this value doesn't matter value: ValueKind.Primitive, + reason: ValueReason.Other, }); break; } @@ -1554,12 +1580,14 @@ function computeSignatureForInstruction( into: value.lvalue.place, // TODO: what kind here??? value: ValueKind.Primitive, + reason: ValueReason.Other, }); effects.push({ kind: 'Create', into: lvalue, // TODO: what kind here??? value: ValueKind.Primitive, + reason: ValueReason.Other, }); break; } @@ -1589,6 +1617,7 @@ function computeSignatureForInstruction( kind: 'Create', into: value.lvalue.place, value: ValueKind.Mutable, + reason: ValueReason.Other, }); } // Which aliases the value @@ -1619,11 +1648,13 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); effects.push({ kind: 'Create', into: value.lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); break; } @@ -1632,19 +1663,11 @@ function computeSignatureForInstruction( kind: 'MutateGlobal', place: value.value, error: { - severity: ErrorSeverity.InvalidReact, - reason: getWriteErrorReason({ - kind: ValueKind.Global, - reason: new Set([ValueReason.Global]), - context: new Set(), - }), - description: - value.value.identifier.name !== null && - value.value.identifier.name.kind === 'named' - ? `Found mutation of \`${value.value.identifier.name}\`` - : null, - loc: value.value.loc, + reason: + 'Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)', + loc: instr.loc, suggestions: null, + severity: ErrorSeverity.InvalidReact, }, }); effects.push({kind: 'Assign', from: value.value, into: lvalue}); @@ -1659,6 +1682,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Global, + reason: ValueReason.Global, }); break; } @@ -1677,6 +1701,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); break; } @@ -1694,6 +1719,7 @@ function computeSignatureForInstruction( kind: 'Create', into: lvalue, value: ValueKind.Primitive, + reason: ValueReason.Other, }); break; } @@ -1719,16 +1745,16 @@ function computeEffectsForLegacySignature( receiver: Place, args: Array, ): Array { + const returnValueReason = signature.returnValueReason ?? ValueReason.Other; const effects: Array = []; effects.push({ kind: 'Create', into: lvalue, value: signature.returnValueKind, + reason: returnValueReason, }); const stores: Array = []; const captures: Array = []; - const returnValueReason = - signature.returnValueReason ?? ValueReason.KnownReturnSignature; function visit(place: Place, effect: Effect): void { switch (effect) { case Effect.Store: { @@ -2046,7 +2072,12 @@ function computeEffectsForSignature( case 'Create': { const into = substitutions.get(effect.into.identifier.id) ?? []; for (const value of into) { - effects.push({kind: 'Create', into: value, value: effect.value}); + effects.push({ + kind: 'Create', + into: value, + value: effect.value, + reason: effect.reason, + }); } break; } @@ -2297,7 +2328,7 @@ export type AliasingEffect = /** * Creates a value of the given type at the given place */ - | {kind: 'Create'; into: Place; value: ValueKind} + | {kind: 'Create'; into: Place; value: ValueKind; reason: ValueReason} /** * Creates a new value with the same kind as the starting value. */ @@ -2376,7 +2407,12 @@ function hashEffect(effect: AliasingEffect): string { ].join(':'); } case 'Create': { - return [effect.kind, effect.into.identifier.id].join(':'); + return [ + effect.kind, + effect.into.identifier.id, + effect.value, + effect.reason, + ].join(':'); } case 'Freeze': { return [effect.kind, effect.value.identifier.id, effect.reason].join(':'); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts index f80c2053a423c..1a0fbaa803b55 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingFunctionEffects.ts @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. */ -import {HIRFunction, IdentifierId, Place, ValueKind} from '../HIR'; +import {HIRFunction, IdentifierId, Place, ValueKind, ValueReason} from '../HIR'; import {getOrInsertDefault} from '../Utils/utils'; import {AliasingEffect} from './InferMutationAliasingEffects'; @@ -157,6 +157,7 @@ export function inferMutationAliasingFunctionEffects( fn.returnType.kind === 'Primitive' ? ValueKind.Primitive : ValueKind.Mutable, + reason: ValueReason.KnownReturnSignature, }); } 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 39a8e71f256b8..181f5d3d0e7df 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -6,7 +6,7 @@ */ import prettyFormat from 'pretty-format'; -import {CompilerError} from '..'; +import {CompilerError, SourceLocation} from '..'; import { BlockId, Effect, @@ -26,6 +26,7 @@ import {assertExhaustive, getOrInsertWith} from '../Utils/utils'; import {printFunction} from '../HIR'; import {printIdentifier, printPlace} from '../HIR/PrintHIR'; import {MutationKind} from './InferMutationAliasingFunctionEffects'; +import {Result} from '../Utils/Result'; const DEBUG = false; const VERBOSE = false; @@ -33,18 +34,11 @@ const VERBOSE = false; /** * Infers mutable ranges for all values. */ -export function inferMutationAliasingRanges(fn: HIRFunction): void { +export function inferMutationAliasingRanges( + fn: HIRFunction, + {isFunctionExpression}: {isFunctionExpression: boolean}, +): Result { if (VERBOSE) { - console.log(); - console.log(); - console.log(); - console.log(); - console.log(); - console.log(); - console.log(); - console.log(); - console.log(); - console.log(); console.log(); console.log(printFunction(fn)); } @@ -75,14 +69,16 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { let index = 0; + const errors = new CompilerError(); + for (const param of [...fn.params, ...fn.context, fn.returns]) { const place = param.kind === 'Identifier' ? param : param.place; - state.create(place); + state.create(place, {kind: 'Object'}); } const seenBlocks = new Set(); for (const block of fn.body.blocks.values()) { for (const phi of block.phis) { - state.create(phi.place, true); + state.create(phi.place, {kind: 'Phi'}); for (const [pred, operand] of phi.operands) { if (!seenBlocks.has(pred)) { // NOTE: annotation required to actually typecheck and not silently infer `any` @@ -100,14 +96,29 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { seenBlocks.add(block.id); for (const instr of block.instructions) { - for (const lvalue of eachInstructionLValue(instr)) { - state.create(lvalue); + if ( + instr.value.kind === 'FunctionExpression' || + instr.value.kind === 'ObjectMethod' + ) { + state.create(instr.lvalue, { + kind: 'Function', + function: instr.value.loweredFunc.func, + }); + } else { + for (const lvalue of eachInstructionLValue(instr)) { + state.create(lvalue, {kind: 'Object'}); + } } if (instr.effects == null) continue; for (const effect of instr.effects) { - if (effect.kind === 'Create' || effect.kind === 'CreateFunction') { - state.create(effect.into); + if (effect.kind === 'Create') { + state.create(effect.into, {kind: 'Object'}); + } else if (effect.kind === 'CreateFunction') { + state.create(effect.into, { + kind: 'Function', + function: effect.function.loweredFunc.func, + }); } else if (effect.kind === 'CreateFrom') { state.createFrom(index++, effect.from, effect.into); } else if (effect.kind === 'Assign' || effect.kind === 'Alias') { @@ -142,6 +153,11 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { : MutationKind.Conditional, place: effect.value, }); + } else if ( + effect.kind === 'MutateFrozen' || + effect.kind === 'MutateGlobal' + ) { + errors.push(effect.error); } } } @@ -184,6 +200,8 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { makeInstructionId(mutation.id + 1), mutation.transitive, mutation.kind, + mutation.place.loc, + errors, ); } if (VERBOSE) { @@ -197,31 +215,35 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { continue; } let mutated = false; - if (node.local === MutationKind.Conditional) { - mutated = true; - fn.aliasingEffects.push({ - kind: 'MutateConditionally', - value: place, - }); - } else if (node.local === MutationKind.Definite) { - mutated = true; - fn.aliasingEffects.push({ - kind: 'Mutate', - value: place, - }); + if (node.local != null) { + if (node.local.kind === MutationKind.Conditional) { + mutated = true; + fn.aliasingEffects.push({ + kind: 'MutateConditionally', + value: {...place, loc: node.local.loc}, + }); + } else if (node.local.kind === MutationKind.Definite) { + mutated = true; + fn.aliasingEffects.push({ + kind: 'Mutate', + value: {...place, loc: node.local.loc}, + }); + } } - if (node.transitive === MutationKind.Conditional) { - mutated = true; - fn.aliasingEffects.push({ - kind: 'MutateTransitiveConditionally', - value: place, - }); - } else if (node.transitive === MutationKind.Definite) { - mutated = true; - fn.aliasingEffects.push({ - kind: 'MutateTransitive', - value: place, - }); + if (node.transitive != null) { + if (node.transitive.kind === MutationKind.Conditional) { + mutated = true; + fn.aliasingEffects.push({ + kind: 'MutateTransitiveConditionally', + value: {...place, loc: node.transitive.loc}, + }); + } else if (node.transitive.kind === MutationKind.Definite) { + mutated = true; + fn.aliasingEffects.push({ + kind: 'MutateTransitive', + value: {...place, loc: node.transitive.loc}, + }); + } } if (mutated) { place.effect = Effect.Capture; @@ -360,14 +382,33 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { operand.effect = effect; } } - for (const operand of eachTerminalOperand(block.terminal)) { - operand.effect = Effect.Read; + if (block.terminal.kind === 'return') { + block.terminal.value.effect = isFunctionExpression + ? Effect.Read + : Effect.Freeze; + } else { + for (const operand of eachTerminalOperand(block.terminal)) { + operand.effect = Effect.Read; + } } } if (VERBOSE) { console.log(printFunction(fn)); } + return errors.asResult(); +} + +function appendFunctionErrors(errors: CompilerError, fn: HIRFunction): void { + for (const effect of fn.aliasingEffects ?? []) { + switch (effect.kind) { + case 'MutateFrozen': + case 'MutateGlobal': { + errors.push(effect.error); + break; + } + } + } } type Node = { @@ -376,28 +417,31 @@ type Node = { captures: Map; aliases: Map; edges: Array<{index: number; node: Identifier; kind: 'capture' | 'alias'}>; - transitive: MutationKind; - local: MutationKind; - phi: boolean; + transitive: {kind: MutationKind; loc: SourceLocation} | null; + local: {kind: MutationKind; loc: SourceLocation} | null; + value: + | {kind: 'Object'} + | {kind: 'Phi'} + | {kind: 'Function'; function: HIRFunction}; }; class AliasingState { nodes: Map = new Map(); - create(place: Place, phi: boolean = false): void { + create(place: Place, value: Node['value']): void { this.nodes.set(place.identifier, { id: place.identifier, createdFrom: new Map(), captures: new Map(), aliases: new Map(), edges: [], - transitive: MutationKind.None, - local: MutationKind.None, - phi, + transitive: null, + local: null, + value, }); } createFrom(index: number, from: Place, into: Place): void { - this.create(into); + this.create(into, {kind: 'Object'}); const fromNode = this.nodes.get(from.identifier); const toNode = this.nodes.get(into.identifier); if (fromNode == null || toNode == null) { @@ -454,6 +498,8 @@ class AliasingState { end: InstructionId, transitive: boolean, kind: MutationKind, + loc: SourceLocation, + errors: CompilerError, ): void { const seen = new Set(); const queue: Array<{ @@ -484,10 +530,21 @@ class AliasingState { node.id.mutableRange.end = makeInstructionId( Math.max(node.id.mutableRange.end, end), ); + if ( + node.value.kind === 'Function' && + node.transitive == null && + node.local == null + ) { + appendFunctionErrors(errors, node.value.function); + } if (transitive) { - node.transitive = Math.max(node.transitive, kind); + if (node.transitive == null || node.transitive.kind < kind) { + node.transitive = {kind, loc}; + } } else { - node.local = Math.max(node.local, kind); + if (node.local == null || node.local.kind < kind) { + node.local = {kind, loc}; + } } /** * all mutations affect "forward" edges by the rules: @@ -506,7 +563,7 @@ class AliasingState { } queue.push({place: alias, transitive: true, direction: 'backwards'}); } - if (direction === 'backwards' || !node.phi) { + if (direction === 'backwards' || node.value.kind !== 'Phi') { /** * all mutations affect backward alias edges by the rules: * - Alias a -> b, mutate(b) => mutate(a) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts index 81612a7441728..2de67ebc5fa7f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts @@ -112,6 +112,31 @@ export function validateNoFreezingKnownMutableFunctions( ); if (knownMutation && knownMutation.kind === 'ContextMutation') { contextMutationEffects.set(lvalue.identifier.id, knownMutation); + } else if ( + fn.env.config.enableNewMutationAliasingModel && + value.loweredFunc.func.aliasingEffects != null + ) { + const context = new Set( + value.loweredFunc.func.context.map(p => p.identifier.id), + ); + effects: for (const effect of value.loweredFunc.func + .aliasingEffects) { + switch (effect.kind) { + case 'Mutate': + case 'MutateTransitive': { + if (context.has(effect.value.identifier.id)) { + contextMutationEffects.set(lvalue.identifier.id, { + kind: 'ContextMutation', + effect: Effect.Mutate, + loc: effect.value.loc, + places: new Set([effect.value]), + }); + break effects; + } + break; + } + } + } } break; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.expect.md new file mode 100644 index 0000000000000..06f5668761cac --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.expect.md @@ -0,0 +1,125 @@ + +## Input + +```javascript +// @enableNewMutationAliasingModel +import {makeArray, mutate} from 'shared-runtime'; + +/** + * Bug repro: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe"}} + * Forget: + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe","wat1":"joe"}} + * + * Fork of `capturing-func-alias-captured-mutate`, but instead of directly + * aliasing `y` via `[y]`, we make an opaque call. + * + * Note that the bug here is that we don't infer that `a = makeArray(y)` + * potentially captures a context variable into a local variable. As a result, + * we don't understand that `a[0].x = b` captures `x` into `y` -- instead, we're + * currently inferring that this lambda captures `y` (for a potential later + * mutation) and simply reads `x`. + * + * Concretely `InferReferenceEffects.hasContextRefOperand` is incorrectly not + * used when we analyze CallExpressions. + */ +function Component({foo, bar}: {foo: number; bar: number}) { + let x = {foo}; + let y: {bar: number; x?: {foo: number}} = {bar}; + const f0 = function () { + let a = makeArray(y); // a = [y] + let b = x; + // this writes y.x = x + a[0].x = b; + }; + f0(); + mutate(y.x); + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{foo: 3, bar: 4}], + sequentialRenders: [ + {foo: 3, bar: 4}, + {foo: 3, bar: 5}, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel +import { makeArray, mutate } from "shared-runtime"; + +/** + * Bug repro: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe"}} + * Forget: + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe","wat1":"joe"}} + * + * Fork of `capturing-func-alias-captured-mutate`, but instead of directly + * aliasing `y` via `[y]`, we make an opaque call. + * + * Note that the bug here is that we don't infer that `a = makeArray(y)` + * potentially captures a context variable into a local variable. As a result, + * we don't understand that `a[0].x = b` captures `x` into `y` -- instead, we're + * currently inferring that this lambda captures `y` (for a potential later + * mutation) and simply reads `x`. + * + * Concretely `InferReferenceEffects.hasContextRefOperand` is incorrectly not + * used when we analyze CallExpressions. + */ +function Component(t0) { + const $ = _c(3); + const { foo, bar } = t0; + let y; + if ($[0] !== bar || $[1] !== foo) { + const x = { foo }; + y = { bar }; + const f0 = function () { + const a = makeArray(y); + const b = x; + + a[0].x = b; + }; + + f0(); + mutate(y.x); + $[0] = bar; + $[1] = foo; + $[2] = y; + } else { + y = $[2]; + } + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ foo: 3, bar: 4 }], + sequentialRenders: [ + { foo: 3, bar: 4 }, + { foo: 3, bar: 5 }, + ], +}; + +``` + +### Eval output +(kind: ok) {"bar":4,"x":{"foo":3,"wat0":"joe"}} +{"bar":5,"x":{"foo":3,"wat0":"joe"}} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.ts new file mode 100644 index 0000000000000..6d183a5700c89 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/repro-capturing-func-maybealias-captured-mutate.ts @@ -0,0 +1,49 @@ +// @enableNewMutationAliasingModel +import {makeArray, mutate} from 'shared-runtime'; + +/** + * Bug repro: + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe"}} + * Forget: + * (kind: ok) + * {"bar":4,"x":{"foo":3,"wat0":"joe"}} + * {"bar":5,"x":{"foo":3,"wat0":"joe","wat1":"joe"}} + * + * Fork of `capturing-func-alias-captured-mutate`, but instead of directly + * aliasing `y` via `[y]`, we make an opaque call. + * + * Note that the bug here is that we don't infer that `a = makeArray(y)` + * potentially captures a context variable into a local variable. As a result, + * we don't understand that `a[0].x = b` captures `x` into `y` -- instead, we're + * currently inferring that this lambda captures `y` (for a potential later + * mutation) and simply reads `x`. + * + * Concretely `InferReferenceEffects.hasContextRefOperand` is incorrectly not + * used when we analyze CallExpressions. + */ +function Component({foo, bar}: {foo: number; bar: number}) { + let x = {foo}; + let y: {bar: number; x?: {foo: number}} = {bar}; + const f0 = function () { + let a = makeArray(y); // a = [y] + let b = x; + // this writes y.x = x + a[0].x = b; + }; + f0(); + mutate(y.x); + return y; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{foo: 3, bar: 4}], + sequentialRenders: [ + {foo: 3, bar: 4}, + {foo: 3, bar: 5}, + ], +};