Skip to content
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.

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

Improve diagnostics deduplication 2 #58318

Merged
merged 11 commits into from
Apr 29, 2024
22 changes: 20 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ import {
getAssignmentDeclarationKind,
getAssignmentDeclarationPropertyAccessKind,
getAssignmentTargetKind,
getCanonicalDiagnostic,
getCheckFlags,
getClassExtendsHeritageElement,
getClassLikeDeclarationOfSymbol,
Expand Down Expand Up @@ -3118,10 +3119,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (suggestion) {
const suggestionName = symbolToString(suggestion);
const isUncheckedJS = isUncheckedJSSuggestion(errorLocation, suggestion, /*excludeClasses*/ false);
const message = meaning === SymbolFlags.Namespace || nameArg && typeof nameArg !== "string" && nodeIsSynthesized(nameArg) ? Diagnostics.Cannot_find_namespace_0_Did_you_mean_1
const message = meaning === SymbolFlags.Namespace ||
nameArg && typeof nameArg !== "string" && nodeIsSynthesized(nameArg) ?
Diagnostics.Cannot_find_namespace_0_Did_you_mean_1
: isUncheckedJS ? Diagnostics.Could_not_find_name_0_Did_you_mean_1
: Diagnostics.Cannot_find_name_0_Did_you_mean_1;
const diagnostic = createError(errorLocation, message, diagnosticName(nameArg), suggestionName);
diagnostic.canonicalHead = getCanonicalDiagnostic(nameNotFoundMessage, diagnosticName(nameArg));
addErrorOrSuggestion(!isUncheckedJS, diagnostic);
if (suggestion.valueDeclaration) {
addRelatedInfo(
Expand Down Expand Up @@ -21697,9 +21701,23 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
else {
errorInfo = elaborateNeverIntersection(errorInfo, originalTarget);
}
// Used by, eg, missing property checking to replace the top-level message with a more informative one.
if (!headMessage && maybeSuppress) {
// We suppress a call to `reportRelationError` or not depending on the state of the type checker, so
// we call `reportRelationError` here and then undo its effects to figure out what would be the diagnostic
// if we hadn't supress it, and save that as a canonical diagnostic for deduplication purposes.
const savedErrorState = captureErrorCalculationState();
reportRelationError(headMessage, source, target);
let canonical;
if (errorInfo && errorInfo !== savedErrorState.errorInfo) {
canonical = { code: errorInfo.code, messageText: errorInfo.messageText };
}
resetErrorInfo(savedErrorState);
if (canonical && errorInfo) {
errorInfo.canonicalHead = canonical;
}

lastSkippedInfo = [source, target];
// Used by, eg, missing property checking to replace the top-level message with a more informative one
return;
}
reportRelationError(headMessage, source, target);
Expand Down
17 changes: 17 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7069,6 +7069,8 @@ export interface DiagnosticMessageChain {
next?: DiagnosticMessageChain[];
/** @internal */
repopulateInfo?: () => RepopulateDiagnosticChainInfo;
/** @internal */
canonicalHead?: CanonicalDiagnostic;
}

export interface Diagnostic extends DiagnosticRelatedInformation {
Expand All @@ -7079,6 +7081,21 @@ export interface Diagnostic extends DiagnosticRelatedInformation {
source?: string;
relatedInformation?: DiagnosticRelatedInformation[];
/** @internal */ skippedOn?: keyof CompilerOptions;
/**
* @internal
* Used for deduplication and comparison.
* Whenever it is possible for two diagnostics that report the same problem to be produced with
* different messages (e.g. "Cannot find name 'foo'" vs "Cannot find name 'foo'. Did you mean 'bar'?"),
* this property can be set to a canonical message,
* so that those two diagnostics are appropriately considered to be the same.
*/
canonicalHead?: CanonicalDiagnostic;
}

/** @internal */
export interface CanonicalDiagnostic {
code: number;
messageText: string;
}

/** @internal */
Expand Down
67 changes: 52 additions & 15 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
canHaveLocals,
canHaveModifiers,
type CanHaveModuleSpecifier,
CanonicalDiagnostic,
CaseBlock,
CaseClause,
CaseOrDefaultClause,
Expand Down Expand Up @@ -2178,6 +2179,7 @@ export function createFileDiagnosticFromMessageChain(file: SourceFile, start: nu
category: messageChain.category,
messageText: messageChain.next ? messageChain : messageChain.messageText,
relatedInformation,
canonicalHead: messageChain.canonicalHead,
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be serialized in buildInfo when serializing diagnostic. Will we need this to deduplicate across multiple files?

Copy link
Member Author

@gabritto gabritto Apr 26, 2024

Choose a reason for hiding this comment

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

Will we need this to deduplicate across multiple files?

I don't think so, this should only be relevant for deduplicating diagnostics of a single file.

Does this need to be serialized in buildInfo when serializing diagnostic?

What's the criterion for needing to serialize this in buildInfo?

Copy link
Member

Choose a reason for hiding this comment

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

you would need this info to deduplicate with errors from other files or errors emitted for same file in other phase (which obviously isn't true - like say same error is reported by declaration emit and semantic diagnostic just giving hypothetical example here)

Copy link
Member

Choose a reason for hiding this comment

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

Given this doesnt get used beyond file boundaries this should be ok to be not serialized.
That is checking files in any order will result in same error in that file (which we were anyways assuming till now) this is good.

};
}

Expand Down Expand Up @@ -2216,6 +2218,14 @@ export function createDiagnosticForRange(sourceFile: SourceFile, range: TextRang
};
}

/** @internal */
export function getCanonicalDiagnostic(message: DiagnosticMessage, ...args: string[]): CanonicalDiagnostic {
return {
code: message.code,
messageText: formatMessage(message, ...args),
};
}

/** @internal */
export function getSpanOfTokenAtPosition(sourceFile: SourceFile, pos: number): TextSpan {
const scanner = createScanner(sourceFile.languageVersion, /*skipTrivia*/ true, sourceFile.languageVariant, sourceFile.text, /*onError*/ undefined, pos);
Expand Down Expand Up @@ -8540,11 +8550,13 @@ export function compareDiagnostics(d1: Diagnostic, d2: Diagnostic): Comparison {

/** @internal */
export function compareDiagnosticsSkipRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison {
const code1 = getDiagnosticCode(d1);
const code2 = getDiagnosticCode(d2);
return compareStringsCaseSensitive(getDiagnosticFilePath(d1), getDiagnosticFilePath(d2)) ||
compareValues(d1.start, d2.start) ||
compareValues(d1.length, d2.length) ||
compareValues(d1.code, d2.code) ||
compareMessageText(d1.messageText, d2.messageText) ||
compareValues(code1, code2) ||
compareMessageText(d1, d2) ||
Comparison.EqualTo;
}

Expand All @@ -8566,25 +8578,38 @@ function compareRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison {
// An diagnostic message with more elaboration should be considered *less than* a diagnostic message
// with less elaboration that is otherwise similar.
function compareMessageText(
t1: string | Pick<DiagnosticMessageChain, "messageText" | "next">,
t2: string | Pick<DiagnosticMessageChain, "messageText" | "next">,
d1: Diagnostic,
d2: Diagnostic,
): Comparison {
if (typeof t1 === "string" && typeof t2 === "string") {
return compareStringsCaseSensitive(t1, t2);
let headMsg1 = getDiagnosticMessage(d1);
let headMsg2 = getDiagnosticMessage(d2);
if (typeof headMsg1 !== "string") {
headMsg1 = headMsg1.messageText;
}

if (typeof t1 === "string") {
t1 = { messageText: t1 };
if (typeof headMsg2 !== "string") {
headMsg2 = headMsg2.messageText;
}
if (typeof t2 === "string") {
t2 = { messageText: t2 };
const chain1 = typeof d1.messageText !== "string" ? d1.messageText.next : undefined;
const chain2 = typeof d2.messageText !== "string" ? d2.messageText.next : undefined;

let res = compareStringsCaseSensitive(headMsg1, headMsg2);
if (res) {
return res;
}
const res = compareStringsCaseSensitive(t1.messageText, t2.messageText);

res = compareMessageChain(chain1, chain2);
if (res) {
return res;
}

return compareMessageChain(t1.next, t2.next);
if (d1.canonicalHead && !d2.canonicalHead) {
return Comparison.LessThan;
}
if (d2.canonicalHead && !d1.canonicalHead) {
return Comparison.GreaterThan;
}

return Comparison.EqualTo;
}

// First compare by size of the message chain,
Expand Down Expand Up @@ -8659,11 +8684,23 @@ function compareMessageChainContent(

/** @internal */
export function diagnosticsEqualityComparer(d1: Diagnostic, d2: Diagnostic): boolean {
const code1 = getDiagnosticCode(d1);
const code2 = getDiagnosticCode(d2);
const msg1 = getDiagnosticMessage(d1);
const msg2 = getDiagnosticMessage(d2);
return compareStringsCaseSensitive(getDiagnosticFilePath(d1), getDiagnosticFilePath(d2)) === Comparison.EqualTo &&
compareValues(d1.start, d2.start) === Comparison.EqualTo &&
compareValues(d1.length, d2.length) === Comparison.EqualTo &&
compareValues(d1.code, d2.code) === Comparison.EqualTo &&
messageTextEqualityComparer(d1.messageText, d2.messageText);
compareValues(code1, code2) === Comparison.EqualTo &&
messageTextEqualityComparer(msg1, msg2);
}

function getDiagnosticCode(d: Diagnostic): number {
return d.canonicalHead?.code || d.code;
}

function getDiagnosticMessage(d: Diagnostic): string | DiagnosticMessageChain {
return d.canonicalHead?.messageText || d.messageText;
}

function messageTextEqualityComparer(m1: string | DiagnosticMessageChain, m2: string | DiagnosticMessageChain): boolean {
Expand Down
22 changes: 22 additions & 0 deletions tests/baselines/reference/duplicateErrorAssignability.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
duplicateErrorAssignability.ts(10,11): error TS2741: Property 'x' is missing in type 'B' but required in type 'A'.
duplicateErrorAssignability.ts(12,5): error TS2538: Type 'B' cannot be used as an index type.


==== duplicateErrorAssignability.ts (2 errors) ====
interface A {
x: number;
}
interface B {
y: string;
}

declare let b: B;
declare let a: A;
const x = a = b;
~
!!! error TS2741: Property 'x' is missing in type 'B' but required in type 'A'.
!!! related TS2728 duplicateErrorAssignability.ts:2:5: 'x' is declared here.
let obj: { 3: string } = { 3: "three" };
obj[x];
~
!!! error TS2538: Type 'B' cannot be used as an index type.
21 changes: 21 additions & 0 deletions tests/baselines/reference/duplicateErrorAssignability.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//// [tests/cases/compiler/duplicateErrorAssignability.ts] ////

//// [duplicateErrorAssignability.ts]
interface A {
x: number;
}
interface B {
y: string;
}

declare let b: B;
declare let a: A;
const x = a = b;
let obj: { 3: string } = { 3: "three" };
obj[x];

//// [duplicateErrorAssignability.js]
"use strict";
var x = a = b;
var obj = { 3: "three" };
obj[x];
38 changes: 38 additions & 0 deletions tests/baselines/reference/duplicateErrorAssignability.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//// [tests/cases/compiler/duplicateErrorAssignability.ts] ////

=== duplicateErrorAssignability.ts ===
interface A {
>A : Symbol(A, Decl(duplicateErrorAssignability.ts, 0, 0))

x: number;
>x : Symbol(A.x, Decl(duplicateErrorAssignability.ts, 0, 13))
}
interface B {
>B : Symbol(B, Decl(duplicateErrorAssignability.ts, 2, 1))

y: string;
>y : Symbol(B.y, Decl(duplicateErrorAssignability.ts, 3, 13))
}

declare let b: B;
>b : Symbol(b, Decl(duplicateErrorAssignability.ts, 7, 11))
>B : Symbol(B, Decl(duplicateErrorAssignability.ts, 2, 1))

declare let a: A;
>a : Symbol(a, Decl(duplicateErrorAssignability.ts, 8, 11))
>A : Symbol(A, Decl(duplicateErrorAssignability.ts, 0, 0))

const x = a = b;
>x : Symbol(x, Decl(duplicateErrorAssignability.ts, 9, 5))
>a : Symbol(a, Decl(duplicateErrorAssignability.ts, 8, 11))
>b : Symbol(b, Decl(duplicateErrorAssignability.ts, 7, 11))

let obj: { 3: string } = { 3: "three" };
>obj : Symbol(obj, Decl(duplicateErrorAssignability.ts, 10, 3))
>3 : Symbol(3, Decl(duplicateErrorAssignability.ts, 10, 10))
>3 : Symbol(3, Decl(duplicateErrorAssignability.ts, 10, 26))

obj[x];
>obj : Symbol(obj, Decl(duplicateErrorAssignability.ts, 10, 3))
>x : Symbol(x, Decl(duplicateErrorAssignability.ts, 9, 5))

52 changes: 52 additions & 0 deletions tests/baselines/reference/duplicateErrorAssignability.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//// [tests/cases/compiler/duplicateErrorAssignability.ts] ////

=== duplicateErrorAssignability.ts ===
interface A {
x: number;
>x : number
> : ^^^^^^
}
interface B {
y: string;
>y : string
> : ^^^^^^
}

declare let b: B;
>b : B
> : ^

declare let a: A;
>a : A
> : ^

const x = a = b;
>x : B
> : ^
>a = b : B
> : ^
>a : A
> : ^
>b : B
> : ^

let obj: { 3: string } = { 3: "three" };
>obj : { 3: string; }
> : ^^^^^ ^^^
>3 : string
> : ^^^^^^
>{ 3: "three" } : { 3: string; }
> : ^^^^^^^^^^^^^^
>3 : string
> : ^^^^^^
>"three" : "three"
> : ^^^^^^^

obj[x];
>obj[x] : any
> : ^^^
>obj : { 3: string; }
> : ^^^^^^^^^^^^^^
>x : B
> : ^

11 changes: 11 additions & 0 deletions tests/baselines/reference/duplicateErrorNameNotFound.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
duplicateErrorNameNotFound.ts(4,5): error TS2552: Cannot find name 'RoomInterface'. Did you mean 'RoomInterfae'?


==== duplicateErrorNameNotFound.ts (1 errors) ====
type RoomInterfae = {};

export type {
RoomInterface
~~~~~~~~~~~~~
!!! error TS2552: Cannot find name 'RoomInterface'. Did you mean 'RoomInterfae'?
}
12 changes: 12 additions & 0 deletions tests/baselines/reference/duplicateErrorNameNotFound.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//// [tests/cases/compiler/duplicateErrorNameNotFound.ts] ////

//// [duplicateErrorNameNotFound.ts]
type RoomInterfae = {};

export type {
RoomInterface
}

//// [duplicateErrorNameNotFound.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
10 changes: 10 additions & 0 deletions tests/baselines/reference/duplicateErrorNameNotFound.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//// [tests/cases/compiler/duplicateErrorNameNotFound.ts] ////

=== duplicateErrorNameNotFound.ts ===
type RoomInterfae = {};
>RoomInterfae : Symbol(RoomInterfae, Decl(duplicateErrorNameNotFound.ts, 0, 0))

export type {
RoomInterface
>RoomInterface : Symbol(RoomInterface, Decl(duplicateErrorNameNotFound.ts, 2, 13))
}
12 changes: 12 additions & 0 deletions tests/baselines/reference/duplicateErrorNameNotFound.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//// [tests/cases/compiler/duplicateErrorNameNotFound.ts] ////

=== duplicateErrorNameNotFound.ts ===
type RoomInterfae = {};
>RoomInterfae : RoomInterfae
> : ^^^^^^^^^^^^

export type {
RoomInterface
>RoomInterface : any
> : ^^^
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
Assignability cache: 5,000
Type Count: 10,000
Instantiation count: 250,000
Symbol count: 100,000
Symbol count: 250,000
Copy link
Member

Choose a reason for hiding this comment

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

...That's a big bump. Is this just caused by the extra reportRelationError call expanding the recursive type way more? Still feels like a lot more symbols.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessarily something that aughta get fixed in this PR, but understanding where so many symbols popped in from could be useful. It might indicate that we're not caching something we aughta be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this just caused by the extra reportRelationError call expanding the recursive type way more?

I believe so. Since this test has an infinitely recurring type, it didn't seem like a great reason for concern to me, but I could be wrong.


=== mappedTypeRecursiveInference.ts ===
interface A { a: A }
Expand Down