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 1c96e65c71..590aba2fdc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -104,6 +104,7 @@ import {validateLocalsNotReassignedAfterRender} from '../Validation/ValidateLoca import {outlineFunctions} from '../Optimization/OutlineFunctions'; import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes'; import {lowerContextAccess} from '../Optimization/LowerContextAccess'; +import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects'; export type CompilerPipelineValue = | {kind: 'ast'; name: string; value: CodegenFunction} @@ -244,6 +245,10 @@ function* runWithEnvironment( validateNoSetStateInRender(hir); } + if (env.config.validateNoSetStateInPassiveEffects) { + validateNoSetStateInPassiveEffects(hir); + } + inferReactivePlaces(hir); yield log({kind: 'hir', name: 'InferReactivePlaces', value: hir}); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index b9bddff6a5..20fac9d610 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -428,19 +428,11 @@ function lowerStatement( loc: id.parentPath.node.loc ?? GeneratedSource, }); continue; - } else if (!binding.path.get('id').isIdentifier()) { - builder.errors.push({ - severity: ErrorSeverity.Todo, - reason: 'Unsupported variable declaration type for hoisting', - description: `variable "${ - binding.identifier.name - }" declared with ${binding.path.get('id').type}`, - suggestions: null, - loc: id.parentPath.node.loc ?? GeneratedSource, - }); - continue; - } else if (binding.kind !== 'const' && binding.kind !== 'var') { - // Avoid double errors on var declarations, which we do not plan to support anyways + } else if ( + binding.kind !== 'const' && + binding.kind !== 'var' && + binding.kind !== 'let' + ) { builder.errors.push({ severity: ErrorSeverity.Todo, reason: 'Handle non-const declarations for hoisting', @@ -463,10 +455,17 @@ function lowerStatement( reactive: false, loc: id.node.loc ?? GeneratedSource, }; + const kind = + // Avoid double errors on var declarations, which we do not plan to support anyways + binding.kind === 'const' || binding.kind === 'var' + ? InstructionKind.HoistedConst + : binding.kind === 'let' + ? InstructionKind.HoistedLet + : assertExhaustive(binding.kind, 'Unexpected binding kind'); lowerValueToTemporary(builder, { kind: 'DeclareContext', lvalue: { - kind: InstructionKind.HoistedConst, + kind, place, }, loc: id.node.loc ?? GeneratedSource, @@ -2386,6 +2385,57 @@ function lowerExpression( case 'UpdateExpression': { let expr = exprPath as NodePath; const argument = expr.get('argument'); + if (argument.isMemberExpression()) { + const binaryOperator = expr.node.operator === '++' ? '+' : '-'; + const leftExpr = argument as NodePath; + const {object, property, value} = lowerMemberExpression( + builder, + leftExpr, + ); + + // Store the previous value to a temporary + const previousValuePlace = lowerValueToTemporary(builder, value); + // Store the new value to a temporary + const updatedValue = lowerValueToTemporary(builder, { + kind: 'BinaryExpression', + operator: binaryOperator, + left: {...previousValuePlace}, + right: lowerValueToTemporary(builder, { + kind: 'Primitive', + value: 1, + loc: GeneratedSource, + }), + loc: leftExpr.node.loc ?? GeneratedSource, + }); + + // Save the result back to the property + let newValuePlace; + if (typeof property === 'string') { + newValuePlace = lowerValueToTemporary(builder, { + kind: 'PropertyStore', + object: {...object}, + property, + value: {...updatedValue}, + loc: leftExpr.node.loc ?? GeneratedSource, + }); + } else { + newValuePlace = lowerValueToTemporary(builder, { + kind: 'ComputedStore', + object: {...object}, + property: {...property}, + value: {...updatedValue}, + loc: leftExpr.node.loc ?? GeneratedSource, + }); + } + + return { + kind: 'LoadLocal', + place: expr.node.prefix + ? {...newValuePlace} + : {...previousValuePlace}, + loc: exprLoc, + }; + } if (!argument.isIdentifier()) { builder.errors.push({ reason: `(BuildHIR::lowerExpression) Handle UpdateExpression with ${argument.type} argument`, @@ -2837,6 +2887,21 @@ function isReorderableExpression( allowLocalIdentifiers, ); } + case 'LogicalExpression': { + const logical = expr as NodePath; + return ( + isReorderableExpression( + builder, + logical.get('left'), + allowLocalIdentifiers, + ) && + isReorderableExpression( + builder, + logical.get('right'), + allowLocalIdentifiers, + ) + ); + } case 'ConditionalExpression': { const conditional = expr as NodePath; return ( 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 7a83f7e3a0..ca03b8a7b1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -223,7 +223,7 @@ const EnvironmentConfigSchema = z.object({ validateHooksUsage: z.boolean().default(true), // Validate that ref values (`ref.current`) are not accessed during render. - validateRefAccessDuringRender: z.boolean().default(false), + validateRefAccessDuringRender: z.boolean().default(true), /* * Validates that setState is not unconditionally called during render, as it can lead to @@ -231,6 +231,12 @@ const EnvironmentConfigSchema = z.object({ */ validateNoSetStateInRender: z.boolean().default(true), + /** + * Validates that setState is not called directly within a passive effect (useEffect). + * Scheduling a setState (with an event listener, subscription, etc) is valid. + */ + validateNoSetStateInPassiveEffects: z.boolean().default(false), + /** * Validates that the dependencies of all effect hooks are memoized. This helps ensure * that Forget does not introduce infinite renders caused by a dependency changing, 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 884372b986..e9066f85b8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts @@ -18,6 +18,7 @@ import { BuiltInUseReducerId, BuiltInUseRefId, BuiltInUseStateId, + BuiltInUseTransitionId, ShapeRegistry, addFunction, addHook, @@ -425,6 +426,17 @@ const REACT_APIS: Array<[string, BuiltInType]> = [ BuiltInUseInsertionEffectHookId, ), ], + [ + 'useTransition', + addHook(DEFAULT_SHAPES, { + positionalParams: [], + restParam: null, + returnType: {kind: 'Object', shapeId: BuiltInUseTransitionId}, + calleeEffect: Effect.Read, + hookKind: 'useTransition', + returnValueKind: ValueKind.Frozen, + }), + ], [ 'use', addFunction( diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index f249329e0c..0810130102 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -741,6 +741,9 @@ export enum InstructionKind { // hoisted const declarations HoistedConst = 'HoistedConst', + + // hoisted const declarations + HoistedLet = 'HoistedLet', } function _staticInvariantInstructionValueHasLocation( @@ -858,7 +861,10 @@ export type InstructionValue = | { kind: 'DeclareContext'; lvalue: { - kind: InstructionKind.Let | InstructionKind.HoistedConst; + kind: + | InstructionKind.Let + | InstructionKind.HoistedConst + | InstructionKind.HoistedLet; place: Place; }; loc: SourceLocation; @@ -1585,6 +1591,10 @@ export function isUseStateType(id: Identifier): boolean { return id.type.kind === 'Object' && id.type.shapeId === 'BuiltInUseState'; } +export function isRefOrRefValue(id: Identifier): boolean { + return isUseRefType(id) || isRefValueType(id); +} + export function isSetStateType(id: Identifier): boolean { return id.type.kind === 'Function' && id.type.shapeId === 'BuiltInSetState'; } @@ -1595,6 +1605,12 @@ export function isUseActionStateType(id: Identifier): boolean { ); } +export function isStartTransitionType(id: Identifier): boolean { + return ( + id.type.kind === 'Function' && id.type.shapeId === 'BuiltInStartTransition' + ); +} + export function isSetActionStateType(id: Identifier): boolean { return ( id.type.kind === 'Function' && id.type.shapeId === 'BuiltInSetActionState' @@ -1610,7 +1626,13 @@ export function isDispatcherType(id: Identifier): boolean { } export function isStableType(id: Identifier): boolean { - return isSetStateType(id) || isSetActionStateType(id) || isDispatcherType(id); + return ( + isSetStateType(id) || + isSetActionStateType(id) || + isDispatcherType(id) || + isUseRefType(id) || + isStartTransitionType(id) + ); } export function isUseEffectHookType(id: Identifier): boolean { 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 3d377dba59..9554878578 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -126,6 +126,7 @@ export type HookKind = | 'useInsertionEffect' | 'useMemo' | 'useCallback' + | 'useTransition' | 'Custom'; /* @@ -209,6 +210,8 @@ export const BuiltInUseOperatorId = 'BuiltInUseOperator'; export const BuiltInUseReducerId = 'BuiltInUseReducer'; export const BuiltInDispatchId = 'BuiltInDispatch'; export const BuiltInUseContextHookId = 'BuiltInUseContextHook'; +export const BuiltInUseTransitionId = 'BuiltInUseTransition'; +export const BuiltInStartTransitionId = 'BuiltInStartTransition'; // ShapeRegistry with default definitions for built-ins. export const BUILTIN_SHAPES: ShapeRegistry = new Map(); @@ -444,6 +447,25 @@ addObject(BUILTIN_SHAPES, BuiltInUseStateId, [ ], ]); +addObject(BUILTIN_SHAPES, BuiltInUseTransitionId, [ + ['0', {kind: 'Primitive'}], + [ + '1', + addFunction( + BUILTIN_SHAPES, + [], + { + positionalParams: [], + restParam: null, + returnType: PRIMITIVE_TYPE, + calleeEffect: Effect.Read, + returnValueKind: ValueKind.Primitive, + }, + BuiltInStartTransitionId, + ), + ], +]); + addObject(BUILTIN_SHAPES, BuiltInUseActionStateId, [ ['0', {kind: 'Poly'}], [ 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 fd17822af0..59f0677873 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -760,6 +760,9 @@ export function printLValue(lval: LValue): string { case InstructionKind.HoistedConst: { return `HoistedConst ${lvalue}$`; } + case InstructionKind.HoistedLet: { + return `HoistedLet ${lvalue}$`; + } default: { assertExhaustive(lval.kind, `Unexpected lvalue kind \`${lval.kind}\``); } 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 b18e19606c..fbb24ea492 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -14,8 +14,7 @@ import { LoweredFunction, Place, ReactiveScopeDependency, - isRefValueType, - isUseRefType, + isRefOrRefValue, makeInstructionId, } from '../HIR'; import {deadCodeElimination} from '../Optimization'; @@ -139,7 +138,7 @@ function infer( name = dep.identifier.name; } - if (isUseRefType(dep.identifier) || isRefValueType(dep.identifier)) { + if (isRefOrRefValue(dep.identifier)) { /* * TODO: this is a hack to ensure we treat functions which reference refs * as having a capture and therefore being considered mutable. this ensures diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableLifetimes.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableLifetimes.ts index 2ce1aebbf8..459baf4e28 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableLifetimes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableLifetimes.ts @@ -11,6 +11,7 @@ import { Identifier, InstructionId, InstructionKind, + isRefOrRefValue, makeInstructionId, Place, } from '../HIR/HIR'; @@ -66,7 +67,9 @@ import {assertExhaustive} from '../Utils/utils'; */ function infer(place: Place, instrId: InstructionId): void { - place.identifier.mutableRange.end = makeInstructionId(instrId + 1); + if (!isRefOrRefValue(place.identifier)) { + place.identifier.mutableRange.end = makeInstructionId(instrId + 1); + } } function inferPlace( @@ -171,7 +174,10 @@ export function inferMutableLifetimes( const declaration = contextVariableDeclarationInstructions.get( instr.value.lvalue.place.identifier, ); - if (declaration != null) { + if ( + declaration != null && + !isRefOrRefValue(instr.value.lvalue.place.identifier) + ) { const range = instr.value.lvalue.place.identifier.mutableRange; if (range.start === 0) { range.start = declaration; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRangesForAlias.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRangesForAlias.ts index 975acf6fbf..a7e8b5c1f7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRangesForAlias.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableRangesForAlias.ts @@ -5,7 +5,12 @@ * LICENSE file in the root directory of this source tree. */ -import {HIRFunction, Identifier, InstructionId} from '../HIR/HIR'; +import { + HIRFunction, + Identifier, + InstructionId, + isRefOrRefValue, +} from '../HIR/HIR'; import DisjointSet from '../Utils/DisjointSet'; export function inferMutableRangesForAlias( @@ -19,7 +24,8 @@ export function inferMutableRangesForAlias( * mutated. */ const mutatingIdentifiers = [...aliasSet].filter( - id => id.mutableRange.end - id.mutableRange.start > 1, + id => + id.mutableRange.end - id.mutableRange.start > 1 && !isRefOrRefValue(id), ); if (mutatingIdentifiers.length > 0) { @@ -36,7 +42,10 @@ export function inferMutableRangesForAlias( * last mutation. */ for (const alias of aliasSet) { - if (alias.mutableRange.end < lastMutatingInstructionId) { + if ( + alias.mutableRange.end < lastMutatingInstructionId && + !isRefOrRefValue(alias) + ) { alias.mutableRange.end = lastMutatingInstructionId as InstructionId; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index 356bc8af08..4cce942c18 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -30,8 +30,7 @@ import { isArrayType, isMutableEffect, isObjectType, - isRefValueType, - isUseRefType, + isRefOrRefValue, } from '../HIR/HIR'; import {FunctionSignature} from '../HIR/ObjectShape'; import { @@ -523,10 +522,7 @@ class InferenceState { break; } case Effect.Mutate: { - if ( - isRefValueType(place.identifier) || - isUseRefType(place.identifier) - ) { + if (isRefOrRefValue(place.identifier)) { // no-op: refs are validate via ValidateNoRefAccessInRender } else if (valueKind.kind === ValueKind.Context) { functionEffect = { @@ -567,10 +563,7 @@ class InferenceState { break; } case Effect.Store: { - if ( - isRefValueType(place.identifier) || - isUseRefType(place.identifier) - ) { + if (isRefOrRefValue(place.identifier)) { // no-op: refs are validate via ValidateNoRefAccessInRender } else if (valueKind.kind === ValueKind.Context) { functionEffect = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index bd3e97f23a..624a4b604d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -994,6 +994,13 @@ function codegenTerminal( loc: iterableItem.loc, suggestions: null, }); + case InstructionKind.HoistedLet: + CompilerError.invariant(false, { + reason: 'Unexpected HoistedLet variable in for..in collection', + description: null, + loc: iterableItem.loc, + suggestions: null, + }); default: assertExhaustive( iterableItem.value.lvalue.kind, @@ -1089,6 +1096,13 @@ function codegenTerminal( loc: iterableItem.loc, suggestions: null, }); + case InstructionKind.HoistedLet: + CompilerError.invariant(false, { + reason: 'Unexpected HoistedLet variable in for..of collection', + description: null, + loc: iterableItem.loc, + suggestions: null, + }); default: assertExhaustive( iterableItem.value.lvalue.kind, @@ -1289,6 +1303,15 @@ function codegenInstructionNullable( case InstructionKind.Catch: { return t.emptyStatement(); } + case InstructionKind.HoistedLet: { + CompilerError.invariant(false, { + reason: + 'Expected HoistedLet to have been pruned in PruneHoistedContexts', + description: null, + loc: instr.loc, + suggestions: null, + }); + } case InstructionKind.HoistedConst: { CompilerError.invariant(false, { reason: diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts index 8608b32298..1df211afc3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts @@ -23,11 +23,11 @@ import { * original instruction kind. */ export function pruneHoistedContexts(fn: ReactiveFunction): void { - const hoistedIdentifiers: HoistedIdentifiers = new Set(); + const hoistedIdentifiers: HoistedIdentifiers = new Map(); visitReactiveFunction(fn, new Visitor(), hoistedIdentifiers); } -type HoistedIdentifiers = Set; +type HoistedIdentifiers = Map; class Visitor extends ReactiveFunctionTransform { override transformInstruction( @@ -39,7 +39,21 @@ class Visitor extends ReactiveFunctionTransform { instruction.value.kind === 'DeclareContext' && instruction.value.lvalue.kind === 'HoistedConst' ) { - state.add(instruction.value.lvalue.place.identifier.declarationId); + state.set( + instruction.value.lvalue.place.identifier.declarationId, + InstructionKind.Const, + ); + return {kind: 'remove'}; + } + + if ( + instruction.value.kind === 'DeclareContext' && + instruction.value.lvalue.kind === 'HoistedLet' + ) { + state.set( + instruction.value.lvalue.place.identifier.declarationId, + InstructionKind.Let, + ); return {kind: 'remove'}; } @@ -47,6 +61,9 @@ class Visitor extends ReactiveFunctionTransform { instruction.value.kind === 'StoreContext' && state.has(instruction.value.lvalue.place.identifier.declarationId) ) { + const kind = state.get( + instruction.value.lvalue.place.identifier.declarationId, + )!; return { kind: 'replace', value: { @@ -57,7 +74,7 @@ class Visitor extends ReactiveFunctionTransform { ...instruction.value, lvalue: { ...instruction.value.lvalue, - kind: InstructionKind.Const, + kind, }, type: null, kind: 'StoreLocal', diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts index 2dab01f86e..0ea1814349 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts @@ -130,6 +130,16 @@ function getContextReassignment( */ contextVariables.add(value.lvalue.place.identifier.id); } + const reassignment = reassigningFunctions.get( + value.value.identifier.id, + ); + if (reassignment !== undefined) { + reassigningFunctions.set( + value.lvalue.place.identifier.id, + reassignment, + ); + reassigningFunctions.set(lvalue.identifier.id, reassignment); + } break; } default: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts index b4fb2a618a..df6241a73f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts @@ -11,10 +11,10 @@ import { IdentifierId, Place, SourceLocation, + isRefOrRefValue, isRefValueType, isUseRefType, } from '../HIR'; -import {printPlace} from '../HIR/PrintHIR'; import { eachInstructionValueOperand, eachTerminalOperand, @@ -52,34 +52,36 @@ function validateNoRefAccessInRenderImpl( refAccessingFunctions: Set, ): Result { const errors = new CompilerError(); + const lookupLocations: Map = new Map(); for (const [, block] of fn.body.blocks) { for (const instr of block.instructions) { switch (instr.value.kind) { case 'JsxExpression': case 'JsxFragment': { for (const operand of eachInstructionValueOperand(instr.value)) { - if (isRefValueType(operand.identifier)) { - errors.push({ - severity: ErrorSeverity.InvalidReact, - reason: - 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: operand.loc, - description: `Cannot access ref value at ${printPlace( - operand, - )}`, - suggestions: null, - }); - } + validateNoDirectRefValueAccess(errors, operand, lookupLocations); } break; } case 'PropertyLoad': { + if ( + isRefValueType(instr.lvalue.identifier) && + instr.value.property === 'current' + ) { + lookupLocations.set(instr.lvalue.identifier.id, instr.loc); + } break; } case 'LoadLocal': { if (refAccessingFunctions.has(instr.value.place.identifier.id)) { refAccessingFunctions.add(instr.lvalue.identifier.id); } + if (isRefValueType(instr.lvalue.identifier)) { + const loc = lookupLocations.get(instr.value.place.identifier.id); + if (loc !== undefined) { + lookupLocations.set(instr.lvalue.identifier.id, loc); + } + } break; } case 'StoreLocal': { @@ -87,6 +89,13 @@ function validateNoRefAccessInRenderImpl( refAccessingFunctions.add(instr.value.lvalue.place.identifier.id); refAccessingFunctions.add(instr.lvalue.identifier.id); } + if (isRefValueType(instr.value.lvalue.place.identifier)) { + const loc = lookupLocations.get(instr.value.value.identifier.id); + if (loc !== undefined) { + lookupLocations.set(instr.value.lvalue.place.identifier.id, loc); + lookupLocations.set(instr.lvalue.identifier.id, loc); + } + } break; } case 'ObjectMethod': @@ -139,7 +148,11 @@ function validateNoRefAccessInRenderImpl( reason: 'This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef)', loc: callee.loc, - description: `Function ${printPlace(callee)} accesses a ref`, + description: + callee.identifier.name !== null && + callee.identifier.name.kind === 'named' + ? `Function \`${callee.identifier.name.value}\` accesses a ref` + : null, suggestions: null, }); } @@ -148,7 +161,7 @@ function validateNoRefAccessInRenderImpl( errors, refAccessingFunctions, operand, - operand.loc, + lookupLocations.get(operand.identifier.id) ?? operand.loc, ); } } @@ -161,7 +174,7 @@ function validateNoRefAccessInRenderImpl( errors, refAccessingFunctions, operand, - operand.loc, + lookupLocations.get(operand.identifier.id) ?? operand.loc, ); } break; @@ -174,26 +187,49 @@ function validateNoRefAccessInRenderImpl( errors, refAccessingFunctions, instr.value.object, - instr.loc, + lookupLocations.get(instr.value.object.identifier.id) ?? instr.loc, ); for (const operand of eachInstructionValueOperand(instr.value)) { if (operand === instr.value.object) { continue; } - validateNoRefValueAccess(errors, refAccessingFunctions, operand); + validateNoRefValueAccess( + errors, + refAccessingFunctions, + lookupLocations, + operand, + ); } break; } + case 'StartMemoize': + case 'FinishMemoize': + break; default: { for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefValueAccess(errors, refAccessingFunctions, operand); + validateNoRefValueAccess( + errors, + refAccessingFunctions, + lookupLocations, + operand, + ); } break; } } } for (const operand of eachTerminalOperand(block.terminal)) { - validateNoRefValueAccess(errors, refAccessingFunctions, operand); + if (block.terminal.kind !== 'return') { + validateNoRefValueAccess( + errors, + refAccessingFunctions, + lookupLocations, + operand, + ); + } else { + // Allow functions containing refs to be returned, but not direct ref values + validateNoDirectRefValueAccess(errors, operand, lookupLocations); + } } } @@ -207,6 +243,7 @@ function validateNoRefAccessInRenderImpl( function validateNoRefValueAccess( errors: CompilerError, refAccessingFunctions: Set, + lookupLocations: Map, operand: Place, ): void { if ( @@ -217,8 +254,12 @@ function validateNoRefValueAccess( severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: operand.loc, - description: `Cannot access ref value at ${printPlace(operand)}`, + loc: lookupLocations.get(operand.identifier.id) ?? operand.loc, + description: + operand.identifier.name !== null && + operand.identifier.name.kind === 'named' + ? `Cannot access ref value \`${operand.identifier.name.value}\`` + : null, suggestions: null, }); } @@ -231,8 +272,7 @@ function validateNoRefAccess( loc: SourceLocation, ): void { if ( - isRefValueType(operand.identifier) || - isUseRefType(operand.identifier) || + isRefOrRefValue(operand.identifier) || refAccessingFunctions.has(operand.identifier.id) ) { errors.push({ @@ -249,3 +289,24 @@ function validateNoRefAccess( }); } } + +function validateNoDirectRefValueAccess( + errors: CompilerError, + operand: Place, + lookupLocations: Map, +): void { + if (isRefValueType(operand.identifier)) { + errors.push({ + severity: ErrorSeverity.InvalidReact, + reason: + 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', + loc: lookupLocations.get(operand.identifier.id) ?? operand.loc, + description: + operand.identifier.name !== null && + operand.identifier.name.kind === 'named' + ? `Cannot access ref value \`${operand.identifier.name.value}\`` + : null, + suggestions: null, + }); + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts new file mode 100644 index 0000000000..2c6e7d8ac6 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInPassiveEffects.ts @@ -0,0 +1,152 @@ +/** + * 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 {CompilerError, ErrorSeverity} from '../CompilerError'; +import { + HIRFunction, + IdentifierId, + isSetStateType, + isUseEffectHookType, + Place, +} from '../HIR'; +import {eachInstructionValueOperand} from '../HIR/visitors'; + +/** + * Validates against calling setState in the body of a *passive* effect (useEffect), + * while allowing calling setState in callbacks scheduled by the effect. + * + * Calling setState during execution of a useEffect triggers a re-render, which is + * often bad for performance and frequently has more efficient and straightforward + * alternatives. See https://react.dev/learn/you-might-not-need-an-effect for examples. + */ +export function validateNoSetStateInPassiveEffects(fn: HIRFunction): void { + const setStateFunctions: Map = new Map(); + const errors = new CompilerError(); + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + switch (instr.value.kind) { + case 'LoadLocal': { + if (setStateFunctions.has(instr.value.place.identifier.id)) { + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.place, + ); + } + break; + } + case 'StoreLocal': { + if (setStateFunctions.has(instr.value.value.identifier.id)) { + setStateFunctions.set( + instr.value.lvalue.place.identifier.id, + instr.value.value, + ); + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.value, + ); + } + break; + } + case 'FunctionExpression': { + if ( + // faster-path to check if the function expression references a setState + [...eachInstructionValueOperand(instr.value)].some( + operand => + isSetStateType(operand.identifier) || + setStateFunctions.has(operand.identifier.id), + ) + ) { + const callee = getSetStateCall( + instr.value.loweredFunc.func, + setStateFunctions, + ); + if (callee !== null) { + setStateFunctions.set(instr.lvalue.identifier.id, callee); + } + } + break; + } + case 'MethodCall': + case 'CallExpression': { + const callee = + instr.value.kind === 'MethodCall' + ? instr.value.receiver + : instr.value.callee; + if (isUseEffectHookType(callee.identifier)) { + const arg = instr.value.args[0]; + if (arg !== undefined && arg.kind === 'Identifier') { + const setState = setStateFunctions.get(arg.identifier.id); + if (setState !== undefined) { + errors.push({ + reason: + 'Calling setState directly within a useEffect causes cascading renders and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)', + description: null, + severity: ErrorSeverity.InvalidReact, + loc: setState.loc, + suggestions: null, + }); + } + } + } + break; + } + } + } + } + + if (errors.hasErrors()) { + throw errors; + } +} + +function getSetStateCall( + fn: HIRFunction, + setStateFunctions: Map, +): Place | null { + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + switch (instr.value.kind) { + case 'LoadLocal': { + if (setStateFunctions.has(instr.value.place.identifier.id)) { + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.place, + ); + } + break; + } + case 'StoreLocal': { + if (setStateFunctions.has(instr.value.value.identifier.id)) { + setStateFunctions.set( + instr.value.lvalue.place.identifier.id, + instr.value.value, + ); + setStateFunctions.set( + instr.lvalue.identifier.id, + instr.value.value, + ); + } + break; + } + case 'CallExpression': { + const callee = instr.value.callee; + if ( + isSetStateType(callee.identifier) || + setStateFunctions.has(callee.identifier.id) + ) { + /* + * TODO: once we support multiple locations per error, we should link to the + * original Place in the case that setStateFunction.has(callee) + */ + return callee; + } + } + } + } + } + return null; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md index 7e6dcaff76..70320c3762 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md @@ -40,62 +40,37 @@ import { c as _c } from "react/compiler-runtime"; // @validateRefAccessDuringRen import { useRef } from "react"; function Component() { - const $ = _c(10); + const $ = _c(2); const ref = useRef(null); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = () => { + const setRef = () => { if (ref.current !== null) { ref.current = ""; } }; + + t0 = () => { + setRef(); + }; $[0] = t0; } else { t0 = $[0]; } - const setRef = t0; + const onClick = t0; let t1; - if ($[1] !== setRef) { - t1 = () => { - setRef(); - }; - $[1] = setRef; - $[2] = t1; - } else { - t1 = $[2]; - } - const onClick = t1; - let t2; - if ($[3] !== ref) { - t2 = ; - $[3] = ref; - $[4] = t2; - } else { - t2 = $[4]; - } - let t3; - if ($[5] !== onClick) { - t3 =