Merge pull request #70743 from genlu/FixCommit

Fix commit behvaior where items use default commit chars as their special filter char

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

@@ -84,20 +84,17 @@ public bool ShouldCommitCompletion(
             char typedChar,
             CancellationToken cancellationToken)
         {
-            if (!PotentialCommitCharacters.Contains(typedChar))
-            {
-                return false;
-            }
-
-            return !(session.Properties.TryGetProperty(CompletionSource.ExcludedCommitCharacters, out ImmutableArray<char> excludedCommitCharacter)
-                && excludedCommitCharacter.Contains(typedChar));
+            // this is called only when the typedChar is in the list returned by PotentialCommitCharacters.
+            // It's possible typedChar is intended to be a filter char for some items (either currently considered for commit of not)
+            // we let this case to be handled in `TryCommit` instead, where we will have all the information needed to decide.
+            return true;
         }
 
         public AsyncCompletionData.CommitResult TryCommit(
             IAsyncCompletionSession session,
             ITextBuffer subjectBuffer,
             VSCompletionItem item,
-            char typeChar,
+            char typedChar,
             CancellationToken cancellationToken)
         {
             // We can make changes to buffers. We would like to be sure nobody can change them at the same time.
@@ -121,20 +118,35 @@ public AsyncCompletionData.CommitResult TryCommit(
                 return CommitResultUnhandled;
             }
 
-            var filterText = session.ApplicableToSpan.GetText(session.ApplicableToSpan.TextBuffer.CurrentSnapshot) + typeChar;
-            if (Helpers.IsFilterCharacter(itemData.RoslynItem, typeChar, filterText))
+            var roslynItem = itemData.RoslynItem;
+            var filterText = session.ApplicableToSpan.GetText(session.ApplicableToSpan.TextBuffer.CurrentSnapshot) + typedChar;
+
+            if (Helpers.IsFilterCharacter(roslynItem, typedChar, filterText))
             {
                 // Returning Cancel means we keep the current session and consider the character for further filtering.
                 return new AsyncCompletionData.CommitResult(isHandled: true, AsyncCompletionData.CommitBehavior.CancelCommit);
             }
 
+            // typedChar could be a filter character for another item. If we find such an item that the current filter
+            // text matches its start, then we should cancel commit and give ItemManager a chance to handle it.
+            // This is done here instead of in `ShouldCommitCompletion` because `ShouldCommitCompletion` might be called before
+            // CompletionSource add the `excludedCommitCharactersMap` to the session property bag.
+            if (session.Properties.TryGetProperty(CompletionSource.ExcludedCommitCharactersMap, out MultiDictionary<char, RoslynCompletionItem> excludedCommitCharactersMap))
+            {
+                foreach (var potentialItemForSelection in excludedCommitCharactersMap[typedChar])
+                {
+                    if (potentialItemForSelection != roslynItem && Helpers.TextTypedSoFarMatchesItem(potentialItemForSelection, filterText))
+                        return new AsyncCompletionData.CommitResult(isHandled: true, AsyncCompletionData.CommitBehavior.CancelCommit);
+                }
+            }
+
             var options = _globalOptions.GetCompletionOptions(document.Project.Language);
             var serviceRules = completionService.GetRules(options);
 
             // We can be called before for ShouldCommitCompletion. However, that call does not provide rules applied for the completion item.
             // Now we check for the commit character in the context of Rules that could change the list of commit characters.
 
-            if (!Helpers.IsStandardCommitCharacter(typeChar) && !IsCommitCharacter(serviceRules, itemData.RoslynItem, typeChar))
+            if (!Helpers.IsStandardCommitCharacter(typedChar) && !IsCommitCharacter(serviceRules, roslynItem, typedChar))
             {
                 // Returning None means we complete the current session with a void commit. 
                 // The Editor then will try to trigger a new completion session for the character.
@@ -162,10 +174,10 @@ public AsyncCompletionData.CommitResult TryCommit(
             }
 
             // Commit with completion service assumes that null is provided is case of invoke. VS provides '\0' in the case.
-            var commitChar = typeChar == '\0' ? null : (char?)typeChar;
+            var commitChar = typedChar == '\0' ? null : (char?)typedChar;
             return Commit(
                 session, triggerDocument, completionService, subjectBuffer,
-                itemData.RoslynItem, sessionData.CompletionListSpan.Value, commitChar, itemData.TriggerLocation.Value.Snapshot, serviceRules,
+                roslynItem, sessionData.CompletionListSpan.Value, commitChar, itemData.TriggerLocation.Value.Snapshot, serviceRules,
                 filterText, cancellationToken);
         }
 

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

@@ -44,6 +44,7 @@ internal sealed class CompletionSource : IAsyncExpandingCompletionSource
 
         // Don't change this property! Editor code currently has a dependency on it.
         internal const string ExcludedCommitCharacters = nameof(ExcludedCommitCharacters);
+        internal const string ExcludedCommitCharactersMap = nameof(ExcludedCommitCharactersMap);
 
         private static readonly ImmutableArray<ImageElement> s_warningImageAttributeImagesArray =
             ImmutableArray.Create(new ImageElement(Glyph.CompletionWarning.GetImageId(), EditorFeaturesResources.Warning_image_element));
@@ -425,16 +426,24 @@ private static void UpdateSessionData(IAsyncCompletionSession session, Completio
             // If there are suggestionItemOptions, then later HandleNormalFiltering should set selection to SoftSelection.
             sessionData.HasSuggestionItemOptions |= completionList.SuggestionModeItem != null;
 
-            var excludedCommitCharacters = GetExcludedCommitCharacters(completionList.ItemsList);
-            if (excludedCommitCharacters.Length > 0)
+            var excludedCommitCharactersFromList = GetExcludedCommitCharacters(completionList.ItemsList);
+            if (session.Properties.TryGetProperty(ExcludedCommitCharactersMap, out MultiDictionary<char, RoslynCompletionItem> excludedCommitCharactersMap))
             {
-                if (session.Properties.TryGetProperty(ExcludedCommitCharacters, out ImmutableArray<char> excludedCommitCharactersBefore))
+                foreach (var kvp in excludedCommitCharactersFromList)
                 {
-                    excludedCommitCharacters = excludedCommitCharacters.Union(excludedCommitCharactersBefore).ToImmutableArray();
+                    foreach (var item in kvp.Value)
+                    {
+                        excludedCommitCharactersMap.Add(kvp.Key, item);
+                    }
                 }
-
-                session.Properties[ExcludedCommitCharacters] = excludedCommitCharacters;
             }
+            else
+            {
+                excludedCommitCharactersMap = excludedCommitCharactersFromList;
+            }
+
+            session.Properties[ExcludedCommitCharactersMap] = excludedCommitCharactersMap;
+            session.Properties[ExcludedCommitCharacters] = excludedCommitCharactersMap.Keys.ToImmutableArray();
 
             // We need to remember the trigger location for when a completion service claims expanded items are available
             // since the initial trigger we are able to get from IAsyncCompletionSession might not be the same (e.g. in projection scenarios)
@@ -564,9 +573,13 @@ private VSCompletionItem Convert(
             return item;
         }
 
-        private static ImmutableArray<char> GetExcludedCommitCharacters(IReadOnlyList<RoslynCompletionItem> roslynItems)
+        /// <summary>
+        /// Build a map from added filter characters to corresponding items.
+        /// CommitManager needs this information to decide whether it should commit selected item.
+        /// </summary>
+        private static MultiDictionary<char, RoslynCompletionItem> GetExcludedCommitCharacters(IReadOnlyList<RoslynCompletionItem> roslynItems)
         {
-            var hashSet = new HashSet<char>();
+            var map = new MultiDictionary<char, RoslynCompletionItem>();
             foreach (var roslynItem in roslynItems)
             {
                 foreach (var rule in roslynItem.Rules.FilterCharacterRules)
@@ -575,13 +588,13 @@ private static ImmutableArray<char> GetExcludedCommitCharacters(IReadOnlyList<Ro
                     {
                         foreach (var c in rule.Characters)
                         {
-                            hashSet.Add(c);
+                            map.Add(c, roslynItem);
                         }
                     }
                 }
             }
 
-            return hashSet.ToImmutableArray();
+            return map;
         }
 
         internal static bool QuestionMarkIsPrecededByIdentifierAndWhitespace(

src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests.vb

@@ -12102,5 +12102,86 @@ class Derived : BaseClass
                 Await state.AssertCompletionItemsContainAll("StaticMember", "InstanceMember")
             End Using
         End Function
+
+        <WorkItem("https://github.com/dotnet/roslyn/issues/70732")>
+        <WpfFact, Trait(Traits.Feature, Traits.Features.Completion)>
+        Public Async Function TypingCommitCharWhichIsAlsoFilterCharOfAnotherNoMatchingItemShouldCommit() As Task
+            Using state = TestStateFactory.CreateCSharpTestState(
+                              <Document>
+namespace A
+{  
+    public struct PointF
+    {
+
+        public float Y { get; set; }
+
+        public float X { get; set; }
+
+        public static int operator +(PointF pt, PointF sz) => 0;
+
+        public static int operator -(PointF ptA, PointF ptB) => 0;
+
+        public static bool operator ==(PointF ptA, PointF ptB) => true;
+
+        public static bool operator !=(PointF ptA, PointF ptB) => true;
+    }
+
+    class Program
+    {
+        static void Main()
+        {
+            PointF point;
+            point$$
+        }
+    }
+}                              </Document>)
+
+                state.SendTypeChars(".x")
+                Await state.AssertSelectedCompletionItem(displayText:="X", isHardSelected:=True)
+                state.SendTypeChars("+")
+                Await state.AssertNoCompletionSession()
+                Assert.Contains("point.X+", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
+            End Using
+        End Function
+
+        <WorkItem("https://github.com/dotnet/roslyn/issues/70732")>
+        <WpfFact, Trait(Traits.Feature, Traits.Features.Completion)>
+        Public Async Function TypingCommitCharWhichIsAlsoFilterCharOfAnotherPotentiallyMatchingItemShouldNotCommit() As Task
+            Using state = TestStateFactory.CreateCSharpTestState(
+                              <Document>
+namespace A
+{  
+    public struct PointF
+    {
+
+        public float Y { get; set; }
+
+        public float X { get; set; }
+
+        public static int operator +(PointF pt, PointF sz) => 0;
+
+        public static int operator -(PointF ptA, PointF ptB) => 0;
+
+        public static bool operator ==(PointF ptA, PointF ptB) => true;
+
+        public static bool operator !=(PointF ptA, PointF ptB) => true;
+    }
+
+    class Program
+    {
+        static void Main()
+        {
+            PointF point;
+            point$$
+        }
+    }
+}                              </Document>)
+
+                state.SendTypeChars(".+")
+                Await state.AssertSelectedCompletionItem(displayText:="+", inlineDescription:="x + y")
+                state.SendTab()
+                Assert.Contains("point +", state.GetLineTextFromCaretPosition(), StringComparison.Ordinal)
+            End Using
+        End Function
     End Class
 End Namespace