Skip to content

Commit c5a10aa

Browse files
josephsavonajosephsavona
committed
[compiler] Use new diagnostics for core inference errors
Uses the new diagnostic type for errors created during mutation/aliasing inference, such as errors for mutating immutable values like props or state, reassigning globals, etc.
1 parent 5e12e88 commit c5a10aa

File tree

61 files changed

+312
-349
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

61 files changed

+312
-349
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -995,13 +995,13 @@ export function printAliasingEffect(effect: AliasingEffect): string {
995995
return `${effect.kind} ${printPlaceForAliasEffect(effect.value)}`;
996996
}
997997
case 'MutateFrozen': {
998-
return `MutateFrozen ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`;
998+
return `MutateFrozen ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.category)}`;
999999
}
10001000
case 'MutateGlobal': {
1001-
return `MutateGlobal ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`;
1001+
return `MutateGlobal ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.category)}`;
10021002
}
10031003
case 'Impure': {
1004-
return `Impure ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`;
1004+
return `Impure ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.category)}`;
10051005
}
10061006
case 'Render': {
10071007
return `Render ${printPlaceForAliasEffect(effect.place)}`;

‎compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8-
import {CompilerErrorDetailOptions} from '../CompilerError';
8+
import {CompilerDiagnostic} from '../CompilerError';
99
import {
1010
FunctionExpression,
1111
GeneratedSource,
@@ -133,19 +133,19 @@ export type AliasingEffect =
133133
/**
134134
* Mutation of a value known to be immutable
135135
*/
136-
| {kind: 'MutateFrozen'; place: Place; error: CompilerErrorDetailOptions}
136+
| {kind: 'MutateFrozen'; place: Place; error: CompilerDiagnostic}
137137
/**
138138
* Mutation of a global
139139
*/
140140
| {
141141
kind: 'MutateGlobal';
142142
place: Place;
143-
error: CompilerErrorDetailOptions;
143+
error: CompilerDiagnostic;
144144
}
145145
/**
146146
* Indicates a side-effect that is not safe during render
147147
*/
148-
| {kind: 'Impure'; place: Place; error: CompilerErrorDetailOptions}
148+
| {kind: 'Impure'; place: Place; error: CompilerDiagnostic}
149149
/**
150150
* Indicates that a given place is accessed during render. Used to distingush
151151
* hook arguments that are known to be called immediately vs those used for
@@ -211,9 +211,9 @@ export function hashEffect(effect: AliasingEffect): string {
211211
effect.kind,
212212
effect.place.identifier.id,
213213
effect.error.severity,
214-
effect.error.reason,
214+
effect.error.category,
215215
effect.error.description,
216-
printSourceLocation(effect.error.loc ?? GeneratedSource),
216+
printSourceLocation(effect.error.primaryLocation() ?? GeneratedSource),
217217
].join(':');
218218
}
219219
case 'Mutate':

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -326,26 +326,26 @@ function isEffectSafeOutsideRender(effect: FunctionEffect): boolean {
326326

327327
export function getWriteErrorReason(abstractValue: AbstractValue): string {
328328
if (abstractValue.reason.has(ValueReason.Global)) {
329-
return 'Writing to a variable defined outside a component or hook is not allowed. Consider using an effect';
329+
return 'Modifying a variable defined outside a component or hook is not allowed. Consider using an effect';
330330
} else if (abstractValue.reason.has(ValueReason.JsxCaptured)) {
331-
return 'Updating a value used previously in JSX is not allowed. Consider moving the mutation before the JSX';
331+
return 'Modifying a value used previously in JSX is not allowed. Consider moving the modification before the JSX';
332332
} else if (abstractValue.reason.has(ValueReason.Context)) {
333-
return `Mutating a value returned from 'useContext()', which should not be mutated`;
333+
return `Modifying a value returned from 'useContext()' is not allowed.`;
334334
} else if (abstractValue.reason.has(ValueReason.KnownReturnSignature)) {
335-
return 'Mutating a value returned from a function whose return value should not be mutated';
335+
return 'Modifying a value returned from a function whose return value should not be mutated';
336336
} else if (abstractValue.reason.has(ValueReason.ReactiveFunctionArgument)) {
337-
return 'Mutating component props or hook arguments is not allowed. Consider using a local variable instead';
337+
return 'Modifying component props or hook arguments is not allowed. Consider using a local variable instead';
338338
} else if (abstractValue.reason.has(ValueReason.State)) {
339-
return "Mutating a value returned from 'useState()', which should not be mutated. Use the setter function to update instead";
339+
return "Modifying a value returned from 'useState()', which should not be modified directly. Use the setter function to update instead";
340340
} else if (abstractValue.reason.has(ValueReason.ReducerState)) {
341-
return "Mutating a value returned from 'useReducer()', which should not be mutated. Use the dispatch function to update instead";
341+
return "Modifying a value returned from 'useReducer()', which should not be modified directly. Use the dispatch function to update instead";
342342
} else if (abstractValue.reason.has(ValueReason.Effect)) {
343-
return 'Updating a value used previously in an effect function or as an effect dependency is not allowed. Consider moving the mutation before calling useEffect()';
343+
return 'Modifying a value used previously in an effect function or as an effect dependency is not allowed. Consider moving the modification before calling useEffect()';
344344
} else if (abstractValue.reason.has(ValueReason.HookCaptured)) {
345-
return 'Updating a value previously passed as an argument to a hook is not allowed. Consider moving the mutation before calling the hook';
345+
return 'Modifying a value previously passed as an argument to a hook is not allowed. Consider moving the modification before calling the hook';
346346
} else if (abstractValue.reason.has(ValueReason.HookReturn)) {
347-
return 'Updating a value returned from a hook is not allowed. Consider moving the mutation into the hook where the value is constructed';
347+
return 'Modifying a value returned from a hook is not allowed. Consider moving the modification into the hook where the value is constructed';
348348
} else {
349-
return 'This mutates a variable that React considers immutable';
349+
return 'This modifies a variable that React considers immutable';
350350
}
351351
}

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts

Lines changed: 65 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
*/
77

88
import {
9+
CompilerDiagnostic,
910
CompilerError,
1011
Effect,
1112
ErrorSeverity,
@@ -446,20 +447,24 @@ function applySignature(
446447
reason: value.reason,
447448
context: new Set(),
448449
});
450+
const message =
451+
effect.value.identifier.name !== null &&
452+
effect.value.identifier.name.kind === 'named'
453+
? `\`${effect.value.identifier.name.value}\` cannot be modified`
454+
: 'This value cannot be modified';
449455
effects.push({
450456
kind: 'MutateFrozen',
451457
place: effect.value,
452-
error: {
458+
error: CompilerDiagnostic.create({
453459
severity: ErrorSeverity.InvalidReact,
454-
reason,
455-
description:
456-
effect.value.identifier.name !== null &&
457-
effect.value.identifier.name.kind === 'named'
458-
? `Found mutation of \`${effect.value.identifier.name.value}\``
459-
: null,
460-
loc: effect.value.loc,
460+
category: 'This value cannot be modified',
461+
description: reason,
461462
suggestions: null,
462-
},
463+
}).withDetail({
464+
kind: 'error',
465+
loc: effect.value.loc,
466+
message,
467+
}),
463468
});
464469
}
465470
}
@@ -1016,44 +1021,36 @@ function applyEffect(
10161021
const description =
10171022
effect.value.identifier.name !== null &&
10181023
effect.value.identifier.name.kind === 'named'
1019-
? `Variable \`${effect.value.identifier.name.value}\` is accessed before it is declared`
1020-
: null;
1024+
? `Variable \`${effect.value.identifier.name.value}\``
1025+
: 'This variable';
10211026
const hoistedAccess = context.hoistedContextDeclarations.get(
10221027
effect.value.identifier.declarationId,
10231028
);
1029+
const diagnostic = CompilerDiagnostic.create({
1030+
severity: ErrorSeverity.InvalidReact,
1031+
category: 'Cannot access variable before it is declared',
1032+
description: `${description} is accessed before it is declared, which prevents the earlier access from updating when this value changes over time`,
1033+
});
10241034
if (hoistedAccess != null && hoistedAccess.loc != effect.value.loc) {
1025-
applyEffect(
1026-
context,
1027-
state,
1028-
{
1029-
kind: 'MutateFrozen',
1030-
place: effect.value,
1031-
error: {
1032-
severity: ErrorSeverity.InvalidReact,
1033-
reason: `This variable is accessed before it is declared, which may prevent it from updating as the assigned value changes over time`,
1034-
description,
1035-
loc: hoistedAccess.loc,
1036-
suggestions: null,
1037-
},
1038-
},
1039-
initialized,
1040-
effects,
1041-
);
1035+
diagnostic.withDetail({
1036+
kind: 'error',
1037+
loc: hoistedAccess.loc,
1038+
message: 'Variable accessed before it is declared',
1039+
});
10421040
}
1041+
diagnostic.withDetail({
1042+
kind: 'error',
1043+
loc: effect.value.loc,
1044+
message: 'The variable is declared here',
1045+
});
10431046

10441047
applyEffect(
10451048
context,
10461049
state,
10471050
{
10481051
kind: 'MutateFrozen',
10491052
place: effect.value,
1050-
error: {
1051-
severity: ErrorSeverity.InvalidReact,
1052-
reason: `This variable is accessed before it is declared, which prevents the earlier access from updating when this value changes over time`,
1053-
description,
1054-
loc: effect.value.loc,
1055-
suggestions: null,
1056-
},
1053+
error: diagnostic,
10571054
},
10581055
initialized,
10591056
effects,
@@ -1064,11 +1061,11 @@ function applyEffect(
10641061
reason: value.reason,
10651062
context: new Set(),
10661063
});
1067-
const description =
1064+
const message =
10681065
effect.value.identifier.name !== null &&
10691066
effect.value.identifier.name.kind === 'named'
1070-
? `Found mutation of \`${effect.value.identifier.name.value}\``
1071-
: null;
1067+
? `\`${effect.value.identifier.name.value}\` cannot be modified`
1068+
: 'This value cannot be modified';
10721069
applyEffect(
10731070
context,
10741071
state,
@@ -1078,13 +1075,15 @@ function applyEffect(
10781075
? 'MutateFrozen'
10791076
: 'MutateGlobal',
10801077
place: effect.value,
1081-
error: {
1078+
error: CompilerDiagnostic.create({
10821079
severity: ErrorSeverity.InvalidReact,
1083-
reason,
1084-
description,
1080+
category: 'This value cannot be modified',
1081+
description: reason,
1082+
}).withDetail({
1083+
kind: 'error',
10851084
loc: effect.value.loc,
1086-
suggestions: null,
1087-
},
1085+
message,
1086+
}),
10881087
},
10891088
initialized,
10901089
effects,
@@ -1991,13 +1990,18 @@ function computeSignatureForInstruction(
19911990
effects.push({
19921991
kind: 'MutateGlobal',
19931992
place: value.value,
1994-
error: {
1995-
reason:
1996-
'Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)',
1997-
loc: instr.loc,
1998-
suggestions: null,
1993+
error: CompilerDiagnostic.create({
19991994
severity: ErrorSeverity.InvalidReact,
2000-
},
1995+
category:
1996+
'Cannot reassign variables declared outside of the component/hook',
1997+
description:
1998+
'Reassigning a variable declared outside of the component/hook is a form of side effect, which can cause unpredictable behavior depending on when the component happens to re-render. If this variable is used in rendering, use useState instead. Otherwise, consider updating it in an effect (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)',
1999+
suggestions: null,
2000+
}).withDetail({
2001+
kind: 'error',
2002+
loc: instr.loc,
2003+
message: 'Cannot reassign variable',
2004+
}),
20012005
});
20022006
effects.push({kind: 'Assign', from: value.value, into: lvalue});
20032007
break;
@@ -2087,17 +2091,20 @@ function computeEffectsForLegacySignature(
20872091
effects.push({
20882092
kind: 'Impure',
20892093
place: receiver,
2090-
error: {
2091-
reason:
2092-
'Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)',
2093-
description:
2094-
signature.canonicalName != null
2095-
? `\`${signature.canonicalName}\` is an impure function whose results may change on every call`
2096-
: null,
2094+
error: CompilerDiagnostic.create({
20972095
severity: ErrorSeverity.InvalidReact,
2098-
loc,
2096+
category: 'Cannot call impure function during render',
2097+
description:
2098+
(signature.canonicalName != null
2099+
? `\`${signature.canonicalName}\` is an impure function. `
2100+
: '') +
2101+
'Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)',
20992102
suggestions: null,
2100-
},
2103+
}).withDetail({
2104+
kind: 'error',
2105+
loc,
2106+
message: 'Cannot call impure function',
2107+
}),
21012108
});
21022109
}
21032110
const stores: Array<Place> = [];

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ export function inferMutationAliasingRanges(
195195
effect.kind === 'MutateGlobal' ||
196196
effect.kind === 'Impure'
197197
) {
198-
errors.push(effect.error);
198+
errors.pushDiagnostic(effect.error);
199199
functionEffects.push(effect);
200200
} else if (effect.kind === 'Render') {
201201
renders.push({index: index++, place: effect.place});
@@ -549,7 +549,7 @@ function appendFunctionErrors(errors: CompilerError, fn: HIRFunction): void {
549549
case 'Impure':
550550
case 'MutateFrozen':
551551
case 'MutateGlobal': {
552-
errors.push(effect.error);
552+
errors.pushDiagnostic(effect.error);
553553
break;
554554
}
555555
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.expect.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,18 @@ function Component() {
1616

1717
```
1818
Found 1 error:
19-
Error: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)
19+
Error: Cannot reassign variables declared outside of the component/hook
20+
21+
Reassigning a variable declared outside of the component/hook is a form of side effect, which can cause unpredictable behavior depending on when the component happens to re-render. If this variable is used in rendering, use useState instead. Otherwise, consider updating it in an effect (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)
2022
2123
error.assign-global-in-component-tag-function.ts:3:4
2224
1 | function Component() {
2325
2 | const Foo = () => {
2426
> 3 | someGlobal = true;
25-
| ^^^^^^^^^^ Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)
27+
| ^^^^^^^^^^ Cannot reassign variable
2628
4 | };
2729
5 | return <Foo />;
2830
6 | }
29-
30-
3131
```
3232
3333

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.expect.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,18 @@ function Component() {
1919

2020
```
2121
Found 1 error:
22-
Error: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)
22+
Error: Cannot reassign variables declared outside of the component/hook
23+
24+
Reassigning a variable declared outside of the component/hook is a form of side effect, which can cause unpredictable behavior depending on when the component happens to re-render. If this variable is used in rendering, use useState instead. Otherwise, consider updating it in an effect (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)
2325
2426
error.assign-global-in-jsx-children.ts:3:4
2527
1 | function Component() {
2628
2 | const foo = () => {
2729
> 3 | someGlobal = true;
28-
| ^^^^^^^^^^ Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)
30+
| ^^^^^^^^^^ Cannot reassign variable
2931
4 | };
3032
5 | // Children are generally access/called during render, so
3133
6 | // modifying a global in a children function is almost
32-
33-
3434
```
3535
3636

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,18 +30,18 @@ export const FIXTURE_ENTRYPOINT = {
3030

3131
```
3232
Found 1 error:
33-
Error: Updating a value previously passed as an argument to a hook is not allowed. Consider moving the mutation before calling the hook
33+
Error: This value cannot be modified
34+
35+
Modifying a value previously passed as an argument to a hook is not allowed. Consider moving the modification before calling the hook
3436
3537
error.hook-call-freezes-captured-identifier.ts:13:2
3638
11 | });
3739
12 |
3840
> 13 | x.value += count;
39-
| ^ Updating a value previously passed as an argument to a hook is not allowed. Consider moving the mutation before calling the hook
41+
| ^ This value cannot be modified
4042
14 | return <Stringify x={x} cb={cb} />;
4143
15 | }
4244
16 |
43-
44-
4545
```
4646
4747

0 commit comments

Comments
 (0)