-
Notifications
You must be signed in to change notification settings - Fork 538
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
default interface methods support #1939
Conversation
(unassigning automatic request review for now, there will be a bunch of commits and comments.) |
So far I have a PoC code (written as part of
... which shows weird run time error, shown as the attached sshot: What's interesting is that the strongly-typed version ( full repro: Xamarin.Android.DefaultInterfaceMethods-Tests.zip |
Now here is an interesting test code -
|
Not quite; it's the linker: https://developer.xamarin.com/releases/android/xamarin.android_6/xamarin.android_6.1/#Java_Interface_Versioning
We'll need to improve @@ -167,7 +167,7 @@ namespace MonoDroid.Tuner
bool exists = false;
foreach (var tMethod in typeMethods) {
- if (HaveSameSignature (iface, iMethod, tMethod)) {
+ if (HaveSameSignature (iface, iMethod, tMethod) && !iMethod.HasBody) {
exists = true;
break;
} |
After fixing the linker, I'm getting the same managedPeerType issue (mentioned at #1939 (comment)) at the binding class too. Before the fix, it was raising AbstractMethodError without going down to |
Now Mono.Android.dll builds. With a handful of ABI breakage https://gist.github.com/atsushieno/50affef19ff37597842583c27874d8e7 |
All those ABI breakages are gone. It's done. With some csc-dim tooling fixes, it's ready for merging. |
1) You have to manually provide CscToolPath MSBuild property, unless you install xamarin.android.csc.dim package. 2) You have to explicitly specify AndroidEnableDefaultInterfaceMethods MSBuild property as True. 3) You have to explicitly specify LangVersion MSBuild property as "latest". When C# 8.0 becomes stable, 1) and 3) will become unnecessary. Then we should make changes to 2) as default.
They fail so far, because generator is not generating expected code yet.
…and fix test names.
It results in a bunch of build failures due to lack of metadata fixup for any API glitch so far.
There are DIMs that results in managed events and args types, but those names conflict each other (onAnimationStart/onAnimationEnd overloads for each). Therefore the new methods had to be renamed.
They result in "nonexistent" `Spliterator.OfXyz` return values, and somehow `managedReturn` fixup doesn't work for them. Disable generating them, removal of them is harmless due to `BaseStream#spliterator()`.
Now it is practically doable with DIM support.
Since we already had some method overrides for NetworkChannel methods (that could have returned common base types by adjusting managedReturn), those return types could not be changed (because of ABI compatibility). Therefore added manual bindings to those interface members instead.
Context: f96fcf9 Context: https://github.com/jpobst/dimtest Context: #1939 As of commit f96fcf9, the linker will "fix abstract methods" in order to preserve binary compatibility. For example, if a type built for e.g. `$(TargetFrameworkVersion)`=v2.3 implements a Java side interface, and the assembly containing that type is included into an app with `$(TargetFrameworkVersion)`=v10.0 and the Java-side interface added members between v2.3 and v10, then those "added"/ "missing" interface members will be *inserted* into the type, with a implementation which throws `Java.Lang.AbstractMethodError`. Unfortunately, f96fcf9 assumes that *all* interface members are "abstract" members which need to be implemented (or fixed up). This is no longer true with [C#8 Default Interface Members][0], as interface methods can now contain bodies: namespace MyNamespace { public IMyInterface { void MyAbstractMethod (); public void MyDefaultMethod () { } } } Types which implement `IMyInterface` don't need to provide an implementation for `IMyInterface.MyDefaultMethod()`, which is *fine*: // What the developer wrote class MyExample : IMyInterface { public void MyAbstractMethod() {} // Does not provide MyDefaultMethod() } Unfortunately, `FixAbstractMethodsStep` didn't know about such methods (they didn't exist in 2017), so it would insert the "missing" abstract methods: // What the linker writes: class MyExample : IMyInterface { public void MyAbstractMethod () {} public void MyDefaultMethod () => throw new Java.Lang.AbstractMethodError (); } Additionally, `FixAbstractMethodsStep` was not constrained to operate on only Java-related types and interfaces. It fixes *everything*. This breaks a "pure C#" scenario in which Java isn't involved at all: IMyInterface e = new MyExample (); e.MyDefaultMethod (); The expectation is that this will invoke the `IMyInterface.MyDefaultMethod()` method. Instead, it throws an `AbstractMethodError`! Update `FixAbstractMethodsStep` so that it only processes *`abstract`* methods, not all interface methods. This will prevent it from emitting a `MyExample.MyDefaultMethod()` override and prevent the `AbstractMethodError` from being thrown. [0]: https://github.com/dotnet/csharplang/blob/f7952cdddf85316a4beec493a0ecc14fcb3241c8/proposals/csharp-8.0/default-interface-methods.md
Superseded by PR #3533. |
unless you install xamarin.android.csc.dim package.
MSBuild property as True.
When C# 8.0 becomes stable, 1) and 3) will become unnecessary.
Then we should make changes to 2) as default.
Remaining tasks:
(optional) update xamarin-android Xamarin.Android.Bindings.targets to customize csc tool task invocation, if no CscToolPath is provided.-> there is no csc-dim on Windows, so skip that.