From 8d6d9e4b0fbbb8765c58c30586040bc0ef007ba9 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 18 Jun 2025 15:09:32 -0700 Subject: [PATCH 1/2] [compiler] update fixtures --- ...map-named-callback-cross-context.expect.md | 81 +++++++++---------- ...map-named-callback-cross-context.expect.md | 81 +++++++++---------- .../shared-hook-calls.expect.md | 77 ++++++++---------- .../shared-hook-calls.expect.md | 77 ++++++++---------- 4 files changed, 140 insertions(+), 176 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-function/nullable-objects/array-map-named-callback-cross-context.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-function/nullable-objects/array-map-named-callback-cross-context.expect.md index d24536d616a4..c1a6dfb3eae1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-function/nullable-objects/array-map-named-callback-cross-context.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/inner-function/nullable-objects/array-map-named-callback-cross-context.expect.md @@ -57,67 +57,62 @@ import { Stringify } from "shared-runtime"; * - cb1 is not assumed to be called since it's only used as a call operand */ function useFoo(t0) { - const $ = _c(14); - let arr1; - let arr2; + const $ = _c(13); + const { arr1, arr2 } = t0; let t1; - if ($[0] !== t0) { - ({ arr1, arr2 } = t0); - let t2; - if ($[4] !== arr1[0]) { - t2 = (e) => arr1[0].value + e.value; - $[4] = arr1[0]; - $[5] = t2; - } else { - t2 = $[5]; - } - const cb1 = t2; - t1 = () => arr1.map(cb1); - $[0] = t0; - $[1] = arr1; - $[2] = arr2; - $[3] = t1; + if ($[0] !== arr1[0]) { + t1 = (e) => arr1[0].value + e.value; + $[0] = arr1[0]; + $[1] = t1; } else { - arr1 = $[1]; - arr2 = $[2]; - t1 = $[3]; + t1 = $[1]; } - const getArrMap1 = t1; + const cb1 = t1; let t2; - if ($[6] !== arr2) { - t2 = (e_0) => arr2[0].value + e_0.value; - $[6] = arr2; - $[7] = t2; + if ($[2] !== arr1 || $[3] !== cb1) { + t2 = () => arr1.map(cb1); + $[2] = arr1; + $[3] = cb1; + $[4] = t2; } else { - t2 = $[7]; + t2 = $[4]; } - const cb2 = t2; + const getArrMap1 = t2; let t3; - if ($[8] !== arr1 || $[9] !== cb2) { - t3 = () => arr1.map(cb2); - $[8] = arr1; - $[9] = cb2; - $[10] = t3; + if ($[5] !== arr2) { + t3 = (e_0) => arr2[0].value + e_0.value; + $[5] = arr2; + $[6] = t3; } else { - t3 = $[10]; + t3 = $[6]; } - const getArrMap2 = t3; + const cb2 = t3; let t4; - if ($[11] !== getArrMap1 || $[12] !== getArrMap2) { - t4 = ( + if ($[7] !== arr1 || $[8] !== cb2) { + t4 = () => arr1.map(cb2); + $[7] = arr1; + $[8] = cb2; + $[9] = t4; + } else { + t4 = $[9]; + } + const getArrMap2 = t4; + let t5; + if ($[10] !== getArrMap1 || $[11] !== getArrMap2) { + t5 = ( ); - $[11] = getArrMap1; - $[12] = getArrMap2; - $[13] = t4; + $[10] = getArrMap1; + $[11] = getArrMap2; + $[12] = t5; } else { - t4 = $[13]; + t5 = $[12]; } - return t4; + return t5; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md index a36b862052ab..7bc2c193cf7c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md @@ -58,67 +58,62 @@ import { Stringify } from "shared-runtime"; * - cb1 is not assumed to be called since it's only used as a call operand */ function useFoo(t0) { - const $ = _c(14); - let arr1; - let arr2; + const $ = _c(13); + const { arr1, arr2 } = t0; let t1; - if ($[0] !== t0) { - ({ arr1, arr2 } = t0); - let t2; - if ($[4] !== arr1[0]) { - t2 = (e) => arr1[0].value + e.value; - $[4] = arr1[0]; - $[5] = t2; - } else { - t2 = $[5]; - } - const cb1 = t2; - t1 = () => arr1.map(cb1); - $[0] = t0; - $[1] = arr1; - $[2] = arr2; - $[3] = t1; + if ($[0] !== arr1[0]) { + t1 = (e) => arr1[0].value + e.value; + $[0] = arr1[0]; + $[1] = t1; } else { - arr1 = $[1]; - arr2 = $[2]; - t1 = $[3]; + t1 = $[1]; } - const getArrMap1 = t1; + const cb1 = t1; let t2; - if ($[6] !== arr2) { - t2 = (e_0) => arr2[0].value + e_0.value; - $[6] = arr2; - $[7] = t2; + if ($[2] !== arr1 || $[3] !== cb1) { + t2 = () => arr1.map(cb1); + $[2] = arr1; + $[3] = cb1; + $[4] = t2; } else { - t2 = $[7]; + t2 = $[4]; } - const cb2 = t2; + const getArrMap1 = t2; let t3; - if ($[8] !== arr1 || $[9] !== cb2) { - t3 = () => arr1.map(cb2); - $[8] = arr1; - $[9] = cb2; - $[10] = t3; + if ($[5] !== arr2) { + t3 = (e_0) => arr2[0].value + e_0.value; + $[5] = arr2; + $[6] = t3; } else { - t3 = $[10]; + t3 = $[6]; } - const getArrMap2 = t3; + const cb2 = t3; let t4; - if ($[11] !== getArrMap1 || $[12] !== getArrMap2) { - t4 = ( + if ($[7] !== arr1 || $[8] !== cb2) { + t4 = () => arr1.map(cb2); + $[7] = arr1; + $[8] = cb2; + $[9] = t4; + } else { + t4 = $[9]; + } + const getArrMap2 = t4; + let t5; + if ($[10] !== getArrMap1 || $[11] !== getArrMap2) { + t5 = ( ); - $[11] = getArrMap1; - $[12] = getArrMap2; - $[13] = t4; + $[10] = getArrMap1; + $[11] = getArrMap2; + $[12] = t5; } else { - t4 = $[13]; + t5 = $[12]; } - return t4; + return t5; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/shared-hook-calls.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/shared-hook-calls.expect.md index 3f361c201904..39bd61aaf34a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/shared-hook-calls.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/shared-hook-calls.expect.md @@ -30,60 +30,47 @@ import { c as _c, useFire } from "react/compiler-runtime"; // @enableFire @enabl import { fire } from "react"; function Component(t0) { - const $ = _c(13); - let bar; - let baz; - let foo; - if ($[0] !== t0) { - ({ bar, baz } = t0); - let t1; - if ($[4] !== bar) { - t1 = () => { - console.log(bar); - }; - $[4] = bar; - $[5] = t1; - } else { - t1 = $[5]; - } - foo = t1; - $[0] = t0; - $[1] = bar; - $[2] = baz; - $[3] = foo; - } else { - bar = $[1]; - baz = $[2]; - foo = $[3]; - } - const t1 = useFire(foo); - const t2 = useFire(baz); - let t3; - if ($[6] !== bar || $[7] !== t1 || $[8] !== t2) { - t3 = () => { - t1(bar); - t2(bar); + const $ = _c(9); + const { bar, baz } = t0; + let t1; + if ($[0] !== bar) { + t1 = () => { + console.log(bar); }; - $[6] = bar; - $[7] = t1; - $[8] = t2; - $[9] = t3; + $[0] = bar; + $[1] = t1; } else { - t3 = $[9]; + t1 = $[1]; } - useEffect(t3); + const foo = t1; + const t2 = useFire(foo); + const t3 = useFire(baz); let t4; - if ($[10] !== bar || $[11] !== t1) { + if ($[2] !== bar || $[3] !== t2 || $[4] !== t3) { t4 = () => { - t1(bar); + t2(bar); + t3(bar); }; - $[10] = bar; - $[11] = t1; - $[12] = t4; + $[2] = bar; + $[3] = t2; + $[4] = t3; + $[5] = t4; } else { - t4 = $[12]; + t4 = $[5]; } useEffect(t4); + let t5; + if ($[6] !== bar || $[7] !== t2) { + t5 = () => { + t2(bar); + }; + $[6] = bar; + $[7] = t2; + $[8] = t5; + } else { + t5 = $[8]; + } + useEffect(t5); return null; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/shared-hook-calls.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/shared-hook-calls.expect.md index d98b3e03ab92..92dbf9843ad6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/shared-hook-calls.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/shared-hook-calls.expect.md @@ -30,60 +30,47 @@ import { c as _c, useFire } from "react/compiler-runtime"; // @enableFire import { fire } from "react"; function Component(t0) { - const $ = _c(13); - let bar; - let baz; - let foo; - if ($[0] !== t0) { - ({ bar, baz } = t0); - let t1; - if ($[4] !== bar) { - t1 = () => { - console.log(bar); - }; - $[4] = bar; - $[5] = t1; - } else { - t1 = $[5]; - } - foo = t1; - $[0] = t0; - $[1] = bar; - $[2] = baz; - $[3] = foo; - } else { - bar = $[1]; - baz = $[2]; - foo = $[3]; - } - const t1 = useFire(foo); - const t2 = useFire(baz); - let t3; - if ($[6] !== bar || $[7] !== t1 || $[8] !== t2) { - t3 = () => { - t1(bar); - t2(bar); + const $ = _c(9); + const { bar, baz } = t0; + let t1; + if ($[0] !== bar) { + t1 = () => { + console.log(bar); }; - $[6] = bar; - $[7] = t1; - $[8] = t2; - $[9] = t3; + $[0] = bar; + $[1] = t1; } else { - t3 = $[9]; + t1 = $[1]; } - useEffect(t3); + const foo = t1; + const t2 = useFire(foo); + const t3 = useFire(baz); let t4; - if ($[10] !== bar || $[11] !== t1) { + if ($[2] !== bar || $[3] !== t2 || $[4] !== t3) { t4 = () => { - t1(bar); + t2(bar); + t3(bar); }; - $[10] = bar; - $[11] = t1; - $[12] = t4; + $[2] = bar; + $[3] = t2; + $[4] = t3; + $[5] = t4; } else { - t4 = $[12]; + t4 = $[5]; } useEffect(t4); + let t5; + if ($[6] !== bar || $[7] !== t2) { + t5 = () => { + t2(bar); + }; + $[6] = bar; + $[7] = t2; + $[8] = t5; + } else { + t5 = $[8]; + } + useEffect(t5); return null; } From 1fa18f62b2b9a9f5c2960662f09fb2e96789b1b9 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 18 Jun 2025 13:32:54 -0700 Subject: [PATCH 2/2] [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 521f55026ec7..6bb078e8fa50 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 3fcc84c9a48d..1be37ef83098 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 7bf3cd0cd3b3..a95ace1df5dd 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