Avoid adding the CompletionItem when the alias type exists #49488
Conversation
Cosifne
force-pushed
the
dev/shech/nameAliasCompletion
branch
from
0412f0e to
c38fd0c
Compare
|
Tag @genlu in case you didn't see this PR |
...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...ditorFeatures/CSharpTest/Completion/CompletionProviders/TypeImportCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
...ditorFeatures/CSharpTest/Completion/CompletionProviders/TypeImportCompletionProviderTests.cs
Show resolved
Hide resolved
...arp/Portable/Completion/CompletionProviders/ImportCompletion/TypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
|
Done with pass. Overall looks good. Let me know when it's ready for another look. |
|
Any update on this? I'm just going through all PRs I've been involved to make sure I didn't miss anything, so no hurry :) |
I just forgot this PR for a while. Working on that to get it done |
...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs
Show resolved
Hide resolved
...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/Core/Portable/Completion/Providers/ImportCompletionProvider/ImportCompletionItem.cs
Show resolved
Hide resolved
| { | ||
| public class Main | ||
| { | ||
| $$ |
There was a problem hiding this comment.
I think to test attribute items, the trigger needs to be in attribute context. Otherwise, it's just behaves as regular types
| // check if the type name is in the dictionary. | ||
| // It is done in this way to avoid calling ImportCompletionItem.GetTypeName for all the CompletionItems | ||
| if (!aliasTargetNamespaceToTypeNameMap.IsEmpty | ||
| && aliasTargetNamespaceToTypeNameMap[containingNamespace].Contains(ImportCompletionItem.GetTypeName(item))) |
There was a problem hiding this comment.
nit: I guess we really don't care if it's a generic type, since no generic target will be in the dictionary.
There was a problem hiding this comment.
why do you need the aliasTargetNamespaceToTypeNameMap.IsEmpty check?
There was a problem hiding this comment.
Because it cheaper than indexing into the dictionary. We are dealing with large amount of item and it's very rare there's using alias in the document
...arp/Portable/Completion/CompletionProviders/ImportCompletion/TypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...rtable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionProvider.cs
Outdated
Show resolved
Hide resolved
|
do we have perf information around this? i ask because completion seems to feel much slower than it did a few months back. i'm worried that it may be because of import completion. when we try this while typing in roslyn, what sort of absolute time do we se taken up by import completion? |
I would have a perfView test report post here before I checked in. |
Collect 5 etls file in Roslyn repo at diffferent test project and the only trace I could see is AbstractTypeImportCompletionService.GetCachedEntriesAsync for TypeCompletionProvider. So given the amount of alias type in each document should be very few(in Roslyn we only have 1-5 alias), I don't think this would have a big impact for the perf |
ghost
added this to the 
Linked Issue: #44366 and #39785
Right now if there is a type alias in the code, VS would still keep suggesting the item from unimported namespace.
After this PR, if there is an alias for the type, we won't provide this CompletionItem from TypeImportCompletionProvider
