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 1b76dfb43e..4d9a791f62 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -96,7 +96,6 @@ import {propagateScopeDependenciesHIR} from '../HIR/PropagateScopeDependenciesHI import {outlineJSX} from '../Optimization/OutlineJsx'; import {optimizePropsMethodCalls} from '../Optimization/OptimizePropsMethodCalls'; import {transformFire} from '../Transform'; -import {validateNoImpureFunctionsInRender} from '../Validation/ValidateNoImpureFunctionsInRender'; import {validateStaticComponents} from '../Validation/ValidateStaticComponents'; import {validateNoFreezingKnownMutableFunctions} from '../Validation/ValidateNoFreezingKnownMutableFunctions'; import {inferMutationAliasingEffects} from '../Inference/InferMutationAliasingEffects'; @@ -293,10 +292,6 @@ function runWithEnvironment( env.logErrors(validateNoJSXInTryStatement(hir)); } - if (env.config.validateNoImpureFunctionsInRender) { - validateNoImpureFunctionsInRender(hir).unwrap(); - } - validateNoFreezingKnownMutableFunctions(hir).unwrap(); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts index 441b5d5452..5a4d9cf8c9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -38,7 +38,7 @@ import { addObject, } from './ObjectShape'; import {BuiltInType, ObjectType, PolyType} from './Types'; -import {TypeConfig} from './TypeSchema'; +import {AliasingSignatureConfig, TypeConfig} from './TypeSchema'; import {assertExhaustive} from '../Utils/utils'; import {isHookName} from './Environment'; import {CompilerError, SourceLocation} from '..'; @@ -626,6 +626,78 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [ // TODO: rest of Global objects ]; +const RenderHookAliasing: ( + reason: ValueReason, +) => AliasingSignatureConfig = reason => ({ + receiver: '@receiver', + params: [], + rest: '@rest', + returns: '@returns', + temporaries: [], + effects: [ + // Freeze the arguments + { + kind: 'Freeze', + value: '@rest', + reason: ValueReason.HookCaptured, + }, + // Render the arguments + { + kind: 'Render', + place: '@rest', + }, + // Returns a frozen value + { + kind: 'Create', + into: '@returns', + value: ValueKind.Frozen, + reason, + }, + // May alias any arguments into the return + { + kind: 'Alias', + from: '@rest', + into: '@returns', + }, + ], +}); + +const EffectHookAliasing: AliasingSignatureConfig = { + receiver: '@receiver', + params: [], + rest: '@rest', + returns: '@returns', + temporaries: ['@effect'], + effects: [ + // Freezes the function and deps + { + kind: 'Freeze', + value: '@rest', + reason: ValueReason.Effect, + }, + // Internally creates an effect object that captures the function and deps + { + kind: 'Create', + into: '@effect', + value: ValueKind.Frozen, + reason: ValueReason.KnownReturnSignature, + }, + // The effect stores the function and dependencies + { + kind: 'Capture', + from: '@rest', + into: '@effect', + }, + // Returns undefined + { + kind: 'Create', + into: '@returns', + value: ValueKind.Primitive, + reason: ValueReason.KnownReturnSignature, + }, + ], +}; + /* * TODO(mofeiZ): We currently only store rest param effects for hooks. * now that FeatureFlag `enableTreatHooksAsFunctions` is removed we can @@ -644,6 +716,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ hookKind: 'useContext', returnValueKind: ValueKind.Frozen, returnValueReason: ValueReason.Context, + aliasing: RenderHookAliasing(ValueReason.Context), }, BuiltInUseContextHookId, ), @@ -658,6 +731,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ hookKind: 'useState', returnValueKind: ValueKind.Frozen, returnValueReason: ValueReason.State, + aliasing: RenderHookAliasing(ValueReason.State), }), ], [ @@ -670,6 +744,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ hookKind: 'useActionState', returnValueKind: ValueKind.Frozen, returnValueReason: ValueReason.State, + aliasing: RenderHookAliasing(ValueReason.HookCaptured), }), ], [ @@ -682,6 +757,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ hookKind: 'useReducer', returnValueKind: ValueKind.Frozen, returnValueReason: ValueReason.ReducerState, + aliasing: RenderHookAliasing(ValueReason.ReducerState), }), ], [ @@ -715,6 +791,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ calleeEffect: Effect.Read, hookKind: 'useMemo', returnValueKind: ValueKind.Frozen, + aliasing: RenderHookAliasing(ValueReason.HookCaptured), }), ], [ @@ -726,6 +803,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ calleeEffect: Effect.Read, hookKind: 'useCallback', returnValueKind: ValueKind.Frozen, + aliasing: RenderHookAliasing(ValueReason.HookCaptured), }), ], [ @@ -739,41 +817,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ calleeEffect: Effect.Read, hookKind: 'useEffect', returnValueKind: ValueKind.Frozen, - aliasing: { - receiver: '@receiver', - params: [], - rest: '@rest', - returns: '@returns', - temporaries: ['@effect'], - effects: [ - // Freezes the function and deps - { - kind: 'Freeze', - value: '@rest', - reason: ValueReason.Effect, - }, - // Internally creates an effect object that captures the function and deps - { - kind: 'Create', - into: '@effect', - value: ValueKind.Frozen, - reason: ValueReason.KnownReturnSignature, - }, - // The effect stores the function and dependencies - { - kind: 'Capture', - from: '@rest', - into: '@effect', - }, - // Returns undefined - { - kind: 'Create', - into: '@returns', - value: ValueKind.Primitive, - reason: ValueReason.KnownReturnSignature, - }, - ], - }, + aliasing: EffectHookAliasing, }, BuiltInUseEffectHookId, ), @@ -789,6 +833,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ calleeEffect: Effect.Read, hookKind: 'useLayoutEffect', returnValueKind: ValueKind.Frozen, + aliasing: EffectHookAliasing, }, BuiltInUseLayoutEffectHookId, ), @@ -804,6 +849,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ calleeEffect: Effect.Read, hookKind: 'useInsertionEffect', returnValueKind: ValueKind.Frozen, + aliasing: EffectHookAliasing, }, BuiltInUseInsertionEffectHookId, ), @@ -817,6 +863,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ calleeEffect: Effect.Read, hookKind: 'useTransition', returnValueKind: ValueKind.Frozen, + aliasing: RenderHookAliasing(ValueReason.HookCaptured), }), ], [ @@ -829,6 +876,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ hookKind: 'useOptimistic', returnValueKind: ValueKind.Frozen, returnValueReason: ValueReason.State, + aliasing: RenderHookAliasing(ValueReason.HookCaptured), }), ], [ @@ -842,6 +890,7 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ returnType: {kind: 'Poly'}, calleeEffect: Effect.Read, returnValueKind: ValueKind.Frozen, + aliasing: RenderHookAliasing(ValueReason.HookCaptured), }, BuiltInUseOperatorId, ), 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 c92f9e5562..e828c27fde 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -190,14 +190,18 @@ function parseAliasingSignatureConfig( }; } case 'Impure': { - const place = lookup(effect.place); + const into = lookup(effect.into); return { kind: 'Impure', + into, + reason: effect.reason, + }; + } + case 'Render': { + const place = lookup(effect.place); + return { + kind: 'Render', place, - error: CompilerError.throwTodo({ - reason: 'Support impure effect declarations', - loc: GeneratedSource, - }), }; } case 'Apply': { @@ -1513,6 +1517,11 @@ export const DefaultNonmutatingHook = addHook( value: '@rest', reason: ValueReason.HookCaptured, }, + // Render the arguments + { + kind: 'Render', + place: '@rest', + }, // Returns a frozen value { kind: 'Create', 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 71fb4c43b3..ef0d7c039a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -1009,7 +1009,7 @@ export function printAliasingEffect(effect: AliasingEffect): string { return `MutateGlobal ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`; } case 'Impure': { - return `Impure ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`; + return `Impure ${printPlaceForAliasEffect(effect.into)} reason=${effect.reason}`; } case 'Render': { return `Render ${printPlaceForAliasEffect(effect.place)}`; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts index eeaaebf7a3..fe13197ca7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts @@ -185,11 +185,23 @@ export const ApplyEffectSchema: z.ZodType = z.object({ export type ImpureEffectConfig = { kind: 'Impure'; - place: string; + into: string; + reason: string; }; export const ImpureEffectSchema: z.ZodType = z.object({ kind: z.literal('Impure'), + into: LifetimeIdSchema, + reason: z.string(), +}); + +export type RenderEffectConfig = { + kind: 'Render'; + place: string; +}; + +export const RenderEffectSchema: z.ZodType = z.object({ + kind: z.literal('Render'), place: LifetimeIdSchema, }); @@ -204,7 +216,8 @@ export type AliasingEffectConfig = | ImpureEffectConfig | MutateEffectConfig | MutateTransitiveConditionallyConfig - | ApplyEffectConfig; + | ApplyEffectConfig + | RenderEffectConfig; export const AliasingEffectSchema: z.ZodType = z.union([ FreezeEffectSchema, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts index 7f30e25a5c..4fa7cdd9b4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts @@ -162,7 +162,7 @@ export type AliasingEffect = /** * Indicates a side-effect that is not safe during render */ - | {kind: 'Impure'; place: Place; error: CompilerDiagnostic} + | {kind: 'Impure'; into: Place; reason: string} /** * Indicates that a given place is accessed during render. Used to distingush * hook arguments that are known to be called immediately vs those used for @@ -222,6 +222,7 @@ export function hashEffect(effect: AliasingEffect): string { return [effect.kind, effect.value.identifier.id, effect.reason].join(':'); } case 'Impure': + return [effect.kind, effect.into.identifier.id, effect.reason].join(':'); case 'Render': { return [effect.kind, effect.place.identifier.id].join(':'); } 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 4a027b87b6..94f6f948b0 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -27,11 +27,11 @@ import { InstructionKind, InstructionValue, isArrayType, - isJsxType, isMapType, isPrimitiveType, isRefOrRefValue, isSetType, + isUseRefType, makeIdentifierId, Phi, Place, @@ -569,14 +569,21 @@ function inferBlock( terminal.effects = effects.length !== 0 ? effects : null; } } else if (terminal.kind === 'return') { + terminal.effects = [ + context.internEffect({ + kind: 'Alias', + from: terminal.value, + into: context.fn.returns, + }), + ]; if (!context.isFuctionExpression) { - terminal.effects = [ + terminal.effects.push( context.internEffect({ kind: 'Freeze', value: terminal.value, reason: ValueReason.JsxCaptured, }), - ]; + ); } } } @@ -1973,27 +1980,51 @@ function computeSignatureForInstruction( into: lvalue, }); } + if (isUseRefType(value.object.identifier)) { + effects.push({ + kind: 'Impure', + into: lvalue, + reason: 'Cannot access ref values during render', + }); + } break; } case 'PropertyStore': case 'ComputedStore': { - /** - * Add a hint about naming as "ref"/"-Ref", but only if we weren't able to infer any - * type for the object. In some cases the variable may be named like a ref, but is - * also used as a ref callback such that we infer the type as a function rather than - * a ref. - */ - const mutationReason: MutationReason | null = - value.kind === 'PropertyStore' && - value.property === 'current' && - value.object.identifier.type.kind === 'Type' - ? {kind: 'AssignCurrentProperty'} - : null; - effects.push({ - kind: 'Mutate', - value: value.object, - reason: mutationReason, - }); + if (isUseRefType(value.object.identifier)) { + const diagnostic = CompilerDiagnostic.create({ + category: ErrorCategory.Immutability, + reason: 'Cannot write to refs during render', + description: 'Cannot write to refs during render', + }).withDetails({ + kind: 'error', + loc: value.loc, + message: `Cannot write to refs during render`, + }); + effects.push({ + kind: 'MutateGlobal', + error: diagnostic, + place: value.object, + }); + } else { + /** + * Add a hint about naming as "ref"/"-Ref", but only if we weren't able to infer any + * type for the object. In some cases the variable may be named like a ref, but is + * also used as a ref callback such that we infer the type as a function rather than + * a ref. + */ + const mutationReason: MutationReason | null = + value.kind === 'PropertyStore' && + value.property === 'current' && + value.object.identifier.type.kind === 'Type' + ? {kind: 'AssignCurrentProperty'} + : null; + effects.push({ + kind: 'Mutate', + value: value.object, + reason: mutationReason, + }); + } effects.push({ kind: 'Capture', from: value.value, @@ -2155,21 +2186,13 @@ function computeSignatureForInstruction( } } for (const prop of value.props) { - if ( - prop.kind === 'JsxAttribute' && - prop.place.identifier.type.kind === 'Function' && - (isJsxType(prop.place.identifier.type.return) || - (prop.place.identifier.type.return.kind === 'Phi' && - prop.place.identifier.type.return.operands.some(operand => - isJsxType(operand), - ))) - ) { - // Any props which return jsx are assumed to be called during render - effects.push({ - kind: 'Render', - place: prop.place, - }); + if (prop.kind === 'JsxAttribute' && /^on[A-Z]/.test(prop.name)) { + continue; } + effects.push({ + kind: 'Render', + place: prop.kind === 'JsxAttribute' ? prop.place : prop.argument, + }); } } break; @@ -2423,7 +2446,7 @@ function computeEffectsForLegacySignature( lvalue: Place, receiver: Place, args: Array, - loc: SourceLocation, + _loc: SourceLocation, ): Array { const returnValueReason = signature.returnValueReason ?? ValueReason.Other; const effects: Array = []; @@ -2436,20 +2459,11 @@ function computeEffectsForLegacySignature( if (signature.impure && state.env.config.validateNoImpureFunctionsInRender) { effects.push({ kind: 'Impure', - place: receiver, - error: CompilerDiagnostic.create({ - category: ErrorCategory.Purity, - reason: 'Cannot call impure function during render', - description: - (signature.canonicalName != null - ? `\`${signature.canonicalName}\` is an impure function. ` - : '') + - 'Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)', - }).withDetails({ - kind: 'error', - loc, - message: 'Cannot call impure function', - }), + into: lvalue, + reason: + signature.canonicalName != null + ? `\`${signature.canonicalName}\` is an impure function. ` + : 'impure function', }); } if (signature.knownIncompatible != null && state.env.enableValidations) { @@ -2748,7 +2762,13 @@ function computeEffectsForSignature( } break; } - case 'Impure': + case 'Impure': { + const values = substitutions.get(effect.into.identifier.id) ?? []; + for (const value of values) { + effects.push({kind: effect.kind, into: value, reason: effect.reason}); + } + break; + } case 'MutateFrozen': case 'MutateGlobal': { const values = substitutions.get(effect.place.identifier.id) ?? []; 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 43148dc4c6..ec854ce6e5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -5,7 +5,12 @@ * LICENSE file in the root directory of this source tree. */ -import {CompilerError, SourceLocation} from '..'; +import { + CompilerDiagnostic, + CompilerError, + ErrorCategory, + SourceLocation, +} from '..'; import { BlockId, Effect, @@ -19,6 +24,7 @@ import { ValueReason, Place, isPrimitiveType, + isUseRefType, } from '../HIR/HIR'; import { eachInstructionLValue, @@ -28,6 +34,9 @@ import { import {assertExhaustive, getOrInsertWith} from '../Utils/utils'; import {Err, Ok, Result} from '../Utils/Result'; import {AliasingEffect, MutationReason} from './AliasingEffects'; +import {printIdentifier, printType} from '../HIR/PrintHIR'; + +const DEBUG = false; /** * This pass builds an abstract model of the heap and interprets the effects of the @@ -104,6 +113,10 @@ export function inferMutationAliasingRanges( reason: MutationReason | null; }> = []; const renders: Array<{index: number; place: Place}> = []; + const impure: Array<{ + index: number; + effect: Extract; + }> = []; let index = 0; @@ -197,14 +210,18 @@ export function inferMutationAliasingRanges( }); } else if ( effect.kind === 'MutateFrozen' || - effect.kind === 'MutateGlobal' || - effect.kind === 'Impure' + effect.kind === 'MutateGlobal' ) { errors.pushDiagnostic(effect.error); functionEffects.push(effect); } else if (effect.kind === 'Render') { renders.push({index: index++, place: effect.place}); functionEffects.push(effect); + } else if (effect.kind === 'Impure') { + impure.push({ + index: index++, + effect, + }); } } } @@ -214,10 +231,6 @@ export function inferMutationAliasingRanges( state.assign(index, from, into); } } - if (block.terminal.kind === 'return') { - state.assign(index++, block.terminal.value, fn.returns); - } - if ( (block.terminal.kind === 'maybe-throw' || block.terminal.kind === 'return') && @@ -243,7 +256,20 @@ export function inferMutationAliasingRanges( } } + for (const {index, effect} of impure) { + if (DEBUG) { + console.log( + `[${index}] impure ${printIdentifier(effect.into.identifier)}`, + ); + } + state.impure(index, effect); + } for (const mutation of mutations) { + if (DEBUG) { + console.log( + `[${mutation.index}] mutate ${printIdentifier(mutation.place.identifier)}`, + ); + } state.mutate( mutation.index, mutation.place.identifier, @@ -255,7 +281,15 @@ export function inferMutationAliasingRanges( errors, ); } + if (DEBUG) { + console.log(state.debug()); + } for (const render of renders) { + if (DEBUG) { + console.log( + `[${render.index}] render ${printIdentifier(render.place.identifier)}`, + ); + } state.render(render.index, render.place.identifier, errors); } for (const param of [...fn.context, ...fn.params]) { @@ -515,6 +549,17 @@ export function inferMutationAliasingRanges( const ignoredErrors = new CompilerError(); for (const param of [...fn.params, ...fn.context, fn.returns]) { const place = param.kind === 'Identifier' ? param : param.place; + const node = state.nodes.get(place.identifier); + if (node != null && node.impure != null) { + if (node.impure.kind === 'Impure') { + functionEffects.push({...node.impure, into: place}); + } else { + const source = state.nodes.get(node.impure.node); + if (source != null && source.impure?.kind === 'Impure') { + functionEffects.push({...source.impure, into: place}); + } + } + } tracked.push(place); } for (const into of tracked) { @@ -577,7 +622,6 @@ export function inferMutationAliasingRanges( function appendFunctionErrors(errors: CompilerError, fn: HIRFunction): void { for (const effect of fn.aliasingEffects ?? []) { switch (effect.kind) { - case 'Impure': case 'MutateFrozen': case 'MutateGlobal': { errors.pushDiagnostic(effect.error); @@ -612,10 +656,82 @@ type Node = { | {kind: 'Object'} | {kind: 'Phi'} | {kind: 'Function'; function: HIRFunction}; + impure: + | Extract + | {kind: 'Indirect'; node: Identifier} + | null; }; + +function _printNode(node: Node): string { + const out: Array = []; + debugNode(out, node); + return out.join('\n'); +} +function debugNode(out: Array, node: Node): void { + out.push( + printIdentifier(node.id) + + printType(node.id.type) + + ` lastMutated=[${node.lastMutated}]`, + ); + if (node.transitive != null) { + out.push(` transitive=${node.transitive.kind}`); + } + if (node.local != null) { + out.push(` local=${node.local.kind}`); + } + if (node.mutationReason != null) { + out.push(` mutationReason=${node.mutationReason?.kind}`); + } + if (node.impure != null) { + out.push( + ` impure=${node.impure.kind === 'Impure' ? node.impure.reason : 'source=$' + node.impure.node.id}`, + ); + } + const edges: Array<{ + index: number; + direction: '<=' | '=>'; + kind: string; + id: Identifier; + }> = []; + for (const [alias, index] of node.createdFrom) { + edges.push({index, direction: '<=', kind: 'createFrom', id: alias}); + } + for (const [alias, index] of node.aliases) { + edges.push({index, direction: '<=', kind: 'alias', id: alias}); + } + for (const [alias, index] of node.maybeAliases) { + edges.push({index, direction: '<=', kind: 'alias?', id: alias}); + } + for (const [alias, index] of node.captures) { + edges.push({index, direction: '<=', kind: 'capture', id: alias}); + } + for (const edge of node.edges) { + edges.push({ + index: edge.index, + direction: '=>', + kind: edge.kind, + id: edge.node, + }); + } + edges.sort((a, b) => a.index - b.index); + for (const edge of edges) { + out.push( + ` [${edge.index}] ${edge.direction} ${edge.kind} ${printIdentifier(edge.id)}`, + ); + } +} + class AliasingState { nodes: Map = new Map(); + debug(): string { + const items: Array = []; + for (const [_id, node] of this.nodes) { + debugNode(items, node); + } + return items.join('\n'); + } + create(place: Place, value: Node['value']): void { this.nodes.set(place.identifier, { id: place.identifier, @@ -629,6 +745,7 @@ class AliasingState { lastMutated: 0, mutationReason: null, value, + impure: null, }); } @@ -681,6 +798,34 @@ class AliasingState { } } + impure( + index: number, + effect: Extract, + ): void { + const seen = new Set(); + const queue: Array = [effect.into.identifier]; + while (queue.length !== 0) { + const current = queue.pop()!; + if (seen.has(current)) { + continue; + } + seen.add(current); + const node = this.nodes.get(current); + if (node == null || node.impure != null || isUseRefType(node.id)) { + continue; + } + node.impure = + node.id.id === effect.into.identifier.id + ? effect + : {kind: 'Indirect', node: effect.into.identifier}; + for (const edge of node.edges) { + if (index < edge.index) { + queue.push(edge.node); + } + } + } + } + render(index: number, start: Identifier, errors: CompilerError): void { const seen = new Set(); const queue: Array = [start]; @@ -691,11 +836,49 @@ class AliasingState { } seen.add(current); const node = this.nodes.get(current); - if (node == null || node.transitive != null || node.local != null) { + if ( + node == null || + node.transitive != null || + node.local != null || + isUseRefType(node.id) + ) { + if (DEBUG) { + console.log(` render ${printIdentifier(current)}: skip mutated/ref`); + } continue; } if (node.value.kind === 'Function') { - appendFunctionErrors(errors, node.value.function); + const returns = node.value.function.returns; + if ( + isJsxType(returns.identifier.type) || + (returns.identifier.type.kind === 'Phi' && + returns.identifier.type.operands.some(operand => + isJsxType(operand), + )) + ) { + appendFunctionErrors(errors, node.value.function); + } + if (DEBUG) { + console.log(` render ${printIdentifier(current)}: skip function`); + } + continue; + } + if (node.impure != null && node.impure.kind === 'Impure') { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.Purity, + reason: 'Cannot call impure function during render', + description: + node.impure.reason + + 'Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)', + suggestions: null, + }).withDetails({ + kind: 'error', + loc: node.impure.into.loc, + message: 'Cannot call impure function', + }), + ); + node.impure = null; } for (const [alias, when] of node.createdFrom) { if (when >= index) { @@ -709,6 +892,12 @@ class AliasingState { } queue.push(alias); } + for (const [alias, when] of node.maybeAliases) { + if (when >= index) { + continue; + } + queue.push(alias); + } for (const [capture, when] of node.captures) { if (when >= index) { continue; @@ -761,6 +950,23 @@ class AliasingState { ) { appendFunctionErrors(errors, node.value.function); } + if (node.impure != null && node.impure.kind === 'Impure') { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.Purity, + reason: 'Cannot call impure function during render', + description: + node.impure.reason + + 'Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)', + suggestions: null, + }).withDetails({ + kind: 'error', + loc: node.impure.into.loc, + message: 'Cannot call impure function', + }), + ); + node.impure = null; + } if (transitive) { if (node.transitive == null || node.transitive.kind < kind) { node.transitive = {kind, loc}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts deleted file mode 100644 index ca0612d80c..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureFunctionsInRender.ts +++ /dev/null @@ -1,59 +0,0 @@ -/** - * 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 {CompilerDiagnostic, CompilerError} from '..'; -import {ErrorCategory} from '../CompilerError'; -import {HIRFunction} from '../HIR'; -import {getFunctionCallSignature} from '../Inference/InferMutationAliasingEffects'; -import {Result} from '../Utils/Result'; - -/** - * Checks that known-impure functions are not called during render. Examples of invalid functions to - * call during render are `Math.random()` and `Date.now()`. Users may extend this set of - * impure functions via a module type provider and specifying functions with `impure: true`. - * - * TODO: add best-effort analysis of functions which are called during render. We have variations of - * this in several of our validation passes and should unify those analyses into a reusable helper - * and use it here. - */ -export function validateNoImpureFunctionsInRender( - fn: HIRFunction, -): Result { - const errors = new CompilerError(); - for (const [, block] of fn.body.blocks) { - for (const instr of block.instructions) { - const value = instr.value; - if (value.kind === 'MethodCall' || value.kind == 'CallExpression') { - const callee = - value.kind === 'MethodCall' ? value.property : value.callee; - const signature = getFunctionCallSignature( - fn.env, - callee.identifier.type, - ); - if (signature != null && signature.impure === true) { - errors.pushDiagnostic( - CompilerDiagnostic.create({ - category: ErrorCategory.Purity, - reason: 'Cannot call impure function during render', - description: - (signature.canonicalName != null - ? `\`${signature.canonicalName}\` is an impure function. ` - : '') + - 'Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)', - suggestions: null, - }).withDetails({ - kind: 'error', - loc: callee.loc, - message: 'Cannot call impure function', - }), - ); - } - } - } - } - return errors.asResult(); -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.expect.md index b7b707b3e7..f0472f7bca 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.expect.md @@ -5,6 +5,7 @@ function Component() { const Foo = () => { someGlobal = true; + return
; }; return ; } @@ -26,9 +27,9 @@ error.assign-global-in-component-tag-function.ts:3:4 2 | const Foo = () => { > 3 | someGlobal = true; | ^^^^^^^^^^ `someGlobal` cannot be reassigned - 4 | }; - 5 | return ; - 6 | } + 4 | return
; + 5 | }; + 6 | return ; ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.js index 2982fdf708..eaf1eceac2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.js @@ -1,6 +1,7 @@ function Component() { const Foo = () => { someGlobal = true; + return
; }; return ; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.expect.md index 1f5ac0c83d..d57997b8fa 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.expect.md @@ -5,6 +5,7 @@ function Component() { const foo = () => { someGlobal = true; + return
; }; // Children are generally access/called during render, so // modifying a global in a children function is almost @@ -29,9 +30,9 @@ error.assign-global-in-jsx-children.ts:3:4 2 | const foo = () => { > 3 | someGlobal = true; | ^^^^^^^^^^ `someGlobal` cannot be reassigned - 4 | }; - 5 | // Children are generally access/called during render, so - 6 | // modifying a global in a children function is almost + 4 | return
; + 5 | }; + 6 | // Children are generally access/called during render, so ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.js index 82554e8ac4..1def89dd7d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.js @@ -1,6 +1,7 @@ function Component() { const foo = () => { someGlobal = true; + return
; }; // Children are generally access/called during render, so // modifying a global in a children function is almost diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.expect.md new file mode 100644 index 0000000000..af8a30ce36 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.expect.md @@ -0,0 +1,38 @@ + +## Input + +```javascript +// @validateNoImpureFunctionsInRender + +import {identity, makeArray} from 'shared-runtime'; + +function Component() { + const getDate = () => Date.now(); + const array = makeArray(getDate()); + const hasDate = identity(array); + return ; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: Cannot call impure function during render + +`Date.now` is an impure function. Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). + +error.invalid-impure-functions-in-render-indirect.ts:7:26 + 5 | function Component() { + 6 | const getDate = () => Date.now(); +> 7 | const array = makeArray(getDate()); + | ^^^^^^^^^ Cannot call impure function + 8 | const hasDate = identity(array); + 9 | return ; + 10 | } +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.js new file mode 100644 index 0000000000..4cf0e46d9d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect.js @@ -0,0 +1,10 @@ +// @validateNoImpureFunctionsInRender + +import {identity, makeArray} from 'shared-runtime'; + +function Component() { + const getDate = () => Date.now(); + const array = makeArray(getDate()); + const hasDate = identity(array); + return ; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-use-impure-value-in-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-use-impure-value-in-ref.expect.md new file mode 100644 index 0000000000..175246f5af --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-use-impure-value-in-ref.expect.md @@ -0,0 +1,53 @@ + +## Input + +```javascript +// @validateNoImpureFunctionsInRender +import {useIdentity} from 'shared-runtime'; + +function Component() { + const f = () => Math.random(); + const ref = useRef(f()); + return
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoImpureFunctionsInRender +import { useIdentity } from "shared-runtime"; + +function Component() { + const $ = _c(3); + let t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const f = _temp; + t0 = useRef; + t1 = f(); + $[0] = t0; + $[1] = t1; + } else { + t0 = $[0]; + t1 = $[1]; + } + const ref = t0(t1); + let t2; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 =
; + $[2] = t2; + } else { + t2 = $[2]; + } + return t2; +} +function _temp() { + return Math.random(); +} + +``` + +### 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/valid-use-impure-value-in-ref.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-use-impure-value-in-ref.js new file mode 100644 index 0000000000..064a25c9d1 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/valid-use-impure-value-in-ref.js @@ -0,0 +1,8 @@ +// @validateNoImpureFunctionsInRender +import {useIdentity} from 'shared-runtime'; + +function Component() { + const f = () => Math.random(); + const ref = useRef(f()); + return
; +}