Skip to content

Commit 7084e6c

Browse files
weswighamweswigham
authored
Adds related spans and error grouping for duplicate identifier errors (microsoft#25328)
* Adds related spans and error grouping for duplicate identifier errors * Trim trailing whitespace * Record related info in error baselines * Make error more whimsical
1 parent 656f356 commit 7084e6c

60 files changed

Lines changed: 1902 additions & 24 deletions

File tree

Some content is hidden

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

‎src/compiler/checker.ts‎

Lines changed: 108 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,7 @@ namespace ts {
422422
const jsObjectLiteralIndexInfo = createIndexInfo(anyType, /*isReadonly*/ false);
423423

424424
const globals = createSymbolTable();
425+
let amalgamatedDuplicates: Map<{ firstFile: SourceFile, secondFile: SourceFile, firstFileInstances: Map<{ instances: Node[], blockScoped: boolean }>, secondFileInstances: Map<{ instances: Node[], blockScoped: boolean }> }> | undefined;
425426
const reverseMappedCache = createMap<Type | undefined>();
426427
let ambientModulesCache: Symbol[] | undefined;
427428
/**
@@ -693,6 +694,28 @@ namespace ts {
693694
return emitResolver;
694695
}
695696

697+
function lookupOrIssueError(location: Node | undefined, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): Diagnostic {
698+
const diagnostic = location
699+
? createDiagnosticForNode(location, message, arg0, arg1, arg2, arg3)
700+
: createCompilerDiagnostic(message, arg0, arg1, arg2, arg3);
701+
const existing = diagnostics.lookup(diagnostic);
702+
if (existing) {
703+
return existing;
704+
}
705+
else {
706+
diagnostics.add(diagnostic);
707+
return diagnostic;
708+
}
709+
}
710+
711+
function addRelatedInfo(diagnostic: Diagnostic, ...relatedInformation: DiagnosticRelatedInformation[]) {
712+
if (!diagnostic.relatedInformation) {
713+
diagnostic.relatedInformation = [];
714+
}
715+
diagnostic.relatedInformation.push(...relatedInformation);
716+
return diagnostic;
717+
}
718+
696719
function error(location: Node | undefined, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): Diagnostic {
697720
const diagnostic = location
698721
? createDiagnosticForNode(location, message, arg0, arg1, arg2, arg3)
@@ -803,23 +826,63 @@ namespace ts {
803826
error(getNameOfDeclaration(source.declarations[0]), Diagnostics.Cannot_augment_module_0_with_value_exports_because_it_resolves_to_a_non_module_entity, symbolToString(target));
804827
}
805828
else {
806-
const message = target.flags & SymbolFlags.Enum || source.flags & SymbolFlags.Enum
829+
const isEitherEnum = !!(target.flags & SymbolFlags.Enum || source.flags & SymbolFlags.Enum);
830+
const isEitherBlockScoped = !!(target.flags & SymbolFlags.BlockScopedVariable || source.flags & SymbolFlags.BlockScopedVariable);
831+
const message = isEitherEnum
807832
? Diagnostics.Enum_declarations_can_only_merge_with_namespace_or_other_enum_declarations
808-
: target.flags & SymbolFlags.BlockScopedVariable || source.flags & SymbolFlags.BlockScopedVariable
833+
: isEitherBlockScoped
809834
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
810835
: Diagnostics.Duplicate_identifier_0;
811-
forEach(source.declarations, node => {
812-
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
813-
error(errorNode, message, symbolToString(source));
814-
});
815-
forEach(target.declarations, node => {
816-
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
817-
error(errorNode, message, symbolToString(source));
818-
});
836+
const sourceSymbolFile = source.declarations && getSourceFileOfNode(source.declarations[0]);
837+
const targetSymbolFile = target.declarations && getSourceFileOfNode(target.declarations[0]);
838+
839+
// Collect top-level duplicate identifier errors into one mapping, so we can then merge their diagnostics if there are a bunch
840+
if (sourceSymbolFile && targetSymbolFile && amalgamatedDuplicates && !isEitherEnum && sourceSymbolFile !== targetSymbolFile) {
841+
const firstFile = comparePaths(sourceSymbolFile.path, targetSymbolFile.path) === Comparison.LessThan ? sourceSymbolFile : targetSymbolFile;
842+
const secondFile = firstFile === sourceSymbolFile ? targetSymbolFile : sourceSymbolFile;
843+
const cacheKey = `${firstFile.path}|${secondFile.path}`;
844+
const existing = amalgamatedDuplicates.get(cacheKey) || { firstFile, secondFile, firstFileInstances: createMap(), secondFileInstances: createMap() };
845+
const symbolName = symbolToString(source);
846+
const firstInstanceList = existing.firstFileInstances.get(symbolName) || { instances: [], blockScoped: isEitherBlockScoped };
847+
const secondInstanceList = existing.secondFileInstances.get(symbolName) || { instances: [], blockScoped: isEitherBlockScoped };
848+
849+
forEach(source.declarations, node => {
850+
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
851+
const targetList = sourceSymbolFile === firstFile ? firstInstanceList : secondInstanceList;
852+
targetList.instances.push(errorNode);
853+
});
854+
forEach(target.declarations, node => {
855+
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
856+
const targetList = targetSymbolFile === firstFile ? firstInstanceList : secondInstanceList;
857+
targetList.instances.push(errorNode);
858+
});
859+
860+
existing.firstFileInstances.set(symbolName, firstInstanceList);
861+
existing.secondFileInstances.set(symbolName, secondInstanceList);
862+
amalgamatedDuplicates.set(cacheKey, existing);
863+
return target;
864+
}
865+
const symbolName = symbolToString(source);
866+
addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target);
867+
addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source);
819868
}
820869
return target;
821870
}
822871

872+
function addDuplicateDeclarationErrorsForSymbols(target: Symbol, message: DiagnosticMessage, symbolName: string, source: Symbol) {
873+
forEach(target.declarations, node => {
874+
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
875+
addDuplicateDeclarationError(errorNode, message, symbolName, source.declarations && source.declarations[0]);
876+
});
877+
}
878+
879+
function addDuplicateDeclarationError(errorNode: Node, message: DiagnosticMessage, symbolName: string, relatedNode: Node | undefined) {
880+
const err = lookupOrIssueError(errorNode, message, symbolName);
881+
if (relatedNode && length(err.relatedInformation) < 5) {
882+
addRelatedInfo(err, !length(err.relatedInformation) ? createDiagnosticForNode(relatedNode, Diagnostics._0_was_also_declared_here, symbolName) : createDiagnosticForNode(relatedNode, Diagnostics.and_here));
883+
}
884+
}
885+
823886
function combineSymbolTables(first: SymbolTable | undefined, second: SymbolTable | undefined): SymbolTable | undefined {
824887
if (!hasEntries(first)) return second;
825888
if (!hasEntries(second)) return first;
@@ -27449,6 +27512,8 @@ namespace ts {
2744927512
bindSourceFile(file, compilerOptions);
2745027513
}
2745127514

27515+
amalgamatedDuplicates = createMap();
27516+
2745227517
// Initialize global symbol table
2745327518
let augmentations: ReadonlyArray<StringLiteral | Identifier>[] | undefined;
2745427519
for (const file of host.getSourceFiles()) {
@@ -27526,6 +27591,39 @@ namespace ts {
2752627591
}
2752727592
}
2752827593
}
27594+
27595+
amalgamatedDuplicates.forEach(({ firstFile, secondFile, firstFileInstances, secondFileInstances }) => {
27596+
const conflictingKeys = arrayFrom(firstFileInstances.keys());
27597+
// If not many things conflict, issue individual errors
27598+
if (conflictingKeys.length < 8) {
27599+
addErrorsForDuplicates(firstFileInstances, secondFileInstances);
27600+
addErrorsForDuplicates(secondFileInstances, firstFileInstances);
27601+
return;
27602+
}
27603+
// Otheriwse issue top-level error since the files appear very identical in terms of what they appear
27604+
const list = conflictingKeys.join(", ");
27605+
diagnostics.add(addRelatedInfo(
27606+
createDiagnosticForNode(firstFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
27607+
createDiagnosticForNode(secondFile, Diagnostics.Conflicts_are_in_this_file)
27608+
));
27609+
diagnostics.add(addRelatedInfo(
27610+
createDiagnosticForNode(secondFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
27611+
createDiagnosticForNode(firstFile, Diagnostics.Conflicts_are_in_this_file)
27612+
));
27613+
});
27614+
amalgamatedDuplicates = undefined;
27615+
27616+
function addErrorsForDuplicates(secondFileInstances: Map<{ instances: Node[]; blockScoped: boolean; }>, firstFileInstances: Map<{ instances: Node[]; blockScoped: boolean; }>) {
27617+
secondFileInstances.forEach((locations, symbolName) => {
27618+
const firstFileEquivalent = firstFileInstances.get(symbolName)!;
27619+
const message = locations.blockScoped
27620+
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
27621+
: Diagnostics.Duplicate_identifier_0;
27622+
locations.instances.forEach(node => {
27623+
addDuplicateDeclarationError(node, message, symbolName, firstFileEquivalent.instances[0]);
27624+
});
27625+
});
27626+
}
2752927627
}
2753027628

2753127629
function checkExternalEmitHelpers(location: Node, helpers: ExternalEmitHelpers) {

‎src/compiler/diagnosticMessages.json‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3603,6 +3603,22 @@
36033603
"code": 6199,
36043604
"reportsUnnecessary": true
36053605
},
3606+
"Definitions of the following identifiers conflict with those in another file: {0}": {
3607+
"category": "Error",
3608+
"code": 6200
3609+
},
3610+
"Conflicts are in this file.": {
3611+
"category": "Message",
3612+
"code": 6201
3613+
},
3614+
"'{0}' was also declared here.": {
3615+
"category": "Message",
3616+
"code": 6203
3617+
},
3618+
"and here.": {
3619+
"category": "Message",
3620+
"code": 6204
3621+
},
36063622

36073623
"Projects to reference": {
36083624
"category": "Message",

‎src/compiler/program.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -329,16 +329,17 @@ namespace ts {
329329
return context;
330330
}
331331

332-
function formatLocation(file: SourceFile, start: number, host: FormatDiagnosticsHost) {
332+
/* @internal */
333+
export function formatLocation(file: SourceFile, start: number, host: FormatDiagnosticsHost, color = formatColorAndReset) {
333334
const { line: firstLine, character: firstLineChar } = getLineAndCharacterOfPosition(file, start); // TODO: GH#18217
334335
const relativeFileName = host ? convertToRelativePath(file.fileName, host.getCurrentDirectory(), fileName => host.getCanonicalFileName(fileName)) : file.fileName;
335336

336337
let output = "";
337-
output += formatColorAndReset(relativeFileName, ForegroundColorEscapeSequences.Cyan);
338+
output += color(relativeFileName, ForegroundColorEscapeSequences.Cyan);
338339
output += ":";
339-
output += formatColorAndReset(`${firstLine + 1}`, ForegroundColorEscapeSequences.Yellow);
340+
output += color(`${firstLine + 1}`, ForegroundColorEscapeSequences.Yellow);
340341
output += ":";
341-
output += formatColorAndReset(`${firstLineChar + 1}`, ForegroundColorEscapeSequences.Yellow);
342+
output += color(`${firstLineChar + 1}`, ForegroundColorEscapeSequences.Yellow);
342343
return output;
343344
}
344345

‎src/compiler/types.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5330,6 +5330,9 @@ namespace ts {
53305330
// Adds a diagnostic to this diagnostic collection.
53315331
add(diagnostic: Diagnostic): void;
53325332

5333+
// Returns the first existing diagnostic that is equivalent to the given one (sans related information)
5334+
lookup(diagnostic: Diagnostic): Diagnostic | undefined;
5335+
53335336
// Gets all the diagnostics that aren't associated with a file.
53345337
getGlobalDiagnostics(): Diagnostic[];
53355338

‎src/compiler/utilities.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2878,6 +2878,7 @@ namespace ts {
28782878

28792879
return {
28802880
add,
2881+
lookup,
28812882
getGlobalDiagnostics,
28822883
getDiagnostics,
28832884
reattachFileDiagnostics
@@ -2887,6 +2888,24 @@ namespace ts {
28872888
forEach(fileDiagnostics.get(newFile.fileName), diagnostic => diagnostic.file = newFile);
28882889
}
28892890

2891+
function lookup(diagnostic: Diagnostic): Diagnostic | undefined {
2892+
let diagnostics: SortedArray<Diagnostic> | undefined;
2893+
if (diagnostic.file) {
2894+
diagnostics = fileDiagnostics.get(diagnostic.file.fileName);
2895+
}
2896+
else {
2897+
diagnostics = nonFileDiagnostics;
2898+
}
2899+
if (!diagnostics) {
2900+
return undefined;
2901+
}
2902+
const result = binarySearch(diagnostics, diagnostic, identity, compareDiagnosticsSkipRelatedInformation);
2903+
if (result >= 0) {
2904+
return diagnostics[result];
2905+
}
2906+
return undefined;
2907+
}
2908+
28902909
function add(diagnostic: Diagnostic): void {
28912910
let diagnostics: SortedArray<Diagnostic> | undefined;
28922911
if (diagnostic.file) {
@@ -6838,6 +6857,13 @@ namespace ts {
68386857

68396858
/* @internal */
68406859
export function compareDiagnostics(d1: Diagnostic, d2: Diagnostic): Comparison {
6860+
return compareDiagnosticsSkipRelatedInformation(d1, d2) ||
6861+
compareRelatedInformation(d1, d2) ||
6862+
Comparison.EqualTo;
6863+
}
6864+
6865+
/* @internal */
6866+
export function compareDiagnosticsSkipRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison {
68416867
return compareStringsCaseSensitive(getDiagnosticFilePath(d1), getDiagnosticFilePath(d2)) ||
68426868
compareValues(d1.start, d2.start) ||
68436869
compareValues(d1.length, d2.length) ||
@@ -6846,6 +6872,19 @@ namespace ts {
68466872
Comparison.EqualTo;
68476873
}
68486874

6875+
function compareRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison {
6876+
if (!d1.relatedInformation && !d2.relatedInformation) {
6877+
return Comparison.EqualTo;
6878+
}
6879+
if (d1.relatedInformation && d2.relatedInformation) {
6880+
return compareValues(d1.relatedInformation.length, d2.relatedInformation.length) || forEach(d1.relatedInformation, (d1i, index) => {
6881+
const d2i = d2.relatedInformation![index];
6882+
return compareDiagnostics(d1i, d2i); // EqualTo is 0, so falsy, and will cause the next item to be compared
6883+
}) || Comparison.EqualTo;
6884+
}
6885+
return d1.relatedInformation ? Comparison.LessThan : Comparison.GreaterThan;
6886+
}
6887+
68496888
function compareMessageText(t1: string | DiagnosticMessageChain, t2: string | DiagnosticMessageChain): Comparison {
68506889
let text1: string | DiagnosticMessageChain | undefined = t1;
68516890
let text2: string | DiagnosticMessageChain | undefined = t2;

‎src/harness/harness.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1348,6 +1348,12 @@ namespace Harness {
13481348
return "\r\n";
13491349
}
13501350

1351+
const formatDiagnsoticHost = {
1352+
getCurrentDirectory: () => options && options.currentDirectory ? options.currentDirectory : "",
1353+
getNewLine: () => IO.newLine(),
1354+
getCanonicalFileName: ts.createGetCanonicalFileName(options && options.caseSensitive !== undefined ? options.caseSensitive : true),
1355+
};
1356+
13511357
function outputErrorText(error: ts.Diagnostic) {
13521358
const message = ts.flattenDiagnosticMessageText(error.messageText, IO.newLine());
13531359

@@ -1356,6 +1362,11 @@ namespace Harness {
13561362
.map(s => s.length > 0 && s.charAt(s.length - 1) === "\r" ? s.substr(0, s.length - 1) : s)
13571363
.filter(s => s.length > 0)
13581364
.map(s => "!!! " + ts.diagnosticCategoryName(error) + " TS" + error.code + ": " + s);
1365+
if (error.relatedInformation) {
1366+
for (const info of error.relatedInformation) {
1367+
errLines.push(`!!! related TS${info.code}${info.file ? " " + ts.formatLocation(info.file, info.start!, formatDiagnsoticHost, ts.identity) : ""}: ${ts.flattenDiagnosticMessageText(info.messageText, IO.newLine())}`);
1368+
}
1369+
}
13591370
errLines.forEach(e => outputLines += (newLine() + e));
13601371
errorsReported++;
13611372

‎tests/baselines/reference/api/tsserverlibrary.d.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4505,6 +4505,7 @@ declare namespace ts {
45054505
}
45064506
interface DiagnosticCollection {
45074507
add(diagnostic: Diagnostic): void;
4508+
lookup(diagnostic: Diagnostic): Diagnostic | undefined;
45084509
getGlobalDiagnostics(): Diagnostic[];
45094510
getDiagnostics(fileName: string): DiagnosticWithLocation[];
45104511
getDiagnostics(): Diagnostic[];
@@ -5716,6 +5717,10 @@ declare namespace ts {
57165717
Include_modules_imported_with_json_extension: DiagnosticMessage;
57175718
All_destructured_elements_are_unused: DiagnosticMessage;
57185719
All_variables_are_unused: DiagnosticMessage;
5720+
Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0: DiagnosticMessage;
5721+
Conflicts_are_in_this_file: DiagnosticMessage;
5722+
_0_was_also_declared_here: DiagnosticMessage;
5723+
and_here: DiagnosticMessage;
57195724
Projects_to_reference: DiagnosticMessage;
57205725
Enable_project_compilation: DiagnosticMessage;
57215726
Project_references_may_not_form_a_circular_graph_Cycle_detected_Colon_0: DiagnosticMessage;
@@ -7083,6 +7088,7 @@ declare namespace ts {
70837088
function chainDiagnosticMessages(details: DiagnosticMessageChain | undefined, message: DiagnosticMessage, ...args: (string | undefined)[]): DiagnosticMessageChain;
70847089
function concatenateDiagnosticMessageChains(headChain: DiagnosticMessageChain, tailChain: DiagnosticMessageChain): DiagnosticMessageChain;
70857090
function compareDiagnostics(d1: Diagnostic, d2: Diagnostic): Comparison;
7091+
function compareDiagnosticsSkipRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison;
70867092
function getEmitScriptTarget(compilerOptions: CompilerOptions): ScriptTarget;
70877093
function getEmitModuleKind(compilerOptions: {
70887094
module?: CompilerOptions["module"];
@@ -8913,6 +8919,7 @@ declare namespace ts {
89138919
}
89148920
/** @internal */
89158921
function formatColorAndReset(text: string, formatStyle: string): string;
8922+
function formatLocation(file: SourceFile, start: number, host: FormatDiagnosticsHost, color?: typeof formatColorAndReset): string;
89168923
function formatDiagnosticsWithColorAndContext(diagnostics: ReadonlyArray<Diagnostic>, host: FormatDiagnosticsHost): string;
89178924
function flattenDiagnosticMessageText(messageText: string | DiagnosticMessageChain | undefined, newLine: string): string;
89188925
/**

0 commit comments

Comments
 (0)