Skip to content

fix(45713) Improve error report summaries #45742

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
orta merged 6 commits into from
Dec 7, 2021
Merged

fix(45713) Improve error report summaries #45742

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0638417
Improve error report summaries (#45713)
rbargholz Sep 5, 2021
08906dd
fixup! Improve error report summaries (#45713)
rbargholz Sep 13, 2021
1af84d2
fixup! fixup! Improve error report summaries (#45713)
rbargholz Sep 14, 2021
f543c40
Adds support for handling localization renaming the 'files' header du…
orta Oct 1, 2021
b2988e7
Merge branch 'main' into improved_error_summary
Oct 1, 2021
db56f9f
fixup! Adds support for handling localization renaming the 'files' he…
rbargholz Oct 2, 2021
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
Prev Previous commit
Next Next commit
fixup! Improve error report summaries (#45713)
  • Loading branch information
@rbargholz
rbargholz committed Sep 13, 2021
commit 08906dd069b626e372070dc3655bc42d5c7dc487
9 changes: 7 additions & 2 deletions src/compiler/tsbuildPublic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,12 @@ namespace ts {
return fileExtensionIs(fileName, Extension.Dts);
}

export type ReportEmitErrorSummary = (errorCount: number, filesInError: (string | undefined)[]) => void;
export type ReportEmitErrorSummary = (errorCount: number, filesInError: (ReportFileInError | undefined)[]) => void;

export interface ReportFileInError {
fileName: string;
line: number;
}

export interface SolutionBuilderHostBase<T extends BuilderProgram> extends ProgramHost<T> {
createDirectory?(path: string): void;
Expand Down Expand Up @@ -2002,7 +2007,7 @@ namespace ts {
const canReportSummary = state.watch || !!state.host.reportErrorSummary;
const { diagnostics } = state;
let totalErrors = 0;
let filesInError: (string | undefined)[] = [];
let filesInError: (ReportFileInError | undefined)[] = [];
if (isCircularBuildOrder(buildOrder)) {
reportBuildQueue(state, buildOrder.buildOrder);
reportErrors(state, buildOrder.circularDiagnostics);
Expand Down
63 changes: 43 additions & 20 deletions src/compiler/watch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,24 +101,27 @@ namespace ts {
return countWhere(diagnostics, diagnostic => diagnostic.category === DiagnosticCategory.Error);
}

export function getFilesInErrorForSummary(diagnostics: readonly Diagnostic[]) {
return filter(diagnostics, diagnostic => diagnostic.category === DiagnosticCategory.Error)
export function getFilesInErrorForSummary(diagnostics: readonly Diagnostic[]): (ReportFileInError | undefined)[] {
const filesInError =
filter(diagnostics, diagnostic => diagnostic.category === DiagnosticCategory.Error)
.map(
errorDiagnostic => {
if(errorDiagnostic.file === undefined) return;
return `${errorDiagnostic.file.fileName}`;
})
.filter((value, index, self) => self !== undefined && self.indexOf(value) === index)
.map((fileName: string) => {
const diagnosticForFileName = find(diagnostics, diagnostic =>
diagnostic.file !== undefined && diagnostic.file.fileName === fileName
);

if(diagnosticForFileName !== undefined) {
const { line } = getLineAndCharacterOfPosition(diagnosticForFileName.file!, diagnosticForFileName.start!);
return `${fileName}:${line + 1}`;
}
});
return filesInError.map((fileName: string) => {
const diagnosticForFileName = find(diagnostics, diagnostic =>
diagnostic.file !== undefined && diagnostic.file.fileName === fileName
);

if(diagnosticForFileName !== undefined) {
const { line } = getLineAndCharacterOfPosition(diagnosticForFileName.file!, diagnosticForFileName.start!);
return {
fileName,
line: line + 1,
};
}
});
}

export function getWatchErrorSummaryDiagnosticMessage(errorCount: number) {
Expand All @@ -129,24 +132,44 @@ namespace ts {

export function getErrorSummaryText(
errorCount: number,
filesInError: readonly (string | undefined)[],
filesInError: readonly (ReportFileInError | undefined)[],
newLine: string
) {
if (errorCount === 0) return "";
const nonNilFiles = filesInError.filter(fileInError => fileInError !== undefined);
const distinctFileNamesWithLines = nonNilFiles.map(fileInError => `${fileInError!.fileName}:${fileInError!.line}`)
.filter((value, index, self) => self.indexOf(value) === index);
const d = errorCount === 1 ?
createCompilerDiagnostic(
filesInError[0] !== undefined ?
Diagnostics.Found_1_error_in_1 :
Diagnostics.Found_1_error,
errorCount,
filesInError[0]) :
distinctFileNamesWithLines[0]) :
createCompilerDiagnostic(
filesInError.length === 1 ?
Diagnostics.Found_0_errors_in_1_file :
Diagnostics.Found_0_errors_in_1_files,
distinctFileNamesWithLines.length === 0 ?
Diagnostics.Found_0_errors :
distinctFileNamesWithLines.length === 1 ?
Diagnostics.Found_0_errors_in_1_file :
Diagnostics.Found_0_errors_in_1_files,
errorCount,
filesInError.length);
return `${newLine}${flattenDiagnosticMessageText(d.messageText, newLine)}${newLine}${newLine}`;
distinctFileNamesWithLines.length);
Copy link
Copy Markdown
Contributor

@orta orta Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite a dense statement, not sure I have any good ideas off the top off my head on how to improve it though

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look back at it and see what I could do. I also try to avoid double ternaries if I can avoid it.

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about something like this?

        if(distinctFileNamesWithLines.length === 0 && errorCount !== 1) {
            const foundErrorsWithNoFilesDiagnostic = createCompilerDiagnostic(
                Diagnostics.Found_0_errors,
                errorCount);
            return `${newLine}${flattenDiagnosticMessageText(foundErrorsWithNoFilesDiagnostic.messageText, newLine)}${newLine}${newLine}`;
        }
        const d = errorCount === 1 ?
            createCompilerDiagnostic(
                filesInError[0] !== undefined ?
                    Diagnostics.Found_1_error_in_1 :
                    Diagnostics.Found_1_error,
                errorCount,
                distinctFileNamesWithLines[0]) :
            createCompilerDiagnostic(
                distinctFileNamesWithLines.length === 1 ?
                    Diagnostics.Found_0_errors_in_1_file :
                    Diagnostics.Found_0_errors_in_1_files,
                errorCount,
                distinctFileNamesWithLines.length);
        return `${newLine}${flattenDiagnosticMessageText(d.messageText, newLine)}${newLine}${newLine}${errorCount > 1 ? createTabularErrorsDisplay(nonNilFiles) : ""}${newLine}`;

Copy link
Copy Markdown
Contributor

@orta orta Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, wes also said this is pretty common in the codebase, so the original is also fine 👍🏻

return `${newLine}${flattenDiagnosticMessageText(d.messageText, newLine)}${newLine}${newLine}${errorCount > 1 ? createTabularErrorsDisplay(nonNilFiles) : ''}`;
}

function createTabularErrorsDisplay(filesInError: (ReportFileInError | undefined)[]) {
let tabularData = "";
const distinctFiles = filesInError.filter((value, index, self) => index === self.findIndex(file => file!.fileName === value!.fileName));
if(distinctFiles.length === 0) return tabularData;

const headerRow = "Errors Files\n";
Copy link
Copy Markdown
Member

@weswigham weswigham Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs to be a localized message~

Copy link
Copy Markdown
Contributor

@orta orta Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do that, (which I think we probably do need to do) then the numbers about length etc will need to not be hardcoded anymore. For example the Chinese version of "errors" might be 1-2 characters characters wide.

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think of that, yes this should probably be localised.

@orta

If we do that, (which I think we probably do need to do) then the numbers about length etc will need to not be hardcoded anymore

May I ask how you mean by "hardcoded" here? Do you mean we don't need to bother so much about left padding?

Copy link
Copy Markdown
Contributor

@orta orta Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By hardcoding, I mean const BASE_LEFT_PADDING = 6; only makes sense when 'errors' the string is 6 characters long for example.

Armed with google translate, perhaps this is a reasonable design answer:

错误  案卷
  3  build/lib/builtInExtensionsCG.ts

 错误  案卷
  12  build/lib/builtInExtensionsCG.ts
 123  extensions/html-language-features/client/src/htmlClient.ts

  错误  案卷
   12  build/lib/builtInExtensionsCG.ts
  123  extensions/html-language-features/client/src/htmlClient.ts
 1234  extensions/html-language-features/client/src/htmlEmptyTagsShared.ts

Where tsc pulls out the longest length of a value from either the highest number of errors in a file or the translated word for "errors", allowing people to have a MAX_INT amount of errors because it just moves the column to the right once it hits a particular size

       错误  案卷
        12  build/lib/builtInExtensionsCG.ts
       123  extensions/html-language-features/client/src/htmlClient.ts
      1234  extensions/html-language-features/client/src/htmlEmptyTagsShared.ts
 123456789  extensions/html-language-features/client/src/tagClosing.ts

    Errors  Files
        12  build/lib/builtInExtensionsCG.ts
       123  extensions/html-language-features/client/src/htmlClient.ts
      1234  extensions/html-language-features/client/src/htmlEmptyTagsShared.ts
 123456789  extensions/html-language-features/client/src/tagClosing.ts

Does that feel reasonable to you?

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will work for me too. I'll work on it and see what I can come up with. Thank you for the explanation.

tabularData += headerRow;
distinctFiles.forEach(file => {
const errorCountForFile = countWhere(filesInError, fileInError => fileInError!.fileName === file!.fileName);
tabularData += ` ${errorCountForFile} ${file!.fileName}:${file!.line}\n`;
})

return tabularData;
}

export function isBuilderProgram(program: Program | BuilderProgram): program is BuilderProgram {
Expand Down
2 changes: 1 addition & 1 deletion src/testRunner/unittests/tscWatch/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ namespace ts.tscWatch {
export function checkNormalBuildErrors(
host: WatchedSystem,
errors: readonly Diagnostic[] | readonly string[],
files: readonly string[],
files: readonly ReportFileInError[],
reportErrorSummary?: boolean
) {
checkOutputErrors(
Expand Down
6 changes: 5 additions & 1 deletion tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5249,7 +5249,11 @@ declare namespace ts {
traceResolution?: boolean;
[option: string]: CompilerOptionsValue | undefined;
}
type ReportEmitErrorSummary = (errorCount: number, filesInError: (string | undefined)[]) => void;
type ReportEmitErrorSummary = (errorCount: number, filesInError: (ReportFileInError | undefined)[]) => void;
interface ReportFileInError {
fileName: string;
line: number;
}
interface SolutionBuilderHostBase<T extends BuilderProgram> extends ProgramHost<T> {
createDirectory?(path: string): void;
/**
Expand Down
6 changes: 5 additions & 1 deletion tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5249,7 +5249,11 @@ declare namespace ts {
traceResolution?: boolean;
[option: string]: CompilerOptionsValue | undefined;
}
type ReportEmitErrorSummary = (errorCount: number, filesInError: (string | undefined)[]) => void;
type ReportEmitErrorSummary = (errorCount: number, filesInError: (ReportFileInError | undefined)[]) => void;
interface ReportFileInError {
fileName: string;
line: number;
}
interface SolutionBuilderHostBase<T extends BuilderProgram> extends ProgramHost<T> {
createDirectory?(path: string): void;
/**
Expand Down
2 changes: 2 additions & 0 deletions tests/baselines/reference/deeplyNestedAssignabilityIssue.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,3 +67,5 @@

Found 2 errors in 1 file.

Errors Files
2 tests/cases/compiler/deeplyNestedAssignabilityIssue.ts:22
4 changes: 4 additions & 0 deletions tests/baselines/reference/duplicateIdentifierRelatedSpans1.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,7 @@

Found 6 errors in 3 files.

Errors Files
2 tests/cases/compiler/file1.ts:1
2 tests/cases/compiler/file2.ts:1
2 tests/cases/compiler/file3.ts:1
3 changes: 3 additions & 0 deletions tests/baselines/reference/duplicateIdentifierRelatedSpans2.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,6 @@

Found 2 errors in 2 files.

Errors Files
1 tests/cases/compiler/file1.ts:1
1 tests/cases/compiler/file2.ts:1
3 changes: 3 additions & 0 deletions tests/baselines/reference/duplicateIdentifierRelatedSpans3.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,6 @@

Found 6 errors in 2 files.

Errors Files
3 tests/cases/compiler/file1.ts:2
3 tests/cases/compiler/file2.ts:2
3 changes: 3 additions & 0 deletions tests/baselines/reference/duplicateIdentifierRelatedSpans4.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,6 @@

Found 2 errors in 2 files.

Errors Files
1 tests/cases/compiler/file1.ts:1
1 tests/cases/compiler/file2.ts:1
3 changes: 3 additions & 0 deletions tests/baselines/reference/duplicateIdentifierRelatedSpans5.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,6 @@

Found 6 errors in 2 files.

Errors Files
3 tests/cases/compiler/file1.ts:3
3 tests/cases/compiler/file2.ts:4
3 changes: 3 additions & 0 deletions tests/baselines/reference/duplicateIdentifierRelatedSpans6.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,6 @@
}
Found 6 errors in 2 files.

Errors Files
3 tests/cases/compiler/file1.ts:3
3 tests/cases/compiler/file2.ts:5
3 changes: 3 additions & 0 deletions tests/baselines/reference/duplicateIdentifierRelatedSpans7.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,6 @@
}
Found 2 errors in 2 files.

Errors Files
1 tests/cases/compiler/file1.ts:1
1 tests/cases/compiler/file2.ts:3
2 changes: 2 additions & 0 deletions tests/baselines/reference/prettyFileWithErrorsAndTabs.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@
}
Found 3 errors in 1 file.

Errors Files
3 tests/cases/compiler/prettyFileWithErrorsAndTabs.ts:3
2 changes: 1 addition & 1 deletion ...eference/tsbuild/configFileErrors/initial-build/when-tsconfig-extends-the-missing-file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ Output::
error TS18003: No inputs were found in config file '/src/tsconfig.second.json'. Specified 'include' paths were '["**/*"]' and 'exclude' paths were '[]'.


Found 4 errors in 0 files.
Found 4 errors.

exitCode:: ExitStatus.DiagnosticsPresent_OutputsSkipped

Expand Down
2 changes: 1 addition & 1 deletion .../in-bad-ref-branch-reports-the-error-about-files-not-in-rootDir-at-the-import-location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ Output::
[12:00:00 AM] Skipping build of project '/src/zoo/tsconfig.json' because its dependency '/src/animals' was not built


Found 7 errors in 0 files.
Found 7 errors.

exitCode:: ExitStatus.DiagnosticsPresent_OutputsSkipped

Expand Down
21 changes: 21 additions & 0 deletions tests/baselines/reference/tsc/incremental/initial-build/noEmit-changes-composite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,9 @@ Output::

Found 2 errors in 2 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated

Copy link
Copy Markdown
Contributor

@orta orta Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In comparison to the original I think we might need an extra newline after the table to be consistent with our newlines 👍🏻

Copy link
Copy Markdown
Contributor Author

@rbargholz rbargholz Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I'll add an extra newline.


Expand Down Expand Up @@ -718,6 +721,10 @@ Output::

Found 3 errors in 3 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
1 /src/project/src/noChangeFileWithEmitSpecificError.ts:1
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated


Expand Down Expand Up @@ -931,6 +938,10 @@ Output::

Found 3 errors in 3 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
1 /src/project/src/noChangeFileWithEmitSpecificError.ts:1
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated


Expand Down Expand Up @@ -965,6 +976,9 @@ Output::

Found 2 errors in 2 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
exitCode:: ExitStatus.DiagnosticsPresent_OutputsSkipped


Expand Down Expand Up @@ -999,6 +1013,9 @@ Output::

Found 2 errors in 2 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
exitCode:: ExitStatus.DiagnosticsPresent_OutputsSkipped


Expand Down Expand Up @@ -1038,6 +1055,10 @@ Output::

Found 3 errors in 3 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
1 /src/project/src/noChangeFileWithEmitSpecificError.ts:1
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated


Expand Down
21 changes: 21 additions & 0 deletions ...selines/reference/tsc/incremental/initial-build/noEmit-changes-incremental-declaration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,9 @@ Output::

Found 2 errors in 2 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated


Expand Down Expand Up @@ -718,6 +721,10 @@ Output::

Found 3 errors in 3 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
1 /src/project/src/noChangeFileWithEmitSpecificError.ts:1
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated


Expand Down Expand Up @@ -931,6 +938,10 @@ Output::

Found 3 errors in 3 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
1 /src/project/src/noChangeFileWithEmitSpecificError.ts:1
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated


Expand Down Expand Up @@ -965,6 +976,9 @@ Output::

Found 2 errors in 2 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
exitCode:: ExitStatus.DiagnosticsPresent_OutputsSkipped


Expand Down Expand Up @@ -999,6 +1013,9 @@ Output::

Found 2 errors in 2 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
exitCode:: ExitStatus.DiagnosticsPresent_OutputsSkipped


Expand Down Expand Up @@ -1038,6 +1055,10 @@ Output::

Found 3 errors in 3 files.

Errors Files
1 /src/project/src/directUse.ts:2
1 /src/project/src/indirectUse.ts:2
1 /src/project/src/noChangeFileWithEmitSpecificError.ts:1
exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated


Expand Down
Loading