From c0364df681039eed61e96ddc0cdb95dd4f75c6b2 Mon Sep 17 00:00:00 2001 From: Juan Mugica Date: Mon, 9 Mar 2026 22:29:15 -0700 Subject: [PATCH 1/3] added iterative HIR pass in ValidateNoSetStateInEffects.ts --- .../Validation/ValidateNoSetStateInEffects.ts | 266 ++++++++++-------- 1 file changed, 146 insertions(+), 120 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts index 2457e0d7b99e..791f512d2924 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts @@ -48,135 +48,161 @@ export function validateNoSetStateInEffects( ): Result { 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, - ); + + // In order to ensure that all HIR components that could contain setState have been iterated, + // we run the reconnaissance logic until a stable number of state functions has been achieved. + // this makes setState HIR analysis order independent. + // We will then handle error on a separate pass. + let iterationSize = Number.MAX_SAFE_INTEGER; + while (iterationSize !== setStateFunctions.size) { + iterationSize = setStateFunctions.size; + 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; } - 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, - env, - ); - if (callee !== null) { - setStateFunctions.set(instr.lvalue.identifier.id, callee); + 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; } - break; - } - case 'MethodCall': - case 'CallExpression': { - const callee = - instr.value.kind === 'MethodCall' - ? instr.value.property - : instr.value.callee; - - if (isUseEffectEventType(callee.identifier)) { - const arg = instr.value.args[0]; - if (arg !== undefined && arg.kind === 'Identifier') { - const setState = setStateFunctions.get(arg.identifier.id); - if (setState !== undefined) { - /** - * This effect event function calls setState synchonously, - * treat it as a setState function for transitive tracking - */ - setStateFunctions.set(instr.lvalue.identifier.id, setState); + 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, + env, + ); + if (callee !== null) { + setStateFunctions.set(instr.lvalue.identifier.id, callee); } } - } else if ( - isUseEffectHookType(callee.identifier) || - isUseLayoutEffectHookType(callee.identifier) || - isUseInsertionEffectHookType(callee.identifier) - ) { - const arg = instr.value.args[0]; - if (arg !== undefined && arg.kind === 'Identifier') { - const setState = setStateFunctions.get(arg.identifier.id); - if (setState !== undefined) { - const enableVerbose = - env.config.enableVerboseNoSetStateInEffect; - if (enableVerbose) { - errors.pushDiagnostic( - CompilerDiagnostic.create({ - category: ErrorCategory.EffectSetState, - reason: - 'Calling setState synchronously within an effect can trigger cascading renders', - description: - 'Effects are intended to synchronize state between React and external systems. ' + - 'Calling setState synchronously causes cascading renders that hurt performance.\n\n' + - 'This pattern may indicate one of several issues:\n\n' + - '**1. Non-local derived data**: If the value being set could be computed from props/state ' + - 'but requires data from a parent component, consider restructuring state ownership so the ' + - 'derivation can happen during render in the component that owns the relevant state.\n\n' + - "**2. Derived event pattern**: If you're detecting when a prop changes (e.g., `isPlaying` " + - 'transitioning from false to true), this often indicates the parent should provide an event ' + - 'callback (like `onPlay`) instead of just the current state. Request access to the original event.\n\n' + - "**3. Force update / external sync**: If you're forcing a re-render to sync with an external " + - 'data source (mutable values outside React), use `useSyncExternalStore` to properly subscribe ' + - 'to external state changes.\n\n' + - 'See: https://react.dev/learn/you-might-not-need-an-effect', - suggestions: null, - }).withDetails({ - kind: 'error', - loc: setState.loc, - message: - 'Avoid calling setState() directly within an effect', - }), - ); - } else { - errors.pushDiagnostic( - CompilerDiagnostic.create({ - category: ErrorCategory.EffectSetState, - reason: - 'Calling setState synchronously within an effect can trigger cascading renders', - description: - 'Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. ' + - 'In general, the body of an effect should do one or both of the following:\n' + - '* Update external systems with the latest state from React.\n' + - '* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\n' + - 'Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. ' + - '(https://react.dev/learn/you-might-not-need-an-effect)', - suggestions: null, - }).withDetails({ - kind: 'error', - loc: setState.loc, - message: - 'Avoid calling setState() directly within an effect', - }), - ); + break; + } + case 'MethodCall': + case 'CallExpression': { + const callee = + instr.value.kind === 'MethodCall' + ? instr.value.property + : instr.value.callee; + + if (isUseEffectEventType(callee.identifier)) { + const arg = instr.value.args[0]; + if (arg !== undefined && arg.kind === 'Identifier') { + const setState = setStateFunctions.get(arg.identifier.id); + if (setState !== undefined) { + /** + * This effect event function calls setState synchonously, + * treat it as a setState function for transitive tracking + */ + setStateFunctions.set(instr.lvalue.identifier.id, setState); } } } + break; + } + } + } + } + } + + // Report errors in second pass to ensure HIR has been fully evaluated at error reporting time. + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + if ( + instr.value.kind !== 'CallExpression' && + instr.value.kind !== 'MethodCall' + ) { + continue; + } + const callee = + instr.value.kind === 'MethodCall' + ? instr.value.property + : instr.value.callee; + if ( + isUseEffectHookType(callee.identifier) || + isUseLayoutEffectHookType(callee.identifier) || + isUseInsertionEffectHookType(callee.identifier) + ) { + const arg = instr.value.args[0]; + if (arg !== undefined && arg.kind === 'Identifier') { + const setState = setStateFunctions.get(arg.identifier.id); + if (setState !== undefined) { + const enableVerbose = + env.config.enableVerboseNoSetStateInEffect; + if (enableVerbose) { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.EffectSetState, + reason: + 'Calling setState synchronously within an effect can trigger cascading renders', + description: + 'Effects are intended to synchronize state between React and external systems. ' + + 'Calling setState synchronously causes cascading renders that hurt performance.\n\n' + + 'This pattern may indicate one of several issues:\n\n' + + '**1. Non-local derived data**: If the value being set could be computed from props/state ' + + 'but requires data from a parent component, consider restructuring state ownership so the ' + + 'derivation can happen during render in the component that owns the relevant state.\n\n' + + "**2. Derived event pattern**: If you're detecting when a prop changes (e.g., `isPlaying` " + + 'transitioning from false to true), this often indicates the parent should provide an event ' + + 'callback (like `onPlay`) instead of just the current state. Request access to the original event.\n\n' + + "**3. Force update / external sync**: If you're forcing a re-render to sync with an external " + + 'data source (mutable values outside React), use `useSyncExternalStore` to properly subscribe ' + + 'to external state changes.\n\n' + + 'See: https://react.dev/learn/you-might-not-need-an-effect', + suggestions: null, + }).withDetails({ + kind: 'error', + loc: setState.loc, + message: + 'Avoid calling setState() directly within an effect', + }), + ); + } else { + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.EffectSetState, + reason: + 'Calling setState synchronously within an effect can trigger cascading renders', + description: + 'Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. ' + + 'In general, the body of an effect should do one or both of the following:\n' + + '* Update external systems with the latest state from React.\n' + + '* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\n' + + 'Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. ' + + '(https://react.dev/learn/you-might-not-need-an-effect)', + suggestions: null, + }).withDetails({ + kind: 'error', + loc: setState.loc, + message: + 'Avoid calling setState() directly within an effect', + }), + ); + } } - break; } } } From d16b252e9947bd7ca213b20db36492281ccb1777 Mon Sep 17 00:00:00 2001 From: Juan Mugica Date: Mon, 9 Mar 2026 22:49:37 -0700 Subject: [PATCH 2/3] added tests --- .../src/__tests__/Logger-test.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/Logger-test.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/Logger-test.ts index ca386fb2402e..6518d139b7be 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/Logger-test.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/Logger-test.ts @@ -7,6 +7,7 @@ import * as t from '@babel/types'; import invariant from 'invariant'; +import {ErrorCategory} from '../CompilerError'; import {runBabelPluginReactCompiler} from '../Babel/RunReactCompilerBabelPlugin'; import type {Logger, LoggerEvent} from '../Entrypoint'; @@ -68,3 +69,39 @@ it('logs failed compilation', () => { expect(event.fnLoc?.start).toEqual({column: 0, index: 0, line: 1}); expect(event.fnLoc?.end).toEqual({column: 70, index: 70, line: 1}); }); + +it('reports set-state-in-effect error when setState is called inside useEffect via useEffectEvent', () => { + const logs: [string | null, LoggerEvent][] = []; + const logger: Logger = { + logEvent(filename, event) { + logs.push([filename, event]); + }, + }; + + runBabelPluginReactCompiler( + `import {useEffect, useEffectEvent, useState} from 'react'; + function Component() { + const [, setState] = useState(''); + const onSetState = useEffectEvent(() => { + setState('test'); + }); + useEffect(() => { + onSetState(); + }, []); + return null; + }`, + 'test.js', + 'flow', + { + logger, + panicThreshold: 'none', + outputMode: 'lint', + environment: {validateNoSetStateInEffects: true}, + }, + ); + + const [, event] = logs.at(0)!; + expect(event).toBeDefined(); + expect(event.kind).toBe('CompileError'); + expect(event.detail.category).toBe(ErrorCategory.EffectSetState); +}); From 20d7ab62030e623c53ac9a2fb015e533f64e3f97 Mon Sep 17 00:00:00 2001 From: Juan Mugica Date: Mon, 9 Mar 2026 22:59:13 -0700 Subject: [PATCH 3/3] logic reasesment --- .../Validation/ValidateNoSetStateInEffects.ts | 126 +++++++++--------- 1 file changed, 60 insertions(+), 66 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts index 791f512d2924..9499cc300144 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts @@ -50,85 +50,79 @@ export function validateNoSetStateInEffects( const errors = new CompilerError(); // In order to ensure that all HIR components that could contain setState have been iterated, - // we run the reconnaissance logic until a stable number of state functions has been achieved. - // this makes setState HIR analysis order independent. - // We will then handle error on a separate pass. - let iterationSize = Number.MAX_SAFE_INTEGER; - while (iterationSize !== setStateFunctions.size) { - iterationSize = setStateFunctions.size; - 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; + // we fully run the reconnaissance logic, then handle error finding on a separate pass. + // This makes setState HIR error analysis order independent. + 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, + ); } - 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; + 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, + ); } - 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, - env, - ); - if (callee !== null) { - setStateFunctions.set(instr.lvalue.identifier.id, callee); - } + 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, + env, + ); + if (callee !== null) { + setStateFunctions.set(instr.lvalue.identifier.id, callee); } - break; } - case 'MethodCall': - case 'CallExpression': { - const callee = - instr.value.kind === 'MethodCall' - ? instr.value.property - : instr.value.callee; + break; + } + case 'MethodCall': + case 'CallExpression': { + const callee = + instr.value.kind === 'MethodCall' + ? instr.value.property + : instr.value.callee; - if (isUseEffectEventType(callee.identifier)) { - const arg = instr.value.args[0]; - if (arg !== undefined && arg.kind === 'Identifier') { - const setState = setStateFunctions.get(arg.identifier.id); - if (setState !== undefined) { - /** - * This effect event function calls setState synchonously, - * treat it as a setState function for transitive tracking - */ - setStateFunctions.set(instr.lvalue.identifier.id, setState); - } + if (isUseEffectEventType(callee.identifier)) { + const arg = instr.value.args[0]; + if (arg !== undefined && arg.kind === 'Identifier') { + const setState = setStateFunctions.get(arg.identifier.id); + if (setState !== undefined) { + /** + * This effect event function calls setState synchonously, + * treat it as a setState function for transitive tracking + */ + setStateFunctions.set(instr.lvalue.identifier.id, setState); } } - break; } + break; } } } } - // Report errors in second pass to ensure HIR has been fully evaluated at error reporting time. for (const [, block] of fn.body.blocks) { for (const instr of block.instructions) { if (