Skip to content

Commit ebcb09d

Browse files
gabrittogabritto
authored
Improve diagnostics deduplication (microsoft#58220)
1 parent 1db1376 commit ebcb09d

13 files changed

+474
-33
lines changed

‎src/compiler/core.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -808,14 +808,30 @@ export function createSortedArray<T>(): SortedArray<T> {
808808
}
809809

810810
/** @internal */
811-
export function insertSorted<T>(array: SortedArray<T>, insert: T, compare: Comparer<T>, allowDuplicates?: boolean): boolean {
811+
export function insertSorted<T>(
812+
array: SortedArray<T>,
813+
insert: T,
814+
compare: Comparer<T>,
815+
equalityComparer?: EqualityComparer<T>,
816+
allowDuplicates?: boolean,
817+
): boolean {
812818
if (array.length === 0) {
813819
array.push(insert);
814820
return true;
815821
}
816822

817823
const insertIndex = binarySearch(array, insert, identity, compare);
818824
if (insertIndex < 0) {
825+
if (equalityComparer && !allowDuplicates) {
826+
const idx = ~insertIndex;
827+
if (idx > 0 && equalityComparer(insert, array[idx - 1])) {
828+
return false;
829+
}
830+
if (idx < array.length && equalityComparer(insert, array[idx])) {
831+
array.splice(idx, 1, insert);
832+
return true;
833+
}
834+
}
819835
array.splice(~insertIndex, 0, insert);
820836
return true;
821837
}

‎src/compiler/utilities.ts‎

Lines changed: 94 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5864,6 +5864,9 @@ export function createDiagnosticCollection(): DiagnosticCollection {
58645864
if (result >= 0) {
58655865
return diagnostics[result];
58665866
}
5867+
if (~result > 0 && diagnosticsEqualityComparer(diagnostic, diagnostics[~result - 1])) {
5868+
return diagnostics[~result - 1];
5869+
}
58675870
return undefined;
58685871
}
58695872

@@ -5887,7 +5890,7 @@ export function createDiagnosticCollection(): DiagnosticCollection {
58875890
diagnostics = nonFileDiagnostics;
58885891
}
58895892

5890-
insertSorted(diagnostics, diagnostic, compareDiagnosticsSkipRelatedInformation);
5893+
insertSorted(diagnostics, diagnostic, compareDiagnosticsSkipRelatedInformation, diagnosticsEqualityComparer);
58915894
}
58925895

58935896
function getGlobalDiagnostics(): Diagnostic[] {
@@ -8545,58 +8548,130 @@ export function compareDiagnosticsSkipRelatedInformation(d1: Diagnostic, d2: Dia
85458548
Comparison.EqualTo;
85468549
}
85478550

8551+
// A diagnostic with more elaboration should be considered *less than* a diagnostic
8552+
// with less elaboration that is otherwise similar.
85488553
function compareRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison {
85498554
if (!d1.relatedInformation && !d2.relatedInformation) {
85508555
return Comparison.EqualTo;
85518556
}
85528557
if (d1.relatedInformation && d2.relatedInformation) {
8553-
return compareValues(d1.relatedInformation.length, d2.relatedInformation.length) || forEach(d1.relatedInformation, (d1i, index) => {
8558+
return compareValues(d2.relatedInformation.length, d1.relatedInformation.length) || forEach(d1.relatedInformation, (d1i, index) => {
85548559
const d2i = d2.relatedInformation![index];
85558560
return compareDiagnostics(d1i, d2i); // EqualTo is 0, so falsy, and will cause the next item to be compared
85568561
}) || Comparison.EqualTo;
85578562
}
85588563
return d1.relatedInformation ? Comparison.LessThan : Comparison.GreaterThan;
85598564
}
85608565

8561-
function compareMessageText(t1: string | DiagnosticMessageChain, t2: string | DiagnosticMessageChain): Comparison {
8566+
// An diagnostic message with more elaboration should be considered *less than* a diagnostic message
8567+
// with less elaboration that is otherwise similar.
8568+
function compareMessageText(
8569+
t1: string | Pick<DiagnosticMessageChain, "messageText" | "next">,
8570+
t2: string | Pick<DiagnosticMessageChain, "messageText" | "next">,
8571+
): Comparison {
85628572
if (typeof t1 === "string" && typeof t2 === "string") {
85638573
return compareStringsCaseSensitive(t1, t2);
85648574
}
8565-
else if (typeof t1 === "string") {
8566-
return Comparison.LessThan;
8575+
8576+
if (typeof t1 === "string") {
8577+
t1 = { messageText: t1 };
85678578
}
8568-
else if (typeof t2 === "string") {
8569-
return Comparison.GreaterThan;
8579+
if (typeof t2 === "string") {
8580+
t2 = { messageText: t2 };
85708581
}
8571-
let res = compareStringsCaseSensitive(t1.messageText, t2.messageText);
8582+
const res = compareStringsCaseSensitive(t1.messageText, t2.messageText);
85728583
if (res) {
85738584
return res;
85748585
}
8575-
if (!t1.next && !t2.next) {
8586+
8587+
return compareMessageChain(t1.next, t2.next);
8588+
}
8589+
8590+
// First compare by size of the message chain,
8591+
// then compare by content of the message chain.
8592+
function compareMessageChain(
8593+
c1: DiagnosticMessageChain[] | undefined,
8594+
c2: DiagnosticMessageChain[] | undefined,
8595+
): Comparison {
8596+
if (c1 === undefined && c2 === undefined) {
85768597
return Comparison.EqualTo;
85778598
}
8578-
if (!t1.next) {
8599+
if (c1 === undefined) {
8600+
return Comparison.GreaterThan;
8601+
}
8602+
if (c2 === undefined) {
85798603
return Comparison.LessThan;
85808604
}
8581-
if (!t2.next) {
8605+
8606+
return compareMessageChainSize(c1, c2) || compareMessageChainContent(c1, c2);
8607+
}
8608+
8609+
function compareMessageChainSize(
8610+
c1: DiagnosticMessageChain[] | undefined,
8611+
c2: DiagnosticMessageChain[] | undefined,
8612+
): Comparison {
8613+
if (c1 === undefined && c2 === undefined) {
8614+
return Comparison.EqualTo;
8615+
}
8616+
if (c1 === undefined) {
85828617
return Comparison.GreaterThan;
85838618
}
8584-
const len = Math.min(t1.next.length, t2.next.length);
8585-
for (let i = 0; i < len; i++) {
8586-
res = compareMessageText(t1.next[i], t2.next[i]);
8619+
if (c2 === undefined) {
8620+
return Comparison.LessThan;
8621+
}
8622+
8623+
let res = compareValues(c2.length, c1.length);
8624+
if (res) {
8625+
return res;
8626+
}
8627+
8628+
for (let i = 0; i < c2.length; i++) {
8629+
res = compareMessageChainSize(c1[i].next, c2[i].next);
85878630
if (res) {
85888631
return res;
85898632
}
85908633
}
8591-
if (t1.next.length < t2.next.length) {
8592-
return Comparison.LessThan;
8593-
}
8594-
else if (t1.next.length > t2.next.length) {
8595-
return Comparison.GreaterThan;
8634+
8635+
return Comparison.EqualTo;
8636+
}
8637+
8638+
// Assumes the two chains have the same shape.
8639+
function compareMessageChainContent(
8640+
c1: DiagnosticMessageChain[],
8641+
c2: DiagnosticMessageChain[],
8642+
): Comparison {
8643+
let res;
8644+
for (let i = 0; i < c2.length; i++) {
8645+
res = compareStringsCaseSensitive(c1[i].messageText, c2[i].messageText);
8646+
if (res) {
8647+
return res;
8648+
}
8649+
if (c1[i].next === undefined) {
8650+
continue;
8651+
}
8652+
res = compareMessageChainContent(c1[i].next!, c2[i].next!);
8653+
if (res) {
8654+
return res;
8655+
}
85968656
}
85978657
return Comparison.EqualTo;
85988658
}
85998659

8660+
/** @internal */
8661+
export function diagnosticsEqualityComparer(d1: Diagnostic, d2: Diagnostic): boolean {
8662+
return compareStringsCaseSensitive(getDiagnosticFilePath(d1), getDiagnosticFilePath(d2)) === Comparison.EqualTo &&
8663+
compareValues(d1.start, d2.start) === Comparison.EqualTo &&
8664+
compareValues(d1.length, d2.length) === Comparison.EqualTo &&
8665+
compareValues(d1.code, d2.code) === Comparison.EqualTo &&
8666+
messageTextEqualityComparer(d1.messageText, d2.messageText);
8667+
}
8668+
8669+
function messageTextEqualityComparer(m1: string | DiagnosticMessageChain, m2: string | DiagnosticMessageChain): boolean {
8670+
const t1 = typeof m1 === "string" ? m1 : m1.messageText;
8671+
const t2 = typeof m2 === "string" ? m2 : m2.messageText;
8672+
return compareStringsCaseSensitive(t1, t2) === Comparison.EqualTo;
8673+
}
8674+
86008675
/** @internal */
86018676
export function getLanguageVariant(scriptKind: ScriptKind) {
86028677
// .tsx and .jsx files are treated as jsx language variant.

‎src/compiler/utilitiesPublic.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ import {
4949
Decorator,
5050
Diagnostic,
5151
Diagnostics,
52+
diagnosticsEqualityComparer,
5253
ElementAccessChain,
5354
ElementAccessExpression,
5455
emptyArray,
@@ -304,7 +305,7 @@ export function isExternalModuleNameRelative(moduleName: string): boolean {
304305
}
305306

306307
export function sortAndDeduplicateDiagnostics<T extends Diagnostic>(diagnostics: readonly T[]): SortedReadonlyArray<T> {
307-
return sortAndDeduplicate<T>(diagnostics, compareDiagnostics);
308+
return sortAndDeduplicate<T>(diagnostics, compareDiagnostics, diagnosticsEqualityComparer);
308309
}
309310

310311
export function getDefaultLibFileName(options: CompilerOptions): string {

‎src/services/completions.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1340,22 +1340,22 @@ function completionInfoFromData(
13401340
!uniqueNames.has(keywordEntry.name)
13411341
) {
13421342
uniqueNames.add(keywordEntry.name);
1343-
insertSorted(entries, keywordEntry, compareCompletionEntries, /*allowDuplicates*/ true);
1343+
insertSorted(entries, keywordEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
13441344
}
13451345
}
13461346
}
13471347

13481348
for (const keywordEntry of getContextualKeywords(contextToken, position)) {
13491349
if (!uniqueNames.has(keywordEntry.name)) {
13501350
uniqueNames.add(keywordEntry.name);
1351-
insertSorted(entries, keywordEntry, compareCompletionEntries, /*allowDuplicates*/ true);
1351+
insertSorted(entries, keywordEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
13521352
}
13531353
}
13541354

13551355
for (const literal of literals) {
13561356
const literalEntry = createCompletionEntryForLiteral(sourceFile, preferences, literal);
13571357
uniqueNames.add(literalEntry.name);
1358-
insertSorted(entries, literalEntry, compareCompletionEntries, /*allowDuplicates*/ true);
1358+
insertSorted(entries, literalEntry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
13591359
}
13601360

13611361
if (!isChecked) {
@@ -2630,7 +2630,7 @@ export function getCompletionEntriesFromSymbols(
26302630
/** True for locals; false for globals, module exports from other files, `this.` completions. */
26312631
const shouldShadowLaterSymbols = (!origin || originIsTypeOnlyAlias(origin)) && !(symbol.parent === undefined && !some(symbol.declarations, d => d.getSourceFile() === location.getSourceFile()));
26322632
uniques.set(name, shouldShadowLaterSymbols);
2633-
insertSorted(entries, entry, compareCompletionEntries, /*allowDuplicates*/ true);
2633+
insertSorted(entries, entry, compareCompletionEntries, /*equalityComparer*/ undefined, /*allowDuplicates*/ true);
26342634
}
26352635

26362636
log("getCompletionsAtPosition: getCompletionEntriesFromSymbols: " + (timestamp() - start));

‎src/testRunner/tests.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import "./unittests/comments";
66
import "./unittests/compilerCore";
77
import "./unittests/convertToBase64";
88
import "./unittests/customTransforms";
9+
import "./unittests/diagnosticCollection";
910
import "./unittests/factory";
1011
import "./unittests/incrementalParser";
1112
import "./unittests/jsDocParsing";

‎src/testRunner/unittests/diagnosticCollection.ts‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
import * as ts from "../_namespaces/ts";
2+
3+
describe("unittests:: internalApi:: diagnosticCollection", () => {
4+
describe("add", () => {
5+
it("keeps equivalent diagnostic with elaboration", () => {
6+
const collection = ts.createDiagnosticCollection();
7+
const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
8+
const node = file.statements[0];
9+
10+
const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
11+
const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
12+
collection.add(dy);
13+
collection.add(da);
14+
15+
const chain = ts.chainDiagnosticMessages(
16+
ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
17+
ts.Diagnostics.Cannot_find_name_0,
18+
"y",
19+
);
20+
const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
21+
22+
collection.add(dyBetter);
23+
const result = collection.getDiagnostics();
24+
assert.deepEqual(result, [da, dyBetter]);
25+
});
26+
27+
it("keeps equivalent diagnostic with deeper elaboration", () => {
28+
const collection = ts.createDiagnosticCollection();
29+
const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
30+
const node = file.statements[0];
31+
32+
const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
33+
collection.add(da);
34+
35+
const chain = ts.chainDiagnosticMessages(
36+
ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "a"),
37+
ts.Diagnostics.Cannot_find_name_0,
38+
"y",
39+
);
40+
const dy = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
41+
collection.add(dy);
42+
43+
const chainBetter = ts.chainDiagnosticMessages(
44+
ts.chainDiagnosticMessages(
45+
ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
46+
ts.Diagnostics.Did_you_mean_0,
47+
"b",
48+
),
49+
ts.Diagnostics.Cannot_find_name_0,
50+
"y",
51+
);
52+
const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chainBetter);
53+
54+
collection.add(dyBetter);
55+
const result = collection.getDiagnostics();
56+
assert.deepEqual(result, [da, dyBetter]);
57+
});
58+
59+
it("doesn't keep equivalent diagnostic with no elaboration", () => {
60+
const collection = ts.createDiagnosticCollection();
61+
const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
62+
const node = file.statements[0];
63+
64+
const chain = ts.chainDiagnosticMessages(
65+
ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
66+
ts.Diagnostics.Cannot_find_name_0,
67+
"y",
68+
);
69+
const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
70+
const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
71+
collection.add(da);
72+
collection.add(dyBetter);
73+
74+
const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
75+
collection.add(dy);
76+
const result = collection.getDiagnostics();
77+
assert.deepEqual(result, [da, dyBetter]);
78+
});
79+
});
80+
81+
describe("lookup", () => {
82+
it("returns an equivalent diagnostic with more elaboration", () => {
83+
const collection = ts.createDiagnosticCollection();
84+
const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
85+
const node = file.statements[0];
86+
87+
const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
88+
collection.add(da);
89+
90+
const chain = ts.chainDiagnosticMessages(
91+
ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
92+
ts.Diagnostics.Cannot_find_name_0,
93+
"y",
94+
);
95+
const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
96+
collection.add(dyBetter);
97+
98+
const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
99+
assert.equal(collection.lookup(dy), dyBetter);
100+
});
101+
it("doesn't return an equivalent diagnostic with less elaboration", () => {
102+
const collection = ts.createDiagnosticCollection();
103+
const file = ts.createSourceFile("index.ts", "const x = 1", ts.ScriptTarget.ESNext, /*setParentNodes*/ true);
104+
const node = file.statements[0];
105+
106+
const da = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "a");
107+
collection.add(da);
108+
const dy = ts.createDiagnosticForNode(node, ts.Diagnostics.Cannot_find_name_0, "y");
109+
collection.add(dy);
110+
111+
const chain = ts.chainDiagnosticMessages(
112+
ts.chainDiagnosticMessages(/*details*/ undefined, ts.Diagnostics.Did_you_mean_0, "x"),
113+
ts.Diagnostics.Cannot_find_name_0,
114+
"y",
115+
);
116+
const dyBetter = ts.createDiagnosticForNodeFromMessageChain(file, node, chain);
117+
assert.equal(collection.lookup(dyBetter), undefined);
118+
});
119+
});
120+
});

0 commit comments

Comments
 (0)