Hash code of string is broken in .NET Core 2.1, but works in 2.0

[karelz]

From @mwikstrom on October 30, 2018 5:23

I recently upgraded one of my projects from .NET Core 2.0 to .NET Core 2.1. After doing so several of my tests started to fail.

After narrowing this down I've found that in .NET Core 2.1 it is not possible to compute the hash code of a string using a culture aware comparer with the string sort compare option.

I've created a test that reproduce my problem:

    [TestMethod]
    public void Can_compute_hash_code_using_invariant_string_sort_comparer()
    {
        var compareInfo = CultureInfo.InvariantCulture.CompareInfo;
        var stringComparer = compareInfo.GetStringComparer(CompareOptions.StringSort);
        stringComparer.GetHashCode("test"); // should not throw!
    }

I've tested it on a couple of frameworks with the following results:

• .NET Core 2.0: ✔ PASS
• .NET Core 2.1: ✖ FAIL
• .NET Framework 4.7: ✖ FAIL
• .NET Framework 4.6.2: ✖ FAIL

When failing an ArgumentException is thrown from CompareInfo.GetHashCodeOfString saying:

Value of flags is invalid

Now, to my questions:

1. Why is it not allowed to use CompareOptions.StringSort when computing a hash code?

2. Why was it allowed in .NET Core 2.0?`

As far as I understand CompareOptions.StringSort only affects the relative sort order of strings and should not affect hash code computation. MSDN says:

StringSort Indicates that the string comparison must use the string sort algorithm. In a string sort, the hyphen and the apostrophe, as well as other nonalphanumeric symbols, come before alphanumeric characters.

Copied from original issue: dotnet/core#2030

[karelz]

From @mjwills on October 30, 2018 6:21

The issue might be that https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs#L53 and https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/StringComparer.cs#L177 (purposely or otherwise) are out of sync?

[karelz]

@tarekgh is that in your area of expertise?

[tarekgh]

Yes, this looks a regression from @atsushikan change in the commit: dotnet/corert@c6c0125#diff-63aa554c73fc39ae34377cf46321a8b2R208

The original code used to do the following: https://github.com/dotnet/corefx/blob/release/2.0.0/src/System.Globalization.Extensions/src/System/Globalization/Extensions.cs#L80

@mwikstrom

Thanks for reporting that. Considering this is the behavior on .NET Full framework from version 4.6, it'll be hard to change this behavior there and I am inclining not changing it on .NET Core 2.1 either for the sake of consistency with the full framework.
How important this issue to you especially working around it should be trivial?

[mwikstrom]

The workaround I've come up with is to use the following class:

internal sealed class CultureAwareStringSortComparer : StringComparer
{
    public CultureAwareStringSortComparer(CompareInfo compareInfo)
    {
        this.SortComparer = compareInfo.GetStringComparer(CompareOptions.StringSort);
        this.HashCodeComparer = compareInfo.GetStringComparer(CompareOptions.None);
    }

    internal StringComparer SortComparer { get; }

    internal StringComparer HashCodeComparer { get; }

    public override int Compare(string x, string y) => this.SortComparer.Compare(x, y);

    public override bool Equals(string x, string y) => this.HashCodeComparer.Equals(x, y);

    public override int GetHashCode(string obj) => this.HashCodeComparer.GetHashCode(obj);
}

The idea is to not use the StringSort option for GetHashCode.

All my tests are passing using this workaround. However I'm concerned with how the Equals method should be implemented.

As you can see I've chosen to use the hash code comparer for that. My reasoning behind that is that code that rely on hash codes also rely on equality, while code that rely on sort comparison should rely on a zero return value instead of equals.

I don't know how the implementation of Equals would differ whether I use StringSort or not. Maybe there is no difference? Then this workaround would be just fine.

Maybe you (@tarekgh) could ease my mind on this?

[tarekgh]

You should implement Equals as follow:

public override bool Equals(string x, string y) => this.SortComparer.Equals(x, y);

StringSort can affect the result of the string comparisons. other than that it looks OK to me.

also you may write it in more generic way like

    internal sealed class CultureAwareStringSortComparer : StringComparer
    {
        public CultureAwareStringSortComparer(CompareInfo compareInfo, CompareOptions options)
        {
            this.SortComparer = compareInfo.GetStringComparer(options);
            this.HashCodeComparer = compareInfo.GetStringComparer(options & ~CompareOptions.StringSort);
        }

        internal StringComparer SortComparer { get; }

        internal StringComparer HashCodeComparer { get; }

        public override int Compare(string x, string y) => this.SortComparer.Compare(x, y);

        public override bool Equals(string x, string y) => this.SortComparer.Equals(x, y);

        public override int GetHashCode(string obj) => this.HashCodeComparer.GetHashCode(obj);
    }

I am closing this issue but feel free to reply back with any question.

[mwikstrom]

Excellent. Thank you!