Fix span write in InvariantCreateSortKeyOrdinalIgnoreCase

[xtqqczze]

cc: @tarekgh

Existing logic appears to be incorrect:

BinaryPrimitives.WriteUInt16BigEndian(sortKey, hr);
BinaryPrimitives.WriteUInt16BigEndian(sortKey, lr)

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

[tarekgh]

@xtqqczze Thanks for your contribution. At the moment, we are only accepting critical changes for the .NET 9.0 release. Nice-to-have fixes will need to wait until we fork the main branch to accept changes for future releases. Since this PR is already a draft, we can keep it open for a while until the fork. Alternatively, you can close it and reopen it later after the fork. I wanted to set the expectation in case you notice no traction on the PR.

CC @ericstj @stephentoub

[xtqqczze]

@tarekgh This fixes a probable bug.

[tarekgh]

@xtqqczze I'll take a look. Thanks!

[tarekgh]

@xtqqczze could you please explain more why you think there is a bug?

InvariantCreateSortKeyOrdinalIgnoreCase is called guaranteeing the condition Debug.Assert(sortKey.Length >= source.Length * sizeof(char));.

[xtqqczze]

@xtqqczze could you please explain more why you think there is a bug?

@tarekgh When writing to the span, we overwrite the high surrogate with the low surrogate:

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs

Lines 69 to 70 in f644110

	BinaryPrimitives.WriteUInt16BigEndian(sortKey, hr);
	BinaryPrimitives.WriteUInt16BigEndian(sortKey, lr);

This is unexpected, at least.

[stephentoub]

Can you share a test that fails because of the issue?

[tarekgh]

When writing to the span, we overwrite the high surrogate with the low surrogate:

I see. I was confused with the line Span<byte> tmp = sortKey.Slice(0, 2 * sizeof(ushort)); // help with bounds check elimination which I don't think we need it.

Can you share a test that fails because of the issue?

This is a good idea to add a test for this case.

[tarekgh]

Here is the code can be used in the test case:

            CultureInfo culture = CultureInfo.InvariantCulture;
            var sortKey = culture.CompareInfo.GetSortKey("\uD800\uDC00", CompareOptions.IgnoreCase);

            foreach (var b in sortKey.KeyData)
            {
                Console.Write($"{b:X2} "); // should produce something like D8-00-DC-00
            }

We need to add the test to https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime/tests/System.Globalization.Tests/Invariant

[xtqqczze]

This is technically a breaking change to GetSortKey(ReadOnlySpan, Span, CompareOptions) in the Bucket 3: Unlikely Grey Area category.

[xtqqczze]

@MihuBot

[tarekgh]

This is technically a breaking change to GetSortKey(ReadOnlySpan, Span, CompareOptions) in the Bucket 3: Unlikely Grey Area category.

I am not worried about that. The issue is specific for Invariant Globalization mode with Surrogate pairs. Sort keys usually interesting with the full globalization support.

[tarekgh]

Thanks @xtqqczze!

[stephentoub]

/ba-g failures are known