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
Closed

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

Show file tree
Hide file tree
Changes from 3 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 @@ -11,7 +11,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 @@ -66,6 +66,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
37 changes: 28 additions & 9 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,64 @@ private static FrozenDictionary<TKey, TValue> CreateFromDictionary<TKey, TValue>
{
if (analysis.RightJustifiedSubstring)
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
frozenDictionary = analysis.AllAsciiIfIgnoreCase
? 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.IgnoreCaseForEquals
? 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.IgnoreCaseForEquals
? 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
? 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.IgnoreCaseForEquals
? 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.IgnoreCaseForEquals
? 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
? new OrdinalStringFrozenDictionary_FullCaseInsensitiveAscii<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff)
: new OrdinalStringFrozenDictionary_FullCaseInsensitive<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff);
}
else
{
// if (IgnoreCaseForEquals) => Can only be true if there are no letters, thus case sensitive comparison still works here.
frozenDictionary = new OrdinalStringFrozenDictionary_Full<TValue>(keys, values, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff);
}
}
Expand Down
37 changes: 28 additions & 9 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,64 @@ private static FrozenSet<T> CreateFromSet<T>(HashSet<T> source)
{
if (analysis.RightJustifiedSubstring)
{
if (analysis.IgnoreCase)
if (analysis.IgnoreCaseForHash)
{
frozenSet = analysis.AllAsciiIfIgnoreCase
? 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.IgnoreCaseForEquals
? 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.IgnoreCaseForEquals
? 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
? 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.IgnoreCaseForEquals
? 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.IgnoreCaseForEquals
? 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
? new OrdinalStringFrozenSet_FullCaseInsensitiveAscii(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff)
: new OrdinalStringFrozenSet_FullCaseInsensitive(entries, stringComparer, analysis.MinimumLength, analysis.MaximumLengthDiff);
}
else
{
// if (IgnoreCaseForEquals) => 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
12 changes: 8 additions & 4 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 @@ -120,6 +120,8 @@ private static AnalysisResults CreateAnalysisResults(
// Start off by assuming all strings are ASCII
bool allAsciiIfIgnoreCase = true;

bool ignoreCaseForEquals = 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.
Expand Down Expand Up @@ -160,7 +162,7 @@ private static AnalysisResults CreateAnalysisResults(
}

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

private delegate ReadOnlySpan<char> GetSpan(string s, int index, int count);
Expand Down Expand Up @@ -243,17 +245,19 @@ 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 ignoreCaseForHash, bool ignoreCaseForEquals, bool allAsciiIfIgnoreCase, int hashIndex, int hashCount, int minLength, int maxLength)
{
IgnoreCase = ignoreCase;
IgnoreCaseForHash = ignoreCaseForHash;
IgnoreCaseForEquals = ignoreCaseForEquals;
andrewjsaid marked this conversation as resolved.
Show resolved Hide resolved
AllAsciiIfIgnoreCase = allAsciiIfIgnoreCase;
HashIndex = hashIndex;
HashCount = hashCount;
MinimumLength = minLength;
MaximumLengthDiff = maxLength - minLength;
}

public bool IgnoreCase { get; }
public bool IgnoreCaseForHash { get; }
public bool IgnoreCaseForEquals { get; }
public bool AllAsciiIfIgnoreCase { get; }
public int HashIndex { get; }
public int HashCount { get; }
Expand Down
29 changes: 29 additions & 0 deletions ...ons/Frozen/String/OrdinalStringFrozenDictionary_LeftJustifiedSingleCharCaseInsensitive.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

Following naming convention of other files already committed, shouldn't this be OrdinalStringFrozenDictionary_LeftJustifiedCaseInsensitiveSingleChar.cs?

// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;

namespace System.Collections.Frozen
{
internal sealed class OrdinalStringFrozenDictionary_LeftJustifiedSingleCharCaseInsensitive<TValue> : OrdinalStringFrozenDictionary<TValue>
{
internal OrdinalStringFrozenDictionary_LeftJustifiedSingleCharCaseInsensitive(
string[] keys,
TValue[] values,
IEqualityComparer<string> comparer,
int minimumLength,
int maximumLengthDiff,
int hashIndex)
: base(keys, values, comparer, minimumLength, maximumLengthDiff, hashIndex, 1)
{
}

// This override is necessary to force the jit to emit the code in such a way that it
// avoids virtual dispatch overhead when calling the Equals/GetHashCode methods. Don't
// remove this, or you'll tank performance.
private protected override ref readonly TValue GetValueRefOrNullRefCore(string key) => ref base.GetValueRefOrNullRefCore(key);

private protected override bool Equals(string? x, string? y) => StringComparer.OrdinalIgnoreCase.Equals(x, y);
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
private protected override int GetHashCode(string s) => s[HashIndex];
}
}
30 changes: 30 additions & 0 deletions ...ions/Frozen/String/OrdinalStringFrozenDictionary_LeftJustifiedSubstringCaseInsensitive.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;

namespace System.Collections.Frozen
{
internal sealed class OrdinalStringFrozenDictionary_LeftJustifiedSubstringCaseInsensitive<TValue> : OrdinalStringFrozenDictionary<TValue>
Copy link
Member

Choose a reason for hiding this comment

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

There's already an implementation like this here:

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/String/OrdinalStringFrozenDictionary_LeftJustifiedCaseInsensitiveSubstring.cs

{
internal OrdinalStringFrozenDictionary_LeftJustifiedSubstringCaseInsensitive(
string[] keys,
TValue[] values,
IEqualityComparer<string> comparer,
int minimumLength,
int maximumLengthDiff,
int hashIndex,
int hashCount)
: base(keys, values, comparer, minimumLength, maximumLengthDiff, hashIndex, hashCount)
{
}

// This override is necessary to force the jit to emit the code in such a way that it
// avoids virtual dispatch overhead when calling the Equals/GetHashCode methods. Don't
// remove this, or you'll tank performance.
private protected override ref readonly TValue GetValueRefOrNullRefCore(string key) => ref base.GetValueRefOrNullRefCore(key);

private protected override bool Equals(string? x, string? y) => StringComparer.OrdinalIgnoreCase.Equals(x, y);
private protected override int GetHashCode(string s) => Hashing.GetHashCodeOrdinal(s.AsSpan(HashIndex, HashCount));
}
}
Loading
Loading