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 7524f4fff509..236385801292 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 8d2e72b22e4e..5f063ad2f999 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 aa3865097f0e..10c45ccb3f5e 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 30675e2820d1..91f99395b03e 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 1842b8fe3645..b0f406568ac5 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 f80c2053a423..1a0fbaa803b5 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 39a8e71f256b..181f5d3d0e7d 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 81612a744172..2de67ebc5fa7 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 000000000000..06f5668761ca --- /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 000000000000..6d183a5700c8 --- /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}, + ], +};