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.

Sign up for GitHub

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

Fix incorrect case sensitivity in FrozenDictionary and FrozenSet for some cases #93986

Closed
wants to merge 6 commits into from
+419 −64
Closed

Fix incorrect case sensitivity in FrozenDictionary and FrozenSet for some cases #93986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a42c3e
Add failing tests
andrewjsaid Oct 25, 2023
a94513c
Fix incorrect case sensitivity in FrozenDictionary and FrozenSet for …
andrewjsaid Oct 26, 2023
328d9f1
When hashing the entire string, case sensitivity of hash and equals s…
andrewjsaid Oct 26, 2023
9d982d6
Merge branch 'main' into frozen-collections-ordinal-ignore-case
andrewjsaid Nov 8, 2023
2f2a018
Address code review comments
andrewjsaid Nov 8, 2023
fdbb96a
Merge branch 'main' into frozen-collections-ordinal-ignore-case
andrewjsaid Nov 9, 2023
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
10 changes: 9 additions & 1 deletion src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ The System.Collections.Immutable library is built-in as part of the shared frame

<ItemGroup>
<Compile Include="Properties\InternalsVisibleTo.cs" />

<Compile Include="System\Polyfills.cs" />
<Compile Include="System\Collections\ThrowHelper.cs" />
<Compile Include="$(CoreLibSharedDir)System\Collections\HashHelpers.cs" Link="System\Collections\HashHelpers.cs" />
Expand Down Expand Up @@ -67,6 +67,14 @@ The System.Collections.Immutable library is built-in as part of the shared frame
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenSet_RightJustifiedCaseInsensitiveSubstring.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenSet_RightJustifiedSubstring.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenSet_RightJustifiedSingleChar.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenDictionary_LeftJustifiedSingleCharCaseInsensitive.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenDictionary_LeftJustifiedSubstringCaseInsensitive.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenDictionary_RightJustifiedSingleCharCaseInsensitive.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenDictionary_RightJustifiedSubstringCaseInsensitive.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenSet_LeftJustifiedSingleCharCaseInsensitive.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenSet_LeftJustifiedSubstringCaseInsensitive.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenSet_RightJustifiedSingleCharCaseInsensitive.cs" />
<Compile Include="System\Collections\Frozen\String\OrdinalStringFrozenSet_RightJustifiedSubstringCaseInsensitive.cs" />

<Compile Include="System\Collections\Generic\IHashKeyCollection.cs" />
<Compile Include="System\Collections\Generic\ISortKeyCollection.cs" />
Expand Down
46 changes: 34 additions & 12 deletions src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,45 +181,67 @@ private static FrozenDictionary<TKey, TValue> CreateFromDictionary<TKey, TValue>
{
if (analysis.RightJustifiedSubstring)
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
frozenDictionary = analysis.AllAsciiIfIgnoreCase
Debug.Assert(analysis.IgnoreCase);
frozenDictionary = analysis.AllAsciiIfIgnoreCaseForHash
? new OrdinalStringFrozenDictionary_RightJustifiedCaseInsensitiveAsciiSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenDictionary_RightJustifiedCaseInsensitiveSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
else
{
frozenDictionary = analysis.HashCount == 1
? new OrdinalStringFrozenDictionary_RightJustifiedSingleChar<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenDictionary_RightJustifiedSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
if (analysis.HashCount == 1)
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
{
frozenDictionary = analysis.IgnoreCase
? new OrdinalStringFrozenDictionary_RightJustifiedSingleCharCaseInsensitive<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenDictionary_RightJustifiedSingleChar<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex);
}
else
{
frozenDictionary = analysis.IgnoreCase
? new OrdinalStringFrozenDictionary_RightJustifiedSubstringCaseInsensitive<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenDictionary_RightJustifiedSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
}
}
else
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
frozenDictionary = analysis.AllAsciiIfIgnoreCase
Debug.Assert(analysis.IgnoreCase);
frozenDictionary = analysis.AllAsciiIfIgnoreCaseForHash
? new OrdinalStringFrozenDictionary_LeftJustifiedCaseInsensitiveAsciiSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenDictionary_LeftJustifiedCaseInsensitiveSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
else
{
frozenDictionary = analysis.HashCount == 1
? new OrdinalStringFrozenDictionary_LeftJustifiedSingleChar<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenDictionary_LeftJustifiedSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
if (analysis.HashCount == 1)
{
frozenDictionary = analysis.IgnoreCase
? new OrdinalStringFrozenDictionary_LeftJustifiedSingleCharCaseInsensitive<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenDictionary_LeftJustifiedSingleChar<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex);
}
else
{
frozenDictionary = analysis.IgnoreCase
? new OrdinalStringFrozenDictionary_LeftJustifiedSubstringCaseInsensitive<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenDictionary_LeftJustifiedSubstring<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
}
}
}
else
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
frozenDictionary = analysis.AllAsciiIfIgnoreCase
Debug.Assert(analysis.IgnoreCase);
frozenDictionary = analysis.AllAsciiIfIgnoreCaseForHash
? new OrdinalStringFrozenDictionary_FullCaseInsensitiveAscii<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff)
: new OrdinalStringFrozenDictionary_FullCaseInsensitive<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff);
}
else
{
// if (IgnoreCase) => Can only be true if there are no letters, thus case sensitive comparison still works here.
Copy link
Member

@eiriktsarpalis eiriktsarpalis Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there an assertion that could replace this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so because the assertion would just be re-calculating what the KeyAnalyzer does.

frozenDictionary = new OrdinalStringFrozenDictionary_Full<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff);
}
}
Expand Down
46 changes: 34 additions & 12 deletions src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,45 +129,67 @@ private static FrozenSet<T> CreateFromSet<T>(HashSet<T> source)
{
if (analysis.RightJustifiedSubstring)
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
frozenSet = analysis.AllAsciiIfIgnoreCase
Debug.Assert(analysis.IgnoreCase);
frozenSet = analysis.AllAsciiIfIgnoreCaseForHash
? new OrdinalStringFrozenSet_RightJustifiedCaseInsensitiveAsciiSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenSet_RightJustifiedCaseInsensitiveSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
else
{
frozenSet = analysis.HashCount == 1
? new OrdinalStringFrozenSet_RightJustifiedSingleChar(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenSet_RightJustifiedSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
if (analysis.HashCount == 1)
{
frozenSet = analysis.IgnoreCase
? new OrdinalStringFrozenSet_RightJustifiedSingleCharCaseInsensitive(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenSet_RightJustifiedSingleChar(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex); ;
}
else
{
frozenSet = analysis.IgnoreCase
? new OrdinalStringFrozenSet_RightJustifiedSubstringCaseInsensitive(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenSet_RightJustifiedSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
}
}
else
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
andrewjsaid marked this conversation as resolved.
Show resolved Hide resolved
frozenSet = analysis.AllAsciiIfIgnoreCase
Debug.Assert(analysis.IgnoreCase);
frozenSet = analysis.AllAsciiIfIgnoreCaseForHash
? new OrdinalStringFrozenSet_LeftJustifiedCaseInsensitiveAsciiSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenSet_LeftJustifiedCaseInsensitiveSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
else
{
frozenSet = analysis.HashCount == 1
? new OrdinalStringFrozenSet_LeftJustifiedSingleChar(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenSet_LeftJustifiedSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
if (analysis.HashCount == 1)
{
frozenSet = analysis.IgnoreCase
? new OrdinalStringFrozenSet_LeftJustifiedSingleCharCaseInsensitive(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex)
: new OrdinalStringFrozenSet_LeftJustifiedSingleChar(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex);
}
else
{
frozenSet = analysis.IgnoreCase
? new OrdinalStringFrozenSet_LeftJustifiedSubstringCaseInsensitive(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount)
: new OrdinalStringFrozenSet_LeftJustifiedSubstring(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff, analysis.HashIndex, analysis.HashCount);
}
}
}
}
else
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
frozenSet = analysis.AllAsciiIfIgnoreCase
Debug.Assert(analysis.IgnoreCase);
frozenSet = analysis.AllAsciiIfIgnoreCaseForHash
? new OrdinalStringFrozenSet_FullCaseInsensitiveAscii(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff)
: new OrdinalStringFrozenSet_FullCaseInsensitive(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff);
}
else
{
// if (IgnoreCase) => Can only be true if there are no letters, thus case sensitive comparison still works here.
frozenSet = new OrdinalStringFrozenSet_Full(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff);
}
}
Expand Down
30 changes: 17 additions & 13 deletions ...ibraries/System.Collections.Immutable/src/System/Collections/Frozen/String/KeyAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,18 +118,20 @@ private static AnalysisResults CreateAnalysisResults(
ReadOnlySpan<string> uniqueStrings, bool ignoreCase, int minLength, int maxLength, int index, int count, GetSpan getSubstringSpan)
{
// Start off by assuming all strings are ASCII
bool allAsciiIfIgnoreCase = true;
bool allAsciiIfIgnoreCaseForHash = true;

bool ignoreCaseForHash = ignoreCase;

// If we're case-sensitive, it doesn't matter if the strings are ASCII or not.
// But if we're case-insensitive, we can switch to a faster comparer if all the
// substrings are ASCII, so we check each.
if (ignoreCase)
if (ignoreCaseForHash)
{
// Further, if the ASCII substrings don't contain any letters, then we can
// actually perform the comparison as case-sensitive even if case-insensitive
// was requested, as there's nothing that would compare equally to the substring
// other than the substring itself.
bool canSwitchIgnoreCaseToCaseSensitive = true;
bool canSwitchIgnoreCaseHashToCaseSensitive = true;

foreach (string s in uniqueStrings)
{
Expand All @@ -139,28 +141,28 @@ private static AnalysisResults CreateAnalysisResults(
// If the substring isn't ASCII, bail out to return the results.
if (!IsAllAscii(substring))
{
allAsciiIfIgnoreCase = false;
canSwitchIgnoreCaseToCaseSensitive = false;
allAsciiIfIgnoreCaseForHash = false;
canSwitchIgnoreCaseHashToCaseSensitive = false;
break;
}

// All substrings so far are still ASCII only. If this one contains any ASCII
// letters, mark that we can't switch to case-sensitive.
if (canSwitchIgnoreCaseToCaseSensitive && ContainsAnyLetters(substring))
if (canSwitchIgnoreCaseHashToCaseSensitive && ContainsAnyLetters(substring))
{
canSwitchIgnoreCaseToCaseSensitive = false;
canSwitchIgnoreCaseHashToCaseSensitive = false;
}
}

// If we can switch to case-sensitive, do so.
if (canSwitchIgnoreCaseToCaseSensitive)
if (canSwitchIgnoreCaseHashToCaseSensitive)
{
ignoreCase = false;
ignoreCaseForHash = false;
}
}

// Return the analysis results.
return new AnalysisResults(ignoreCase, allAsciiIfIgnoreCase, index, count, minLength, maxLength);
return new AnalysisResults(ignoreCase, ignoreCaseForHash, allAsciiIfIgnoreCaseForHash, index, count, minLength, maxLength);
}

private delegate ReadOnlySpan<char> GetSpan(string s, int index, int count);
Expand Down Expand Up @@ -243,18 +245,20 @@ internal static bool HasSufficientUniquenessFactor(HashSet<string> set, ReadOnly

internal readonly struct AnalysisResults
{
public AnalysisResults(bool ignoreCase, bool allAsciiIfIgnoreCase, int hashIndex, int hashCount, int minLength, int maxLength)
public AnalysisResults(bool ignoreCase, bool ignoreCaseForHash, bool allAsciiIfIgnoreCaseForHash, int hashIndex, int hashCount, int minLength, int maxLength)
{
IgnoreCase = ignoreCase;
AllAsciiIfIgnoreCase = allAsciiIfIgnoreCase;
IgnoreCaseForHash = ignoreCaseForHash;
AllAsciiIfIgnoreCaseForHash = allAsciiIfIgnoreCaseForHash;
HashIndex = hashIndex;
HashCount = hashCount;
MinimumLength = minLength;
MaximumLengthDiff = maxLength - minLength;
}

public bool IgnoreCase { get; }
public bool AllAsciiIfIgnoreCase { get; }
public bool IgnoreCaseForHash { get; }
public bool AllAsciiIfIgnoreCaseForHash { get; }
public int HashIndex { get; }
public int HashCount { get; }
public int MinimumLength { get; }
Expand Down
Loading
Loading