Fix duplicate auto-import completions #42850
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| export function addToSeen(seen: ESMap<string, true>, key: string | number): boolean; | ||
| export function addToSeen<T>(seen: ESMap<string, T>, key: string | number, value: T): boolean; | ||
| export function addToSeen<T>(seen: ESMap<string, T>, key: string | number, value: T = true as any): boolean { | ||
| key = String(key); |
There was a problem hiding this comment.
I wanted to use addToSeen with a Map<number, Something>, and this coercion prevented me from doing that. It doesn’t seem to make a lot of sense now that we have proper ES Maps that can take keys of any type. But I can revert this and the cascading small updates if there’s any concern about it.
| { | ||
| name: "foo", | ||
| source: "/a/index", | ||
| sourceDisplay: "./a", | ||
| text: "(alias) function foo(): void\nexport foo", | ||
| kind: "alias", | ||
| kindModifiers: "export", | ||
| hasAction: true, | ||
| sortText: completion.SortText.AutoImportSuggestions | ||
| }, |
There was a problem hiding this comment.
So are we now preferring completions with a longer path? It seems like we've removed the duplicate, but are leaving in a suggestion for a farther module.
There was a problem hiding this comment.
No. The real gotcha is that the actual value of the source property, as long as it does in fact point to some module that exports the thing you want to import, is completely irrelevant in determining the final module specifier we use. When we get completion details, we use that source and name to find the export, then we skip all aliases and find the symbol at its actual declaration, then we again find every re-export of it and decide which one is best. So literally any module in a chain of re-exporting modules is just as good as any other, making the whole bucketing thing I deleted completely pointless. Notice that the sourceDisplay on the remaining entry (and equivalently, the resulting module specifier after applying the completion) is still "./a" (the best and shortest module specifier) even though the source property happens to be a longer path.
There was a problem hiding this comment.
The exception to this “completely irrelevant” claim is when one symbol is re-exported from different ambient modules, as coded into moduleSymbolsAreDuplicateOrigins. Any number of re-exporting external modules can be considered together, but each ambient module is unique, since we really have no concept of whether a module named "ambient-module-one" or "ambient-module-two" is a better place to import from. We treat those as unique, whereas we don’t give you a choice between "./a" and "./a/index" and "./a/actual/declaration/module" if the same symbol can be accessed from any of them.
There was a problem hiding this comment.
Maybe change verify.completions to allow dropping (or not ever verifying) source then? I guess it's needed to distinguish two otherwise-identical completions...
Co-authored-by: Daniel Rosenwasser <[email protected]>
| @@ -1609,7 +1609,7 @@ namespace ts.Completions { | |||
| } | |||
|
|
|||
| /** True if symbol is a type or a module containing at least one type. */ | |||
| function symbolCanBeReferencedAtTypeLocation(symbol: Symbol, seenModules = new Map<string, true>()): boolean { | |||
| function symbolCanBeReferencedAtTypeLocation(symbol: Symbol, seenModules = new Map<SymbolId, true>()): boolean { | |||
There was a problem hiding this comment.
initial question: Is type SymbolId = number? Why do all the previous updates switch string to number, but this one goes to SymbolId?
If it's just symbol.id, can you just use the symbol itself as the key instead? The hashed key is likely to be some globally unique number either way.
There was a problem hiding this comment.
Yes, SymbolId is just number. Most of the other keys are node ids, which are also number, but don’t have a cute type alias. And yeah, we could use the Symbol itself as the key these days, but I was just making the minimal change needed to work with my update to addToSeen.
| { | ||
| name: "foo", | ||
| source: "/a/index", | ||
| sourceDisplay: "./a", | ||
| text: "(alias) function foo(): void\nexport foo", | ||
| kind: "alias", | ||
| kindModifiers: "export", | ||
| hasAction: true, | ||
| sortText: completion.SortText.AutoImportSuggestions | ||
| }, |
There was a problem hiding this comment.
Maybe change verify.completions to allow dropping (or not ever verifying) source then? I guess it's needed to distinguish two otherwise-identical completions...
| log(`getSymbolsFromOtherSourceFileExports: ${timestamp() - startTime}`); | ||
| return results; | ||
| return flatten(arrayFrom(results.values())); |
There was a problem hiding this comment.
side question: is flatten a one-level array flatten : T[][] => T[]?
Yeah, it’s needed in order to call |
Fixes the part of #42752 that is a TypeScript bug. Also, simplifies logic that was notoriously complex enough to require ASCII box diagram comments.
There was actually already a test for this asserting a totally bananas incorrect behavior. A brief historical exploration revealed that the initial implementation of the test did not assert the incorrect behavior, but was too vague to catch this bug. Later, a massive PR made every completions test more specific, and it seems like the new contents of the test were simply determined by what would pass, in an assumption that whatever behavior was currently manifesting was actually correct.