Remove editor item from MatchResult #64229

genlu · 2022-09-22T21:33:44Z

And a few other clean-ups

And use MatchResult for filtering in CompletionService API so we have more information to work with across API boundary

genlu · 2022-09-22T21:37:52Z

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

    {
-        public readonly RoslynCompletionItem RoslynCompletionItem;
+        public readonly CompletionItem CompletionItem;


We don't actually need to keep a reference to editor item anymore. It can be found using IndexInOriginalSortedOrder isntead.

genlu · 2022-09-22T21:39:42Z

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

            string filterText,
-            IList<CompletionItem> builder)
+            IList<MatchResult> builder)


Now we simply return a subset of MatchResults passed in, this would provide more information across the API boundry and simplify the implementation.

consider updating comment

genlu · 2022-09-22T21:40:42Z

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

           string filterText,
-           IList<CompletionItem> builder)
+           IList<MatchResult> builder)


We probably should deprecate the public version of this and promote this to public

genlu · 2022-09-22T21:43:25Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

@@ -140,12 +140,16 @@ private bool ShouldSelectSuggestionItemWhenNoItemMatchesFilterText
                    if (itemsToBeIncluded.Count == 0)
                        return HandleAllItemsFilteredOut();

+                    var highlightAndFilterTask = Task.Run(


This is ~~the only~~ one of two behavior changes in this PR, we now calculate highlight and filter states in parallel to item selection.

genlu · 2022-09-22T21:57:58Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

-            private static readonly ObjectPool<List<MatchResult<VSCompletionItem>>> s_listOfMatchResultPool = new(factory: () => new());
-            private static readonly ObjectPool<List<(RoslynCompletionItem, PatternMatch?)>> s_listOfItemMatchPairPool = new(factory: () => new());
-            private static readonly ObjectPool<List<RoslynCompletionItem>> s_filteredItemBuilderPool = new(factory: () => new());
+            private static readonly ObjectPool<List<MatchResult>> s_listOfMatchResultPool = new(factory: () => new());


eliminated two object pools with this change

genlu · 2022-09-22T22:05:28Z

.../Core/Portable/ExternalAccess/VSTypeScript/Api/VSTypeScriptCompletionServiceWithProviders.cs

-           IList<CompletionItem> builder)
-            => FilterItemsImpl(document, itemsWithPatternMatch, filterText, builder);
+           IList<MatchResult> builder)
+            => FilterItemsImpl(document, matchResults, filterText, builder);

        internal virtual void FilterItemsImpl(


TS isn't using this at the moment, so we can change it w/o breaking them. We should move them off the public one though since it's not performant

I have verified that TS completion is still working properly with this PR

when everything else are considered equal

genlu · 2022-09-23T00:11:45Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

-                    UniqueItem: moreThanOneMatch || !bestMatchResult.HasValue ? null : bestMatchResult.Value.EditorCompletionItem);
+                    UniqueItem: moreThanOneMatch || !bestMatchResult.HasValue ? null : GetCorrespondingVsCompletionItem(bestMatchResult.Value, cancellationToken));
+
+                static int CompareForDeletion(MatchResult x, MatchResult y, string pattern)


This is moved here from type MatchResult, since this is the only usage.

genlu · 2022-09-23T00:13:30Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

-                    if ((currentItemPriority > bestItemPriority) ||
-                        ((currentItemPriority == bestItemPriority) && !bestItem.IsPreferredItem() && chosenItem.IsPreferredItem()))
+                    // final preference is match to FilterText over AdditionalFilterTexts
+                    if (bestResult.MatchedWithAdditionalFilterTexts && !currentResult.MatchedWithAdditionalFilterTexts)


This is another behavior change in this PR. if everything else considered equal (see comments in the code above), we will select item that matches FilterText over the one matches AdditionalFilterTexts.

This should have very limited effect at the moment, since only a few semantic snippet items actually use AdditionalFilterTexts. My goal is to centralize the filtering logic for all items that requires multiple filtertext (e.g. enum members), which is now buried in random matching logics and very hard to find.

arunchndr · 2022-09-23T17:33:02Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.cs

@@ -123,7 +123,7 @@ private static SegmentedList<VSCompletionItem> SortCompletionitems(AsyncCompleti
                }

                var updater = new CompletionListUpdater(session.ApplicableToSpan, sessionData, data, _recentItemsManager, _globalOptions);
-                return updater.UpdateCompletionList(session, cancellationToken);
+                return await updater.UpdateCompletionListAsync(session, cancellationToken).ConfigureAwait(false);


Would it make sense instead to give the await configuration control to the caller by just returning the task and removing the async in the signature?

I have to await a task in the code above (line 107) though

arunchndr · 2022-09-23T17:35:01Z

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

            string filterText,
-            IList<CompletionItem> builder)
+            IList<MatchResult> builder)


consider updating comment

arunchndr · 2022-09-23T17:36:13Z

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


        /// <summary>
-        /// The actual editor completion item associated with this <see cref="RoslynCompletionItem"/>
-        /// In VS for example, this is the associated VS async completion item.
+        /// If `CompletionItem.AdditionalFilterTexts` was used to create this MatchResult, then this is set to the one that was used.


Im not familiar with our comment pattern yet. Would cref be appropriate here?

yup, I definitely need to buff up the doc-comment in this type

genlu added 3 commits September 20, 2022 14:44

Remove editor CompletionItem from MatchResult

1efc338

And use MatchResult for filtering in CompletionService API so we have more information to work with across API boundary

Remove ItemWithMatchResult

ed32dc1

Fix names

90ede7d

genlu requested a review from a team as a code owner September 22, 2022 21:33

dotnet-issue-labeler bot added the Area-IDE label Sep 22, 2022

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

03f9204

genlu commented Sep 22, 2022

View reviewed changes

Fix merge error

6938c2b

genlu force-pushed the RemoveEditorItem branch from 5b35dd2 to 6938c2b Compare September 22, 2022 22:02

genlu commented Sep 22, 2022

View reviewed changes

genlu added 3 commits September 22, 2022 15:29

Fix

2258c7b

More cleanup in MatchResult

5d6d8fd

Prefer match to FilterText over AdditionalFilterText

6bbba64

when everything else are considered equal

genlu commented Sep 23, 2022

View reviewed changes

runfoapp bot mentioned this pull request Sep 23, 2022

FixAllOccurrencesIgnoresGeneratedCode is flaky #56625

Open

arunchndr approved these changes Sep 23, 2022

View reviewed changes

genlu merged commit 60ef17d into dotnet:main Sep 23, 2022

genlu deleted the RemoveEditorItem branch September 23, 2022 19:04

ghost added this to the Next milestone Sep 23, 2022

allisonchou mentioned this pull request Sep 26, 2022

[Automated] PRs inserted in VS build main-32926.152 #64293

Closed

Cosifne modified the milestones: Next, 17.4 P3 Sep 26, 2022

genlu mentioned this pull request Sep 27, 2022

Allow TS to derive from the more performant internal API #64258

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove editor item from MatchResult #64229

Remove editor item from MatchResult #64229

genlu commented Sep 22, 2022

genlu Sep 22, 2022

genlu Sep 22, 2022 •

edited

Loading

arunchndr Sep 23, 2022

genlu Sep 22, 2022

genlu Sep 22, 2022 •

edited

Loading

genlu Sep 22, 2022

genlu Sep 22, 2022

genlu Sep 23, 2022

genlu Sep 23, 2022

genlu Sep 23, 2022 •

edited

Loading

arunchndr Sep 23, 2022

genlu Sep 23, 2022

arunchndr Sep 23, 2022

arunchndr Sep 23, 2022

genlu Sep 23, 2022

Remove editor item from MatchResult #64229

Remove editor item from MatchResult #64229

Conversation

genlu commented Sep 22, 2022

Choose a reason for hiding this comment

genlu Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu Sep 22, 2022 •

edited

Loading

genlu Sep 22, 2022 •

edited

Loading

genlu Sep 23, 2022 •

edited

Loading