From b98bf118e2a514cdd0519d5e211eeb5a33585050 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes Acosta Date: Thu, 28 Aug 2025 10:16:19 -0700 Subject: [PATCH] Add catching useStates that shadow a reactive value --- .../ValidateNoDerivedComputationsInEffects.ts | 124 ++++++++++++------ 1 file changed, 83 insertions(+), 41 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 eca16f44f2..20893b5e54 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoDerivedComputationsInEffects.ts @@ -41,7 +41,7 @@ type TypeOfValue = 'ignored' | 'fromProps' | 'fromState' | 'fromPropsOrState'; type DerivationMetadata = { typeOfValue: TypeOfValue; place: Place; - sources: Set; + sources: Array; }; type ErrorMetadata = { @@ -49,6 +49,7 @@ type ErrorMetadata = { description: string | undefined; loc: SourceLocation; setStateName: string | undefined | null; + derivedDepsNames: Array; }; /** @@ -79,6 +80,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const functions: Map = new Map(); const locals: Map = new Map(); const derivationCache: Map = new Map(); + const shadowingUseState: Map> = new Map(); const effectSetStates: Map< string | undefined | null, @@ -93,7 +95,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { if (param.kind === 'Identifier') { derivationCache.set(param.identifier.id, { place: param, - sources: new Set([param]), + sources: [param], typeOfValue: 'fromProps', }); } @@ -103,7 +105,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { if (props != null && props.kind === 'Identifier') { derivationCache.set(props.identifier.id, { place: props, - sources: new Set([props]), + sources: [props], typeOfValue: 'fromProps', }); } @@ -115,7 +117,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { for (const instr of block.instructions) { const {lvalue, value} = instr; - parseInstr(instr, derivationCache, setStateCalls); + parseInstr(instr, derivationCache, setStateCalls, shadowingUseState); if (value.kind === 'LoadLocal') { locals.set(lvalue.identifier.id, value.place.identifier.id); @@ -167,6 +169,7 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { const compilerError = generateCompilerError( setStateCalls, effectSetStates, + shadowingUseState, errors, ); @@ -178,21 +181,14 @@ export function validateNoDerivedComputationsInEffects(fn: HIRFunction): void { function generateCompilerError( setStateCalls: Map>, effectSetStates: Map>, + shadowingUseState: Map>, errors: Array, ): CompilerError { const throwableErrors = new CompilerError(); for (const error of errors) { let compilerDiagnostic: CompilerDiagnostic | undefined = undefined; - let detailMessage = ''; - switch (error.type) { - case 'fromProps': - detailMessage = 'This state value shadows a value passed as a prop.'; - break; - case 'fromPropsOrState': - detailMessage = - 'This state value shadows a value passed as a prop or a value from state.'; - break; - } + console.log('ERROR: ', error); + console.log('ERROR: ', shadowingUseState); /* * If we use a setState from an invalid useEffect elsewhere then we probably have to @@ -207,7 +203,21 @@ function generateCompilerError( description: `${error.description} This state value shadows a value passed as a prop. Instead of shadowing the prop with local state, hoist the state to the parent component and update it there.`, category: `Local state shadows parent state.`, severity: ErrorSeverity.InvalidReact, - }).withDetail({ + }); + + for (const derivedDep of error.derivedDepsNames) { + if (shadowingUseState.has(derivedDep)) { + for (const loc of shadowingUseState.get(derivedDep)!) { + compilerDiagnostic.withDetail({ + kind: 'error', + loc: loc, + message: 'this useState shadows a reactive value', + }); + } + } + } + + compilerDiagnostic.withDetail({ kind: 'error', loc: error.loc, message: 'this setState synchronizes the state', @@ -270,7 +280,7 @@ function updateDerivationMetadata( ): void { let newValue: DerivationMetadata = { place: target, - sources: new Set(), + sources: [], typeOfValue: typeOfValue ?? 'ignored', }; @@ -285,9 +295,9 @@ function updateDerivationMetadata( place.identifier.name === null || place.identifier.name?.kind === 'promoted' ) { - newValue.sources.add(target); + newValue.sources.push(target); } else { - newValue.sources.add(place); + newValue.sources.push(place); } } } @@ -300,37 +310,21 @@ function parseInstr( instr: Instruction, derivationCache: Map, setStateCalls: Map>, + shadowingUseState: 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, derivationCache, setStateCalls); + parseInstr(instr, derivationCache, setStateCalls, shadowingUseState); } } } let typeOfValue: TypeOfValue = 'ignored'; - // Catch any useState hook calls let sources: Array = []; - if ( - instr.value.kind === 'Destructure' && - instr.value.lvalue.pattern.kind === 'ArrayPattern' && - isUseStateType(instr.value.value.identifier) - ) { - typeOfValue = 'fromState'; - - const stateValueSource = instr.value.lvalue.pattern.items[0]; - if (stateValueSource.kind === 'Identifier') { - sources.push({ - place: stateValueSource, - typeOfValue: typeOfValue, - sources: new Set([stateValueSource]), - }); - } - } - + // Catch setState calls if ( instr.value.kind === 'CallExpression' && isSetStateType(instr.value.callee.identifier) && @@ -357,6 +351,49 @@ function parseInstr( typeOfValue = joinValue(typeOfValue, opSource.typeOfValue); sources.push(opSource); + + if ( + instr.value.kind === 'Destructure' && + instr.value.lvalue.pattern.kind === 'ArrayPattern' && + isUseStateType(instr.value.value.identifier) && + opSource.typeOfValue === 'fromProps' + ) { + opSource.sources.forEach(source => { + if (instr.value.kind !== 'Destructure') { + return; + } + + if (source.identifier.name !== null) { + if (shadowingUseState.has(source.identifier.name.value)) { + shadowingUseState + .get(source.identifier.name.value) + ?.push(instr.value.value.loc); + } else { + shadowingUseState.set(source.identifier.name.value, [ + instr.value.value.loc, + ]); + } + } + }); + } + } + + // Catch useState hook calls + if ( + instr.value.kind === 'Destructure' && + instr.value.lvalue.pattern.kind === 'ArrayPattern' && + isUseStateType(instr.value.value.identifier) + ) { + const stateValueSource = instr.value.lvalue.pattern.items[0]; + if (stateValueSource.kind === 'Identifier') { + sources.push({ + place: stateValueSource, + typeOfValue: typeOfValue, + sources: [stateValueSource], + }); + } + + typeOfValue = joinValue(typeOfValue, 'fromState'); } if (typeOfValue !== 'ignored') { @@ -523,7 +560,7 @@ function validateEffect( } for (const call of derivedSetStateCall) { - const placeNames = Array.from(call.derivedDep.sources) + const derivedDepsStr = Array.from(call.derivedDep.sources) .map(place => { return place.identifier.name?.value; }) @@ -533,11 +570,11 @@ function validateEffect( let errorDescription = ''; if (call.derivedDep.typeOfValue === 'fromProps') { - errorDescription = `props [${placeNames}].`; + errorDescription = `props [${derivedDepsStr}].`; } else if (call.derivedDep.typeOfValue === 'fromState') { - errorDescription = `local state [${placeNames}].`; + errorDescription = `local state [${derivedDepsStr}].`; } else { - errorDescription = `both props and local state [${placeNames}].`; + errorDescription = `both props and local state [${derivedDepsStr}].`; } errors.push({ @@ -546,6 +583,11 @@ function validateEffect( loc: call.loc, setStateName: call.loc !== GeneratedSource ? call.loc.identifierName : undefined, + derivedDepsNames: Array.from(call.derivedDep.sources) + .map(place => { + return place.identifier.name?.value ?? ''; + }) + .filter(Boolean), }); } }