Skip to content

Commit

Permalink
Adds related spans and error grouping for duplicate identifier errors (…
Browse files Browse the repository at this point in the history 
…#25328)

* Adds related spans and error grouping for duplicate identifier errors

* Trim trailing whitespace

* Record related info in error baselines

* Make error more whimsical
  • Loading branch information
@weswigham
weswigham authored Jul 2, 2018
1 parent 656f356 commit 7084e6c
Show file tree
Hide file tree
Showing 60 changed files with 1,902 additions and 24 deletions.
118 changes: 108 additions & 10 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,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 @@ -693,6 +694,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 @@ -803,23 +826,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 @@ -27449,6 +27512,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 @@ -27526,6 +27591,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
View file
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
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5330,6 +5330,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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2878,6 +2878,7 @@ namespace ts {

return {
add,
lookup,
getGlobalDiagnostics,
getDiagnostics,
reattachFileDiagnostics
Expand All @@ -2887,6 +2888,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 @@ -6838,6 +6857,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 @@ -6846,6 +6872,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
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4505,6 +4505,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 @@ -5716,6 +5717,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 @@ -7083,6 +7088,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 @@ -8913,6 +8919,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

0 comments on commit 7084e6c

Please sign in to comment.