Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider binding historical new abstract methods as virtual #9557

Open
sbomer opened this issue Nov 25, 2024 · 7 comments
Open

Consider binding historical new abstract methods as virtual #9557

sbomer opened this issue Nov 25, 2024 · 7 comments
Assignees
Labels
Area: Bindings Issues in Java Library Binding projects.

Comments

@sbomer
Copy link
Member

sbomer commented Nov 25, 2024

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.

@jonathanpeppers @vitek-karas @jtschuster @agocke

@sbomer sbomer added the needs-triage Issues that need to be assigned. label Nov 25, 2024
@sbomer sbomer changed the title Consider bindin historical new abstract methods as virtual Consider binding historical new abstract methods as virtual Nov 25, 2024
@jpobst jpobst added Area: Bindings Issues in Java Library Binding projects. and removed needs-triage Issues that need to be assigned. labels Nov 26, 2024
@jpobst
Copy link
Contributor

jpobst commented Nov 26, 2024

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. 😁

@sbomer
Copy link
Member Author

sbomer commented Nov 26, 2024

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.

@jpobst
Copy link
Contributor

jpobst commented Nov 26, 2024

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.

@sbomer
Copy link
Member Author

sbomer commented Nov 26, 2024

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.)

@jpobst
Copy link
Contributor

jpobst commented Nov 26, 2024

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.

@jonathanpeppers
Copy link
Member

.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.

@jonpryor
Copy link
Member

jonpryor commented Dec 3, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Bindings Issues in Java Library Binding projects.
Projects
None yet
Development

No branches or pull requests

4 participants