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 89591aca2dfb0..13855ac63be28 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -893,7 +893,8 @@ export function printType(type: Type): string { if (type.kind === 'Object' && type.shapeId != null) { return `:T${type.kind}<${type.shapeId}>`; } else if (type.kind === 'Function' && type.shapeId != null) { - return `:T${type.kind}<${type.shapeId}>`; + const returnType = printType(type.return); + return `:T${type.kind}<${type.shapeId}>()${returnType !== '' ? returnType : ''}`; } else { return `:T${type.kind}`; } 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 b91b606d507e6..f266015a45b72 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -23,8 +23,8 @@ import { IdentifierId, Instruction, InstructionKind, - InstructionValue, isArrayType, + isJsxType, isMapType, isPrimitiveType, isRefOrRefValue, @@ -59,7 +59,6 @@ import { printAliasingSignature, printIdentifier, printInstruction, - printInstructionValue, printPlace, printSourceLocation, } from '../HIR/PrintHIR'; @@ -105,11 +104,11 @@ export function inferMutationAliasingEffects( const statesByBlock: Map = new Map(); for (const ref of fn.context) { - // TODO: using InstructionValue as a bit of a hack, but it's pragmatic - const value: InstructionValue = { - kind: 'ObjectExpression', - properties: [], - loc: ref.loc, + const value: AliasingEffect = { + kind: 'Create', + into: ref, + value: ValueKind.Context, + reason: ValueReason.Other, }; initialState.initialize(value, { kind: ValueKind.Context, @@ -142,10 +141,11 @@ export function inferMutationAliasingEffects( } if (ref != null) { const place = ref.kind === 'Identifier' ? ref : ref.place; - const value: InstructionValue = { - kind: 'ObjectExpression', - properties: [], - loc: place.loc, + const value: AliasingEffect = { + kind: 'Create', + into: place, + value: ValueKind.Mutable, + reason: ValueReason.Other, }; initialState.initialize(value, { kind: ValueKind.Mutable, @@ -262,8 +262,6 @@ function findHoistedContextDeclarations( class Context { internedEffects: Map = new Map(); instructionSignatureCache: Map = new Map(); - effectInstructionValueCache: Map = - new Map(); applySignatureCache: Map< AliasingSignature, Map | null> @@ -315,10 +313,11 @@ function inferParam( paramKind: AbstractValue, ): void { const place = param.kind === 'Identifier' ? param : param.place; - const value: InstructionValue = { - kind: 'Primitive', - loc: place.loc, - value: undefined, + const value: AliasingEffect = { + kind: 'Create', + into: place, + value: paramKind.kind, + reason: ValueReason.Other, }; initialState.initialize(value, paramKind); initialState.define(place, value); @@ -526,20 +525,11 @@ function applyEffect( }); initialized.add(effect.into.identifier.id); - let value = context.effectInstructionValueCache.get(effect); - if (value == null) { - value = { - kind: 'ObjectExpression', - properties: [], - loc: effect.into.loc, - }; - context.effectInstructionValueCache.set(effect, value); - } - state.initialize(value, { + state.initialize(effect, { kind: effect.value, reason: new Set([effect.reason]), }); - state.define(effect.into, value); + state.define(effect.into, effect); effects.push(effect); break; } @@ -566,20 +556,11 @@ function applyEffect( initialized.add(effect.into.identifier.id); const fromValue = state.kind(effect.from); - let value = context.effectInstructionValueCache.get(effect); - if (value == null) { - value = { - kind: 'ObjectExpression', - properties: [], - loc: effect.into.loc, - }; - context.effectInstructionValueCache.set(effect, value); - } - state.initialize(value, { + state.initialize(effect, { kind: fromValue.kind, reason: new Set(fromValue.reason), }); - state.define(effect.into, value); + state.define(effect.into, effect); switch (fromValue.kind) { case ValueKind.Primitive: case ValueKind.Global: { @@ -667,11 +648,11 @@ function applyEffect( operand.effect = Effect.Read; } } - state.initialize(effect.function, { + state.initialize(effect, { kind: isMutable ? ValueKind.Mutable : ValueKind.Frozen, reason: new Set([]), }); - state.define(effect.into, effect.function); + state.define(effect.into, effect); for (const capture of effect.captures) { applyEffect( context, @@ -776,38 +757,20 @@ function applyEffect( initialized, effects, ); - let value = context.effectInstructionValueCache.get(effect); - if (value == null) { - value = { - kind: 'Primitive', - value: undefined, - loc: effect.from.loc, - }; - context.effectInstructionValueCache.set(effect, value); - } - state.initialize(value, { + state.initialize(effect, { kind: fromKind, reason: new Set(fromValue.reason), }); - state.define(effect.into, value); + state.define(effect.into, effect); break; } case ValueKind.Global: case ValueKind.Primitive: { - let value = context.effectInstructionValueCache.get(effect); - if (value == null) { - value = { - kind: 'Primitive', - value: undefined, - loc: effect.from.loc, - }; - context.effectInstructionValueCache.set(effect, value); - } - state.initialize(value, { + state.initialize(effect, { kind: fromKind, reason: new Set(fromValue.reason), }); - state.define(effect.into, value); + state.define(effect.into, effect); break; } default: { @@ -822,14 +785,15 @@ function applyEffect( const functionValues = state.values(effect.function); if ( functionValues.length === 1 && - functionValues[0].kind === 'FunctionExpression' && - functionValues[0].loweredFunc.func.aliasingEffects != null + functionValues[0].kind === 'CreateFunction' && + functionValues[0].function.kind === 'FunctionExpression' && + functionValues[0].function.loweredFunc.func.aliasingEffects != null ) { /* * We're calling a locally declared function, we already know it's effects! * We just have to substitute in the args for the params */ - const functionExpr = functionValues[0]; + const functionExpr = functionValues[0].function; let signature = context.functionSignatureCache.get(functionExpr); if (signature == null) { signature = buildSignatureFromFunctionExpression( @@ -1114,19 +1078,19 @@ class InferenceState { #isFunctionExpression: boolean; // The kind of each value, based on its allocation site - #values: Map; + #values: Map; /* * The set of values pointed to by each identifier. This is a set * to accomodate phi points (where a variable may have different * values from different control flow paths). */ - #variables: Map>; + #variables: Map>; constructor( env: Environment, isFunctionExpression: boolean, - values: Map, - variables: Map>, + values: Map, + variables: Map>, ) { this.env = env; this.#isFunctionExpression = isFunctionExpression; @@ -1146,18 +1110,11 @@ class InferenceState { } // (Re)initializes a @param value with its default @param kind. - initialize(value: InstructionValue, kind: AbstractValue): void { - CompilerError.invariant(value.kind !== 'LoadLocal', { - reason: - '[InferMutationAliasingEffects] Expected all top-level identifiers to be defined as variables, not values', - description: null, - loc: value.loc, - suggestions: null, - }); + initialize(value: AliasingEffect, kind: AbstractValue): void { this.#values.set(value, kind); } - values(place: Place): Array { + values(place: Place): Array { const values = this.#variables.get(place.identifier.id); CompilerError.invariant(values != null, { reason: `[InferMutationAliasingEffects] Expected value kind to be initialized`, @@ -1220,13 +1177,13 @@ class InferenceState { } // Defines (initializing or updating) a variable with a specific kind of value. - define(place: Place, value: InstructionValue): void { + define(place: Place, value: AliasingEffect): void { CompilerError.invariant(this.#values.has(value), { reason: `[InferMutationAliasingEffects] Expected value to be initialized at '${printSourceLocation( - value.loc, + place.loc, )}'`, - description: printInstructionValue(value), - loc: value.loc, + description: printAliasingEffect(value), + loc: place.loc, suggestions: null, }); this.#variables.set(place.identifier.id, new Set([value])); @@ -1266,17 +1223,17 @@ class InferenceState { } } - freezeValue(value: InstructionValue, reason: ValueReason): void { + freezeValue(value: AliasingEffect, reason: ValueReason): void { this.#values.set(value, { kind: ValueKind.Frozen, reason: new Set([reason]), }); if ( - value.kind === 'FunctionExpression' && + value.kind === 'CreateFunction' && (this.env.config.enablePreserveExistingMemoizationGuarantees || this.env.config.enableTransitivelyFreezeFunctionExpressions) ) { - for (const place of value.loweredFunc.func.context) { + for (const place of value.function.loweredFunc.func.context) { this.freeze(place, reason); } } @@ -1351,8 +1308,8 @@ class InferenceState { * termination. */ merge(other: InferenceState): InferenceState | null { - let nextValues: Map | null = null; - let nextVariables: Map> | null = null; + let nextValues: Map | null = null; + let nextVariables: Map> | null = null; for (const [id, thisValue] of this.#values) { const otherValue = other.#values.get(id); @@ -1376,7 +1333,7 @@ class InferenceState { for (const [id, thisValues] of this.#variables) { const otherValues = other.#variables.get(id); if (otherValues !== undefined) { - let mergedValues: Set | null = null; + let mergedValues: Set | null = null; for (const otherValue of otherValues) { if (!thisValues.has(otherValue)) { mergedValues = mergedValues ?? new Set(thisValues); @@ -1429,8 +1386,8 @@ class InferenceState { */ debug(): any { const result: any = {values: {}, variables: {}}; - const objects: Map = new Map(); - function identify(value: InstructionValue): number { + const objects: Map = new Map(); + function identify(value: AliasingEffect): number { let id = objects.get(value); if (id == null) { id = objects.size; @@ -1442,7 +1399,7 @@ class InferenceState { const id = identify(value); result.values[id] = { abstract: this.debugAbstractValue(kind), - value: printInstructionValue(value), + value: printAliasingEffect(value), }; } for (const [variable, values] of this.#variables) { @@ -1459,7 +1416,7 @@ class InferenceState { } inferPhi(phi: Phi): void { - const values: Set = new Set(); + const values: Set = new Set(); for (const [_, operand] of phi.operands) { const operandValues = this.#variables.get(operand.identifier.id); // This is a backedge that will be handled later by State.merge @@ -1842,6 +1799,19 @@ function computeSignatureForInstruction( }); } } + for (const prop of value.props) { + if ( + prop.kind === 'JsxAttribute' && + prop.place.identifier.type.kind === 'Function' && + isJsxType(prop.place.identifier.type.return) + ) { + // Any props which return jsx are assumed to be called during render + effects.push({ + kind: 'Render', + place: prop.place, + }); + } + } } break; } @@ -2277,8 +2247,9 @@ function areArgumentsImmutableAndNonMutating( const values = state.values(place); for (const value of values) { if ( - value.kind === 'FunctionExpression' && - value.loweredFunc.func.params.some(param => { + value.kind === 'CreateFunction' && + value.function.kind === 'FunctionExpression' && + value.function.loweredFunc.func.params.some(param => { const place = param.kind === 'Identifier' ? param : param.place; const range = place.identifier.mutableRange; return range.end > range.start + 1; @@ -2470,10 +2441,47 @@ function computeEffectsForSignature( break; } case 'CreateFunction': { - CompilerError.throwTodo({ - reason: `Support CreateFrom effects in signatures`, - loc: receiver.loc, + const applyInto = substitutions.get(effect.into.identifier.id); + if (applyInto == null || applyInto.length !== 1) { + return null; + } + const captures: Array = []; + for (let i = 0; i < effect.captures.length; i++) { + const substitution = substitutions.get( + effect.captures[i].identifier.id, + ); + if (substitution == null || substitution.length !== 1) { + return null; + } + captures.push(substitution[0]); + } + const context: Array = []; + const originalContext = effect.function.loweredFunc.func.context; + for (let i = 0; i < originalContext.length; i++) { + const substitution = substitutions.get( + originalContext[i].identifier.id, + ); + if (substitution == null || substitution.length !== 1) { + return null; + } + context.push(substitution[0]); + } + effects.push({ + kind: 'CreateFunction', + into: applyInto[0], + function: { + ...effect.function, + loweredFunc: { + ...effect.function.loweredFunc, + func: { + ...effect.function.loweredFunc.func, + context, + }, + }, + }, + captures, }); + break; } default: { assertExhaustive( 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 79f8cf8c0e85b..0dce5f81d0891 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -140,7 +140,7 @@ export function inferMutationAliasingRanges( } else if (effect.kind === 'CreateFunction') { state.create(effect.into, { kind: 'Function', - function: effect.function.loweredFunc.func, + effect, }); } else if (effect.kind === 'CreateFrom') { state.createFrom(index++, effect.from, effect.into); @@ -155,7 +155,7 @@ export function inferMutationAliasingRanges( * invariant here. */ if (!state.nodes.has(effect.into.identifier)) { - state.create(effect.into, {kind: 'Object'}); + state.create(effect.into, {kind: 'Assign'}); } state.assign(index++, effect.from, effect.into); } else if (effect.kind === 'Alias') { @@ -465,6 +465,99 @@ export function inferMutationAliasingRanges( } } + const tracked: Array = []; + for (const param of [...fn.params, ...fn.context, fn.returns]) { + const place = param.kind === 'Identifier' ? param : param.place; + tracked.push(place); + } + + const returned: Set = new Set(); + const queue: Array = [state.nodes.get(fn.returns.identifier)!]; + const seen: Set = new Set(); + while (queue.length !== 0) { + const node = queue.pop()!; + if (seen.has(node)) { + continue; + } + seen.add(node); + for (const id of node.aliases.keys()) { + queue.push(state.nodes.get(id)!); + } + for (const id of node.createdFrom.keys()) { + queue.push(state.nodes.get(id)!); + } + if (node.id.id === fn.returns.identifier.id) { + continue; + } + switch (node.value.kind) { + case 'Assign': + case 'CreateFrom': { + break; + } + case 'Phi': + case 'Object': + case 'Function': { + returned.add(node); + break; + } + default: { + assertExhaustive( + node.value, + `Unexpected node value kind '${(node.value as any).kind}'`, + ); + } + } + } + const returnedValues = [...returned]; + if ( + returnedValues.length === 1 && + returnedValues[0].value.kind === 'Object' && + tracked.some(place => place.identifier.id === returnedValues[0].id.id) + ) { + const from = tracked.find( + place => place.identifier.id === returnedValues[0].id.id, + )!; + functionEffects.push({ + kind: 'Assign', + from, + into: fn.returns, + }); + } else if ( + returnedValues.length === 1 && + returnedValues[0].value.kind === 'Function' + ) { + const outerContext = new Set(fn.context.map(p => p.identifier.id)); + const effect = returnedValues[0].value.effect; + functionEffects.push({ + kind: 'CreateFunction', + function: { + ...effect.function, + loweredFunc: { + func: { + ...effect.function.loweredFunc.func, + context: effect.function.loweredFunc.func.context.filter(p => + outerContext.has(p.identifier.id), + ), + }, + }, + }, + captures: effect.captures.filter(p => outerContext.has(p.identifier.id)), + into: fn.returns, + }); + } else { + const returns = fn.returns.identifier; + functionEffects.push({ + kind: 'Create', + into: fn.returns, + value: isPrimitiveType(returns) + ? ValueKind.Primitive + : isJsxType(returns.type) + ? ValueKind.Frozen + : ValueKind.Mutable, + reason: ValueReason.KnownReturnSignature, + }); + } + /** * Part 3 * Finish populating the externally visible effects. Above we bubble-up the side effects @@ -472,28 +565,12 @@ export function inferMutationAliasingRanges( * Here we populate an effect to create the return value as well as populating alias/capture * effects for how data flows between the params, context vars, and return. */ - const returns = fn.returns.identifier; - functionEffects.push({ - kind: 'Create', - into: fn.returns, - value: isPrimitiveType(returns) - ? ValueKind.Primitive - : isJsxType(returns.type) - ? ValueKind.Frozen - : ValueKind.Mutable, - reason: ValueReason.KnownReturnSignature, - }); /** * Determine precise data-flow effects by simulating transitive mutations of the params/ * captures and seeing what other params/context variables are affected. Anything that * would be transitively mutated needs a capture relationship. */ - const tracked: Array = []; const ignoredErrors = new CompilerError(); - for (const param of [...fn.params, ...fn.context, fn.returns]) { - const place = param.kind === 'Identifier' ? param : param.place; - tracked.push(place); - } for (const into of tracked) { const mutationIndex = index++; state.mutate( @@ -572,9 +649,14 @@ type Node = { local: {kind: MutationKind; loc: SourceLocation} | null; lastMutated: number; value: + | {kind: 'Assign'} + | {kind: 'CreateFrom'} | {kind: 'Object'} | {kind: 'Phi'} - | {kind: 'Function'; function: HIRFunction}; + | { + kind: 'Function'; + effect: Extract; + }; }; class AliasingState { nodes: Map = new Map(); @@ -594,7 +676,7 @@ class AliasingState { } createFrom(index: number, from: Place, into: Place): void { - this.create(into, {kind: 'Object'}); + this.create(into, {kind: 'CreateFrom'}); const fromNode = this.nodes.get(from.identifier); const toNode = this.nodes.get(into.identifier); if (fromNode == null || toNode == null) { @@ -644,7 +726,10 @@ class AliasingState { continue; } if (node.value.kind === 'Function') { - appendFunctionErrors(errors, node.value.function); + appendFunctionErrors( + errors, + node.value.effect.function.loweredFunc.func, + ); } for (const [alias, when] of node.createdFrom) { if (when >= index) { @@ -704,7 +789,10 @@ class AliasingState { node.transitive == null && node.local == null ) { - appendFunctionErrors(errors, node.value.function); + appendFunctionErrors( + errors, + node.value.effect.function.loweredFunc.func, + ); } if (transitive) { if (node.transitive == null || node.transitive.kind < kind) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 859c871c263ad..9070d143bedcb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -712,6 +712,15 @@ class Unifier { return {kind: 'Phi', operands: type.operands.map(o => this.get(o))}; } + if (type.kind === 'Function') { + return { + kind: 'Function', + isConstructor: type.isConstructor, + shapeId: type.shapeId, + return: this.get(type.return), + }; + } + return type; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md new file mode 100644 index 0000000000000..037b2997bcdd9 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +function Component() { + const renderItem = item => { + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} + +``` + + +## Error + +``` + 6 | // called during render, and thus it's unsafe to mutate globals or call + 7 | // other impure code. +> 8 | global.property = true; + | ^^^^^^ InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect (8:8) + 9 | return ; + 10 | }; + 11 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js new file mode 100644 index 0000000000000..9355c482fb61a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-global-in-render-helper-prop.js @@ -0,0 +1,12 @@ +function Component() { + const renderItem = item => { + // Normally we assume that it's safe to mutate globals in a function passed + // as a prop, because the prop could be used as an event handler or effect. + // But if the function returns JSX we can assume it's a render helper, ie + // called during render, and thus it's unsafe to mutate globals or call + // other impure code. + global.property = true; + return ; + }; + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-returned-inner-fn-reassigns-context.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-returned-inner-fn-reassigns-context.expect.md index da6b57defe0b3..f7742f8f8b667 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-returned-inner-fn-reassigns-context.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-returned-inner-fn-reassigns-context.expect.md @@ -55,7 +55,7 @@ import { makeArray, Stringify, useIdentity } from "shared-runtime"; */ function Foo(t0) { "use memo"; - const $ = _c(3); + const $ = _c(5); const { b } = t0; const fnFactory = () => () => { @@ -66,18 +66,26 @@ function Foo(t0) { useIdentity(); const fn = fnFactory(); - const arr = makeArray(b); - fn(arr); let t1; - if ($[0] !== arr || $[1] !== myVar) { - t1 = ; - $[0] = arr; - $[1] = myVar; - $[2] = t1; + if ($[0] !== b) { + t1 = makeArray(b); + $[0] = b; + $[1] = t1; + } else { + t1 = $[1]; + } + const arr = t1; + fn(arr); + let t2; + if ($[2] !== arr || $[3] !== myVar) { + t2 = ; + $[2] = arr; + $[3] = myVar; + $[4] = t2; } else { - t1 = $[2]; + t2 = $[4]; } - return t1; + return t2; } function _temp2() { return console.log("b");