Skip to content

Commit 8feddcd

Browse files
author
Andy
authored
Clean up amalgamatedDuplicates (#27285)
* Clean up amalgamatedDuplicates * Code review
1 parent 115636b commit 8feddcd

7 files changed

+181
-56
lines changed

‎src/compiler/checker.ts

Lines changed: 58 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,19 @@ namespace ts {
465465
const enumNumberIndexInfo = createIndexInfo(stringType, /*isReadonly*/ true);
466466

467467
const globals = createSymbolTable();
468-
let amalgamatedDuplicates: Map<{ firstFile: SourceFile, secondFile: SourceFile, firstFileInstances: Map<{ instances: Node[], blockScoped: boolean }>, secondFileInstances: Map<{ instances: Node[], blockScoped: boolean }> }> | undefined;
468+
interface DuplicateInfoForSymbol {
469+
readonly firstFileLocations: Node[];
470+
readonly secondFileLocations: Node[];
471+
readonly isBlockScoped: boolean;
472+
}
473+
interface DuplicateInfoForFiles {
474+
readonly firstFile: SourceFile;
475+
readonly secondFile: SourceFile;
476+
/** Key is symbol name. */
477+
readonly conflictingSymbols: Map<DuplicateInfoForSymbol>;
478+
}
479+
/** Key is "/path/to/a.ts|/path/to/b.ts". */
480+
let amalgamatedDuplicates: Map<DuplicateInfoForFiles> | undefined;
469481
const reverseMappedCache = createMap<Type | undefined>();
470482
let ambientModulesCache: Symbol[] | undefined;
471483
/**
@@ -886,50 +898,45 @@ namespace ts {
886898
: Diagnostics.Duplicate_identifier_0;
887899
const sourceSymbolFile = source.declarations && getSourceFileOfNode(source.declarations[0]);
888900
const targetSymbolFile = target.declarations && getSourceFileOfNode(target.declarations[0]);
901+
const symbolName = symbolToString(source);
889902

890903
// Collect top-level duplicate identifier errors into one mapping, so we can then merge their diagnostics if there are a bunch
891904
if (sourceSymbolFile && targetSymbolFile && amalgamatedDuplicates && !isEitherEnum && sourceSymbolFile !== targetSymbolFile) {
892905
const firstFile = comparePaths(sourceSymbolFile.path, targetSymbolFile.path) === Comparison.LessThan ? sourceSymbolFile : targetSymbolFile;
893906
const secondFile = firstFile === sourceSymbolFile ? targetSymbolFile : sourceSymbolFile;
894-
const cacheKey = `${firstFile.path}|${secondFile.path}`;
895-
const existing = amalgamatedDuplicates.get(cacheKey) || { firstFile, secondFile, firstFileInstances: createMap(), secondFileInstances: createMap() };
896-
const symbolName = symbolToString(source);
897-
const firstInstanceList = existing.firstFileInstances.get(symbolName) || { instances: [], blockScoped: isEitherBlockScoped };
898-
const secondInstanceList = existing.secondFileInstances.get(symbolName) || { instances: [], blockScoped: isEitherBlockScoped };
899-
900-
forEach(source.declarations, node => {
901-
const errorNode = (getExpandoInitializer(node, /*isPrototypeAssignment*/ false) ? getNameOfExpando(node) : getNameOfDeclaration(node)) || node;
902-
const targetList = sourceSymbolFile === firstFile ? firstInstanceList : secondInstanceList;
903-
targetList.instances.push(errorNode);
904-
});
905-
forEach(target.declarations, node => {
906-
const errorNode = (getExpandoInitializer(node, /*isPrototypeAssignment*/ false) ? getNameOfExpando(node) : getNameOfDeclaration(node)) || node;
907-
const targetList = targetSymbolFile === firstFile ? firstInstanceList : secondInstanceList;
908-
targetList.instances.push(errorNode);
909-
});
910-
911-
existing.firstFileInstances.set(symbolName, firstInstanceList);
912-
existing.secondFileInstances.set(symbolName, secondInstanceList);
913-
amalgamatedDuplicates.set(cacheKey, existing);
914-
return target;
907+
const filesDuplicates = getOrUpdate<DuplicateInfoForFiles>(amalgamatedDuplicates, `${firstFile.path}|${secondFile.path}`, () =>
908+
({ firstFile, secondFile, conflictingSymbols: createMap() }));
909+
const conflictingSymbolInfo = getOrUpdate<DuplicateInfoForSymbol>(filesDuplicates.conflictingSymbols, symbolName, () =>
910+
({ isBlockScoped: isEitherBlockScoped, firstFileLocations: [], secondFileLocations: [] }));
911+
addDuplicateLocations(conflictingSymbolInfo.firstFileLocations, source);
912+
addDuplicateLocations(conflictingSymbolInfo.secondFileLocations, target);
913+
}
914+
else {
915+
addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target);
916+
addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source);
915917
}
916-
const symbolName = symbolToString(source);
917-
addDuplicateDeclarationErrorsForSymbols(source, message, symbolName, target);
918-
addDuplicateDeclarationErrorsForSymbols(target, message, symbolName, source);
919918
}
920919
return target;
920+
921+
function addDuplicateLocations(locs: Node[], symbol: Symbol): void {
922+
for (const decl of symbol.declarations) {
923+
pushIfUnique(locs, (getExpandoInitializer(decl, /*isPrototypeAssignment*/ false) ? getNameOfExpando(decl) : getNameOfDeclaration(decl)) || decl);
924+
}
925+
}
921926
}
922927

923928
function addDuplicateDeclarationErrorsForSymbols(target: Symbol, message: DiagnosticMessage, symbolName: string, source: Symbol) {
924929
forEach(target.declarations, node => {
925930
const errorNode = (getExpandoInitializer(node, /*isPrototypeAssignment*/ false) ? getNameOfExpando(node) : getNameOfDeclaration(node)) || node;
926-
addDuplicateDeclarationError(errorNode, message, symbolName, source.declarations && source.declarations[0]);
931+
addDuplicateDeclarationError(errorNode, message, symbolName, source.declarations);
927932
});
928933
}
929934

930-
function addDuplicateDeclarationError(errorNode: Node, message: DiagnosticMessage, symbolName: string, relatedNode: Node | undefined) {
935+
function addDuplicateDeclarationError(errorNode: Node, message: DiagnosticMessage, symbolName: string, relatedNodes: ReadonlyArray<Node> | undefined) {
931936
const err = lookupOrIssueError(errorNode, message, symbolName);
932-
if (relatedNode && length(err.relatedInformation) < 5) {
937+
for (const relatedNode of relatedNodes || emptyArray) {
938+
err.relatedInformation = err.relatedInformation || [];
939+
if (length(err.relatedInformation) >= 5) continue;
933940
addRelatedInfo(err, !length(err.relatedInformation) ? createDiagnosticForNode(relatedNode, Diagnostics._0_was_also_declared_here, symbolName) : createDiagnosticForNode(relatedNode, Diagnostics.and_here));
934941
}
935942
}
@@ -28901,38 +28908,33 @@ namespace ts {
2890128908
}
2890228909
}
2890328910

28904-
amalgamatedDuplicates.forEach(({ firstFile, secondFile, firstFileInstances, secondFileInstances }) => {
28905-
const conflictingKeys = arrayFrom(firstFileInstances.keys());
28911+
amalgamatedDuplicates.forEach(({ firstFile, secondFile, conflictingSymbols }) => {
2890628912
// If not many things conflict, issue individual errors
28907-
if (conflictingKeys.length < 8) {
28908-
addErrorsForDuplicates(firstFileInstances, secondFileInstances);
28909-
addErrorsForDuplicates(secondFileInstances, firstFileInstances);
28910-
return;
28913+
if (conflictingSymbols.size < 8) {
28914+
conflictingSymbols.forEach(({ isBlockScoped, firstFileLocations, secondFileLocations }, symbolName) => {
28915+
const message = isBlockScoped ? Diagnostics.Cannot_redeclare_block_scoped_variable_0 : Diagnostics.Duplicate_identifier_0;
28916+
for (const node of firstFileLocations) {
28917+
addDuplicateDeclarationError(node, message, symbolName, secondFileLocations);
28918+
}
28919+
for (const node of secondFileLocations) {
28920+
addDuplicateDeclarationError(node, message, symbolName, firstFileLocations);
28921+
}
28922+
});
28923+
}
28924+
else {
28925+
// Otherwise issue top-level error since the files appear very identical in terms of what they contain
28926+
const list = arrayFrom(conflictingSymbols.keys()).join(", ");
28927+
diagnostics.add(addRelatedInfo(
28928+
createDiagnosticForNode(firstFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
28929+
createDiagnosticForNode(secondFile, Diagnostics.Conflicts_are_in_this_file)
28930+
));
28931+
diagnostics.add(addRelatedInfo(
28932+
createDiagnosticForNode(secondFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
28933+
createDiagnosticForNode(firstFile, Diagnostics.Conflicts_are_in_this_file)
28934+
));
2891128935
}
28912-
// Otheriwse issue top-level error since the files appear very identical in terms of what they appear
28913-
const list = conflictingKeys.join(", ");
28914-
diagnostics.add(addRelatedInfo(
28915-
createDiagnosticForNode(firstFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
28916-
createDiagnosticForNode(secondFile, Diagnostics.Conflicts_are_in_this_file)
28917-
));
28918-
diagnostics.add(addRelatedInfo(
28919-
createDiagnosticForNode(secondFile, Diagnostics.Definitions_of_the_following_identifiers_conflict_with_those_in_another_file_Colon_0, list),
28920-
createDiagnosticForNode(firstFile, Diagnostics.Conflicts_are_in_this_file)
28921-
));
2892228936
});
2892328937
amalgamatedDuplicates = undefined;
28924-
28925-
function addErrorsForDuplicates(secondFileInstances: Map<{ instances: Node[]; blockScoped: boolean; }>, firstFileInstances: Map<{ instances: Node[]; blockScoped: boolean; }>) {
28926-
secondFileInstances.forEach((locations, symbolName) => {
28927-
const firstFileEquivalent = firstFileInstances.get(symbolName)!;
28928-
const message = locations.blockScoped
28929-
? Diagnostics.Cannot_redeclare_block_scoped_variable_0
28930-
: Diagnostics.Duplicate_identifier_0;
28931-
locations.instances.forEach(node => {
28932-
addDuplicateDeclarationError(node, message, symbolName, firstFileEquivalent.instances[0]);
28933-
});
28934-
});
28935-
}
2893628938
}
2893728939

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

‎src/compiler/utilities.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8369,4 +8369,16 @@ namespace ts {
83698369
export function isJsonEqual(a: unknown, b: unknown): boolean {
83708370
return a === b || typeof a === "object" && a !== null && typeof b === "object" && b !== null && equalOwnProperties(a as MapLike<unknown>, b as MapLike<unknown>, isJsonEqual);
83718371
}
8372+
8373+
export function getOrUpdate<T>(map: Map<T>, key: string, getDefault: () => T): T {
8374+
const got = map.get(key);
8375+
if (got === undefined) {
8376+
const value = getDefault();
8377+
map.set(key, value);
8378+
return value;
8379+
}
8380+
else {
8381+
return got;
8382+
}
8383+
}
83728384
}

‎tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.errors.txt

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/dir/a.ts(1,14): error TS2451: Cannot redeclare block-scoped variable 'x'.
2+
/dir/b.ts(4,18): error TS2451: Cannot redeclare block-scoped variable 'x'.
3+
/dir/b.ts(8,18): error TS2451: Cannot redeclare block-scoped variable 'x'.
4+
5+
6+
==== /dir/a.ts (1 errors) ====
7+
export const x = 0;
8+
~
9+
!!! error TS2451: Cannot redeclare block-scoped variable 'x'.
10+
!!! related TS6203 /dir/b.ts:4:18: 'x' was also declared here.
11+
!!! related TS6204 /dir/b.ts:8:18: and here.
12+
13+
==== /dir/b.ts (2 errors) ====
14+
export {};
15+
16+
declare module "./a" {
17+
export const x = 0;
18+
~
19+
!!! error TS2451: Cannot redeclare block-scoped variable 'x'.
20+
!!! related TS6203 /dir/a.ts:1:14: 'x' was also declared here.
21+
}
22+
23+
declare module "../dir/a" {
24+
export const x = 0;
25+
~
26+
!!! error TS2451: Cannot redeclare block-scoped variable 'x'.
27+
!!! related TS6203 /dir/a.ts:1:14: 'x' was also declared here.
28+
}
29+

‎tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
//// [tests/cases/compiler/duplicateIdentifierRelatedSpans_moduleAugmentation.ts] ////
2+
3+
//// [a.ts]
4+
export const x = 0;
5+
6+
//// [b.ts]
7+
export {};
8+
9+
declare module "./a" {
10+
export const x = 0;
11+
}
12+
13+
declare module "../dir/a" {
14+
export const x = 0;
15+
}
16+
17+
18+
//// [a.js]
19+
"use strict";
20+
exports.__esModule = true;
21+
exports.x = 0;
22+
//// [b.js]
23+
"use strict";
24+
exports.__esModule = true;

‎tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.symbols

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
=== /dir/a.ts ===
2+
export const x = 0;
3+
>x : Symbol(x, Decl(a.ts, 0, 12))
4+
5+
=== /dir/b.ts ===
6+
export {};
7+
8+
declare module "./a" {
9+
>"./a" : Symbol("/dir/a", Decl(a.ts, 0, 0), Decl(b.ts, 0, 10), Decl(b.ts, 4, 1))
10+
11+
export const x = 0;
12+
>x : Symbol(x, Decl(b.ts, 3, 16))
13+
}
14+
15+
declare module "../dir/a" {
16+
>"../dir/a" : Symbol("/dir/a", Decl(a.ts, 0, 0), Decl(b.ts, 0, 10), Decl(b.ts, 4, 1))
17+
18+
export const x = 0;
19+
>x : Symbol(x, Decl(b.ts, 7, 16))
20+
}
21+

‎tests/baselines/reference/duplicateIdentifierRelatedSpans_moduleAugmentation.types

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
=== /dir/a.ts ===
2+
export const x = 0;
3+
>x : 0
4+
>0 : 0
5+
6+
=== /dir/b.ts ===
7+
export {};
8+
9+
declare module "./a" {
10+
>"./a" : typeof import("/dir/a")
11+
12+
export const x = 0;
13+
>x : 0
14+
>0 : 0
15+
}
16+
17+
declare module "../dir/a" {
18+
>"../dir/a" : typeof import("/dir/a")
19+
20+
export const x = 0;
21+
>x : 0
22+
>0 : 0
23+
}
24+

‎tests/cases/compiler/duplicateIdentifierRelatedSpans_moduleAugmentation.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// @Filename: /dir/a.ts
2+
export const x = 0;
3+
4+
// @Filename: /dir/b.ts
5+
export {};
6+
7+
declare module "./a" {
8+
export const x = 0;
9+
}
10+
11+
declare module "../dir/a" {
12+
export const x = 0;
13+
}

0 commit comments

Comments
 (0)