From 9c388faf2d08651ebcd47054fbfce00ba762187b Mon Sep 17 00:00:00 2001 From: andy-ms Date: Fri, 21 Sep 2018 17:31:44 -0700 Subject: [PATCH 1/2] Clean up amalgamatedDuplicates --- src/compiler/checker.ts | 110 +++++++++--------- src/compiler/utilities.ts | 12 ++ ...RelatedSpans_moduleAugmentation.errors.txt | 29 +++++ ...entifierRelatedSpans_moduleAugmentation.js | 24 ++++ ...ierRelatedSpans_moduleAugmentation.symbols | 21 ++++ ...ifierRelatedSpans_moduleAugmentation.types | 24 ++++ ...entifierRelatedSpans_moduleAugmentation.ts | 13 +++ 7 files changed, 177 insertions(+), 56 deletions(-) create mode 100644 tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.errors.txt create mode 100644 tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.js create mode 100644 tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.symbols create mode 100644 tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.types create mode 100644 tests/cases/compiler/duplicateIdentifierRelatedSpans_moduleAugmentation.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index cecc70c3e2d97..a1cc0066d2693 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -464,7 +464,19 @@ namespace ts { const enumNumberIndexInfo = createIndexInfo(stringType, /*isReadonly*/ true); const globals = createSymbolTable(); - let amalgamatedDuplicates: Map<{ firstFile: SourceFile, secondFile: SourceFile, firstFileInstances: Map<{ instances: Node[], blockScoped: boolean }>, secondFileInstances: Map<{ instances: Node[], blockScoped: boolean }> }> | undefined; + interface DuplicateInfoForSymbol { + readonly firstFileLocations: Node[]; + readonly secondFileLocations: Node[]; + readonly isBlockScoped: boolean; + } + interface DuplicateInfoForFiles { + readonly firstFile: SourceFile; + readonly secondFile: SourceFile; + /** Key is symbol name. */ + readonly conflictingSymbols: Map; + } + /** Key is "/path/to/a.ts|/path/to/b.ts". */ + let amalgamatedDuplicates: Map | undefined; const reverseMappedCache = createMap(); let ambientModulesCache: Symbol[] | undefined; /** @@ -883,36 +895,26 @@ namespace ts { : Diagnostics.Duplicate_identifier_0; const sourceSymbolFile = source.declarations && getSourceFileOfNode(source.declarations[0]); const targetSymbolFile = target.declarations && getSourceFileOfNode(target.declarations[0]); + const symbolName = symbolToString(source); // 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 = (getExpandoInitializer(node, /*isPrototypeAssignment*/ false) ? getNameOfExpando(node) : getNameOfDeclaration(node)) || node; - const targetList = sourceSymbolFile === firstFile ? firstInstanceList : secondInstanceList; - targetList.instances.push(errorNode); - }); - forEach(target.declarations, node => { - const errorNode = (getExpandoInitializer(node, /*isPrototypeAssignment*/ false) ? getNameOfExpando(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 filesDuplicates = getOrUpdate(amalgamatedDuplicates, `${firstFile.path}|${secondFile.path}`, () => + ({ firstFile, secondFile, conflictingSymbols: createMap() })); + const conflictingSymbolInfo = getOrUpdate(filesDuplicates.conflictingSymbols, symbolName, () => + ({ isBlockScoped: isEitherBlockScoped, firstFileLocations: [], secondFileLocations: [] })); + for (const { locs, symbol } of [{ locs: conflictingSymbolInfo.firstFileLocations, symbol: source }, { locs: conflictingSymbolInfo.secondFileLocations, symbol: target }]) { + for (const decl of symbol.declarations) { + pushIfUnique(locs, (getExpandoInitializer(decl, /*isPrototypeAssignment*/ false) ? getNameOfExpando(decl) : getNameOfDeclaration(decl)) || decl); + } + } + } + else { + addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target); + addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source); } - const symbolName = symbolToString(source); - addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target); - addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source); } return target; } @@ -920,13 +922,15 @@ namespace ts { function addDuplicateDeclarationErrorsForSymbols(target: Symbol, message: DiagnosticMessage, symbolName: string, source: Symbol) { forEach(target.declarations, node => { const errorNode = (getExpandoInitializer(node, /*isPrototypeAssignment*/ false) ? getNameOfExpando(node) : getNameOfDeclaration(node)) || node; - addDuplicateDeclarationError(errorNode, message, symbolName, source.declarations && source.declarations[0]); + addDuplicateDeclarationError(errorNode, message, symbolName, source.declarations); }); } - function addDuplicateDeclarationError(errorNode: Node, message: DiagnosticMessage, symbolName: string, relatedNode: Node | undefined) { + function addDuplicateDeclarationError(errorNode: Node, message: DiagnosticMessage, symbolName: string, relatedNodes: ReadonlyArray | undefined) { const err = lookupOrIssueError(errorNode, message, symbolName); - if (relatedNode && length(err.relatedInformation) < 5) { + for (const relatedNode of relatedNodes || emptyArray) { + err.relatedInformation = err.relatedInformation || []; + if (length(err.relatedInformation) >= 5) continue; addRelatedInfo(err, !length(err.relatedInformation) ? createDiagnosticForNode(relatedNode, Diagnostics._0_was_also_declared_here, symbolName) : createDiagnosticForNode(relatedNode, Diagnostics.and_here)); } } @@ -28842,38 +28846,32 @@ namespace ts { } } - amalgamatedDuplicates.forEach(({ firstFile, secondFile, firstFileInstances, secondFileInstances }) => { - const conflictingKeys = arrayFrom(firstFileInstances.keys()); + amalgamatedDuplicates.forEach(({ firstFile, secondFile, conflictingSymbols }) => { // If not many things conflict, issue individual errors - if (conflictingKeys.length < 8) { - addErrorsForDuplicates(firstFileInstances, secondFileInstances); - addErrorsForDuplicates(secondFileInstances, firstFileInstances); - return; + if (conflictingSymbols.size < 8) { + conflictingSymbols.forEach(({ isBlockScoped, firstFileLocations, secondFileLocations }, symbolName) => { + const message = isBlockScoped ? Diagnostics.Cannot_redeclare_block_scoped_variable_0 : Diagnostics.Duplicate_identifier_0; + for (const { firstLocs, secondLocs } of [{ firstLocs: firstFileLocations, secondLocs: secondFileLocations }, { firstLocs: secondFileLocations, secondLocs: firstFileLocations }]) { + for (const node of firstLocs) { + addDuplicateDeclarationError(node, message, symbolName, secondLocs); + } + } + }); + } + else { + // Otheriwse issue top-level error since the files appear very identical in terms of what they appear + const list = arrayFrom(conflictingSymbols.keys()).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) + )); } - // 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) { diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 00e35fb0a8588..adf70d9353751 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -8369,4 +8369,16 @@ namespace ts { export function isJsonEqual(a: unknown, b: unknown): boolean { return a === b || typeof a === "object" && a !== null && typeof b === "object" && b !== null && equalOwnProperties(a as MapLike, b as MapLike, isJsonEqual); } + + export function getOrUpdate(map: Map, key: string, getValue: () => T): T { + const got = map.get(key); + if (got === undefined) { + const value = getValue(); + map.set(key, value); + return value; + } + else { + return got; + } + } } diff --git a/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.errors.txt b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.errors.txt new file mode 100644 index 0000000000000..bd0ab1f3cfe6e --- /dev/null +++ b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.errors.txt @@ -0,0 +1,29 @@ +/dir/a.ts(1,14): error TS2451: Cannot redeclare block-scoped variable 'x'. +/dir/b.ts(4,18): error TS2451: Cannot redeclare block-scoped variable 'x'. +/dir/b.ts(8,18): error TS2451: Cannot redeclare block-scoped variable 'x'. + + +==== /dir/a.ts (1 errors) ==== + export const x = 0; + ~ +!!! error TS2451: Cannot redeclare block-scoped variable 'x'. +!!! related TS6203 /dir/b.ts:4:18: 'x' was also declared here. +!!! related TS6204 /dir/b.ts:8:18: and here. + +==== /dir/b.ts (2 errors) ==== + export {}; + + declare module "./a" { + export const x = 0; + ~ +!!! error TS2451: Cannot redeclare block-scoped variable 'x'. +!!! related TS6203 /dir/a.ts:1:14: 'x' was also declared here. + } + + declare module "../dir/a" { + export const x = 0; + ~ +!!! error TS2451: Cannot redeclare block-scoped variable 'x'. +!!! related TS6203 /dir/a.ts:1:14: 'x' was also declared here. + } + \ No newline at end of file diff --git a/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.js b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.js new file mode 100644 index 0000000000000..265e948f309f0 --- /dev/null +++ b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.js @@ -0,0 +1,24 @@ +//// [tests/cases/compiler/duplicateIdentifierRelatedSpans_moduleAugmentation.ts] //// + +//// [a.ts] +export const x = 0; + +//// [b.ts] +export {}; + +declare module "./a" { + export const x = 0; +} + +declare module "../dir/a" { + export const x = 0; +} + + +//// [a.js] +"use strict"; +exports.__esModule = true; +exports.x = 0; +//// [b.js] +"use strict"; +exports.__esModule = true; diff --git a/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.symbols b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.symbols new file mode 100644 index 0000000000000..ad332076db966 --- /dev/null +++ b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.symbols @@ -0,0 +1,21 @@ +=== /dir/a.ts === +export const x = 0; +>x : Symbol(x, Decl(a.ts, 0, 12)) + +=== /dir/b.ts === +export {}; + +declare module "./a" { +>"./a" : Symbol("/dir/a", Decl(a.ts, 0, 0), Decl(b.ts, 0, 10), Decl(b.ts, 4, 1)) + + export const x = 0; +>x : Symbol(x, Decl(b.ts, 3, 16)) +} + +declare module "../dir/a" { +>"../dir/a" : Symbol("/dir/a", Decl(a.ts, 0, 0), Decl(b.ts, 0, 10), Decl(b.ts, 4, 1)) + + export const x = 0; +>x : Symbol(x, Decl(b.ts, 7, 16)) +} + diff --git a/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.types b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.types new file mode 100644 index 0000000000000..9b749fa651046 --- /dev/null +++ b/tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.types @@ -0,0 +1,24 @@ +=== /dir/a.ts === +export const x = 0; +>x : 0 +>0 : 0 + +=== /dir/b.ts === +export {}; + +declare module "./a" { +>"./a" : typeof import("/dir/a") + + export const x = 0; +>x : 0 +>0 : 0 +} + +declare module "../dir/a" { +>"../dir/a" : typeof import("/dir/a") + + export const x = 0; +>x : 0 +>0 : 0 +} + diff --git a/tests/cases/compiler/duplicateIdentifierRelatedSpans_moduleAugmentation.ts b/tests/cases/compiler/duplicateIdentifierRelatedSpans_moduleAugmentation.ts new file mode 100644 index 0000000000000..8df0b66da6041 --- /dev/null +++ b/tests/cases/compiler/duplicateIdentifierRelatedSpans_moduleAugmentation.ts @@ -0,0 +1,13 @@ +// @Filename: /dir/a.ts +export const x = 0; + +// @Filename: /dir/b.ts +export {}; + +declare module "./a" { + export const x = 0; +} + +declare module "../dir/a" { + export const x = 0; +} From 45779c6e00fcdf3ecd75aa1f003e2114758b040b Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 1 Oct 2018 10:49:52 -0700 Subject: [PATCH 2/2] Code review --- src/compiler/checker.ts | 24 ++++++++++++++---------- src/compiler/utilities.ts | 4 ++-- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index a1cc0066d2693..23a6fb939b075 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -905,11 +905,8 @@ namespace ts { ({ firstFile, secondFile, conflictingSymbols: createMap() })); const conflictingSymbolInfo = getOrUpdate(filesDuplicates.conflictingSymbols, symbolName, () => ({ isBlockScoped: isEitherBlockScoped, firstFileLocations: [], secondFileLocations: [] })); - for (const { locs, symbol } of [{ locs: conflictingSymbolInfo.firstFileLocations, symbol: source }, { locs: conflictingSymbolInfo.secondFileLocations, symbol: target }]) { - for (const decl of symbol.declarations) { - pushIfUnique(locs, (getExpandoInitializer(decl, /*isPrototypeAssignment*/ false) ? getNameOfExpando(decl) : getNameOfDeclaration(decl)) || decl); - } - } + addDuplicateLocations(conflictingSymbolInfo.firstFileLocations, source); + addDuplicateLocations(conflictingSymbolInfo.secondFileLocations, target); } else { addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target); @@ -917,6 +914,12 @@ namespace ts { } } return target; + + function addDuplicateLocations(locs: Node[], symbol: Symbol): void { + for (const decl of symbol.declarations) { + pushIfUnique(locs, (getExpandoInitializer(decl, /*isPrototypeAssignment*/ false) ? getNameOfExpando(decl) : getNameOfDeclaration(decl)) || decl); + } + } } function addDuplicateDeclarationErrorsForSymbols(target: Symbol, message: DiagnosticMessage, symbolName: string, source: Symbol) { @@ -28851,15 +28854,16 @@ namespace ts { if (conflictingSymbols.size < 8) { conflictingSymbols.forEach(({ isBlockScoped, firstFileLocations, secondFileLocations }, symbolName) => { const message = isBlockScoped ? Diagnostics.Cannot_redeclare_block_scoped_variable_0 : Diagnostics.Duplicate_identifier_0; - for (const { firstLocs, secondLocs } of [{ firstLocs: firstFileLocations, secondLocs: secondFileLocations }, { firstLocs: secondFileLocations, secondLocs: firstFileLocations }]) { - for (const node of firstLocs) { - addDuplicateDeclarationError(node, message, symbolName, secondLocs); - } + for (const node of firstFileLocations) { + addDuplicateDeclarationError(node, message, symbolName, secondFileLocations); + } + for (const node of secondFileLocations) { + addDuplicateDeclarationError(node, message, symbolName, firstFileLocations); } }); } else { - // Otheriwse issue top-level error since the files appear very identical in terms of what they appear + // Otherwise issue top-level error since the files appear very identical in terms of what they contain const list = arrayFrom(conflictingSymbols.keys()).join(", "); diagnostics.add(addRelatedInfo( createDiagnosticForNode(firstFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list), diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index adf70d9353751..0df9ade1ddb99 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -8370,10 +8370,10 @@ namespace ts { return a === b || typeof a === "object" && a !== null && typeof b === "object" && b !== null && equalOwnProperties(a as MapLike, b as MapLike, isJsonEqual); } - export function getOrUpdate(map: Map, key: string, getValue: () => T): T { + export function getOrUpdate(map: Map, key: string, getDefault: () => T): T { const got = map.get(key); if (got === undefined) { - const value = getValue(); + const value = getDefault(); map.set(key, value); return value; }