From cd1d0dc2dc9e99ba1eaebc4e97e408bdf36f7c15 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Thu, 13 Nov 2025 15:48:46 -0800 Subject: [PATCH 1/2] [Compiler] Don't count a setState in the dependency array of the effect it is called on as a usage Summary: The validation only allows setState declaration as a usage outside of the effect. Another edge case is that if you add the setState being validated in the dependency array you also make the validation opt out since it counts as a usage outside of the effect. Added a bit of logic to consider the effect's deps when creating the cache for setState usages within the effect Test Plan: Added a fixture --- ...idateNoDerivedComputationsInEffects_exp.ts | 42 ++++++++++--- ...t-used-in-dep-array-still-errors.expect.md | 63 +++++++++++++++++++ .../effect-used-in-dep-array-still-errors.js | 10 +++ 3 files changed, 108 insertions(+), 7 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts index 5c9462fd55106..096e1ce1a0374 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -22,6 +22,7 @@ import { BasicBlock, isUseRefType, SourceLocation, + ArrayExpression, } from '../HIR'; import {eachInstructionLValue, eachInstructionOperand} from '../HIR/visitors'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; @@ -36,11 +37,17 @@ type DerivationMetadata = { isStateSource: boolean; }; +type EffectMetadata = { + effect: HIRFunction; + dependencies: ArrayExpression; +}; + type ValidationContext = { readonly functions: Map; + readonly candidateDependencies: Map; readonly errors: CompilerError; readonly derivationCache: DerivationCache; - readonly effects: Set; + readonly effectsCache: Map; readonly setStateLoads: Map; readonly setStateUsages: Map>; }; @@ -175,18 +182,20 @@ export function validateNoDerivedComputationsInEffects_exp( fn: HIRFunction, ): Result { const functions: Map = new Map(); + const candidateDependencies: Map = new Map(); const derivationCache = new DerivationCache(); const errors = new CompilerError(); - const effects: Set = new Set(); + const effectsCache: Map = new Map(); const setStateLoads: Map = new Map(); const setStateUsages: Map> = new Map(); const context: ValidationContext = { functions, + candidateDependencies, errors, derivationCache, - effects, + effectsCache, setStateLoads, setStateUsages, }; @@ -229,8 +238,8 @@ export function validateNoDerivedComputationsInEffects_exp( isFirstPass = false; } while (context.derivationCache.snapshot()); - for (const effect of effects) { - validateEffect(effect, context); + for (const [, effect] of effectsCache) { + validateEffect(effect.effect, effect.dependencies, context); } return errors.asResult(); @@ -354,8 +363,14 @@ function recordInstructionDerivations( value.args[1].kind === 'Identifier' ) { const effectFunction = context.functions.get(value.args[0].identifier.id); - if (effectFunction != null) { - context.effects.add(effectFunction.loweredFunc.func); + const deps = context.candidateDependencies.get( + value.args[1].identifier.id, + ); + if (effectFunction != null && deps != null) { + context.effectsCache.set(value.args[0].identifier.id, { + effect: effectFunction.loweredFunc.func, + dependencies: deps, + }); } } else if (isUseStateType(lvalue.identifier) && value.args.length > 0) { typeOfValue = 'fromState'; @@ -367,6 +382,8 @@ function recordInstructionDerivations( ); return; } + } else if (value.kind === 'ArrayExpression') { + context.candidateDependencies.set(lvalue.identifier.id, value); } for (const operand of eachInstructionOperand(instr)) { @@ -596,6 +613,7 @@ function getFnLocalDeps( function validateEffect( effectFunction: HIRFunction, + dependencies: ArrayExpression, context: ValidationContext, ): void { const seenBlocks: Set = new Set(); @@ -612,6 +630,16 @@ function validateEffect( Set > = new Map(); + // Consider setStates in the effect's dependency array as being part of effectSetStateUsages + for (const dep of dependencies.elements) { + if (dep.kind === 'Identifier') { + const root = getRootSetState(dep.identifier.id, context.setStateLoads); + if (root !== null) { + effectSetStateUsages.set(root, new Set([dep.loc])); + } + } + } + let cleanUpFunctionDeps: Set | undefined; const globals: Set = new Set(); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md new file mode 100644 index 0000000000000..6a3593a3b27c3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.expect.md @@ -0,0 +1,63 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component({prop}) { + const [s, setS] = useState(0); + useEffect(() => { + setS(prop); + }, [prop, setS]); + + return
{prop}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component(t0) { + const $ = _c(5); + const { prop } = t0; + const [, setS] = useState(0); + let t1; + let t2; + if ($[0] !== prop) { + t1 = () => { + setS(prop); + }; + t2 = [prop, setS]; + $[0] = prop; + $[1] = t1; + $[2] = t2; + } else { + t1 = $[1]; + t2 = $[2]; + } + useEffect(t1, t2); + let t3; + if ($[3] !== prop) { + t3 =
{prop}
; + $[3] = prop; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} + +``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nProps: [prop]\n\nData Flow Tree:\n└── prop (Prop)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":6,"column":4,"index":150},"end":{"line":6,"column":8,"index":154},"filename":"effect-used-in-dep-array-still-errors.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":10,"column":1,"index":212},"filename":"effect-used-in-dep-array-still-errors.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### 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/effect-derived-computations/effect-used-in-dep-array-still-errors.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.js new file mode 100644 index 0000000000000..1df99a191dcfe --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/effect-used-in-dep-array-still-errors.js @@ -0,0 +1,10 @@ +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly + +function Component({prop}) { + const [s, setS] = useState(0); + useEffect(() => { + setS(prop); + }, [prop, setS]); + + return
{prop}
; +} From 15ea89b75347a8df3196d0cdd815bc0403bc9300 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Thu, 13 Nov 2025 16:23:30 -0800 Subject: [PATCH 2/2] [Compiler] Avoid capturing global setStates for no-derived-computations lint Summary: This only matters when enableTreatSetIdentifiersAsStateSetters=true This pattern is still bad. But Right now the validation can only recommend to move stuff to "calculate in render" A global setState should not be moved to render, not even conditionally and you can't remove state without crossing Component boundaries, which makes this a different kind of fix. So while we are only suggesting "calculate in render" as a fix we should disallow the lint from throwing in this case IMO Test Plan: Added a fixture --- ...idateNoDerivedComputationsInEffects_exp.ts | 12 ++++ ...rops-setstate-in-effect-no-error.expect.md | 65 +++++++++++++++++++ .../from-props-setstate-in-effect-no-error.js | 9 +++ ...m-prop-no-show-in-data-flow-tree.expect.md | 1 - 4 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts index 096e1ce1a0374..af5927548af52 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects_exp.ts @@ -690,6 +690,18 @@ function validateEffect( instr.value.args.length === 1 && instr.value.args[0].kind === 'Identifier' ) { + const calleeMetadata = context.derivationCache.cache.get( + instr.value.callee.identifier.id, + ); + + /* + * If the setState comes from a source other than local state skip + * since the fix is not to calculate in render + */ + if (calleeMetadata?.typeOfValue != 'fromState') { + continue; + } + const argMetadata = context.derivationCache.cache.get( instr.value.args[0].identifier.id, ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.expect.md new file mode 100644 index 0000000000000..f23f51d6cb8bb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.expect.md @@ -0,0 +1,65 @@ + +## Input + +```javascript +// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly + +function Component({setParentState, prop}) { + useEffect(() => { + setParentState(prop); + }, [prop]); + + return
{prop}
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly + +function Component(t0) { + const $ = _c(7); + const { setParentState, prop } = t0; + let t1; + if ($[0] !== prop || $[1] !== setParentState) { + t1 = () => { + setParentState(prop); + }; + $[0] = prop; + $[1] = setParentState; + $[2] = t1; + } else { + t1 = $[2]; + } + let t2; + if ($[3] !== prop) { + t2 = [prop]; + $[3] = prop; + $[4] = t2; + } else { + t2 = $[4]; + } + useEffect(t1, t2); + let t3; + if ($[5] !== prop) { + t3 =
{prop}
; + $[5] = prop; + $[6] = t3; + } else { + t3 = $[6]; + } + return t3; +} + +``` + +## Logs + +``` +{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":105},"end":{"line":9,"column":1,"index":240},"filename":"from-props-setstate-in-effect-no-error.ts"},"fnName":"Component","memoSlots":7,"memoBlocks":3,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` + +### 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/effect-derived-computations/from-props-setstate-in-effect-no-error.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.js new file mode 100644 index 0000000000000..4075975b32549 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/from-props-setstate-in-effect-no-error.js @@ -0,0 +1,9 @@ +// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly + +function Component({setParentState, prop}) { + useEffect(() => { + setParentState(prop); + }, [prop]); + + return
{prop}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md index 87cf7722da353..602fb9fff3a6c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md @@ -64,7 +64,6 @@ function Component(t0) { ## Logs ``` -{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [second]\n\nData Flow Tree:\n└── second (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":14,"column":4,"index":443},"end":{"line":14,"column":8,"index":447},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null} {"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":18,"column":1,"index":500},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} ```