Remove duplication in AbstractSymbolCompletionProvider.CreateItems#36523
Conversation
We had two overloads of AbstractSymbolCompletionProvider.CreateItems, once which handles the case of a single file and one that handles linked files. The code is almost identical, but the linked file one incorrectly handled the case of not having any symbols. Rather than fixing the bug in just that branch, I'm refactoring it to simply remove the second copy so both the single file and linked file cases go through the same helper. The intent here is to avoid hiding simple bugs that had nothing to do with linked files in the linked-file-only path. Fixes dotnet#36080 by virtue of deleting the offending code entirely. No new tests are added since I've confirmed that the existing tests (once the refactoring was complete) would have been sufficient for discovering this problem.
|
Yaaay, dedupe. It's also worth noting that hte entire internal architecture of completion is pretty awful. There's a lot of independent calls to compute information, lots of which have to recompute intermediary state computed by prvious passes. There's a lot of opportunity for cleanup and dedupe here, that will likely also improve perf a lot. |
8bdc124 to
d3fe0eb
Compare
| { | ||
| foreach (var symbol in symbolGroup) | ||
| { | ||
| foreach (var syntaxContext in originatingContextMap.Values) |
There was a problem hiding this comment.
Deleting this code is also fixing a potentially big perf problem: to determine if a target type matched, we were accidentally binding each symbol up to n times, where n is the number of total symbols in the list. This should have been only finding the syntaxContexts for the symbol by looking it up in the dictionary, but instead by matching all was binding far more than it needed to.
| { | ||
| foreach (var symbol in symbolGroup) | ||
| { | ||
| foreach (var syntaxContext in originatingContextMap.Values) |
|
Tagging @jinujoseph for delegated M2 approval. |
|
I'm testing this out locally as well. |
We had two overloads of AbstractSymbolCompletionProvider.CreateItems, once which handles the case of a single file and one that handles linked files. The code is almost identical, but the linked file one incorrectly handled the case of not having any symbols. Rather than fixing the bug in just that branch, I'm refactoring it to simply remove the second copy so both the single file and linked file cases
go through the same helper. The intent here is to avoid hiding simple bugs that had nothing to do with linked files in the linked-file-only path.
Fixes #36080 by virtue of deleting the offending code entirely. No new tests are added since I've confirmed that the existing tests (once the refactoring was complete) would have been sufficient for discovering this problem.