CompletionService refactoring and small tweaks to import completion #59125

genlu · 2022-01-28T00:27:38Z

This is extracted from #59045.

Other than refactoring, there's also a few notable changes to the behavior of import completion (which lays the foundation for the additional changes in #59045):

Returns partial result if cache is not fully populated (this is for unimported types, provider for unimported extension methods already does this)
Selecting expander no longer force cache population and returning of complete result. Partial result will be returned if cache is not fully populated.
Timebox (500ms) for unimported extension method provider is removed. This is not the intended final behavior, additonal change in Don't block completion for items from unimported namespaces #59045 would ensure completion do not block on import completion. This also fixed CancellationTokenSource disposed in AbstractExtensionMethodImportCompletionProvider.cs #58932
Always show expander in the filter list. If we are not in the proper context or no expanded items would be available, then selecting it would be a no-op. This simplifies the implementation w/o (IMO) causing any UX issue.

And minor cleanup in CompletionService APIs where CompletionList is used

by setting capacity of the builder to avoid call to EnsureCapacity

Instead of passing in a boolean to indicated if its triggered by selecting expander button, now import completion providers only need to check if import completion is enabled.

Was broken by previous refactoring

So we can always wait for unimported items and force index creation

CyrusNajmabadi · 2022-02-02T16:54:30Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs

+        private readonly record struct VSCompletionItemData
+            (string DisplayText, ImageElement Icon, ImmutableArray<AsyncCompletionData.CompletionFilter> Filters,
+                int FilterSetData, ImmutableArray<ImageElement> AttributeIcons, string InsertionText);


move to separate file. my pref on formatting would be:

private readonly record struct VSCompletionItemData( string DisplayText, ImageElement Icon, ImmutableArray<AsyncCompletionData.CompletionFilter> Filters, int FilterSetData, ImmutableArray<ImageElement> AttributeIcons, string InsertionText);

Taht way all the properties are very clear.

CyrusNajmabadi · 2022-02-02T16:55:12Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/FilterSet.cs

-                Mask = mask;
-            }
-        }
+        private readonly record struct FilterWithMask(CompletionFilter Filter, int Mask);


please move to separate file.

That feels unnecessary to me, given this is a private, one-line auxiliary type. Is this required by our convention?

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

CyrusNajmabadi · 2022-02-02T16:55:43Z

...atures/Core/Implementation/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                }
+
+                using var _ = ArrayBuilder<CompletionItemWithHighlight>.GetInstance(items.Count, out var builder);
+                builder.AddRange(items.Select(matchResult =>


why not use SleectAsArray? i'm not sure how this new code is better.

items is a pooled list, not an ImmutableArray, I think the overload of SelectAsArray for it doesn't create the instance of ArrayBuilder with a capacity. Give the potentially large size of this list, this would avoid call to EnsureCapacity thus reduce allocation.

note that eventually we'd want to avoid potentially LOH allocation here, by asking for a new API from editor so we don't need to realize any ImmutableArray, and use pooled object instead.

src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs

src/Features/Core/Portable/Completion/CompletionOptions.cs

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.ProviderManager.cs

CyrusNajmabadi · 2022-02-02T17:00:53Z

...e/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs

@@ -40,44 +39,42 @@ public Task NotifyCommittingItemAsync(Document document, CompletionItem item, ch

        public override async Task ProvideCompletionsAsync(CompletionContext completionContext)
        {
+            // Don't trigger import completion if the option value is "default" and the experiment is disabled for the user. 


what experiment do we still have here?

Import completion is still disabled by default, we use an experiment to turn it on for all internal users.

CyrusNajmabadi · 2022-02-02T17:01:35Z

...ortable/Completion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.cs

@@ -67,12 +66,10 @@ public Task WarmUpCacheAsync(Project? project, CancellationToken cancellationTok
                        s_cachingTask = Task.Run(() => WarmUpCacheAsync(workspace.CurrentSolution.GetProject(projectId), CancellationToken.None), CancellationToken.None);
                    }
                }


have you considered just using a AsyncBatchignWorkQueue instead?

Will look into it, and change in a follow up PR if it's feasible.

CyrusNajmabadi · 2022-02-02T17:03:48Z

i'm overall ok with this. my general feedback is similar to before. the refactorings are helping with complexity. but as you go inot methods have a strong eye toward doc'ing what is going on and making it so the methods are clearer in purpose and logic.

Since you're doing several PRs this can be slowly done as you do all the work. but ideally the end result is that all the individual pieces feel clearer and far less complex when people are workign with them.

thanks!

CyrusNajmabadi

approved, with requests for some changes.

genlu added 18 commits January 6, 2022 13:31

Small cleanup in CompletionSource and CompletionService

04b3c78

Add ExpandItemsAvailable flag in CompletionList

e497b35

And minor cleanup in CompletionService APIs where CompletionList is used

Small cleanup of context iteration/aggregation

76ea805

Move provider handling into a helper class

c203032

File rename

5bb4567

More minor cleanup in CompletionServiceWithProviders

d88f72c

Remove timebox for unimported extension method completion provider

3478436

Don't block completion expander on index creation

25293b9

Reduce allocation

a69ac01

by setting capacity of the builder to avoid call to EnsureCapacity

Simplify code

dc9a8f0

Always show expander in filter list

ccf660a

Simplify how "expanded completion" is controlled

afb54b4

Instead of passing in a boolean to indicated if its triggered by selecting expander button, now import completion providers only need to check if import completion is enabled.

Pass in CompletionOptions to CompletionSource worker method

3cf6e20

Use record struct

a4cf108

Merge remote-tracking branch 'dotnet/main' into refactoring

08a3385

Fix completion trigger

b099904

Was broken by previous refactoring

Return partial result for unimported types completion when cache miss

aac47aa

Add test only options for import completion

1235a42

So we can always wait for unimported items and force index creation

genlu requested review from a team as code owners January 28, 2022 00:27

dotnet-issue-labeler bot added the Area-IDE label Jan 28, 2022

genlu mentioned this pull request Jan 28, 2022

Don't block completion for items from unimported namespaces #59045

Merged

2 tasks

Clean up telemetry

f06e2ab

genlu force-pushed the CompletionRefactoring branch from 64565d3 to a83e943 Compare January 29, 2022 00:11

runfoapp bot mentioned this pull request Jan 31, 2022

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

Fix

217ae3b

genlu force-pushed the CompletionRefactoring branch from a83e943 to 217ae3b Compare January 31, 2022 20:12