dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
672 stars 349 forks source link

Microsoft.DotNet.ApiCompat incorrectly flags ValueTuple<T1, T2, ... ,TRest> missing in impl when using (T1, T2, ..., TRest) notation in the ref #3368

Open ahsonkhan opened 5 years ago

ahsonkhan commented 5 years ago

From https://github.com/dotnet/corefx/pull/39681#issuecomment-514016734

If we generate the System.Runtime ref using GenAPI, it uses ( , ) syntax to represent ValueTuple and hence changes the following type definition within the ref, along with the interface methods: https://github.com/dotnet/corefx/blob/0e91120b1b2c7b8af1357e06b33b74747fd99151/src/System.Runtime/ref/System.Runtime.cs#L3714

- public struct ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> : IComparable<ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>>, ...
+ public struct ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> : IComparable<(T1, T2, T3, T4, T5, T6, T7, TRest)>, ...
- public int CompareTo(ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> other) { throw null; }
+ public int CompareTo((T1, T2, T3, T4, T5, T6, T7, TRest) other) { throw null; }

image

This causes API compat to fail with the following error, even though the implementation (within corelib) hasn't changed and still satisfies the interface: https://github.com/dotnet/corefx/blob/0e91120b1b2c7b8af1357e06b33b74747fd99151/src/Common/src/CoreLib/System/ValueTuple.cs#L2057

C:\Users\ahkha\.nuget\packages\microsoft.dotnet.apicompat\1.0.0-beta.19369.2\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotRemoveBaseTypeOrInterface : Type 'System.ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>' does not implement interface 'System.IComparable<System.ValueTuple<T1, T2, T3, T4, T5, T6, T7, System.ValueTuple<TRest>>>' in the implementation but it does in the contract. [E:\GitHub\Fork\corefx\src\System.Runtime\src\System.Runtime.csproj]
C:\Users\ahkha\.nuget\packages\microsoft.dotnet.apicompat\1.0.0-beta.19369.2\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : MembersMustExist : Member 'System.ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>.CompareTo(System.ValueTuple<T1, T2, T3, T4, T5, T6, T7, System.ValueTuple<TRest>>)' does not exist in the implementation but it does exist in the contract. [E:\GitHub\Fork\corefx\src\System.Runtime\src\System.Runtime.csproj]
C:\Users\ahkha\.nuget\packages\microsoft.dotnet.apicompat\1.0.0-beta.19369.2\build\Microsoft.DotNet.ApiCompat.targets(82,5): error : MembersMustExist : Member 'System.ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>.Equals(System.ValueTuple<T1, T2, T3, T4, T5, T6, T7, System.ValueTuple<TRest>>)' does not exist in the implementation but it does exist in the contract. [E:\GitHub\Fork\corefx\src\System.Runtime\src\System.Runtime.csproj]
C:\Users\ahkha\.nuget\packages\microsoft.dotnet.apicompat\1.0.0-beta.19369.2\build\Microsoft.DotNet.ApiCompat.targets(96,5): error : ApiCompat failed for 'E:\GitHub\Fork\corefx\artifacts\bin\System.Runtime\netcoreapp-Windows_NT-Debug\System.Runtime.dll' [E:\GitHub\Fork\corefx\src\System.Runtime\src\System.Runtime.csproj]

Note: This is only a problem for ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>, where the number of generics is over 7, and we have TRest. For the other types such as ValueTuple<T1>, ValueTuple<T1, T2>, etc. using the ( , ) notation to represent ValueTuples works fine.

Do we need to special case that into a manual ref file?

cc @ericstj

ericstj commented 5 years ago

Caused by https://github.com/dotnet/arcade/pull/2029 @pakrym

I vaguely remember looking at this before and found that the signature used by corelib here will never be emitted by the C# compiler when using (, ...) syntax. It always wraps the 8th item in another ValueTuple. Do you remember looking into this @safern?

We could workaround it with a manual file, but I believe that ultimately this is a bug in https://github.com/dotnet/arcade/pull/2029, since that's not producing C# that compiles back to the same metadata.

safern commented 5 years ago

I haven’t special cased this in my changes yet, but I think we should be able to fix this by special casing it. I’ll take care of this as I’m already touching a lot of it now.