Skip to content

Commit 29b2c29

Browse files
josephsavonajosephsavona
committed
[compiler] Cleanup diagnostic messages
Minor sytlistic cleanup
1 parent c5a10aa commit 29b2c29

File tree

317 files changed

+640
-684
lines changed

Some content is hidden

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

317 files changed

+640
-684
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,9 @@ export default function BabelPluginReactCompiler(
8484
}
8585
} catch (e) {
8686
if (e instanceof CompilerError) {
87-
throw new Error(e.printErrorMessage(pass.file.code));
87+
throw new Error(
88+
e.printErrorMessage(pass.file.code, {eslint: false}),
89+
);
8890
}
8991
throw e;
9092
}

‎compiler/packages/babel-plugin-react-compiler/src/CompilerError.ts

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

8+
import * as t from '@babel/types';
89
import {codeFrameColumns} from '@babel/code-frame';
910
import type {SourceLocation} from './HIR';
1011
import {Err, Ok, Result} from './Utils/Result';
@@ -93,6 +94,14 @@ export type CompilerErrorDetailOptions = {
9394
suggestions?: Array<CompilerSuggestion> | null | undefined;
9495
};
9596

97+
export type PrintErrorMessageOptions = {
98+
/**
99+
* ESLint uses 1-indexed columns and prints one error at a time
100+
* So it doesn't require the "Found # error(s)" text
101+
*/
102+
eslint: boolean;
103+
};
104+
96105
export class CompilerDiagnostic {
97106
options: CompilerDiagnosticOptions;
98107

@@ -128,7 +137,7 @@ export class CompilerDiagnostic {
128137
return this.options.details.filter(d => d.kind === 'error')[0]?.loc ?? null;
129138
}
130139

131-
printErrorMessage(source: string): string {
140+
printErrorMessage(source: string, options: PrintErrorMessageOptions): string {
132141
const buffer = [
133142
printErrorSummary(this.severity, this.category),
134143
'\n\n',
@@ -143,28 +152,18 @@ export class CompilerDiagnostic {
143152
}
144153
let codeFrame: string;
145154
try {
146-
codeFrame = codeFrameColumns(
147-
source,
148-
{
149-
start: {
150-
line: loc.start.line,
151-
column: loc.start.column + 1,
152-
},
153-
end: {
154-
line: loc.end.line,
155-
column: loc.end.column + 1,
156-
},
157-
},
158-
{
159-
message: detail.message,
160-
},
161-
);
155+
codeFrame = printCodeFrame(source, loc, detail.message);
162156
} catch (e) {
163157
codeFrame = detail.message;
164158
}
165-
buffer.push(
166-
`\n\n${loc.filename}:${loc.start.line}:${loc.start.column}\n`,
167-
);
159+
buffer.push('\n\n');
160+
if (loc.filename != null) {
161+
const line = loc.start.line;
162+
const column = options.eslint
163+
? loc.start.column + 1
164+
: loc.start.column;
165+
buffer.push(`${loc.filename}:${line}:${column}\n`);
166+
}
168167
buffer.push(codeFrame);
169168
break;
170169
}
@@ -223,7 +222,7 @@ export class CompilerErrorDetail {
223222
return this.loc;
224223
}
225224

226-
printErrorMessage(source: string): string {
225+
printErrorMessage(source: string, options: PrintErrorMessageOptions): string {
227226
const buffer = [printErrorSummary(this.severity, this.reason)];
228227
if (this.description != null) {
229228
buffer.push(`\n\n${this.description}.`);
@@ -232,28 +231,16 @@ export class CompilerErrorDetail {
232231
if (loc != null && typeof loc !== 'symbol') {
233232
let codeFrame: string;
234233
try {
235-
codeFrame = codeFrameColumns(
236-
source,
237-
{
238-
start: {
239-
line: loc.start.line,
240-
column: loc.start.column + 1,
241-
},
242-
end: {
243-
line: loc.end.line,
244-
column: loc.end.column + 1,
245-
},
246-
},
247-
{
248-
message: this.reason,
249-
},
250-
);
234+
codeFrame = printCodeFrame(source, loc, this.reason);
251235
} catch (e) {
252236
codeFrame = '';
253237
}
254-
buffer.push(
255-
`\n\n${loc.filename}:${loc.start.line}:${loc.start.column}\n`,
256-
);
238+
buffer.push(`\n\n`);
239+
if (loc.filename != null) {
240+
const line = loc.start.line;
241+
const column = options.eslint ? loc.start.column + 1 : loc.start.column;
242+
buffer.push(`${loc.filename}:${line}:${column}\n`);
243+
}
257244
buffer.push(codeFrame);
258245
buffer.push('\n\n');
259246
}
@@ -372,10 +359,15 @@ export class CompilerError extends Error {
372359
return this.name;
373360
}
374361

375-
printErrorMessage(source: string): string {
362+
printErrorMessage(source: string, options: PrintErrorMessageOptions): string {
363+
if (options.eslint && this.details.length === 1) {
364+
return this.details[0].printErrorMessage(source, options);
365+
}
376366
return (
377-
`Found ${this.details.length} error${this.details.length === 1 ? '' : 's'}:\n` +
378-
this.details.map(detail => detail.printErrorMessage(source)).join('\n')
367+
`Found ${this.details.length} error${this.details.length === 1 ? '' : 's'}:\n\n` +
368+
this.details
369+
.map(detail => detail.printErrorMessage(source, options).trim())
370+
.join('\n\n')
379371
);
380372
}
381373

@@ -438,6 +430,29 @@ export class CompilerError extends Error {
438430
}
439431
}
440432

433+
function printCodeFrame(
434+
source: string,
435+
loc: t.SourceLocation,
436+
message: string,
437+
): string {
438+
return codeFrameColumns(
439+
source,
440+
{
441+
start: {
442+
line: loc.start.line,
443+
column: loc.start.column + 1,
444+
},
445+
end: {
446+
line: loc.end.line,
447+
column: loc.end.column + 1,
448+
},
449+
},
450+
{
451+
message,
452+
},
453+
);
454+
}
455+
441456
function printErrorSummary(severity: ErrorSeverity, message: string): string {
442457
let severityCategory: string;
443458
switch (severity) {

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

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -107,10 +107,9 @@ export function lower(
107107
if (binding.kind !== 'Identifier') {
108108
builder.errors.pushDiagnostic(
109109
CompilerDiagnostic.create({
110-
category: 'Could not find binding',
111-
description: `[BuildHIR] Could not find binding for param \`${param.node.name}\``,
112110
severity: ErrorSeverity.Invariant,
113-
suggestions: null,
111+
category: 'Could not find binding',
112+
description: `[BuildHIR] Could not find binding for param \`${param.node.name}\`.`,
114113
}).withDetail({
115114
kind: 'error',
116115
loc: param.node.loc ?? null,
@@ -172,10 +171,9 @@ export function lower(
172171
} else {
173172
builder.errors.pushDiagnostic(
174173
CompilerDiagnostic.create({
175-
category: `Handle ${param.node.type} parameters`,
176-
description: `[BuildHIR] Add support for ${param.node.type} parameters`,
177174
severity: ErrorSeverity.Todo,
178-
suggestions: null,
175+
category: `Handle ${param.node.type} parameters`,
176+
description: `[BuildHIR] Add support for ${param.node.type} parameters.`,
179177
}).withDetail({
180178
kind: 'error',
181179
loc: param.node.loc ?? null,
@@ -205,8 +203,7 @@ export function lower(
205203
CompilerDiagnostic.create({
206204
severity: ErrorSeverity.InvalidJS,
207205
category: `Unexpected function body kind`,
208-
description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
209-
suggestions: null,
206+
description: `Expected function body to be an expression or a block statement, got \`${body.type}\`.`,
210207
}).withDetail({
211208
kind: 'error',
212209
loc: body.node.loc ?? null,

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

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -447,23 +447,22 @@ function applySignature(
447447
reason: value.reason,
448448
context: new Set(),
449449
});
450-
const message =
450+
const variable =
451451
effect.value.identifier.name !== null &&
452452
effect.value.identifier.name.kind === 'named'
453-
? `\`${effect.value.identifier.name.value}\` cannot be modified`
454-
: 'This value cannot be modified';
453+
? `\`${effect.value.identifier.name.value}\``
454+
: 'value';
455455
effects.push({
456456
kind: 'MutateFrozen',
457457
place: effect.value,
458458
error: CompilerDiagnostic.create({
459459
severity: ErrorSeverity.InvalidReact,
460460
category: 'This value cannot be modified',
461-
description: reason,
462-
suggestions: null,
461+
description: `${reason}.`,
463462
}).withDetail({
464463
kind: 'error',
465464
loc: effect.value.loc,
466-
message,
465+
message: `${variable} cannot be modified`,
467466
}),
468467
});
469468
}
@@ -1018,30 +1017,30 @@ function applyEffect(
10181017
effect.value.identifier.declarationId,
10191018
)
10201019
) {
1021-
const description =
1020+
const variable =
10221021
effect.value.identifier.name !== null &&
10231022
effect.value.identifier.name.kind === 'named'
1024-
? `Variable \`${effect.value.identifier.name.value}\``
1025-
: 'This variable';
1023+
? `\`${effect.value.identifier.name.value}\``
1024+
: null;
10261025
const hoistedAccess = context.hoistedContextDeclarations.get(
10271026
effect.value.identifier.declarationId,
10281027
);
10291028
const diagnostic = CompilerDiagnostic.create({
10301029
severity: ErrorSeverity.InvalidReact,
10311030
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`,
1031+
description: `${variable ?? 'This variable'} is accessed before it is declared, which prevents the earlier access from updating when this value changes over time.`,
10331032
});
10341033
if (hoistedAccess != null && hoistedAccess.loc != effect.value.loc) {
10351034
diagnostic.withDetail({
10361035
kind: 'error',
10371036
loc: hoistedAccess.loc,
1038-
message: 'Variable accessed before it is declared',
1037+
message: `${variable ?? 'variable'} accessed before it is declared`,
10391038
});
10401039
}
10411040
diagnostic.withDetail({
10421041
kind: 'error',
10431042
loc: effect.value.loc,
1044-
message: 'The variable is declared here',
1043+
message: `${variable ?? 'variable'} is declared here`,
10451044
});
10461045

10471046
applyEffect(
@@ -1061,11 +1060,11 @@ function applyEffect(
10611060
reason: value.reason,
10621061
context: new Set(),
10631062
});
1064-
const message =
1063+
const variable =
10651064
effect.value.identifier.name !== null &&
10661065
effect.value.identifier.name.kind === 'named'
1067-
? `\`${effect.value.identifier.name.value}\` cannot be modified`
1068-
: 'This value cannot be modified';
1066+
? `\`${effect.value.identifier.name.value}\``
1067+
: 'value';
10691068
applyEffect(
10701069
context,
10711070
state,
@@ -1078,11 +1077,11 @@ function applyEffect(
10781077
error: CompilerDiagnostic.create({
10791078
severity: ErrorSeverity.InvalidReact,
10801079
category: 'This value cannot be modified',
1081-
description: reason,
1080+
description: `${reason}.`,
10821081
}).withDetail({
10831082
kind: 'error',
10841083
loc: effect.value.loc,
1085-
message,
1084+
message: `${variable} cannot be modified`,
10861085
}),
10871086
},
10881087
initialized,
@@ -1987,20 +1986,19 @@ function computeSignatureForInstruction(
19871986
break;
19881987
}
19891988
case 'StoreGlobal': {
1989+
const variable = `\`${value.name}\``;
19901990
effects.push({
19911991
kind: 'MutateGlobal',
19921992
place: value.value,
19931993
error: CompilerDiagnostic.create({
19941994
severity: ErrorSeverity.InvalidReact,
19951995
category:
19961996
'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,
1997+
description: `Variable ${variable} is declared outside of the component/hook. Reassigning this value during render 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)`,
20001998
}).withDetail({
20011999
kind: 'error',
20022000
loc: instr.loc,
2003-
message: 'Cannot reassign variable',
2001+
message: `${variable} cannot be reassigned`,
20042002
}),
20052003
});
20062004
effects.push({kind: 'Assign', from: value.value, into: lvalue});
@@ -2099,7 +2097,6 @@ function computeEffectsForLegacySignature(
20992097
? `\`${signature.canonicalName}\` is an impure function. `
21002098
: '') +
21012099
'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)',
2102-
suggestions: null,
21032100
}).withDetail({
21042101
kind: 'error',
21052102
loc,

‎compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,20 @@ export function validateLocalsNotReassignedAfterRender(fn: HIRFunction): void {
2929
);
3030
if (reassignment !== null) {
3131
const errors = new CompilerError();
32+
const variable =
33+
reassignment.identifier.name != null &&
34+
reassignment.identifier.name.kind === 'named'
35+
? `\`${reassignment.identifier.name.value}\``
36+
: 'variable';
3237
errors.pushDiagnostic(
3338
CompilerDiagnostic.create({
3439
severity: ErrorSeverity.InvalidReact,
35-
category: 'Cannot reassign a variable after render completes',
36-
description: `Reassigning ${reassignment.identifier.name != null && reassignment.identifier.name.kind === 'named' ? `variable \`${reassignment.identifier.name.value}\`` : 'a variable'} after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead`,
40+
category: 'Cannot reassign variable after render completes',
41+
description: `Reassigning ${variable} after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead.`,
3742
}).withDetail({
3843
kind: 'error',
3944
loc: reassignment.loc,
40-
message: 'Cannot reassign variable after render completes',
45+
message: `Cannot reassign ${variable} after render completes`,
4146
}),
4247
);
4348
throw errors;
@@ -78,16 +83,25 @@ function getContextReassignment(
7883
// if the function or its depends reassign, propagate that fact on the lvalue
7984
if (reassignment !== null) {
8085
if (isAsync || value.loweredFunc.func.async) {
81-
CompilerError.throwInvalidReact({
82-
reason:
83-
'Reassigning a variable in an async function can cause inconsistent behavior on subsequent renders. Consider using state instead',
84-
description:
85-
reassignment.identifier.name !== null &&
86-
reassignment.identifier.name.kind === 'named'
87-
? `Variable \`${reassignment.identifier.name.value}\` cannot be reassigned after render`
88-
: '',
89-
loc: reassignment.loc,
90-
});
86+
const errors = new CompilerError();
87+
const variable =
88+
reassignment.identifier.name !== null &&
89+
reassignment.identifier.name.kind === 'named'
90+
? `\`${reassignment.identifier.name.value}\``
91+
: 'variable';
92+
errors.pushDiagnostic(
93+
CompilerDiagnostic.create({
94+
severity: ErrorSeverity.InvalidReact,
95+
category: 'Cannot reassign variable in async function',
96+
description:
97+
'Reassigning a variable in an async function can cause inconsistent behavior on subsequent renders. Consider using state instead',
98+
}).withDetail({
99+
kind: 'error',
100+
loc: reassignment.loc,
101+
message: `Cannot reassign ${variable}`,
102+
}),
103+
);
104+
throw errors;
91105
}
92106
reassigningFunctions.set(lvalue.identifier.id, reassignment);
93107
}

0 commit comments

Comments
 (0)