[api-compat] Provide API diffs around API breakage

[jonpryor]

Context: #4356
Context: 54beb90
Context: a20be39

The use of Microsoft.DotNet.ApiCompat.exe added in 07e7477 has one
major deficiency:

The error messages reported by Microsoft.DotNet.ApiCompat.exe are
awful and borderline useless or misleading.

For example, consider commit PR #4356, which attempts to bring sanity
and consistency around $(AndroidPlatformId) and Mono.Android.dll
builds. It contains an API break, which we'll hand wave away and
accept for preview release purposes, in which the property type for
Android.Telephony.CellInfoGsm.CellIdentity changes from
CellIdentityGsm to CellIdentity:

// API-29
namespace Android.Telephony {
    public sealed partial class CellInfoGsm: Android.Telephony.CellInfo, Android.OS.IParcelable {
        public unsafe Android.Telephony.CellIdentityGsm CellIdentity {
    }
}

// API-R
namespace Android.Telephony {
    public sealed partial class CellInfoGsm : Android.Telephony.CellInfo, Android.OS.IParcelable {
        public unsafe Android.Telephony.CellIdentity CellIdentity {
    }
}

This is clearly a break. How does Microsoft.DotNet.ApiCompat.exe
report the breakage?

error : MembersMustExist : Member 'Android.Telephony.CellInfoGsm.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.

Which is infuriatingly terrible. The message implies that
Android.Telephony.CellInfoGsm.get_CellIdentity() doesn't exist, but
it does exist. The problem is that the return type changed!

Or consider 54beb90, in which we now emit a slew of default interface
members within the Mono.Android.dll binding, which should be API
compatible. Microsoft.DotNet.ApiCompat.exe complains as well:

InterfacesShouldHaveSameMembers : Interface member 'Java.Util.Functions.IUnaryOperator.Identity()' is present in the implementation but not in the contract.

What these messages have in common is that they provide no context,
lack important types, and in no way suggest how to fix the error
other than to just ignore it.

Overhaul this infrastructure so that crucial context is provided.

The context is provided by using "reference assembly source":
the Microsoft.DotNet.GenAPI.exe utility can be run on an
assembly to generate C# source code that shows the same API but no
implementation:

namespace Android.Accounts
{
    [Android.Runtime.RegisterAttribute("android/accounts/AbstractAccountAuthenticator", DoNotGenerateAcw=true, ApiSince=5)]
    public abstract partial class AbstractAccountAuthenticator : Java.Lang.Object
    {
        [Android.Runtime.RegisterAttribute("KEY_CUSTOM_TOKEN_EXPIRY", ApiSince=23)]
        public const string KeyCustomTokenExpiry = "android.accounts.expiry";
        [Android.Runtime.RegisterAttribute(".ctor", "(Landroid/content/Context;)V", "")]
        public AbstractAccountAuthenticator(Android.Content.Context context) { }

Update the src/Mono.Android build so that after every build, after
running Microsoft.DotNet.ApiCompat.exe we also run
Microsoft.DotNet.GenAPI.exe on the generated assembly, then run
diff -u against the recently created assembly and the reference
assembly source for the contract assembly. This allows us to get
useful diffs in the API:

Task "Exec" (TaskId:570)
  Task Parameter:Command=diff -u "../../tests/api-compatibility/reference/Mono.Android.dll.cs" "/Volumes/Xamarin-Work/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0/Mono.Android.dll.cs" (TaskId:570)
  diff -u "../../tests/api-compatibility/reference/Mono.Android.dll.cs" "/Volumes/Xamarin-Work/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0/Mono.Android.dll.cs" (TaskId:570)
  --- ../../tests/api-compatibility/reference/Mono.Android.dll.cs	2020-03-05 13:20:59.000000000 -0500 (TaskId:570)
  +++ /Volumes/Xamarin-Work/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0/Mono.Android.dll.cs	2020-03-05 13:40:12.000000000 -0500 (TaskId:570)
  @@ -27,7 +27,7 @@ (TaskId:570)
           { (TaskId:570)
               [Android.Runtime.RegisterAttribute("ACCEPT_HANDOVER", ApiSince=28)] (TaskId:570)
               public const string AcceptHandover = "android.permission.ACCEPT_HANDOVER"; (TaskId:570)
  -            [Android.Runtime.RegisterAttribute("ACCESS_BACKGROUND_LOCATION")] (TaskId:570)
  +            [Android.Runtime.RegisterAttribute("ACCESS_BACKGROUND_LOCATION", ApiSince=29)] (TaskId:570)

(The above is courtesy commmit 4cd2060, which added
RegisterAttribute.ApiSince on a large number of members.)

Finally, how do we update the "contract" Mono.Android.dll assembly?
Add a new tests/api-compatibility/api-compatibility.targets file
which contains a UpdateMonoAndroidContract target which will update
tests/api-compatibility/reference/Mono.Android.zip with the contents
of a cil-strip'd Mono.Android.dll and updated "reference assembly
source".

Mono.Android.zip contains a contract from Xamarin.Android 10.2.0.100
for $(TargetFrameworkVersion) v10.0.

Note: The diff -u invocation and UpdateMonoAndroidContract targets
currently only work on non-Windows platforms, due to the use of the
diff(1) and zip(1L) utilities.

[gugavaro]

We should consider only run codegen if apicompat reported issues. Perhaps even run codegen form inside the apicompat wrapper.

[jpobst]

I would prefer a solution that:

• Is just one step, not scanning the same assemblies several times.
• Works on any platform .Net runs on (especially Windows).

I don't know what that solution is, but some options are:

• Fixing ApiCompat to do what we want.
• Finding another solution, like https://github.com/dotnet/roslyn-analyzers#microsoftcodeanalysispublicapianalyzers.
• Maintaining our own managed code solution that does what we want.

[gugavaro]

I would prefer a solution that:

• Is just one step, not scanning the same assemblies several times.
• Works on any platform .Net runs on (especially Windows).

I don't know what that solution is, but some options are:

• Fixing ApiCompat to do what we want.
• Finding another solution, like https://github.com/dotnet/roslyn-analyzers#microsoftcodeanalysispublicapianalyzers.
• Maintaining our own managed code solution that does what we want.

I agree with @jpobst, we should fix the issues on ApiCompat, enhancing the msgs, etc, doing that everyone wins, us and the other teams using the tool as well.
Having now 2 tools doing the same thing seems not right. The team who owns Arcade is super cool and is willing to work with us and take our contributions.

[jonpryor]

The fundamental question is this: how long will it take to get in any changes to Microsoft.DotNet.ApiCompat.exe? Is this a matter of days, weeks, or months?

Meanwhile we could really use the additional "API diff" context now. Reviewing PR #4356 and PR #4353 and all future dotnet/java-interop#509 -related API changes to Mono.Android.dll is untenable, as simply getting the PR to build requires a giant wall of text added to tests/api-compatibility/acceptable-breakages*.txt.

Which is also why this PR uses diff -u and zip: it can be done now, with minimal additional effort.

Getting the zip parts working on Windows wouldn't be too hard. The diff part...I have no idea.

But at minimum it would vastly improve the usability of our current workflow.

[gugavaro]

The fundamental question is this: how long will it take to get in any changes to Microsoft.DotNet.ApiCompat.exe? Is this a matter of days, weeks, or months?

Meanwhile we could really use the additional "API diff" context now. Reviewing PR #4356 and PR #4353 and all future xamarin/java.interop#509 -related API changes to Mono.Android.dll is untenable, as simply getting the PR to build requires a giant wall of text added to tests/api-compatibility/acceptable-breakages*.txt.

Which is also why this PR uses diff -u and zip: it can be done now, with minimal additional effort.

Getting the zip parts working on Windows wouldn't be too hard. The diff part...I have no idea.

But at minimum it would vastly improve the usability of our current workflow.

We can get the changes in one day, my understand is that they have nightly builds that push the nuget package every night, but I will confirm that with them.
I submit my changes yesterday and they have reviewed and were going to merge, but then a guy from the compiler team mentioned that we cannot simple do my changes because other languages don't have DIM yet, even C# 7 consuming C#8 does not understand DIM so it would be a break.
I have already submit another proposal and waiting for their response.
Worse case scenario we can also fork it, but I don't think it is necessary.

[jpobst]

Another option might be https://www.nuget.org/packages/Mono.ApiTools, which is done by the Xamarin Components team.

[jpobst]

I think maybe we are conflating two issues:

1. There are bugs in ApiCompat that have led to wrong (DIM members) or unclear (changing return types) error messages.
2. ApiCompat is not an API differ, it only reports API breakages.

This PR seems to implement #2 (an API differ) as a temporary fix for #1, which is probably fine, but we should consider what our long-term goal is here.

Once ApiCompat is fixed, it will no longer show InterfacesShouldHaveSameMembers, but this doesn't really help us understand what is changing. We could accidentally be adding tons of new public members that we didn't intend.

Long-term, I think what we want is: a report generated that shows any changes to our public API (#2). And the process that generates this report raises an error if any of those changes are breaking.

Somehow we want to enforce not only that we don't introduce breaking changes, but that any change to our public API is intentional.

I think the Components team may have built tooling for these scenarios, since they faced the exact same issue.

[jonpryor]

PR updated so that it should now execute on Windows -- courtesy git diff --no-index -- and Microsoft.DotNet.GenAPI.exe is only executed when Microsoft.DotNet.ApiCompat.exe fails execution. This should reduce the verbosity in the build logs (as git diff shouldn't be run anymore unless an API break is triggered).

[jonpryor]

@jpobst mentioned:

Another option might be https://www.nuget.org/packages/Mono.ApiTools, which is done by the Xamarin Components team.

Looking briefly at that, it uses mono-api-info and mono-api-html, which we moved away from in 07e7477, because there were API breakage scenarios which that workflow didn't recognize, such as changes to custom attribute values.

Yes, we could have improved those tools to support checking custom attribute values, but we had wanted to "standardize" on tooling used in the rest of the dotnet ecosystem...not quite realizing how "problematic" that tooling would in turn be. (The grass is always greener on the other side of the fence, right? ;-)

[jonpryor]

@jpobst wrote:

I think maybe we are conflating two issues:

Conflating issues is fun? :-)

You're absolutely correct: the idea was to introduce API diffs so that we could better understand what happened when API breakage is reported.

I think that is of value now, and will be of increasing value as we continue working on API-30.

Kind of; see below.

Long-term, I think what we want is: a report generated that shows any changes to our public API (#2). And the process that generates this report raises an error if any of those changes are breaking.

What's "funny" is that "a report that shows changes to our public API" is what I was originally thinking of in Issue #1118, as that's what mono did, using Microsoft.DotNet.GenAPI.exe!

https://github.com/mono/mono/blob/e8b2b43f9bd0ae5a79ff2dafa97cf78365d0a639/mcs/Makefile#L158-L190

This is what the https://github.com/mono/api-snapshot repo is for.

The mono workflow, paraphrasing greatly, is that mono/api-snapshot contains "reference assembly source" for all assemblies they care about, as produced by Microsoft.DotNet.GetnAPI.exe. Whenever public API changes, regardless of whether or not it's "backward compatible", the mono build breaks. All changes are errors. In order to resolve the error, the API change must be manually approved, some @monojenkins command is issued, and the API change is merged into mono/api-snapshot and the submodule bumped.

Much of this seems a tad "heavy handed", particularly as we wanted to move away from our own xamarin/xamarin-android-api-compatibility repo (submodules are occasionally annoying).

...but there might be something to that.

---

Consider what will be happening around dotnet/java-interop#509: every time we add a new generator --lang-features value to modify the API-R API, we'll be changing the API.

Which we want, but we also want to see what those changes are. Currently, there's no easy way to see those changes at all. This PR is thus an improvement.

However, as currently architected, this PR will quickly get swamped, because the "reference" is from d16-5. We'll thus always see enormous API diffs (whenever Microsoft.DotNet.ApiCompat.exe fails), and those diffs will only get bigger.

To have saner API diffs, e.g. to see what just happens when generator --lang-features=nested-interface-types is added, we'd have to update our "reference" assembly + source on every API change, which sounds like what you're suggesting.

Meaning this PR, instead of using references from d16-5, should instead use references from current master.

The approach within this PR can support that, but this is a policy discussion that we need to agree on.

[jonpryor]

On a related tangent, consider PR #4356 and this build: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3529131&view=results

It failed at once point, and the fix were various removals to the acceptable-breakages-v10.0.99.txt file: https://github.com/xamarin/xamarin-android/pull/4356/files#diff-20b457057126cf5fc563b670934ae273L27

- InterfacesShouldHaveSameMembers : Interface member 'Android.Media.MediaCas.IEventListener.OnPluginStatusUpdate(Android.Media.MediaCas, System.Int32, System.Int32)' is present in the implementation but not in the contract.

But why did those things change? Particularly problematic when a local build didn't encounter those errors (though I didn't fully clean my build tree...).

Here again, an "API diff" perspective would be immensely helpful, especially when if the build fails, there's very little to examine: no uploaded assemblies, no source code...

[jonpryor]

But why did those things change? Particularly problematic when a local build didn't encounter those errors (though I didn't fully clean my build tree...).

...and this is where having a more "up-to-date" reference API hanging around would be handy.

A previous iteration of PR #4356 had a bug in which neither bin/BuildRelease/api/api-R.xml.in nor bin/BuildRelease/api/api-30.xml.in were produced. Consequently, v10.0.99 was ~identical to v10.0 -- except it had default interface members enabled -- so all the "new" methods in API-R were flagged as missing.

I had to nuke & rebuild my checkout to figure that out...

Having a "reference assembly" from d16-5 is somewhat handy -- no breakages compared to the released version! -- but it means it's somewhat easier to inadvertently break more recent changes.

Additionally, there's a Very Important Question about what the reference assembly should be. If it's for the stable v10.0, then how different will it be from d16-5?

Fortunately that's easy enough to do with Microsoft.DotNet.GenAPI.exe, and it's a 27552 line diff (?!), most of which is due to the addition of RegisterAttribute.ApiSince (4cd2060) and ObsoleteAttribue changes (dotnet/java-interop@8f30933a). Exclude those (badly?), and there's no real changes:

$ diff -u d16-5.cs 5ee304c0.cs | grep '^[+]' | grep -v ApiSince | grep -v ObsoleteAtt
+++ 5ee304c0.cs	2020-03-05 22:28:00.000000000 -0500
+        protected Java.Nio.Channels.FileChannel BaseFileChannel { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } }
+        [System.Diagnostics.DebuggerStepThroughAttribute]
+        [System.Diagnostics.DebuggerStepThroughAttribute]
+        [System.Diagnostics.DebuggerStepThroughAttribute]

No idea why [DebuggerStepThrough] was added...

---

We've been Very Good™ and haven't changed v10.0 API between d16-5 and now. Yay.

But this also means that, in the context of dotnet/java-interop#509 and API-R bindings, there's no "good" way to say "this set of changes is acceptable", because we (currently) don't have a "reference assembly" for the preview binding, only for the stable binding. Thus, even with API diffs, every API-R binding change will result in continually growing API diffs, and the current diff -- all 3MB of it/27KLOC! -- will only get larger.

[jpobst]

Consider what will be happening around dotnet/java-interop#509: every time we add a new generator --lang-features value to modify the API-R API, we'll be changing the API.

Which we want, but we also want to see what those changes are. Currently, there's no easy way to see those changes at all. This PR is thus an improvement.

I think this is where my disconnect is happening. The new changes will not be breaking changes (that's the whole point of what we're doing), so they will not show up in this PR.

But I do want to see what non-breaking changes my PR's are causing. To that end, I just ran mono-api-diff locally and added the result to #4363.

[gugavaro]

@jonpryor should the GenApi, run inside the ApiCompat task as a single task, I say that because perhaps we could do some work on better digest the diff result (pluses and minuses) and print back the content in a little less polluted way.