runtime: Reference source for System.Security.Cryptography is generated inconsistently between platforms

@ericstj @joperezr It’s concerning that ApiCompat doesn’t flag the missing overrides

APIcompat’s job has always been to find binary compatibility breaks. Removing an override isn’t binary breaking and it’s actually something that API compat needed to tolerate with a lot of the .NETFramework -> .NET Core refactoring. Today we try to make API compat also check for “ref consistency” by running it backwards but it still ignores some things we would prefer it didn’t. We also have https://github.com/dotnet/arcade/issues/2570 around this behavior in the existing API compat. To better support this scenario the new API compat has a “strict” mode which is meant to catch things like this but I see it still permits an override removal, so I filed an issue to catch that.

and the missing interface

I don’t think APICompat was missing anything here. A missing interface would be a break but according to @vcsjones

Every implementation of CngKey that I can see implements IDisposable.

This was fishy. I tried a local repro and saw the same problem. Here’s what I did to repro (after having a full build):

cd D:\src\dotnet\runtime\src\libraries\System.Security.Cryptography\src
dotnet msbuild /p:TargetFramework=net7.0-Unix /bl:build.binlog
dotnet msbuild /p:TargetFramework=net7.0-Unix /t:GenerateReferenceSource /bl:genref.binlog

I could see the diff, so I checked the built assembly (input to genAPI) to see what’s happening. It was also missing the interface! I checked the input to the compiler and found it was correct https://github.com/dotnet/runtime/blob/c3dbdea91835d67cc461b22125e652ad5063d746/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Cng.NotSupported.cs#L101

I double checked the output of the compiler and it looked like it was missing the interface, which didn’t make sense. So I went to the build log and saw the problem.

Moving file from "D:\src\dotnet\runtime\artifacts\obj\System.Security.Cryptography\Debug\net7.0-unix\System.Security.Cryptography.dll" to "D:\src\dotnet\runtime\artifacts\obj\System.Security.Cryptography\Debug\net7.0-unix\PreTrim/System.Security.Cryptography.dll".

If I check the pre-trimmed assembly it had the interface implementation. So the linker was removing it.

So, two problems:

1. The linker is removing IDisposable, which it should never do for the mode we run on libraries
2. APICompat is running before the linker, so it missed catching this problem

/Users/runner/work/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22159.9/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : Compat issues with assembly System.Security.Cryptography: [/Users/runner/work/1/s/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj]
/Users/runner/work/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22159.9/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : CannotRemoveBaseTypeOrInterface : Type 'System.Security.Cryptography.CngKey' does not implement interface 'System.IDisposable' in the implementation but it does in the contract. [/Users/runner/work/1/s/src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj]
/Users/runner/work/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22159.9/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : Compat issues with assembly System.Net.NetworkInformation: [/Users/runner/work/1/s/src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj]
/Users/runner/work/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22159.9/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : CannotRemoveBaseTypeOrInterface : Type 'System.Net.NetworkInformation.IPAddressInformationCollection' does not implement interface 'System.Collections.Generic.ICollection<System.Net.NetworkInformation.IPAddressInformation>' in the implementation but it does in the contract. [/Users/runner/work/1/s/src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj]

from https://github.com/dotnet/runtime/pull/66706. System.Net.NetworkInformation is impacted as well. https://github.com/dotnet/linker/issues/2238 tracks the linker bug.

To scope the concern to the S.S.Cryptography library, it appears that adding /p:TargetOS=windows ($ …/…/…/…/.dotnet/dotnet msbuild /t:GenerateReferenceAssemblySource /p:TargetOS=windows) does generate the less-noisy ref.cs.

I think this just makes it match more similarly to what is already there, not something that is necessarily more “correct”.

For example, DESCryptoServiceProvider on Linux / macOS has more overrides than what is present on Windows, so GenerateReferenceAssemblySource on Linux currently means a diff that results in additional members. I would assume then that both Linux and Windows need the overrides in their implementations.

So I believe there are legitimate concerns, in that case or in RSAOpenSsl for example, where Windows is missing some overrides.

It does feel a bit backwards that GenAPI is running on post-trim and APICompat is on pre-trim.

I can fix the differences but I’m worried we are going to drift again if ApiCompat doesn’t warn us of this. Should I open a separate issue for that?