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

Adds related spans and error grouping for duplicate identifier errors #25328

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
118 changes: 108 additions & 10 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ namespace ts {
const jsObjectLiteralIndexInfo = createIndexInfo(anyType, /*isReadonly*/ false);

const globals = createSymbolTable();
let amalgamatedDuplicates: Map<{ firstFile: SourceFile, secondFile: SourceFile, firstFileInstances: Map<{ instances: Node[], blockScoped: boolean }>, secondFileInstances: Map<{ instances: Node[], blockScoped: boolean }> }> | undefined;
const reverseMappedCache = createMap<Type | undefined>();
let ambientModulesCache: Symbol[] | undefined;
/**
Expand Down Expand Up @@ -833,6 +834,28 @@ namespace ts {
return emitResolver;
}

function lookupOrIssueError(location: Node | undefined, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): Diagnostic {
const diagnostic = location
? createDiagnosticForNode(location, message, arg0, arg1, arg2, arg3)
: createCompilerDiagnostic(message, arg0, arg1, arg2, arg3);
const existing = diagnostics.lookup(diagnostic);
if (existing) {
return existing;
}
else {
diagnostics.add(diagnostic);
return diagnostic;
}
}

function addRelatedInfo(diagnostic: Diagnostic, ...relatedInformation: DiagnosticRelatedInformation[]) {
if (!diagnostic.relatedInformation) {
diagnostic.relatedInformation = [];
}
diagnostic.relatedInformation.push(...relatedInformation);
return diagnostic;
}

function error(location: Node | undefined, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): Diagnostic {
const diagnostic = location
? createDiagnosticForNode(location, message, arg0, arg1, arg2, arg3)
Expand Down Expand Up @@ -943,23 +966,63 @@ namespace ts {
error(getNameOfDeclaration(source.declarations[0]), Diagnostics.Cannot_augment_module_0_with_value_exports_because_it_resolves_to_a_non_module_entity, symbolToString(target));
}
else {
const message = target.flags & SymbolFlags.Enum || source.flags & SymbolFlags.Enum
const isEitherEnum = !!(target.flags & SymbolFlags.Enum || source.flags & SymbolFlags.Enum);
const isEitherBlockScoped = !!(target.flags & SymbolFlags.BlockScopedVariable || source.flags & SymbolFlags.BlockScopedVariable);
const message = isEitherEnum
? Diagnostics.Enum_declarations_can_only_merge_with_namespace_or_other_enum_declarations
: target.flags & SymbolFlags.BlockScopedVariable || source.flags & SymbolFlags.BlockScopedVariable
: isEitherBlockScoped
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
: Diagnostics.Duplicate_identifier_0;
forEach(source.declarations, node => {
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
error(errorNode, message, symbolToString(source));
});
forEach(target.declarations, node => {
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
error(errorNode, message, symbolToString(source));
});
const sourceSymbolFile = source.declarations && getSourceFileOfNode(source.declarations[0]);
const targetSymbolFile = target.declarations && getSourceFileOfNode(target.declarations[0]);

// Collect top-level duplicate identifier errors into one mapping, so we can then merge their diagnostics if there are a bunch
if (sourceSymbolFile && targetSymbolFile && amalgamatedDuplicates && !isEitherEnum && sourceSymbolFile !== targetSymbolFile) {
const firstFile = comparePaths(sourceSymbolFile.path, targetSymbolFile.path) === Comparison.LessThan ? sourceSymbolFile : targetSymbolFile;
const secondFile = firstFile === sourceSymbolFile ? targetSymbolFile : sourceSymbolFile;
const cacheKey = `${firstFile.path}|${secondFile.path}`;
const existing = amalgamatedDuplicates.get(cacheKey) || { firstFile, secondFile, firstFileInstances: createMap(), secondFileInstances: createMap() };
const symbolName = symbolToString(source);
const firstInstanceList = existing.firstFileInstances.get(symbolName) || { instances: [], blockScoped: isEitherBlockScoped };
const secondInstanceList = existing.secondFileInstances.get(symbolName) || { instances: [], blockScoped: isEitherBlockScoped };

forEach(source.declarations, node => {
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
const targetList = sourceSymbolFile === firstFile ? firstInstanceList : secondInstanceList;
targetList.instances.push(errorNode);
});
forEach(target.declarations, node => {
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
const targetList = targetSymbolFile === firstFile ? firstInstanceList : secondInstanceList;
targetList.instances.push(errorNode);
});

existing.firstFileInstances.set(symbolName, firstInstanceList);
existing.secondFileInstances.set(symbolName, secondInstanceList);
amalgamatedDuplicates.set(cacheKey, existing);
return target;
}
const symbolName = symbolToString(source);
addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target);
addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source);
}
return target;
}

function addDuplicateDeclarationErrorsForSymbols(target: Symbol, message: DiagnosticMessage, symbolName: string, source: Symbol) {
forEach(target.declarations, node => {
const errorNode = (getJavascriptInitializer(node, /*isPrototypeAssignment*/ false) ? getOuterNameOfJsInitializer(node) : getNameOfDeclaration(node)) || node;
addDuplicateDeclarationError(errorNode, message, symbolName, source.declarations && source.declarations[0]);
});
}

function addDuplicateDeclarationError(errorNode: Node, message: DiagnosticMessage, symbolName: string, relatedNode: Node | undefined) {
const err = lookupOrIssueError(errorNode, message, symbolName);
if (relatedNode && length(err.relatedInformation) < 5) {
addRelatedInfo(err, !length(err.relatedInformation) ? createDiagnosticForNode(relatedNode, Diagnostics._0_was_also_declared_here, symbolName) : createDiagnosticForNode(relatedNode, Diagnostics.and_here));
}
}

function combineSymbolTables(first: SymbolTable | undefined, second: SymbolTable | undefined): SymbolTable | undefined {
if (!hasEntries(first)) return second;
if (!hasEntries(second)) return first;
Expand Down Expand Up @@ -27571,6 +27634,8 @@ namespace ts {
bindSourceFile(file, compilerOptions);
}

amalgamatedDuplicates = createMap();

// Initialize global symbol table
let augmentations: ReadonlyArray<StringLiteral | Identifier>[] | undefined;
for (const file of host.getSourceFiles()) {
Expand Down Expand Up @@ -27648,6 +27713,39 @@ namespace ts {
}
}
}

amalgamatedDuplicates.forEach(({ firstFile, secondFile, firstFileInstances, secondFileInstances }) => {
const conflictingKeys = arrayFrom(firstFileInstances.keys());
// If not many things conflict, issue individual errors
if (conflictingKeys.length < 8) {
addErrorsForDuplicates(firstFileInstances, secondFileInstances);
addErrorsForDuplicates(secondFileInstances, firstFileInstances);
return;
}
// Otheriwse issue top-level error since the files appear very identical in terms of what they appear
const list = conflictingKeys.join(", ");
diagnostics.add(addRelatedInfo(
createDiagnosticForNode(firstFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
createDiagnosticForNode(secondFile, Diagnostics.Conflicts_are_in_this_file)
));
diagnostics.add(addRelatedInfo(
createDiagnosticForNode(secondFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
createDiagnosticForNode(firstFile, Diagnostics.Conflicts_are_in_this_file)
));
});
amalgamatedDuplicates = undefined;

function addErrorsForDuplicates(secondFileInstances: Map<{ instances: Node[]; blockScoped: boolean; }>, firstFileInstances: Map<{ instances: Node[]; blockScoped: boolean; }>) {
secondFileInstances.forEach((locations, symbolName) => {
const firstFileEquivalent = firstFileInstances.get(symbolName)!;
const message = locations.blockScoped
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
: Diagnostics.Duplicate_identifier_0;
locations.instances.forEach(node => {
addDuplicateDeclarationError(node, message, symbolName, firstFileEquivalent.instances[0]);
});
});
}
}

function checkExternalEmitHelpers(location: Node, helpers: ExternalEmitHelpers) {
Expand Down
16 changes: 16 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3603,6 +3603,22 @@
"code": 6199,
"reportsUnnecessary": true
},
"Definitions of the following identifiers conflict with those in another file: {0}": {
"category": "Error",
"code": 6200
},
"Conflicts are in this file.": {
"category": "Message",
"code": 6201
},
"'{0}' was also declared here.": {
"category": "Message",
"code": 6203
},
"and here.": {
"category": "Message",
"code": 6204
},

"Projects to reference": {
"category": "Message",
Expand Down
9 changes: 5 additions & 4 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,16 +329,17 @@ namespace ts {
return context;
}

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

let output = "";
output += formatColorAndReset(relativeFileName, ForegroundColorEscapeSequences.Cyan);
output += color(relativeFileName, ForegroundColorEscapeSequences.Cyan);
output += ":";
output += formatColorAndReset(`${firstLine + 1}`, ForegroundColorEscapeSequences.Yellow);
output += color(`${firstLine + 1}`, ForegroundColorEscapeSequences.Yellow);
output += ":";
output += formatColorAndReset(`${firstLineChar + 1}`, ForegroundColorEscapeSequences.Yellow);
output += color(`${firstLineChar + 1}`, ForegroundColorEscapeSequences.Yellow);
return output;
}

Expand Down
3 changes: 3 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5351,6 +5351,9 @@ namespace ts {
// Adds a diagnostic to this diagnostic collection.
add(diagnostic: Diagnostic): void;

// Returns the first existing diagnostic that is equivalent to the given one (sans related information)
lookup(diagnostic: Diagnostic): Diagnostic | undefined;

// Gets all the diagnostics that aren't associated with a file.
getGlobalDiagnostics(): Diagnostic[];

Expand Down
39 changes: 39 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2887,6 +2887,7 @@ namespace ts {

return {
add,
lookup,
getGlobalDiagnostics,
getDiagnostics,
reattachFileDiagnostics
Expand All @@ -2896,6 +2897,24 @@ namespace ts {
forEach(fileDiagnostics.get(newFile.fileName), diagnostic => diagnostic.file = newFile);
}

function lookup(diagnostic: Diagnostic): Diagnostic | undefined {
let diagnostics: SortedArray<Diagnostic> | undefined;
if (diagnostic.file) {
diagnostics = fileDiagnostics.get(diagnostic.file.fileName);
}
else {
diagnostics = nonFileDiagnostics;
}
if (!diagnostics) {
return undefined;
}
const result = binarySearch(diagnostics, diagnostic, identity, compareDiagnosticsSkipRelatedInformation);
if (result >= 0) {
return diagnostics[result];
}
return undefined;
}

function add(diagnostic: Diagnostic): void {
let diagnostics: SortedArray<Diagnostic> | undefined;
if (diagnostic.file) {
Expand Down Expand Up @@ -6852,6 +6871,13 @@ namespace ts {

/* @internal */
export function compareDiagnostics(d1: Diagnostic, d2: Diagnostic): Comparison {
return compareDiagnosticsSkipRelatedInformation(d1, d2) ||
compareRelatedInformation(d1, d2) ||
Comparison.EqualTo;
}

/* @internal */
export function compareDiagnosticsSkipRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison {
return compareStringsCaseSensitive(getDiagnosticFilePath(d1), getDiagnosticFilePath(d2)) ||
compareValues(d1.start, d2.start) ||
compareValues(d1.length, d2.length) ||
Expand All @@ -6860,6 +6886,19 @@ namespace ts {
Comparison.EqualTo;
}

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) => {
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 {
let text1: string | DiagnosticMessageChain | undefined = t1;
let text2: string | DiagnosticMessageChain | undefined = t2;
Expand Down
11 changes: 11 additions & 0 deletions src/harness/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,12 @@ namespace Harness {
return "\r\n";
}

const formatDiagnsoticHost = {
getCurrentDirectory: () => options && options.currentDirectory ? options.currentDirectory : "",
getNewLine: () => IO.newLine(),
getCanonicalFileName: ts.createGetCanonicalFileName(options && options.caseSensitive !== undefined ? options.caseSensitive : true),
};

function outputErrorText(error: ts.Diagnostic) {
const message = ts.flattenDiagnosticMessageText(error.messageText, IO.newLine());

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

Expand Down
7 changes: 7 additions & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4527,6 +4527,7 @@ declare namespace ts {
}
interface DiagnosticCollection {
add(diagnostic: Diagnostic): void;
lookup(diagnostic: Diagnostic): Diagnostic | undefined;
getGlobalDiagnostics(): Diagnostic[];
getDiagnostics(fileName: string): DiagnosticWithLocation[];
getDiagnostics(): Diagnostic[];
Expand Down Expand Up @@ -5738,6 +5739,10 @@ declare namespace ts {
Include_modules_imported_with_json_extension: DiagnosticMessage;
All_destructured_elements_are_unused: DiagnosticMessage;
All_variables_are_unused: DiagnosticMessage;
Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0: DiagnosticMessage;
Conflicts_are_in_this_file: DiagnosticMessage;
_0_was_also_declared_here: DiagnosticMessage;
and_here: DiagnosticMessage;
Projects_to_reference: DiagnosticMessage;
Enable_project_compilation: DiagnosticMessage;
Project_references_may_not_form_a_circular_graph_Cycle_detected_Colon_0: DiagnosticMessage;
Expand Down Expand Up @@ -7116,6 +7121,7 @@ declare namespace ts {
function chainDiagnosticMessages(details: DiagnosticMessageChain | undefined, message: DiagnosticMessage, ...args: (string | undefined)[]): DiagnosticMessageChain;
function concatenateDiagnosticMessageChains(headChain: DiagnosticMessageChain, tailChain: DiagnosticMessageChain): DiagnosticMessageChain;
function compareDiagnostics(d1: Diagnostic, d2: Diagnostic): Comparison;
function compareDiagnosticsSkipRelatedInformation(d1: Diagnostic, d2: Diagnostic): Comparison;
function getEmitScriptTarget(compilerOptions: CompilerOptions): ScriptTarget;
function getEmitModuleKind(compilerOptions: {
module?: CompilerOptions["module"];
Expand Down Expand Up @@ -8946,6 +8952,7 @@ declare namespace ts {
}
/** @internal */
function formatColorAndReset(text: string, formatStyle: string): string;
function formatLocation(file: SourceFile, start: number, host: FormatDiagnosticsHost, color?: typeof formatColorAndReset): string;
function formatDiagnosticsWithColorAndContext(diagnostics: ReadonlyArray<Diagnostic>, host: FormatDiagnosticsHost): string;
function flattenDiagnosticMessageText(messageText: string | DiagnosticMessageChain | undefined, newLine: string): string;
/**
Expand Down
Loading