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

SortedDictionary Behaves Differently Across .NET Core Platforms #43802

Closed
aolszowka opened this issue Oct 25, 2020 · 5 comments
Closed

SortedDictionary Behaves Differently Across .NET Core Platforms #43802

aolszowka opened this issue Oct 25, 2020 · 5 comments

Comments

@aolszowka
Copy link

Description

SortedDictionary appears to behave differently across .NET Core Platforms.

I have created the following GitHub Repository that demonstrates this issue: https://github.com/aolszowka/DotNetCoreSortedDictionaryCrossPlatform

The crux of the issue can be highlighted in this NUnit Test:

[Test]
public void DifferenceBetweenPlatforms()
{
    SortedDictionary<string, int> sortedDictionary = new SortedDictionary<string, int>();

    sortedDictionary.Add("Project(\"{778DAE3C-4631-46EA-AA77-85C1314464D9}\") = \"Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes\", \"src\\CodeStyle\\VisualBasic\\CodeFixes\\Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.vbproj\", \"{0141285D-8F6C-42C7-BAF3-3C0CCD61C716}\"", 0);
    sortedDictionary.Add("Project(\"{778DAE3C-4631-46EA-AA77-85C1314464D9}\") = \"Microsoft.CodeAnalysis.VisualBasic\", \"src\\Compilers\\VisualBasic\\Portable\\Microsoft.CodeAnalysis.VisualBasic.vbproj\", \"{2523D0E6-DF32-4A3E-8AE0-A19BFFAE2EF6}\"", 0);

    StringBuilder sb = new StringBuilder();

    foreach (var kvp in sortedDictionary)
    {
        sb.AppendLine(kvp.Key);
    }

    string actual = sb.ToString();

    // This is how it will appear on Windows, but in Ubuntu this fails? (maybe?)
    string expected =
        "Project(\"{778DAE3C-4631-46EA-AA77-85C1314464D9}\") = \"Microsoft.CodeAnalysis.VisualBasic\", \"src\\Compilers\\VisualBasic\\Portable\\Microsoft.CodeAnalysis.VisualBasic.vbproj\", \"{2523D0E6-DF32-4A3E-8AE0-A19BFFAE2EF6}\"" + Environment.NewLine +
        "Project(\"{778DAE3C-4631-46EA-AA77-85C1314464D9}\") = \"Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes\", \"src\\CodeStyle\\VisualBasic\\CodeFixes\\Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.vbproj\", \"{0141285D-8F6C-42C7-BAF3-3C0CCD61C716}\"" + Environment.NewLine;

    Assert.That(actual, Is.EqualTo(expected));
}

This unit test will pass on Windows based platforms but fail in Ubuntu and MacOS (at least the build agents as provided by GitHub Actions).

Furthermore this issue appears to be length dependent, as the following will pass on all platforms:

[Test]
public void DifferenceBetweenPlatforms_LengthDependent_Theory()
{
    SortedDictionary<string, int> sortedDictionary = new SortedDictionary<string, int>();

    sortedDictionary.Add("Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes", 0);
    sortedDictionary.Add("Microsoft.CodeAnalysis.VisualBasic", 0);

    StringBuilder sb = new StringBuilder();

    foreach (var kvp in sortedDictionary)
    {
        sb.AppendLine(kvp.Key);
    }

    string actual = sb.ToString();

    // This apparently works in both Windows and Ubuntu?
    string expected =
        "Microsoft.CodeAnalysis.VisualBasic" + Environment.NewLine +
        "Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes" + Environment.NewLine;

    Assert.That(actual, Is.EqualTo(expected));
}

I have not bothered to narrow this down due to the limits on GitHub Actions (not looking to burn all my CI Minutes right now).

Configuration

This is using windows-latest, ubuntu-latest, macos-latest as defined by GitHub Actions here: https://github.com/actions/virtual-environments.

You should be able to clone this repository and instantly have GitHub spin up the appropriate CI environments for you allowing you to test across all platforms serviced by GitHub Actions.

Other information

In searching existing bug reports I may be affected by one of the following, but I am not familiar enough with the runtime to know if this is the case:

Neither of these cases mentions the length of the string being in play. In the issues however they all appears to be using various forms of localization/localized strings.

Here are the results from my area in GitHub Actions:

Windows

Run dotnet test --configuration Release
Test run for D:\a\DotNetCoreSortedDictionaryCrossPlatform\DotNetCoreSortedDictionaryCrossPlatform\bin\Release\netcoreapp3.1\DotNetCoreSortedDictionaryCrossPlatform.dll(.NETCoreApp,Version=v3.1)
Microsoft (R) Test Execution Command Line Tool Version 16.7.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...

A total of 1 test files matched the specified pattern.

Test Run Successful.
Total tests: 2
     Passed: 2
 Total time: 1.1261 Seconds

Ubuntu

Run dotnet test --configuration Release
Test run for /home/runner/work/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform/bin/Release/netcoreapp3.1/DotNetCoreSortedDictionaryCrossPlatform.dll(.NETCoreApp,Version=v3.1)
Microsoft (R) Test Execution Command Line Tool Version 16.7.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...

A total of 1 test files matched the specified pattern.
  X DifferenceBetweenPlatforms [192ms]
  Error Message:
     String lengths are both 455. Strings differ at index 87.
  Expected: "...osoft.CodeAnalysis.VisualBasic", "src\\Compilers\\VisualBasi..."
  But was:  "...osoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes", "src\\Code..."
  --------------------------------------------^

  Stack Trace:
     at DotNetCoreSortedDictionaryCrossPlatform.SortedDictionaryTest.DifferenceBetweenPlatforms() in /home/runner/work/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform/SortedDictionaryTest.cs:line 34


Test Run Failed.
Total tests: 2
     Passed: 1
     Failed: 1
 Total time: 1.2799 Seconds
/usr/share/dotnet/sdk/3.1.403/Microsoft.TestPlatform.targets(32,5): error MSB4181: The "Microsoft.TestPlatform.Build.Tasks.VSTestTask" task returned false but did not log an error. [/home/runner/work/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform.csproj]
Error: Process completed with exit code 1.

MacOS

Run dotnet test --configuration Release
Test run for /Users/runner/work/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform/bin/Release/netcoreapp3.1/DotNetCoreSortedDictionaryCrossPlatform.dll(.NETCoreApp,Version=v3.1)
Microsoft (R) Test Execution Command Line Tool Version 16.7.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...

A total of 1 test files matched the specified pattern.
  X DifferenceBetweenPlatforms [405ms]
  Error Message:
     String lengths are both 455. Strings differ at index 87.
  Expected: "...osoft.CodeAnalysis.VisualBasic", "src\\Compilers\\VisualBasi..."
  But was:  "...osoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes", "src\\Code..."
  --------------------------------------------^

  Stack Trace:
     at DotNetCoreSortedDictionaryCrossPlatform.SortedDictionaryTest.DifferenceBetweenPlatforms() in /Users/runner/work/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform/SortedDictionaryTest.cs:line 34


Test Run Failed.
Total tests: 2
     Passed: 1
     Failed: 1
 Total time: 1.9944 Seconds
/Users/runner/.dotnet/sdk/3.1.403/Microsoft.TestPlatform.targets(32,5): error MSB4181: The "Microsoft.TestPlatform.Build.Tasks.VSTestTask" task returned false but did not log an error. [/Users/runner/work/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform/DotNetCoreSortedDictionaryCrossPlatform.csproj]
Error: Process completed with exit code 1.

Thank you

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Oct 25, 2020
@ghost
Copy link

ghost commented Oct 25, 2020

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

@dagood
Copy link
Member

dagood commented Oct 25, 2020

When you create your SortedDictionary<string, int> here, you aren't passing it a comparer, so it uses the default string comparer:

SortedDictionary<string, int> sortedDictionary = new SortedDictionary<string, int>();

The default string comparer can have different results based on the running environment (such as platform or culture). Here's an earlier instance of this expected behavior, specifically about Windows vs. macOS/Linux and the ICU library: #20109.

It's generally best practice to avoid using string comparison defaults so you don't run into this kind of issue: https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#specifying-string-comparisons-explicitly

It looks like you're sorting .sln file lines--I think StringComparer.Ordinal would work fine for this. Can you try that?

@aolszowka
Copy link
Author

aolszowka commented Oct 25, 2020

Thank you @dagood I can confirm in the toy program it works as expected.

It looks like you're sorting .sln file lines

You are correct; I am working on converting this tool to .NET Core/Dotnet Tool - https://github.com/aolszowka/VisualStudioSolutionSorter/. This actually kicked out due to using the Ubuntu Build Agent, but shows that I should be testing this logic across all .NET Core platforms.

It's generally best practice to avoid using string comparison defaults so you don't run into this kind of issue: https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#specifying-string-comparisons-explicitly

Based on the fact that this is documented as so, perhaps it would be worthwhile to submit a feature request to dotnet/roslyn to have an analyzer cover this? I do not see any open items for this yet.

Historically I have used this excellent Roslyn Analyzer from @meziantou https://github.com/meziantou/Meziantou.Analyzer/tree/master/docs. Specifically this warning https://github.com/meziantou/Meziantou.Analyzer/blob/master/docs/Rules/MA0001.md should probably be enhanced to cover this (I will file a separate bug report), but considering how common this maybe perhaps it is worth elevating to a built in?

As far as this issue goes, if this is by design I am OK closing with with a by-design status and moving forward from here. Let me know.

@reflectronic
Copy link
Contributor

For the record, .NET 5 will try to use ICU by default on Windows as well. See the breaking change doc for more info.

@eiriktsarpalis
Copy link
Member

Closing this in favour of the related ICU issues.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants