-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Clean up amalgamatedDuplicates #27285
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
Conversation
e4a7b02
to
9c388fa
Compare
src/compiler/utilities.ts
Outdated
@@ -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<unknown>, b as MapLike<unknown>, isJsonEqual); | |||
} | |||
|
|||
export function getOrUpdate<T>(map: Map<T>, key: string, getValue: () => T): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getValue
should probably be named getDefault
.
amalgamatedDuplicates.set(cacheKey, existing); | ||
return target; | ||
const filesDuplicates = getOrUpdate<DuplicateInfoForFiles>(amalgamatedDuplicates, `${firstFile.path}|${secondFile.path}`, () => | ||
({ firstFile, secondFile, conflictingSymbols: createMap() })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see DuplicateInfoForFiles as an assertion on the object literal: { firstFile, secondFile, conflictingSymbols: createMap() } as DuplicateInfoForFiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would conflict with https://palantir.github.io/tslint/rules/no-object-literal-type-assertion/ -- it's unsafe to cast an object literal because that allows misspelled properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions and suggestion.
Fixes #27280
Cleans up the way
amalgamatedDuplicates
stores data, which happens to fix a bug where it reported the same location multiple times.