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

default interface methods support #1939

Closed
wants to merge 18 commits into from
Closed

Conversation

atsushieno
Copy link
Contributor

@atsushieno atsushieno commented Jul 9, 2018

  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.

Remaining tasks:

  • add support for DIMs in Xamarin.Android.Bindings.targets so that DIM bindings can be generated.
  • add apk tests that assures DIMs work as expected at run time.
  • add DIM-backed methods in Mono.Android.dll (for now the new ones are disabled; those methods that "became default" are generated as non-default)
  • (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.

@atsushieno atsushieno added do-not-merge PR should not be merged. Area: Bindings Issues in Java Library Binding projects. labels Jul 9, 2018
@atsushieno atsushieno removed the request for review from dellis1972 July 10, 2018 05:08
@atsushieno
Copy link
Contributor Author

(unassigning automatic request review for now, there will be a bunch of commits and comments.)

@atsushieno
Copy link
Contributor Author

So far I have a PoC code (written as part of tests/CodeGen-Binding) which has MainActivity.OnCreate() like this:

			JniPeerMembers _members = new JniPeerMembers ("com/xamarin/test/DefaultInterfaceMethods", typeof (IDefaultInterfaceMethods));
			string __id = "foo.()I";
			unsafe {
				var __rm = _members.InstanceMethods.InvokeNonvirtualInt32Method (__id, (IJavaPeerable)this, null);
				Console.WriteLine (__rm);
			}


			IDefaultInterfaceMethods c = new ImplementedClass ();
			Console.WriteLine (c.Foo ());

... which shows weird run time error, shown as the attached sshot:

screenshot from 2018-07-17 18-33-11

What's interesting is that the strongly-typed version (new ImplementedClass()) just passes without such an error.

full repro: Xamarin.Android.DefaultInterfaceMethods-Tests.zip

@atsushieno
Copy link
Contributor Author

Now here is an interesting test code - I1.Foo() does not fail but I2.Foo() fails, throwing the same AbstractMethodError as the previous comment. The only difference between I1 and I2 are that I2 is derived from Java.Lang.Object. Maybe libmonodroid injects something there?

using System;
using Android.App;
using Android.OS;
using Com.Xamarin.Test;

namespace Xamarin.Android.DefaultInterfaceMethodsTests
{
	[Activity (Label = "Xamarin.Android.DefaultInterfaceMethods-Tests", MainLauncher = true)]
	public class MainActivity : global::Android.App.Activity
	{
		protected override void OnCreate (Bundle bundle)
		{
			((I1)new X1 ()).Foo ();
			((I2)new X2 ()).Foo ();
		}
	}

	public interface I1
	{
		string Foo () => "foo";
	}

	public class X1 : I1
	{
	}

	public interface I2
	{
		string Foo () => "foo";
	}

	public class X2 : Java.Lang.Object, I2
	{
	}
}

@jonpryor
Copy link
Member

Maybe libmonodroid injects something there

Not quite; it's the linker: https://developer.xamarin.com/releases/android/xamarin.android_6/xamarin.android_6.1/#Java_Interface_Versioning

Starting with Xamarin.Android 6.1, all classes will be checked to ensure they fully implement their Java interfaces, and if they don’t then the missing members will be generated to throw an AbstractMethodError.

See also:

foreach (var iMethod in ifaceDef.Methods) {
bool exists = false;
foreach (var tMethod in typeMethods) {
if (HaveSameSignature (iface, iMethod, tMethod)) {
exists = true;
break;
}
}
if (!exists) {
AddNewExceptionMethod (type, iMethod);
rv = true;
}
}

We'll need to improve FixAbstractMethodsStep.FixAbstractMethods() so that it doesn't emit the "missing method" if the interface method contains a body, presumably by:

@@ -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;
                                                }

atsushieno added a commit to atsushieno/xamarin-android that referenced this pull request Jul 17, 2018
@atsushieno
Copy link
Contributor Author

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 IDefaultInterfaceMethods static initializer and therefore the error. So it is a progress.

@atsushieno
Copy link
Contributor Author

Now Mono.Android.dll builds. With a handful of ABI breakage https://gist.github.com/atsushieno/50affef19ff37597842583c27874d8e7

@atsushieno
Copy link
Contributor Author

All those ABI breakages are gone. It's done. With some csc-dim tooling fixes, it's ready for merging.

@atsushieno atsushieno changed the title [WIP] default interface methods support default interface methods support Jul 19, 2018
@atsushieno atsushieno removed the do-not-merge PR should not be merged. label Jul 19, 2018
atsushieno added a commit to atsushieno/xamarin-android that referenced this pull request Jul 20, 2018
atsushieno added a commit to atsushieno/xamarin-android that referenced this pull request Jul 23, 2018
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.
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.
jonpryor pushed a commit that referenced this pull request Aug 8, 2019
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
@jonpryor
Copy link
Member

Superseded by PR #3533.

@jonpryor jonpryor closed this Sep 20, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: Bindings Issues in Java Library Binding projects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants