Facebook/react
1
0
Fork 0
mirror of https://github.com/facebook/react.git synced 2025-11-01 09:12:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

[commit] Improve error for hoisting violations

Browse Source 
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.
This commit is contained in:
Joe Savona
2025-06-18 13:32:54 -07:00
parent b067c6fe79
commit 56b0bbc286
3 changed files with 86 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts
+70 -30
View File
@@ -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<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') {
@@ -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;
}
@@ -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;
@@ -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;
}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-hoisting-setstate.expect.md
+7 -5
View File
@@ -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 |
```
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-referencing-frozen-hoisted-storecontext-const.expect.md
+9 -7
View File
@@ -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 <Foo data={data} onSubmit={onSubmit} />;
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
```