From acf70fef44f37009fb0c0103615c6d3700a9b39a Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Tue, 26 Aug 2025 14:21:25 -0700 Subject: [PATCH] Improve code quality and update some tests --- .../ValidateNoDerivedComputationsInEffects.ts | 120 ++++++++---------- ...ug-derived-state-from-mixed-deps.expect.md | 4 +- ...erived-state-from-shadowed-props.expect.md | 2 +- ...id-derived-computation-in-effect.expect.md | 2 +- ...rived-state-from-state-in-effect.expect.md | 2 +- 5 files changed, 55 insertions(+), 75 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts index fb4912d362..eca16f44f2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -7,9 +7,7 @@ import { CompilerDiagnostic, - CompilerDiagnosticDetail, CompilerError, - CompilerErrorDetail, Effect, ErrorSeverity, SourceLocation, @@ -28,26 +26,18 @@ import { isUseStateType, GeneratedSource, } from '../HIR'; -import {printInstruction} from '../HIR/PrintHIR'; -import { - eachInstructionOperand, - eachTerminalOperand, - eachInstructionLValue, -} from '../HIR/visitors'; +import {eachInstructionOperand, eachInstructionLValue} from '../HIR/visitors'; import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; import {assertExhaustive} from '../Utils/utils'; -// TODO: Maybe I can consolidate some types type SetStateCall = { loc: SourceLocation; - invalidDeps: DerivationMetadata; + derivedDep: DerivationMetadata; setStateId: IdentifierId; }; type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState'; -type SetStateName = string | undefined | null; - type DerivationMetadata = { typeOfValue: TypeOfValue; place: Place; @@ -58,7 +48,7 @@ type ErrorMetadata = { type: TypeOfValue; description: string | undefined; loc: SourceLocation; - setStateName: SetStateName; + setStateName: string | undefined | null; }; /** @@ -88,17 +78,20 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const candidateDependencies: Map = new Map(); const functions: Map = new Map(); const locals: Map = new Map(); - const derivedTuple: Map = new Map(); + const derivationCache: Map = new Map(); - const effectSetStates: Map> = new Map(); - const setStateCalls: Map> = new Map(); + const effectSetStates: Map< + string | undefined | null, + Array + > = new Map(); + const setStateCalls: Map> = new Map(); const errors: Array = []; if (fn.fnType === 'Hook') { for (const param of fn.params) { if (param.kind === 'Identifier') { - derivedTuple.set(param.identifier.id, { + derivationCache.set(param.identifier.id, { place: param, sources: new Set([param]), typeOfValue: 'fromProps', @@ -108,7 +101,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } else if (fn.fnType === 'Component') { const props = fn.params[0]; if (props != null && props.kind === 'Identifier') { - derivedTuple.set(props.identifier.id, { + derivationCache.set(props.identifier.id, { place: props, sources: new Set([props]), typeOfValue: 'fromProps', @@ -117,12 +110,12 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } for (const block of fn.body.blocks.values()) { - parseBlockPhi(block, derivedTuple); + parseBlockPhi(block, derivationCache); for (const instr of block.instructions) { const {lvalue, value} = instr; - parseInstr(instr, derivedTuple, setStateCalls); + parseInstr(instr, derivationCache, setStateCalls); if (value.kind === 'LoadLocal') { locals.set(lvalue.identifier.id, value.place.identifier.id); @@ -161,7 +154,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { validateEffect( effectFunction.loweredFunc.func, dependencies, - derivedTuple, + derivationCache, effectSetStates, errors, ); @@ -183,8 +176,8 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { } function generateCompilerError( - setStateCalls: Map>, - effectSetStates: Map>, + setStateCalls: Map>, + effectSetStates: Map>, errors: Array, ): CompilerError { const throwableErrors = new CompilerError(); @@ -217,7 +210,7 @@ function generateCompilerError( }).withDetail({ kind: 'error', loc: error.loc, - message: detailMessage, + message: 'this setState synchronizes the state', }); for (const [key, setStateCallArray] of effectSetStates) { @@ -247,7 +240,7 @@ function generateCompilerError( }).withDetail({ kind: 'error', loc: error.loc, - message: detailMessage, + message: 'This should be computed during render, not in an effect', }); } @@ -273,7 +266,7 @@ function updateDerivationMetadata( target: Place, sources: Array | undefined, typeOfValue: TypeOfValue | undefined, - derivedTuple: Map, + derivationCache: Map, ): void { let newValue: DerivationMetadata = { place: target, @@ -300,19 +293,19 @@ function updateDerivationMetadata( } } - derivedTuple.set(target.identifier.id, newValue); + derivationCache.set(target.identifier.id, newValue); } function parseInstr( instr: Instruction, - derivedTuple: Map, - setStateCalls: Map>, + derivationCache: Map, + setStateCalls: Map>, ): void { // Recursively parse function expressions if (instr.value.kind === 'FunctionExpression') { for (const [, block] of instr.value.loweredFunc.func.body.blocks) { for (const instr of block.instructions) { - parseInstr(instr, derivedTuple, setStateCalls); + parseInstr(instr, derivationCache, setStateCalls); } } } @@ -357,7 +350,7 @@ function parseInstr( } for (const operand of eachInstructionOperand(instr)) { - const opSource = derivedTuple.get(operand.identifier.id); + const opSource = derivationCache.get(operand.identifier.id); if (opSource === undefined) { continue; } @@ -368,7 +361,7 @@ function parseInstr( if (typeOfValue !== 'ignored') { for (const lvalue of eachInstructionLValue(instr)) { - updateDerivationMetadata(lvalue, sources, typeOfValue, derivedTuple); + updateDerivationMetadata(lvalue, sources, typeOfValue, derivationCache); } for (const operand of eachInstructionOperand(instr)) { @@ -383,7 +376,7 @@ function parseInstr( operand, sources, typeOfValue, - derivedTuple, + derivationCache, ); } break; @@ -414,17 +407,17 @@ function parseInstr( function parseBlockPhi( block: BasicBlock, - derivedTuple: Map, + derivationCache: Map, ): void { for (const phi of block.phis) { for (const operand of phi.operands.values()) { - const source = derivedTuple.get(operand.identifier.id); - if (source !== undefined) { + const phiSource = derivationCache.get(operand.identifier.id); + if (phiSource !== undefined) { updateDerivationMetadata( phi.place, - [source], - source?.typeOfValue, - derivedTuple, + [phiSource], + phiSource?.typeOfValue, + derivationCache, ); } } @@ -434,13 +427,13 @@ function parseBlockPhi( function validateEffect( effectFunction: HIRFunction, effectDeps: Array, - derivedTuple: Map, - effectSetStates: Map>, + derivationCache: Map, + effectSetStates: Map>, errors: Array, ): void { let isUsingDerivedDeps = false; for (const dep of effectDeps) { - const depMetadata = derivedTuple.get(dep); + const depMetadata = derivationCache.get(dep); if ( effectFunction.context.find(operand => operand.identifier.id === dep) != null || @@ -457,7 +450,7 @@ function validateEffect( const seenBlocks: Set = new Set(); - const setStateCallsInEffect: Array = []; + const derivedSetStateCall: Array = []; for (const block of effectFunction.body.blocks.values()) { for (const pred of block.preds) { if (!seenBlocks.has(pred)) { @@ -466,7 +459,7 @@ function validateEffect( } } - parseBlockPhi(block, derivedTuple); + parseBlockPhi(block, derivationCache); for (const instr of block.instructions) { if ( @@ -509,15 +502,15 @@ function validateEffect( instr.value.args.length === 1 && instr.value.args[0].kind === 'Identifier' ) { - const invalidDeps = derivedTuple.get( + const derivedDep = derivationCache.get( instr.value.args[0].identifier.id, ); - if (invalidDeps !== undefined) { - setStateCallsInEffect.push({ + if (derivedDep !== undefined) { + derivedSetStateCall.push({ loc: instr.value.callee.loc, setStateId: instr.value.callee.identifier.id, - invalidDeps: invalidDeps, + derivedDep: derivedDep, }); } } @@ -529,40 +522,27 @@ function validateEffect( seenBlocks.add(block.id); } - for (const call of setStateCallsInEffect) { - const placeNames = Array.from(call.invalidDeps.sources) + for (const call of derivedSetStateCall) { + const placeNames = Array.from(call.derivedDep.sources) .map(place => { return place.identifier.name?.value; }) .filter(Boolean) .join(', '); - let sourceNames = ''; let errorDescription = ''; - if (call.invalidDeps.typeOfValue === 'fromProps') { - sourceNames += `[${placeNames}], `; - sourceNames = sourceNames.slice(0, -2); - errorDescription = sourceNames - ? `This setState() appears to derive a value from props ${sourceNames}.` - : ''; - } else if (call.invalidDeps.typeOfValue === 'fromState') { - sourceNames += `[${placeNames}], `; - sourceNames = sourceNames.slice(0, -2); - errorDescription = sourceNames - ? `This setState() appears to derive a value local state ${sourceNames}.` - : ''; + if (call.derivedDep.typeOfValue === 'fromProps') { + errorDescription = `props [${placeNames}].`; + } else if (call.derivedDep.typeOfValue === 'fromState') { + errorDescription = `local state [${placeNames}].`; } else { - sourceNames += `[${placeNames}], `; - sourceNames = sourceNames.slice(0, -2); - errorDescription = sourceNames - ? `This setState() appears to derive a value both props and local state ${sourceNames}.` - : ''; + errorDescription = `both props and local state [${placeNames}].`; } errors.push({ - type: call.invalidDeps.typeOfValue, - description: errorDescription, + type: call.derivedDep.typeOfValue, + description: `This setState() appears to derive a value from ${errorDescription}`, loc: call.loc, setStateName: call.loc !== GeneratedSource ? call.loc.identifierName : undefined, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md index 45dbe7521a..2588a014af 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.bug-derived-state-from-mixed-deps.expect.md @@ -36,13 +36,13 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value both props and local state [prefix, name]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user. +This setState() appears to derive a value from both props and local state [prefix, name]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user. error.bug-derived-state-from-mixed-deps.ts:9:4 7 | 8 | useEffect(() => { > 9 | setDisplayName(prefix + name); - | ^^^^^^^^^^^^^^ This state value shadows a value passed as a prop or a value from state. + | ^^^^^^^^^^^^^^ This should be computed during render, not in an effect 10 | }, [prefix, name]); 11 | 12 | return ( diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md index 5399aaf978..cc30238063 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.derived-state-from-shadowed-props.expect.md @@ -49,7 +49,7 @@ error.derived-state-from-shadowed-props.ts:16:8 14 |
{ > 16 | setDisplayValue('clicked'); - | ^^^^^^^^^^^^^^^ this setState updates the shadowed state, but should call an onChange event from the parent + | ^^^^^^^^^^^^^^^ this setState synchronizes the state 17 | }}> 18 | {displayValue} 19 |
diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md index dedc775d1e..b4faeaa8db 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-computation-in-effect.expect.md @@ -26,7 +26,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value local state [firstName, lastName]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user. +This setState() appears to derive a value from local state [firstName, lastName]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user. error.invalid-derived-computation-in-effect.ts:9:4 7 | const [fullName, setFullName] = useState(''); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md index 416402e4e7..e3a6deff44 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect/error.invalid-derived-state-from-state-in-effect.expect.md @@ -38,7 +38,7 @@ Found 1 error: Error: Derive values in render, not effects. -This setState() appears to derive a value local state [firstName, lastName]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user. +This setState() appears to derive a value from local state [firstName, lastName]. Derived values should be computed during render, rather than in effects. Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user. error.invalid-derived-state-from-state-in-effect.ts:10:4 8 |