From de97ef9ad510352ba37434085098bf026dd46b9b Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Thu, 13 Nov 2025 22:52:23 -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 (#35134) 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 --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35134). * #35135 * __->__ #35134 --- ...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 5c9462fd551..096e1ce1a03 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 00000000000..6a3593a3b27 --- /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 00000000000..1df99a191dc --- /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 257b033fc7b0518e7b1db32ca24e2354933b9d0e Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Thu, 13 Nov 2025 22:56:06 -0800 Subject: [PATCH 2/2] [Compiler] Avoid capturing global setStates for no-derived-computations lint (#35135) 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 --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35135). * __->__ #35135 * #35134 --- ...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 096e1ce1a03..af5927548af 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 00000000000..f23f51d6cb8 --- /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 00000000000..4075975b325 --- /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 87cf7722da3..602fb9fff3a 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} ```