Compare commits

...

2 Commits

Author SHA1 Message Date
Jorge Cabiedes Acosta
15ea89b753 [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
2025-11-13 22:53:35 -08:00
Jorge Cabiedes Acosta
cd1d0dc2dc [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
2025-11-13 16:03:11 -08:00
6 changed files with 194 additions and 8 deletions

View File

@@ -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<IdentifierId, FunctionExpression>;
readonly candidateDependencies: Map<IdentifierId, ArrayExpression>;
readonly errors: CompilerError;
readonly derivationCache: DerivationCache;
readonly effects: Set<HIRFunction>;
readonly effectsCache: Map<IdentifierId, EffectMetadata>;
readonly setStateLoads: Map<IdentifierId, IdentifierId | null>;
readonly setStateUsages: Map<IdentifierId, Set<SourceLocation>>;
};
@@ -175,18 +182,20 @@ export function validateNoDerivedComputationsInEffects_exp(
fn: HIRFunction,
): Result<void, CompilerError> {
const functions: Map<IdentifierId, FunctionExpression> = new Map();
const candidateDependencies: Map<IdentifierId, ArrayExpression> = new Map();
const derivationCache = new DerivationCache();
const errors = new CompilerError();
const effects: Set<HIRFunction> = new Set();
const effectsCache: Map<IdentifierId, EffectMetadata> = new Map();
const setStateLoads: Map<IdentifierId, IdentifierId> = new Map();
const setStateUsages: Map<IdentifierId, Set<SourceLocation>> = 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<BlockId> = new Set();
@@ -612,6 +630,16 @@ function validateEffect(
Set<SourceLocation>
> = 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<IdentifierId> | undefined;
const globals: Set<IdentifierId> = new Set();
@@ -662,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,
);

View File

@@ -0,0 +1,63 @@
## Input
```javascript
// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
function Component({prop}) {
const [s, setS] = useState(0);
useEffect(() => {
setS(prop);
}, [prop, setS]);
return <div>{prop}</div>;
}
```
## 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 = <div>{prop}</div>;
$[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

View File

@@ -0,0 +1,10 @@
// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly
function Component({prop}) {
const [s, setS] = useState(0);
useEffect(() => {
setS(prop);
}, [prop, setS]);
return <div>{prop}</div>;
}

View File

@@ -0,0 +1,65 @@
## Input
```javascript
// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
function Component({setParentState, prop}) {
useEffect(() => {
setParentState(prop);
}, [prop]);
return <div>{prop}</div>;
}
```
## 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 = <div>{prop}</div>;
$[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

View File

@@ -0,0 +1,9 @@
// @validateNoDerivedComputationsInEffects_exp @enableTreatSetIdentifiersAsStateSetters @loggerTestOnly
function Component({setParentState, prop}) {
useEffect(() => {
setParentState(prop);
}, [prop]);
return <div>{prop}</div>;
}

View File

@@ -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}
```