From 56b0bbc286d0e00fcedf5e08b0e151aa07e1a389 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 18 Jun 2025 13:32:54 -0700 Subject: [PATCH] [commit] Improve error for hoisting violations The previous error for hoisting violations pointed only to the variable declaration, but didn't show where the value was accessed before that declaration. We now track where each hoisted variable is first accessed and report two errors, one for the reference and one for the declaration. When we improve our diagnostic infra to support reporting errors at multiple locations we can merge these into a single conceptual error. --- .../Inference/InferMutationAliasingEffects.ts | 100 ++++++++++++------ .../error.invalid-hoisting-setstate.expect.md | 12 ++- ...rozen-hoisted-storecontext-const.expect.md | 16 +-- 3 files changed, 86 insertions(+), 42 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index 521f55026e..6bb078e8fa 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -38,6 +38,7 @@ import { import { eachInstructionValueLValue, eachInstructionValueOperand, + eachTerminalOperand, eachTerminalSuccessor, } from '../HIR/visitors'; import {Ok, Result} from '../Utils/Result'; @@ -221,8 +222,19 @@ export function inferMutationAliasingEffects( return Ok(undefined); } -function findHoistedContextDeclarations(fn: HIRFunction): Set { - const hoisted = new Set(); +function findHoistedContextDeclarations( + fn: HIRFunction, +): Map { + const hoisted = new Map(); + function visit(place: Place): void { + if ( + hoisted.has(place.identifier.declarationId) && + hoisted.get(place.identifier.declarationId) == null + ) { + // If this is the first load of the value, store the location + hoisted.set(place.identifier.declarationId, place); + } + } for (const block of fn.body.blocks.values()) { for (const instr of block.instructions) { if (instr.value.kind === 'DeclareContext') { @@ -232,10 +244,17 @@ function findHoistedContextDeclarations(fn: HIRFunction): Set { kind == InstructionKind.HoistedFunction || kind == InstructionKind.HoistedLet ) { - hoisted.add(instr.value.lvalue.place.identifier.declarationId); + hoisted.set(instr.value.lvalue.place.identifier.declarationId, null); + } + } else { + for (const operand of eachInstructionValueOperand(instr.value)) { + visit(operand); } } } + for (const operand of eachTerminalOperand(block.terminal)) { + visit(operand); + } } return hoisted; } @@ -248,12 +267,12 @@ class Context { catchHandlers: Map = new Map(); isFuctionExpression: boolean; fn: HIRFunction; - hoistedContextDeclarations: Set; + hoistedContextDeclarations: Map; constructor( isFunctionExpression: boolean, fn: HIRFunction, - hoistedContextDeclarations: Set, + hoistedContextDeclarations: Map, ) { this.isFuctionExpression = isFunctionExpression; this.fn = fn; @@ -901,48 +920,69 @@ function applyEffect( console.log(prettyFormat(state.debugAbstractValue(value))); } - let reason: string; - let description: string | null = null; - if ( mutationKind === 'mutate-frozen' && context.hoistedContextDeclarations.has( effect.value.identifier.declarationId, ) ) { - reason = `This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time`; - if ( + const description = effect.value.identifier.name !== null && effect.value.identifier.name.kind === 'named' - ) { - description = `Move the declaration of \`${effect.value.identifier.name.value}\` to before it is first referenced`; + ? `Variable \`${effect.value.identifier.name.value}\` is accessed before it is declared` + : null; + const hoistedAccess = context.hoistedContextDeclarations.get( + effect.value.identifier.declarationId, + ); + if (hoistedAccess != null && hoistedAccess.loc != effect.value.loc) { + effects.push({ + kind: 'MutateFrozen', + place: effect.value, + error: { + severity: ErrorSeverity.InvalidReact, + reason: `This variable is accessed before it is declared, which may prevent it from updating as the assigned value changes over time`, + description, + loc: hoistedAccess.loc, + suggestions: null, + }, + }); } + + effects.push({ + kind: 'MutateFrozen', + place: effect.value, + error: { + severity: ErrorSeverity.InvalidReact, + reason: `This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time`, + description, + loc: effect.value.loc, + suggestions: null, + }, + }); } else { - reason = getWriteErrorReason({ + const reason = getWriteErrorReason({ kind: value.kind, reason: value.reason, context: new Set(), }); - if ( + const description = effect.value.identifier.name !== null && effect.value.identifier.name.kind === 'named' - ) { - description = `Found mutation of \`${effect.value.identifier.name.value}\``; - } + ? `Found mutation of \`${effect.value.identifier.name.value}\`` + : null; + effects.push({ + kind: + value.kind === ValueKind.Frozen ? 'MutateFrozen' : 'MutateGlobal', + place: effect.value, + error: { + severity: ErrorSeverity.InvalidReact, + reason, + description, + loc: effect.value.loc, + suggestions: null, + }, + }); } - - effects.push({ - kind: - value.kind === ValueKind.Frozen ? 'MutateFrozen' : 'MutateGlobal', - place: effect.value, - error: { - severity: ErrorSeverity.InvalidReact, - reason, - description, - loc: effect.value.loc, - suggestions: null, - }, - }); } break; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hoisting-setstate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hoisting-setstate.expect.md index 3fcc84c9a4..1be37ef830 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hoisting-setstate.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hoisting-setstate.expect.md @@ -38,13 +38,15 @@ export const FIXTURE_ENTRYPOINT = { ## Error ``` - 19 | useEffect(() => setState(2), []); + 17 | * $2 = Function context=setState + 18 | */ +> 19 | useEffect(() => setState(2), []); + | ^^^^^^^^ InvalidReact: This variable is accessed before it is declared, which may prevent it from updating as the assigned value changes over time. Variable `setState` is accessed before it is declared (19:19) + +InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Variable `setState` is accessed before it is declared (21:21) 20 | -> 21 | const [state, setState] = useState(0); - | ^^^^^^^^ InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Move the declaration of `setState` to before it is first referenced (21:21) + 21 | const [state, setState] = useState(0); 22 | return ; - 23 | } - 24 | ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-referencing-frozen-hoisted-storecontext-const.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-referencing-frozen-hoisted-storecontext-const.expect.md index 7bf3cd0cd3..a95ace1df5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-referencing-frozen-hoisted-storecontext-const.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-referencing-frozen-hoisted-storecontext-const.expect.md @@ -31,13 +31,15 @@ function Component({content, refetch}) { ## Error ``` - 17 | // This has to error: onRefetch needs to memoize with `content` as a - 18 | // dependency, but the dependency comes later -> 19 | const {data = null} = content; - | ^^^^^^^^^^^ InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Move the declaration of `data` to before it is first referenced (19:19) - 20 | - 21 | return ; - 22 | } + 9 | // TDZ violation! + 10 | const onRefetch = useCallback(() => { +> 11 | refetch(data); + | ^^^^ InvalidReact: This variable is accessed before it is declared, which may prevent it from updating as the assigned value changes over time. Variable `data` is accessed before it is declared (11:11) + +InvalidReact: This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time. Variable `data` is accessed before it is declared (19:19) + 12 | }, [refetch]); + 13 | + 14 | // The context variable gets frozen here since it's passed to a hook ``` \ No newline at end of file