Skip to content

[commit] Improve error for hoisting violations #33514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 18, 2025
Merged

[commit] Improve error for hoisting violations #33514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d6d9e4
[compiler] update fixtures
josephsavona Jun 18, 2025
1fa18f6
[commit] Improve error for hoisting violations
josephsavona Jun 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 70 additions & 30 deletions compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
import {
eachInstructionValueLValue,
eachInstructionValueOperand,
eachTerminalOperand,
eachTerminalSuccessor,
} from '../HIR/visitors';
import {Ok, Result} from '../Utils/Result';
Expand Down Expand Up @@ -221,8 +222,19 @@ export function inferMutationAliasingEffects(
return Ok(undefined);
}

function findHoistedContextDeclarations(fn: HIRFunction): Set<DeclarationId> {
const hoisted = new Set<DeclarationId>();
function findHoistedContextDeclarations(
fn: HIRFunction,
): Map<DeclarationId, Place | null> {
const hoisted = new Map<DeclarationId, Place | null>();
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') {
Expand All @@ -232,10 +244,17 @@ function findHoistedContextDeclarations(fn: HIRFunction): Set<DeclarationId> {
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;
}
Expand All @@ -248,12 +267,12 @@ class Context {
catchHandlers: Map<BlockId, Place> = new Map();
isFuctionExpression: boolean;
fn: HIRFunction;
hoistedContextDeclarations: Set<DeclarationId>;
hoistedContextDeclarations: Map<DeclarationId, Place | null>;

constructor(
isFunctionExpression: boolean,
fn: HIRFunction,
hoistedContextDeclarations: Set<DeclarationId>,
hoistedContextDeclarations: Map<DeclarationId, Place | null>,
) {
this.isFuctionExpression = isFunctionExpression;
this.fn = fn;
Expand Down Expand Up @@ -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;
}
Expand Down
12 changes: 7 additions & 5 deletions ...piler/src/__tests__/fixtures/compiler/error.invalid-hoisting-setstate.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <Stringify state={state} />;
23 | }
24 |
```


81 changes: 38 additions & 43 deletions ...nner-function/nullable-objects/array-map-named-callback-cross-context.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
<Stringify
getArrMap1={getArrMap1}
getArrMap2={getArrMap2}
shouldInvokeFns={true}
/>
);
$[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 = {
Expand Down
81 changes: 38 additions & 43 deletions ...xtures/compiler/new-mutability/array-map-named-callback-cross-context.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
<Stringify
getArrMap1={getArrMap1}
getArrMap2={getArrMap2}
shouldInvokeFns={true}
/>
);
$[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 = {
Expand Down
Loading
Loading