Consider binding historical new abstract methods as virtual
sbomer opened this issue · 7 comments
Mono.Android.dll API Compatibility states that new abstract methods or non-default interface methods added to an existing type are bound to a C# implementation that throws AbstractMethodError, starting from $(TargetFrameworkVersion) v10.0.
As mentioned in History, in the past when new abstract methods were introduced, they were instead bound to abstract C# methods. This was a source breaking change, and binary compat was preserved by a linker step that injected implemented implementations of the abstract methods.
The proposal is to go back and apply the new policy to all historically introduced new abstract methods and non-default interface methods (on types that existed in some previous version). These should instead be bound as virtual, falling back to a throwing implementation. I believe this is supported by the generator via compatVirtualMethod. This would allow us to remove FixAbstractMethodsStep in .NET 10.
Some more context: this came up in dotnet/runtime#103987 (comment) where we discussed the idea of keeping such methods abstract in the ref assembly, and virtual in the implementation, to preserve the compiler errors. But at the time I didn't realize there were already plans to bind them to virtual methods in 10.0+ - so it looks we're ok with binding new Java abstract methods in a way that's not a source-breaking change in .NET.
I think the primary issue with this solution is that it only applies to Mono.Android.dll and not the entire ecosystem of existing bindings on NuGet. Libraries other than android.jar can also add new abstract methods which could trigger this scenario.
I'll let @jonpryor elaborate or correct me if I'm wrong. 😁
That's unfortunate. Have we considered recommending the same approach (bind new abstracts in a way that's not a binary breaking change for .NET) to NuGet authors? This seems like a better long-term direction to me personally. If we did this we could at least begin deprecating FixAbstractMethodsStep and maybe remove it in a future release.
It's actually very hard to maintain API compatibility because our binding process only knows about the Java library version you are currently binding, eg:
<AndroidLibrary Include="androidx.core.2.0.jar" />In order to maintain compatibility the process would have to know what was in androidx.core.1.0.jar and how it was bound into AndroidX.Core.1.0.dll. Given that users often can't successfully complete the process today, adding a lot more complexity isn't likely.
In fact, we do not even do it for the hundreds of NuGet bindings we maintain. We only maintain strict compatibility for Mono.Android.dll.
Thanks, I can see why that would be difficult. I assume for Mono.Android.dll we accomplish this by manually inspecting the new APIs - or do we have tooling that compares APIs across versions? Have we considered binding all (not just newly introduced) abstract methods as virtual in the implementation, but keeping them abstract in the ref assembly?
(Just for context, I'm exploring options for removing or changing FixAbstractMethodsStep, because the current implementation is relying on invariants that are preventing evolution of ILLink's codebase without taking a perf penalty for maintaining those invariants.)
For Mono.Android.dll we use the following tools to alert us to breaking changes which we then manually fix:
Have we considered binding all (not just newly introduced) abstract methods as virtual in the implementation, but keeping them abstract in the ref assembly?
No, that is an interesting idea. I think having a Mono.Android.dll ref assembly is still a pretty new concept for us. (I don't actually know why we have it.) We do not currently use ref assemblies for any other binding library.
.NET workloads use framework references, so they suggest (require?) everything to have two types of packs "ref packs" and "runtime packs". So, we made reference assemblies for Mono.Android.dll, Java.Interop.dll, etc.
It does give flexibility, and I think the C# compiler can load smaller .dll files on disk? So, probably multiple reasons for it.
@jpobst is correct. I would not want to remove the "FixAbstractMethodsStep", ever, both because we still support using Xamarin.Android -era binding assemblies (those built before .NET 6 and API-32 / Android 12L), and because preserving API compatibility for the ecosystem writ-large is currently untenable.