.NET 5 RC1 breaks string CompareTo

[333fred]

Description

Run this code:

using System;

namespace test
{
    class Program
    {
        static void Main(string[] args)
        {
            // The quotes are required for this repro
            var string1 = "'delegate'";
            var string2 = "'delegate sub'";
            Console.WriteLine(string1.CompareTo(string2));
        }
    }
}

On .NET 5 P7 and .NET 472, it prints -1. On .NET 5 RC1, it prints 1. This is preventing Roslyn from updating to a new version.

Configuration

.NET 5 RC1

Regression?

From .NET 5 P7

I'm not entirely sure if this was a regression in P8 or RC1: the Roslyn move to P8 has been delayed by other issues we've been working through.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

[tarekgh]

Marked with 5.0 till we investigate.

[Gnbrkm41]

This seems related with the use of ICU. Configuring the runtime to use NLS fixes the issue:

❯ $env:DOTNET_SYSTEM_GLOBALIZATION_USENLS
true
gotos@SURFACEBOOK  ~\source\repos\icurepro                                                                   [10:38]
❯ dotnet run
-1
gotos@SURFACEBOOK  ~\source\repos\icurepro                                                                   [10:38]
❯ $env:DOTNET_SYSTEM_GLOBALIZATION_USENLS = "false"
gotos@SURFACEBOOK  ~\source\repos\icurepro                                                                   [10:38]
❯ dotnet run
1

(Windows 10 Education Insiders, Build 20211, ko-KR)

[GrabYourPitchforks]

@333fred What's the OS version and language? en-US?

[tarekgh]

Yes I can see it with en-US. it is the behavior change between ICU and NLS. This is by design then. I am able to repro it with 3.1 on Linux. So, this is actually is not a new behavior anyway as we had it on Linux.

My question to Roslyn, why they are not using ordinal comparison here? having Linguistic comparison will make the behavior under the mercy of the current culture.

[tarekgh]

moving it out of 5.0 and I'll close it soon if there is no strong push back.

[tarekgh]

One more note, returning 1 make more sense to me too.

[333fred]

My question to Roslyn, why they are not using ordinal comparison here?

We were using the default behavior of string.Compare. It's very concerning to me that we're accepting a breaking change on the behavior of string.Compare with entirely ascii text, this seems like a massive breaking change.

So, this is actually is not a new behavior anyway as we had it on Linux.

Are you implying that it should have done this on Linux in .NET Core 3.1? If so, that would very much surprise me, as I use Linux day-to-day and haven't noticed test breakage.

[tarekgh]

It's very concerning to me that we're accepting a breaking change on the behavior of string.Compare with entirely ascii text, this seems like a massive breaking change.

Sorting behavior can change per OS and OS version especially when using different cultures. I am still wondering why didn't/don't use Ordinal especially you are talking about ASCII characters.

Are you implying that it should have done this on Linux in .NET Core 3.1? If so, that would very much surprise me, as I use Linux day-to-day and haven't noticed test breakage.

Yes. just run your repro code on Linux with 3.1 and you will see getting 1 as the result.

[333fred]

I am still wondering why didn't/don't use Ordinal especially you are talking about ASCII character

Literally because this is test code, and we just did the simple thing of CompareTo. I can certainly update our code to call a CompareTo overload that uses ordinal comparison. But we currently don't.

[tarekgh]

ok, thanks for clarifying. Could you please check Linux case and update us before I close the issue?

[333fred]

Could you please check Linux case and update us before I close the issue?

Apparently I have never run the tests affected by this bug on Linux, because I do get 1 in this case. While I'm still vaguely concerned that ascii sort is breaking with .NET 5, if you all have determined that you're willing to accept this then I'll shut up. Thanks.

[tarekgh]

We depend on the underlying OS/OS-library behavior for sorting. We understand there is a breaking change in general but we decided to have it for the sake of the benefits of using ICU library on Windows too. We are providing a config switch to go back using NLS (as 3.1 did) but this doesn't mean Windows NLS can decide at anytime to change the behavior too. It is by design the sorting behavior can change for Windows too. by that I am closing the issue. Thanks again for your input and confirming Linux case.

[GrabYourPitchforks]

It is by design the sorting behavior can change for Windows too.

For reference, this behavior is discussed here and here. The latter link has a reference to Shawn's blog post on the subject.

Literally because this is test code, and we just did the simple thing of CompareTo.

Yeah, the default behaviors are painful for non-UI apps. :(
We recently updated some analyzers to help catch these errors in runtime code, but it's typical for devs not to run analyzers over test code. I wonder if we should reconsider this stance and have some targeted analyzers enabled for test code - things that could help test maintainability?