Compare commits

..

3 Commits

Author SHA1 Message Date
Jorge Cabiedes
5cf71b322d [compiler] Validation for values derived from props in useEffect ready 2025-09-09 14:11:22 -07:00
Jorge Cabiedes
f807ce6492 [compiler] Basic solution for instruction based prop derivation validation 2025-09-09 14:11:19 -07:00
Lauren Tan
7b38acca0b [compiler][wip] Extend ValidateNoDerivedComputationsInEffects for props derived effects
This PR adds infra to disambiguate between two types of derived state in effects:
  1. State derived from props
  2. State derived from other state

TODO:
- [ ] Props tracking through destructuring and property access does not seem to be propagated correctly inside of Functions' instructions (or i might be misunderstanding how we track aliasing effects)
- [ ] compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/invalid-derived-state-from-props-computed.js should be failing
- [ ] Handle "mixed" case where deps flow from at least one prop AND state. Should probably have a different error reason, to aid with categorization
2025-09-09 14:10:44 -07:00
7 changed files with 391 additions and 58 deletions

View File

@@ -5,21 +5,217 @@
* LICENSE file in the root directory of this source tree.
*/
import {CompilerError, SourceLocation} from '..';
import {effect} from 'zod';
import {CompilerError, Effect, ErrorSeverity, SourceLocation} from '..';
import {ErrorCategory} from '../CompilerError';
import {
ArrayExpression,
BasicBlock,
BlockId,
Identifier,
FunctionExpression,
HIRFunction,
IdentifierId,
Instruction,
Place,
isSetStateType,
isUseEffectHookType,
isUseStateType,
IdentifierName,
GeneratedSource,
} from '../HIR';
import {printInstruction} from '../HIR/PrintHIR';
import {
eachInstructionValueOperand,
eachInstructionOperand,
eachTerminalOperand,
eachInstructionLValue,
eachPatternOperand,
} from '../HIR/visitors';
import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
import {assertExhaustive} from '../Utils/utils';
type SetStateCall = {
loc: SourceLocation;
invalidDeps: Map<Identifier, Place[]> | undefined;
setStateId: IdentifierId;
};
type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState';
type DerivationMetadata = {
typeOfValue: TypeOfValue;
// TODO: Rename to place
identifierPlace: Place;
sources: Place[];
};
// TODO: This needs refining
type ErrorMetadata = {
errorType: 'HoistState' | 'CalculateInRender';
propInfo: string | undefined;
loc: SourceLocation;
setStateId: IdentifierId;
};
function joinValue(
lvalueType: TypeOfValue,
valueType: TypeOfValue,
): TypeOfValue {
if (lvalueType === 'ignored') return valueType;
if (valueType === 'ignored') return lvalueType;
if (lvalueType === valueType) return lvalueType;
return 'fromPropsOrState';
}
function updateDerivationMetadata(
target: Place,
sources: DerivationMetadata[],
typeOfValue: TypeOfValue,
derivedTuple: Map<IdentifierId, DerivationMetadata>,
): void {
let newValue: DerivationMetadata = {
identifierPlace: target,
sources: [],
typeOfValue: typeOfValue,
};
for (const source of sources) {
// If the identifier of the source is a promoted identifier, then
// we should set the target as the source.
if (source.identifierPlace.identifier.name?.kind === 'promoted') {
newValue.sources.push(target);
} else {
newValue.sources.push(...source.sources);
}
}
derivedTuple.set(target.identifier.id, newValue);
}
function parseInstr(
instr: Instruction,
derivedTuple: Map<IdentifierId, DerivationMetadata>,
setStateCalls: Map<string, Place>,
) {
// console.log(printInstruction(instr));
// console.log(instr);
let typeOfValue: TypeOfValue = 'ignored';
// If the instruction is destructuring a useState hook call
if (
instr.value.kind === 'Destructure' &&
instr.value.lvalue.pattern.kind === 'ArrayPattern' &&
isUseStateType(instr.value.value.identifier)
) {
const value = instr.value.lvalue.pattern.items[0];
if (value.kind === 'Identifier') {
derivedTuple.set(value.identifier.id, {
identifierPlace: value,
sources: [value],
typeOfValue: 'fromState',
});
}
}
// If the instruction is calling a setState
if (
instr.value.kind === 'CallExpression' &&
isSetStateType(instr.value.callee.identifier) &&
instr.value.args.length === 1 &&
instr.value.args[0].kind === 'Identifier' &&
instr.value.callee.loc !== GeneratedSource &&
instr.value.callee.loc.identifierName !== undefined &&
instr.value.callee.loc.identifierName !== null
) {
setStateCalls.set(
instr.value.callee.loc.identifierName,
instr.value.callee,
);
}
let sources: DerivationMetadata[] = [];
for (const operand of eachInstructionOperand(instr)) {
const opSource = derivedTuple.get(operand.identifier.id);
if (opSource === undefined) {
continue;
}
typeOfValue = joinValue(typeOfValue, opSource.typeOfValue);
sources.push(opSource);
}
if (typeOfValue !== 'ignored') {
for (const lvalue of eachInstructionLValue(instr)) {
updateDerivationMetadata(lvalue, sources, typeOfValue, derivedTuple);
}
for (const operand of eachInstructionOperand(instr)) {
switch (operand.effect) {
case Effect.Capture:
case Effect.Store:
case Effect.ConditionallyMutate:
case Effect.ConditionallyMutateIterator:
case Effect.Mutate: {
if (isMutable(instr, operand)) {
updateDerivationMetadata(
operand,
sources,
typeOfValue,
derivedTuple,
);
}
break;
}
case Effect.Freeze:
case Effect.Read: {
// no-op
break;
}
case Effect.Unknown: {
CompilerError.invariant(false, {
reason: 'Unexpected unknown effect',
description: null,
loc: operand.loc,
suggestions: null,
});
}
default: {
assertExhaustive(
operand.effect,
`Unexpected effect kind \`${operand.effect}\``,
);
}
}
}
}
}
function parseBlockPhi(
block: BasicBlock,
derivedTuple: Map<IdentifierId, DerivationMetadata>,
) {
for (const phi of block.phis) {
for (const operand of phi.operands.values()) {
const source = derivedTuple.get(operand.identifier.id);
if (source !== undefined && source.typeOfValue === 'fromProps') {
if (
source.identifierPlace.identifier.name === null ||
source.identifierPlace.identifier.name?.kind === 'promoted'
) {
derivedTuple.set(phi.place.identifier.id, {
identifierPlace: phi.place,
sources: [phi.place],
typeOfValue: 'fromProps',
});
} else {
derivedTuple.set(phi.place.identifier.id, {
identifierPlace: phi.place,
sources: source.sources,
typeOfValue: 'fromProps',
});
}
}
}
}
}
/**
* Validates that useEffect is not used for derived computations which could/should
@@ -48,12 +244,58 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
const candidateDependencies: Map<IdentifierId, ArrayExpression> = new Map();
const functions: Map<IdentifierId, FunctionExpression> = new Map();
const locals: Map<IdentifierId, IdentifierId> = new Map();
const derivedTuple: Map<IdentifierId, DerivationMetadata> = new Map();
const errors = new CompilerError();
// Investigating
const effectSetStates: Map<string, Place> = new Map();
const setStateCalls: Map<string, Place> = new Map();
// let shouldCalculateInRender: boolean = true;
const errors: ErrorMetadata[] = [];
if (fn.fnType === 'Hook') {
for (const param of fn.params) {
if (param.kind === 'Identifier') {
derivedTuple.set(param.identifier.id, {
identifierPlace: param,
sources: [param],
typeOfValue: 'fromProps',
});
}
}
} else if (fn.fnType === 'Component') {
const props = fn.params[0];
if (props != null && props.kind === 'Identifier') {
derivedTuple.set(props.identifier.id, {
identifierPlace: props,
sources: [props],
typeOfValue: 'fromProps',
});
}
}
for (const block of fn.body.blocks.values()) {
parseBlockPhi(block, derivedTuple);
for (const instr of block.instructions) {
const {lvalue, value} = instr;
parseInstr(instr, derivedTuple, setStateCalls);
/*
* Special case for function expressions, we need to parse nested instructions
* TODO: Can there be more recursive levels?
*/
if (value.kind === 'FunctionExpression') {
for (const [, block] of value.loweredFunc.func.body.blocks) {
for (const instr of block.instructions) {
parseInstr(instr, derivedTuple, setStateCalls);
}
}
}
// Maybe this should run for every instruction being parsed
if (value.kind === 'LoadLocal') {
locals.set(lvalue.identifier.id, value.place.identifier.id);
} else if (value.kind === 'ArrayExpression') {
@@ -66,6 +308,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
) {
const callee =
value.kind === 'CallExpression' ? value.callee : value.property;
if (
isUseEffectHookType(callee.identifier) &&
value.args.length === 2 &&
@@ -97,6 +340,8 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
validateEffect(
effectFunction.loweredFunc.func,
dependencies,
derivedTuple,
effectSetStates,
errors,
);
}
@@ -104,43 +349,82 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void {
}
}
}
if (errors.hasAnyErrors()) {
throw errors;
console.log('setStateCalls: ', setStateCalls);
console.log('effectSetStates: ', effectSetStates);
const throwableErrors = new CompilerError();
for (const error of errors) {
throwableErrors.push({
reason: `You may not need an effect. Values derived from state should be calculated in render, not in an effect. `,
description: `You are using a value derived from props${error.propInfo} to update local state in an effect.`,
severity: ErrorSeverity.InvalidReact,
loc: error.loc,
});
}
if (throwableErrors.hasAnyErrors()) {
throw throwableErrors;
}
}
function validateEffect(
effectFunction: HIRFunction,
effectDeps: Array<IdentifierId>,
errors: CompilerError,
derivedTuple: Map<IdentifierId, DerivationMetadata>,
effectSetStates: Map<string, Place>,
errors: ErrorMetadata[],
): void {
/*
* TODO: This makes it so we only capture single line useEffects.
* We should be able to capture multiline as well
*/
for (const operand of effectFunction.context) {
if (isSetStateType(operand.identifier)) {
continue;
} else if (effectDeps.find(dep => dep === operand.identifier.id) != null) {
continue;
} else if (derivedTuple.has(operand.identifier.id)) {
continue;
} else {
// Captured something other than the effect dep or setState
return;
}
}
// This might be wrong gotta double check
let hasInvalidDep = false;
for (const dep of effectDeps) {
const depMetadata = derivedTuple.get(dep);
if (
effectFunction.context.find(operand => operand.identifier.id === dep) ==
null
effectFunction.context.find(operand => operand.identifier.id === dep) !=
null ||
(depMetadata !== undefined && depMetadata.typeOfValue !== 'ignored')
) {
// effect dep wasn't actually used in the function
return;
hasInvalidDep = true;
}
}
const seenBlocks: Set<BlockId> = new Set();
const values: Map<IdentifierId, Array<IdentifierId>> = new Map();
for (const dep of effectDeps) {
values.set(dep, [dep]);
if (!hasInvalidDep) {
console.log('early return 2');
// effect dep wasn't actually used in the function
return;
}
const setStateLocations: Array<SourceLocation> = [];
const seenBlocks: Set<BlockId> = new Set();
// This variable is suspicious maybe we don't need it?
const values: Map<IdentifierId, Array<IdentifierId>> = new Map();
const effectInvalidlyDerived: Map<IdentifierId, DerivationMetadata> =
new Map();
for (const dep of effectDeps) {
values.set(dep, [dep]);
const depMetadata = derivedTuple.get(dep);
if (depMetadata !== undefined) {
effectInvalidlyDerived.set(dep, depMetadata);
}
}
const setStateCallsInEffect: Array<SetStateCall> = [];
for (const block of effectFunction.body.blocks.values()) {
for (const pred of block.preds) {
if (!seenBlocks.has(pred)) {
@@ -148,21 +432,24 @@ function validateEffect(
return;
}
}
for (const phi of block.phis) {
const aggregateDeps: Set<IdentifierId> = new Set();
for (const operand of phi.operands.values()) {
const deps = values.get(operand.identifier.id);
if (deps != null) {
for (const dep of deps) {
aggregateDeps.add(dep);
}
}
}
if (aggregateDeps.size !== 0) {
values.set(phi.place.identifier.id, Array.from(aggregateDeps));
}
}
parseBlockPhi(block, effectInvalidlyDerived);
for (const instr of block.instructions) {
if (
instr.value.kind === 'CallExpression' &&
isSetStateType(instr.value.callee.identifier) &&
instr.value.args.length === 1 &&
instr.value.args[0].kind === 'Identifier' &&
instr.value.callee.loc !== GeneratedSource &&
instr.value.callee.loc.identifierName !== undefined &&
instr.value.callee.loc.identifierName !== null
) {
effectSetStates.set(
instr.value.callee.loc.identifierName,
instr.value.callee,
);
}
switch (instr.value.kind) {
case 'Primitive':
case 'JSXText':
@@ -183,7 +470,7 @@ function validateEffect(
case 'CallExpression':
case 'MethodCall': {
const aggregateDeps: Set<IdentifierId> = new Set();
for (const operand of eachInstructionValueOperand(instr.value)) {
for (const operand of eachInstructionOperand(instr)) {
const deps = values.get(operand.identifier.id);
if (deps != null) {
for (const dep of deps) {
@@ -201,38 +488,74 @@ function validateEffect(
instr.value.args.length === 1 &&
instr.value.args[0].kind === 'Identifier'
) {
const deps = values.get(instr.value.args[0].identifier.id);
if (deps != null && new Set(deps).size === effectDeps.length) {
setStateLocations.push(instr.value.callee.loc);
const propSources = derivedTuple.get(
instr.value.args[0].identifier.id,
);
if (propSources !== undefined) {
setStateCallsInEffect.push({
loc: instr.value.callee.loc,
setStateId: instr.value.callee.identifier.id,
invalidDeps: new Map([
[instr.value.args[0].identifier, propSources.sources],
]),
});
} else {
// doesn't depend on any deps
return;
setStateCallsInEffect.push({
loc: instr.value.callee.loc,
setStateId: instr.value.callee.identifier.id,
invalidDeps: undefined,
});
}
}
break;
}
default: {
console.log('early return 4');
return;
}
}
}
for (const operand of eachTerminalOperand(block.terminal)) {
if (values.has(operand.identifier.id)) {
//
return;
}
}
seenBlocks.add(block.id);
}
for (const loc of setStateLocations) {
errors.push({
category: ErrorCategory.EffectDerivationsOfState,
reason:
'Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)',
description: null,
loc,
suggestions: null,
});
// need to track if the setState call has been used elsewhere
// if it is then the solution should be to lift the state up to the parent component
// if not the solution should be to calculate the value in rende
//
// If the same setState is used both inside and outside the effect
for (const call of setStateCallsInEffect) {
if (call.invalidDeps != null) {
let propNames = '';
for (const [, places] of call.invalidDeps.entries()) {
const placeNames = places
.map(place => place.identifier.name?.value)
.join(', ');
propNames += `[${placeNames}], `;
}
propNames = propNames.slice(0, -2);
const propInfo = propNames ? ` (from props '${propNames}')` : '';
errors.push({
errorType: 'HoistState',
propInfo: propInfo,
loc: call.loc,
setStateId: call.setStateId,
});
} else {
errors.push({
errorType: 'CalculateInRender',
propInfo: undefined,
loc: call.loc,
setStateId: call.setStateId,
});
}
}
}

View File

@@ -24,13 +24,15 @@ function BadExample() {
```
Found 1 error:
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
This effect updates state based on other state values. Consider calculating this value directly during render.
error.invalid-derived-computation-in-effect.ts:9:4
7 | const [fullName, setFullName] = useState('');
8 | useEffect(() => {
> 9 | setFullName(capitalize(firstName + ' ' + lastName));
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
| ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
10 | }, [firstName, lastName]);
11 |
12 | return <div>{fullName}</div>;

View File

@@ -34,13 +34,15 @@ export const FIXTURE_ENTRYPOINT = {
```
Found 1 error:
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
error.derived-state-from-mixed-deps-no-error.ts:9:4
This effect updates state based on other state values. Consider calculating this value directly during render.
error.bug-derived-state-from-mixed-deps.ts:9:4
7 |
8 | useEffect(() => {
> 9 | setDisplayName(prefix + name);
| ^^^^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
| ^^^^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
10 | }, [prefix, name]);
11 |
12 | return (

View File

@@ -28,13 +28,15 @@ export const FIXTURE_ENTRYPOINT = {
```
Found 1 error:
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
This effect updates state based on other state values. Consider calculating this value directly during render.
error.invalid-derived-state-from-props-destructured.ts:8:4
6 |
7 | useEffect(() => {
> 8 | setFullName(firstName + ' ' + lastName);
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
| ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
9 | }, [firstName, lastName]);
10 |
11 | return <div>{fullName}</div>;

View File

@@ -1,7 +1,7 @@
// @validateNoDerivedComputationsInEffects
import {useEffect, useState} from 'react';
function Component({user: {firstName, lastName}}) {
function Component({firstName, lastName}) {
const [fullName, setFullName] = useState('');
useEffect(() => {
@@ -13,5 +13,5 @@ function Component({user: {firstName, lastName}}) {
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{user: {firstName: 'John', lastName: 'Doe'}}],
params: [{firstName: 'John', lastName: 'Doe'}],
};

View File

@@ -28,13 +28,15 @@ export const FIXTURE_ENTRYPOINT = {
```
Found 1 error:
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
This effect updates state based on other state values. Consider calculating this value directly during render.
error.invalid-derived-state-from-props-in-effect.ts:8:4
6 |
7 | useEffect(() => {
> 8 | setFullName(firstName + ' ' + lastName);
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
| ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
9 | }, [firstName, lastName]);
10 |
11 | return <div>{fullName}</div>;

View File

@@ -36,13 +36,15 @@ export const FIXTURE_ENTRYPOINT = {
```
Found 1 error:
Error: Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
Error: You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
This effect updates state based on other state values. Consider calculating this value directly during render.
error.invalid-derived-state-from-state-in-effect.ts:10:4
8 |
9 | useEffect(() => {
> 10 | setFullName(firstName + ' ' + lastName);
| ^^^^^^^^^^^ Values derived from props and state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
| ^^^^^^^^^^^ You may not need this effect. Values derived from state should be calculated during render, not in an effect. (https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state)
11 | }, [firstName, lastName]);
12 |
13 | return (