.NET Core 3 changed how high surrogates are encoded / decoded

[jaredpar]

Reference issue: http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/546621

In moving Roslyn to netcoreapp3.1 it's clear that how unicode high surrogates are encoded / decoded has changed in a way that breaks assumptions in the compiler. Specifically it breaks tests we added to guard against customer reported scenarios. Need to figure out how impactful this is.

For now disabling the relevant test on .NET Core. The Desktop version remains to verify the compiler side of this behavior.

[leppie]

Possibly related? dotnet/runtime#328

[tannergooding]

CC. @GrabYourPitchforks

[GrabYourPitchforks]

@jaredpar - that bug is from 2012. Are you saying the behavior has regressed in 3.1 such that GetCustomAttributes now throws an exception, or it doesn't throw an exception but instead returns an unexpected value, or something else?

What actual bytes get encoded into the DLL when you have [Obsolete("\uD800")] applied to a member? Are the UTF-16 bytes [ 00 D8 ] emitted into the DLL, or are the pseudo-UTF-8 bytes [ ED A0 80 ] emitted into the DLL, or is something else emitted?

The only thing that immediately comes to mind is that we changed the behavior of Encoding.UTF8.GetString to be more standards-compliant when it's given corrupted data.

Console.WriteLine(Encoding.UTF8.GetString(new byte[] { 0xED, 0xA0, 0x80 }));

On netcoreapp2.1 and prior, this prints "\uFFFD\uFFFD". On netcoreapp3.0 and later, this prints "\uFFFD\uFFFD\uFFFD".

[jaredpar]

@GrabYourPitchforks

that bug is from 2012.

Correct but not sure how that's relevant. It's a valid customer scenario even if the bug was filed in 2007.

Are you saying the behavior has regressed in 3.1 such that GetCustomAttributes now throws an exception, or it doesn't throw an exception but instead returns an unexpected value, or something else?

It's returning different values now.

[GrabYourPitchforks]

If the bytes that get burned into the DLL are [ ED A0 80 ] then the different return value is by design. See https://docs.microsoft.com/en-us/dotnet/core/compatibility/corefx#net-core-30-follows-unicode-best-practices-when-replacing-ill-formed-utf-8-byte-sequences for more information. If there are code paths that are now seeing an exception (per the linked VSO bug) that's definitely unintentional.

[tmat]

@GrabYourPitchforks Metadata must be able to encode all values, including incomplete surrogates. When encoding to UTF8 it encodes as ED A0 80. We have a custom UTF8 encoder/decoder in System.Reflection.Metadata that is able to handle this.

[GrabYourPitchforks]

Metadata must be able to encode all values, including incomplete surrogates.

I get that, which is why I'm surprised that there's a unit test that's specifically testing for data loss when using the reflection APIs. Was the subtext of this issue that the reflection APIs should also round-trip these values losslessly, just like the custom decoder used in the System.Reflection.Metadata package?

[tmat]

I might be misremembering. We might not have the custom encoder anymore. I think we did at one point.

[tmat]

Discussion here: https://github.com/dotnet/corefx/issues/2217