Improve diagnostics deduplication (#58220)

src/compiler/core.ts

@@ -808,14 +808,30 @@ export function createSortedArray<T>(): SortedArray<T> {
 }
 
 /** @internal */
-export function insertSorted<T>(array: SortedArray<T>, insert: T, compare: Comparer<T>, allowDuplicates?: boolean): boolean {
+export function insertSorted<T>(
+    array: SortedArray<T>,
+    insert: T,
+    compare: Comparer<T>,
+    equalityComparer?: EqualityComparer<T>,
+    allowDuplicates?: boolean,
+): boolean {
     if (array.length === 0) {
         array.push(insert);
         return true;
     }
 
     const insertIndex = binarySearch(array, insert, identity, compare);
     if (insertIndex < 0) {
+        if (equalityComparer && !allowDuplicates) {
+            const idx = ~insertIndex;
+            if (idx > 0 && equalityComparer(insert, array[idx - 1])) {
+                return false;
+            }
+            if (idx < array.length && equalityComparer(insert, array[idx])) {
+                array.splice(idx, 1, insert);
+                return true;
+            }
+        }
         array.splice(~insertIndex, 0, insert);
         return true;
     }

src/compiler/utilities.ts

@@ -5864,6 +5864,9 @@ export function createDiagnosticCollection(): DiagnosticCollection {
         if (result >= 0) {
             return diagnostics[result];
         }
+        if (~result > 0 && diagnosticsEqualityComparer(diagnostic, diagnostics[~result - 1])) {
+            return diagnostics[~result - 1];
+        }
         return undefined;
     }
 
@@ -5887,7 +5890,7 @@ export function createDiagnosticCollection(): DiagnosticCollection {
             diagnostics = nonFileDiagnostics;
         }
 
-        insertSorted(diagnostics, diagnostic, compareDiagnosticsSkipRelatedInformation);
+        insertSorted(diagnostics, diagnostic, compareDiagnosticsSkipRelatedInformation, diagnosticsEqualityComparer);
     }
 
     function getGlobalDiagnostics(): Diagnostic[] {
@@ -8545,58 +8548,130 @@ export function compareDiagnosticsSkipRelatedInformation(d1: Diagnostic, d2: Dia
         Comparison.EqualTo;
 }
 
+// A diagnostic with more elaboration should be considered *less than* a diagnostic
+// with less elaboration that is otherwise similar.
 function compareRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison {
     if (!d1.relatedInformation && !d2.relatedInformation) {
         return Comparison.EqualTo;
     }
     if (d1.relatedInformation && d2.relatedInformation) {
-        return compareValues(d1.relatedInformation.length, d2.relatedInformation.length) || forEach(d1.relatedInformation, (d1i, index) => {
+        return compareValues(d2.relatedInformation.length, d1.relatedInformation.length) || forEach(d1.relatedInformation, (d1i, index) => {
             const d2i = d2.relatedInformation![index];
             return compareDiagnostics(d1i, d2i); // EqualTo is 0, so falsy, and will cause the next item to be compared
         }) || Comparison.EqualTo;
     }
     return d1.relatedInformation ? Comparison.LessThan : Comparison.GreaterThan;
 }
 
-function compareMessageText(t1: string | DiagnosticMessageChain, t2: string | DiagnosticMessageChain): 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">,
+): Comparison {
     if (typeof t1 === "string" && typeof t2 === "string") {
         return compareStringsCaseSensitive(t1, t2);
     }
-    else if (typeof t1 === "string") {
-        return Comparison.LessThan;
+
+    if (typeof t1 === "string") {
+        t1 = { messageText: t1 };
     }
-    else if (typeof t2 === "string") {
-        return Comparison.GreaterThan;
+    if (typeof t2 === "string") {
+        t2 = { messageText: t2 };
     }
-    let res = compareStringsCaseSensitive(t1.messageText, t2.messageText);
+    const res = compareStringsCaseSensitive(t1.messageText, t2.messageText);
     if (res) {
         return res;
     }
-    if (!t1.next && !t2.next) {
+
+    return compareMessageChain(t1.next, t2.next);
+}
+
+// First compare by size of the message chain,
+// then compare by content of the message chain.
+function compareMessageChain(
+    c1: DiagnosticMessageChain[] | undefined,
+    c2: DiagnosticMessageChain[] | undefined,
+): Comparison {
+    if (c1 === undefined && c2 === undefined) {
         return Comparison.EqualTo;
     }
-    if (!t1.next) {
+    if (c1 === undefined) {
+        return Comparison.GreaterThan;
+    }
+    if (c2 === undefined) {
         return Comparison.LessThan;
     }
-    if (!t2.next) {
+
+    return compareMessageChainSize(c1, c2) || compareMessageChainContent(c1, c2);
+}
+
+function compareMessageChainSize(
+    c1: DiagnosticMessageChain[] | undefined,
+    c2: DiagnosticMessageChain[] | undefined,
+): Comparison {
+    if (c1 === undefined && c2 === undefined) {
+        return Comparison.EqualTo;
+    }
+    if (c1 === undefined) {
         return Comparison.GreaterThan;
     }
-    const len = Math.min(t1.next.length, t2.next.length);
-    for (let i = 0; i < len; i++) {
-        res = compareMessageText(t1.next[i], t2.next[i]);
+    if (c2 === undefined) {
+        return Comparison.LessThan;
+    }
+
+    let res = compareValues(c2.length, c1.length);
+    if (res) {
+        return res;
+    }
+
+    for (let i = 0; i < c2.length; i++) {
+        res = compareMessageChainSize(c1[i].next, c2[i].next);
         if (res) {
             return res;
         }
     }
-    if (t1.next.length < t2.next.length) {
-        return Comparison.LessThan;
-    }
-    else if (t1.next.length > t2.next.length) {
-        return Comparison.GreaterThan;
+
+    return Comparison.EqualTo;
+}
+
+// Assumes the two chains have the same shape.
+function compareMessageChainContent(
+    c1: DiagnosticMessageChain[],
+    c2: DiagnosticMessageChain[],
+): Comparison {
+    let res;
+    for (let i = 0; i < c2.length; i++) {
+        res = compareStringsCaseSensitive(c1[i].messageText, c2[i].messageText);
+        if (res) {
+            return res;
+        }
+        if (c1[i].next === undefined) {
+            continue;
+        }
+        res = compareMessageChainContent(c1[i].next!, c2[i].next!);
+        if (res) {
+            return res;
+        }
     }
     return Comparison.EqualTo;
 }
 
+/** @internal */
+export function diagnosticsEqualityComparer(d1: Diagnostic, d2: Diagnostic): boolean {
+    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);
+}
+
+function messageTextEqualityComparer(m1: string | DiagnosticMessageChain, m2: string | DiagnosticMessageChain): boolean {
+    const t1 = typeof m1 === "string" ? m1 : m1.messageText;
+    const t2 = typeof m2 === "string" ? m2 : m2.messageText;
+    return compareStringsCaseSensitive(t1, t2) === Comparison.EqualTo;
+}
+
 /** @internal */
 export function getLanguageVariant(scriptKind: ScriptKind) {
     // .tsx and .jsx files are treated as jsx language variant.

src/compiler/utilitiesPublic.ts

@@ -49,6 +49,7 @@ import {
     Decorator,
     Diagnostic,
     Diagnostics,
+    diagnosticsEqualityComparer,
     ElementAccessChain,
     ElementAccessExpression,
     emptyArray,
@@ -304,7 +305,7 @@ export function isExternalModuleNameRelative(moduleName: string): boolean {
 }
 
 export function sortAndDeduplicateDiagnostics<T extends Diagnostic>(diagnostics: readonly T[]): SortedReadonlyArray<T> {
-    return sortAndDeduplicate<T>(diagnostics, compareDiagnostics);
+    return sortAndDeduplicate<T>(diagnostics, compareDiagnostics, diagnosticsEqualityComparer);
 }
 
 export function getDefaultLibFileName(options: CompilerOptions): string {

src/services/completions.ts

@@ -1340,22 +1340,22 @@ function completionInfoFromData(
                 !uniqueNames.has(keywordEntry.name)
             ) {
                 uniqueNames.add(keywordEntry.name);
-                insertSorted(entries, keywordEntry, compareCompletionEntries, /*allowDuplicates*/ true);
+                insertSorted(entries, keywordEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
             }
         }
     }
 
     for (const keywordEntry of getContextualKeywords(contextToken, position)) {
         if (!uniqueNames.has(keywordEntry.name)) {
             uniqueNames.add(keywordEntry.name);
-            insertSorted(entries, keywordEntry, compareCompletionEntries, /*allowDuplicates*/ true);
+            insertSorted(entries, keywordEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
         }
     }
 
     for (const literal of literals) {
         const literalEntry = createCompletionEntryForLiteral(sourceFile, preferences, literal);
         uniqueNames.add(literalEntry.name);
-        insertSorted(entries, literalEntry, compareCompletionEntries, /*allowDuplicates*/ true);
+        insertSorted(entries, literalEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
     }
 
     if (!isChecked) {
@@ -2630,7 +2630,7 @@ export function getCompletionEntriesFromSymbols(
         /** True for locals; false for globals, module exports from other files, `this.` completions. */
         const shouldShadowLaterSymbols = (!origin || originIsTypeOnlyAlias(origin)) && !(symbol.parent === undefined && !some(symbol.declarations, d => d.getSourceFile() === location.getSourceFile()));
         uniques.set(name, shouldShadowLaterSymbols);
-        insertSorted(entries, entry, compareCompletionEntries, /*allowDuplicates*/ true);
+        insertSorted(entries, entry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
     }
 
     log("getCompletionsAtPosition: getCompletionEntriesFromSymbols: " + (timestamp() - start));

src/testRunner/tests.ts

@@ -6,6 +6,7 @@ import "./unittests/comments";
 import "./unittests/compilerCore";
 import "./unittests/convertToBase64";
 import "./unittests/customTransforms";
+import "./unittests/diagnosticCollection";
 import "./unittests/factory";
 import "./unittests/incrementalParser";
 import "./unittests/jsDocParsing";

src/testRunner/unittests/diagnosticCollection.ts

@@ -0,0 +1,120 @@
+import * as ts from "../_namespaces/ts";
+
+describe("unittests:: internalApi:: diagnosticCollection", () => {
+    describe("add", () => {
+        it("keeps equivalent diagnostic with elaboration", () => {
+            const collection = ts.createDiagnosticCollection();
+            const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
+            const node = file.statements[0];
+
+            const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
+            const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
+            collection.add(dy);
+            collection.add(da);
+
+            const chain = ts.chainDiagnosticMessages(
+                ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
+                ts.Diagnostics.Cannot_find_name_0,
+                "y",
+            );
+            const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
+
+            collection.add(dyBetter);
+            const result = collection.getDiagnostics();
+            assert.deepEqual(result, [da, dyBetter]);
+        });
+
+        it("keeps equivalent diagnostic with deeper elaboration", () => {
+            const collection = ts.createDiagnosticCollection();
+            const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
+            const node = file.statements[0];
+
+            const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
+            collection.add(da);
+
+            const chain = ts.chainDiagnosticMessages(
+                ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "a"),
+                ts.Diagnostics.Cannot_find_name_0,
+                "y",
+            );
+            const dy = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
+            collection.add(dy);
+
+            const chainBetter = ts.chainDiagnosticMessages(
+                ts.chainDiagnosticMessages(
+                    ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
+                    ts.Diagnostics.Did_you_mean_0,
+                    "b",
+                ),
+                ts.Diagnostics.Cannot_find_name_0,
+                "y",
+            );
+            const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chainBetter);
+
+            collection.add(dyBetter);
+            const result = collection.getDiagnostics();
+            assert.deepEqual(result, [da, dyBetter]);
+        });
+
+        it("doesn't keep equivalent diagnostic with no elaboration", () => {
+            const collection = ts.createDiagnosticCollection();
+            const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
+            const node = file.statements[0];
+
+            const chain = ts.chainDiagnosticMessages(
+                ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
+                ts.Diagnostics.Cannot_find_name_0,
+                "y",
+            );
+            const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
+            const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
+            collection.add(da);
+            collection.add(dyBetter);
+
+            const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
+            collection.add(dy);
+            const result = collection.getDiagnostics();
+            assert.deepEqual(result, [da, dyBetter]);
+        });
+    });
+
+    describe("lookup", () => {
+        it("returns an equivalent diagnostic with more elaboration", () => {
+            const collection = ts.createDiagnosticCollection();
+            const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
+            const node = file.statements[0];
+
+            const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
+            collection.add(da);
+
+            const chain = ts.chainDiagnosticMessages(
+                ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
+                ts.Diagnostics.Cannot_find_name_0,
+                "y",
+            );
+            const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
+            collection.add(dyBetter);
+
+            const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
+            assert.equal(collection.lookup(dy), dyBetter);
+        });
+        it("doesn't return an equivalent diagnostic with less elaboration", () => {
+            const collection = ts.createDiagnosticCollection();
+            const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
+            const node = file.statements[0];
+
+            const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
+            collection.add(da);
+            const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
+            collection.add(dy);
+
+            const chain = ts.chainDiagnosticMessages(
+                ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
+                ts.Diagnostics.Cannot_find_name_0,
+                "y",
+            );
+            const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
+            assert.equal(collection.lookup(dyBetter), undefined);
+        });
+    });
+});