Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve pattern matching when user is likely providing camel humps #75757

Merged
merged 2 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/EditorFeatures/Test/Utilities/PatternMatcherTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,10 @@ private static void VerifyBreakIntoCharacterParts(string original, params string
[InlineData("Combine[|Bin|]ary", "bin", PatternMatchKind.StartOfWordSubstring, CaseInsensitive)]

[InlineData("_ABC_[|Abc|]_", "Abc", PatternMatchKind.StartOfWordSubstring, CaseSensitive)]
[InlineData("[|C|]reate[|R|]ange", "CR", PatternMatchKind.CamelCaseExact, CaseSensitive)]

[WorkItem("https://github.com/dotnet/roslyn/issues/51029")]
[WorkItem("https://github.com/dotnet/roslyn/issues/17275")]
internal void TestNonFuzzyMatch(
string candidate, string pattern, PatternMatchKind matchKind, bool isCaseSensitive)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ Imports Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.AsyncCompletio
Imports Microsoft.CodeAnalysis.Editor.Shared.Options
Imports Microsoft.CodeAnalysis.Editor.[Shared].Utilities
Imports Microsoft.CodeAnalysis.Editor.UnitTests.Extensions
Imports Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces
Imports Microsoft.CodeAnalysis.Host
Imports Microsoft.CodeAnalysis.Host.Mef
Imports Microsoft.CodeAnalysis.Options
Expand All @@ -34,7 +33,7 @@ Imports Roslyn.Test.Utilities.TestGenerators
Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
<UseExportProvider>
<Trait(Traits.Feature, Traits.Features.Completion)>
Public Class CSharpCompletionCommandHandlerTests
Public NotInheritable Class CSharpCompletionCommandHandlerTests
<WpfTheory, CombinatorialData>
Public Async Function CompletionOnFileType_SameFile_NonQualified(showCompletionInArgumentLists As Boolean) As Task
Using state = TestStateFactory.CreateCSharpTestState(
Expand Down Expand Up @@ -12639,6 +12638,7 @@ $$
End Using
End Function

<WpfTheory, CombinatorialData>
<WorkItem("https://github.com/dotnet/roslyn/issues/72872")>
Public Async Function CompletionInsideImplicitObjectCreationInsideCollectionExpression(showCompletionInArgumentLists As Boolean) As Task
Using state = TestStateFactory.CreateCSharpTestState(
Expand Down Expand Up @@ -12682,5 +12682,76 @@ internal class Program
Await state.AssertCompletionItemsContain("Id", displayTextSuffix:="")
End Using
End Function

<WpfTheory, CombinatorialData>
<WorkItem("https://github.com/dotnet/roslyn/issues/17275")>
Public Async Function PreferCamelCasedExactMatchOverPrefixCaseInsensitiveMatch1(showCompletionInArgumentLists As Boolean) As Task
Using state = TestStateFactory.CreateCSharpTestState(
<Document><![CDATA[
class C
{
void Create() { }
void CreateRange() { }

void M()
{
this.$$
}
}
]]>
</Document>,
showCompletionInArgumentLists:=showCompletionInArgumentLists, languageVersion:=LanguageVersion.CSharp12)

state.SendTypeChars("CR")
Await state.AssertSelectedCompletionItem("CreateRange")
End Using
End Function

<WpfTheory, CombinatorialData>
<WorkItem("https://github.com/dotnet/roslyn/issues/17275")>
Public Async Function PreferCamelCasedExactMatchOverPrefixCaseInsensitiveMatch2(showCompletionInArgumentLists As Boolean) As Task
Using state = TestStateFactory.CreateCSharpTestState(
<Document><![CDATA[
class C
{
void Create() { }
void CreateRange() { }

void M()
{
this.$$
}
}
]]>
</Document>,
showCompletionInArgumentLists:=showCompletionInArgumentLists, languageVersion:=LanguageVersion.CSharp12)

state.SendTypeChars("cr")
Await state.AssertSelectedCompletionItem("Create")
End Using
End Function

<WpfTheory, CombinatorialData>
<WorkItem("https://github.com/dotnet/roslyn/issues/17275")>
Public Async Function PreferCamelCasedExactMatchOverPrefixCaseInsensitiveMatch3(showCompletionInArgumentLists As Boolean) As Task
Using state = TestStateFactory.CreateCSharpTestState(
<Document>
class C
{
void Create() { }
void CreateRange() { }

void M()
{
this.$$
}
}
</Document>,
showCompletionInArgumentLists:=showCompletionInArgumentLists, languageVersion:=LanguageVersion.CSharp12)

state.SendTypeChars("Cr")
Await state.AssertSelectedCompletionItem("Create")
End Using
End Function
End Class
End Namespace
24 changes: 20 additions & 4 deletions src/Workspaces/Core/Portable/PatternMatching/PatternMatch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,33 @@ public int CompareTo(PatternMatch? other, bool ignoreCase)

public int CompareTo(PatternMatch other, bool ignoreCase)
{
// 1. In all scenarios, An case sensitive camel match (like CR against CreateRange) should beat a case
// insensitive match (like 'CR' against 'Create').
//
// Other cases can be added here as necessary.

switch (this.IsCaseSensitive, this.Kind.IsCamelCaseKind(), other.IsCaseSensitive, other.Kind.IsCamelCaseKind())
{
case (true, true, false, false):
return -1;
case (false, false, true, true):
return 1;
}

// Compare types
var comparison = this.Kind - other.Kind;
if (comparison != 0)
return comparison;

// Compare cases
if (!ignoreCase)
{
comparison = (!this.IsCaseSensitive).CompareTo(!other.IsCaseSensitive);
if (comparison != 0)
return comparison;
switch (this.IsCaseSensitive, other.IsCaseSensitive)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as before. just clearer imo.

{
case (true, false):
return -1;
case (false, true):
return 1;
}
}

// Consider a match to be better if it was successful without stripping punctuation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,9 @@ internal enum PatternMatchKind
/// </summary>
LowercaseSubstring,
}

internal static class PatternMatchKindExtensions
{
public static bool IsCamelCaseKind(this PatternMatchKind kind)
=> kind is PatternMatchKind.CamelCaseExact or PatternMatchKind.CamelCasePrefix or PatternMatchKind.CamelCaseNonContiguousPrefix or PatternMatchKind.CamelCaseSubstring;
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ private struct TextChunk : IDisposable
public WordSimilarityChecker SimilarityChecker;

public readonly bool IsLowercase;
public readonly bool IsUppercase;

public TextChunk(string text, bool allowFuzzingMatching)
{
Expand All @@ -50,6 +51,7 @@ public TextChunk(string text, bool allowFuzzingMatching)
: default;

IsLowercase = !ContainsUpperCaseLetter(text);
IsUppercase = !ContainsLowerCaseLetter(text);
}

public void Dispose()
Expand Down
68 changes: 52 additions & 16 deletions src/Workspaces/Core/Portable/PatternMatching/PatternMatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,19 @@ private static bool ContainsUpperCaseLetter(string pattern)
for (var i = 0; i < pattern.Length; i++)
{
if (char.IsUpper(pattern[i]))
{
return true;
}
}

return false;
}

private static bool ContainsLowerCaseLetter(string pattern)
{
// Expansion of "foreach(char ch in pattern)" to avoid a CharEnumerator allocation
for (var i = 0; i < pattern.Length; i++)
{
if (char.IsLower(pattern[i]))
return true;
}

return false;
Expand Down Expand Up @@ -153,6 +163,8 @@ private static bool ContainsUpperCaseLetter(string pattern)
in TextChunk patternChunk,
bool punctuationStripped)
{
using var candidateHumps = TemporaryArray<TextSpan>.Empty;

var candidateLength = candidate.Length;

var caseInsensitiveIndex = _compareInfo.IndexOf(candidate, patternChunk.Text, CompareOptions.IgnoreCase);
Expand All @@ -170,15 +182,31 @@ private static bool ContainsUpperCaseLetter(string pattern)
}
else
{
var isCaseSensitive = _compareInfo.IsPrefix(candidate, patternChunk.Text);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isPrefix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. This is stating if it's case sensitive or not. if it's a prefix that it is, otherwise it isn't and it must have been case inseisitive.


if (!isCaseSensitive && patternChunk.IsUppercase)
{
// The user wrote something all upper case, but happened to match the prefix of the word. For
// example, matching `CR` against both `Create` (a case insensitive prefix match) and `CreateRange`
// (a camel hump match). We want to prefer the latter in this case as the all upper case string
// is a strong signal they wanted camel hump matching here.
PopulateCandidateHumps();

// Note: ensure that we match here case sensitively as well. We only want to take this if their
// camel humps actually matched real word starts.
var match = TryCamelCaseMatch(candidate, patternChunk, punctuationStripped, isLowercase: false, candidateHumps);
if (match is { IsCaseSensitive: true })
return match;

// Deliberately fall through.
}

// Lengths were the same, this is either a case insensitive or sensitive prefix match.
return new PatternMatch(
PatternMatchKind.Prefix, punctuationStripped, isCaseSensitive: _compareInfo.IsPrefix(candidate, patternChunk.Text),
matchedSpan: GetMatchedSpan(0, patternChunk.Text.Length));
PatternMatchKind.Prefix, punctuationStripped, isCaseSensitive, matchedSpan: GetMatchedSpan(0, patternChunk.Text.Length));
}
}

using var candidateHumps = TemporaryArray<TextSpan>.Empty;

var patternIsLowercase = patternChunk.IsLowercase;
if (caseInsensitiveIndex > 0)
{
Expand Down Expand Up @@ -221,7 +249,8 @@ private static bool ContainsUpperCaseLetter(string pattern)
// Now do the more expensive check to see if we're at the start of a word. This is to catch
// word matches like CombineBinary. We want to find the hit against '[|Bin|]ary' not
// 'Com[|bin|]e'
StringBreaker.AddWordParts(candidate, ref candidateHumps.AsRef());
PopulateCandidateHumps();

for (int i = 0, n = candidateHumps.Count; i < n; i++)
{
var hump = TextSpan.FromBounds(candidateHumps[i].Start, candidateLength);
Expand All @@ -235,16 +264,17 @@ private static bool ContainsUpperCaseLetter(string pattern)
}
}

// Didn't have an exact/prefix match, or a high enough quality substring match.
// See if we can find a camel case match.
if (candidateHumps.Count == 0)
StringBreaker.AddWordParts(candidate, ref candidateHumps.AsRef());
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code block looks a bit confusing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what to do about that :)

// Didn't have an exact/prefix match, or a high enough quality substring match.
// See if we can find a camel case match.
PopulateCandidateHumps();

// Didn't have an exact/prefix match, or a high enough quality substring match.
// See if we can find a camel case match.
var match = TryCamelCaseMatch(candidate, patternChunk, punctuationStripped, patternIsLowercase, candidateHumps);
if (match != null)
return match;
// Didn't have an exact/prefix match, or a high enough quality substring match.
// See if we can find a camel case match.
var match = TryCamelCaseMatch(candidate, patternChunk, punctuationStripped, patternIsLowercase, candidateHumps);
if (match != null)
return match;
}

// If pattern was all lowercase, we allow it to match an all lowercase section of the candidate. But
// only after we've tried all other forms first. This is the weakest of all matches. For example, if
Expand All @@ -265,6 +295,12 @@ private static bool ContainsUpperCaseLetter(string pattern)
}

return null;

void PopulateCandidateHumps()
{
if (candidateHumps.Count == 0)
StringBreaker.AddWordParts(candidate, ref candidateHumps.AsRef());
}
}

private TextSpan? GetMatchedSpan(int start, int length)
Expand Down
Loading