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.

Sign up for GitHub

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

Jump to bottom

[Xamarin.Android.Build.Tasks] Add MSBuild support for DIM. #3533

Merged
merged 1 commit into from
Sep 6, 2019
Merged

[Xamarin.Android.Build.Tasks] Add MSBuild support for DIM. #3533

merged 1 commit into from
Sep 6, 2019

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Aug 21, 2019 •
edited

Adds MSBuild support for DIM to Android Bindings projects.

To enable, add the following to a binding project:

<LangVersion>preview</LangVersion>
<_EnableInterfaceMembers>True</_EnableInterfaceMembers>

@jpobst jpobst force-pushed the dim-2019 branch 2 times, most recently from 750e599 to e3e3b86 Compare August 26, 2019 21:07
@jonpryor
Copy link
Member

What's the benefit/rationale for adding a new tests/CodeGen-Binding/Xamarin.Android.DefaultInterfaceMethods-Lib project instead of adding these to tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests? Is it to make it easier to produce dim.jar for inclusion (as Base64) in BindingBuildTest.cs?

Even if it is to make it easier to produce dim.jar, the NUnit tests could still be added to tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests so that we "reuse" an existing on-device test fixture, e.g. placing the DimTest methods into tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs.

jonpryor
jonpryor reviewed Aug 27, 2019
View reviewed changes
tests/CodeGen-Binding/Xamarin.Android.DefaultInterfaceMethods-Tests/DimBindingTests.cs Outdated
}

[Test]
public void TestOverriddenDefaultInterfaceMethods ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should also be a [Test] method in which a C# class implements IDefaultMethodsInterface and both accepts the default implementation and overrides the methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, which revealed a new issue that required fixing. (dotnet/java-interop#486)

jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Aug 28, 2019
@jpobst @jonpryor
[Java.Interop, generator] Fix GetPeerMembers for interfaces (#486)
3fee05c 
Context: xamarin/xamarin-android#3533

Consider the following Java interface which contains a default
interface method:

	// Java
	package com.xamarin.android;

	public interface DefaultMethodsInterface
	{
	    default int foo () { return 0; }
	}

This is bound as:

	// C# Binding
	public partial interface IDefaultMethodsInterface : IJavaPeerable {
	    static readonly JniPeerMembers _members = new JniPeerMembers (
	            "java/DefaultMethodsInterface",
	            typeof (IDefaultMethodsInterface));

	    [Register ("foo", "()V", "…")]
	    virtual unsafe int Foo ()
	    {
	        return _members.InstanceMethods.InvokeVirtualInt32Method ("foo.()V", this, null);
	    }
	}

If we "implement" the interface from C# *without* implementing
`IDefaultMethodsInterface.Foo()`:

	// C#
	class ManagedOverrideDefault : Java.Lang.Object, IDefaultMethodsInterface
	{
	}

Then invoke the default interface method:

	var over = new ManagedOverrideDefault ();
	(over as IDefaultMethodsInterface).Foo ();

The attempt to invoke the default implementation of
`IDefaultMethodsInterface.Foo()` causes a crash:

	Java.Lang.NoSuchMethodError : no non-static method "Ljava/lang/Object;.foo()I"

The cause of the crash is as follows: `IDefaultMethodsInterface.Foo()`
calls `JniPeerMembers.JniInstanceMethods.InvokeVirtualInt32Method()`
tries to determine if the method is overridden, and if the method is
*not* overridden -- as is the case here -- then we hit the non-virtual
invocation path:

	// src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods_Invoke.cs
	var j = Members.GetPeerMembers (self);
	var n = j.InstanceMethods.GetMethodInfo (encodedMember);
	return JniEnvironment.InstanceMethods.CallNonvirtualIntMethod (self.PeerReference, j.JniPeerType.PeerReference, n, parameters);

`JniInstanceMethods.Members` will be the
`IDefaultMethodsInterface._members` field, and
`Members.GetPeerMembers()` would return `self.JniPeerMembers`.
Because this is a C# class which inherits `Java.Lang.Object`,
`Java.Lang.Object._members` is returned.  We then attempt to resolve
`foo()` from `java.lang.Object`, which *does not exist*, which is why
the `NoSuchMethodError` is thrown.

The fix is to "intercept" the `Members.GetPeerMembers()` invocation
so that `j` refers to `IDefaultMethodsInterface._members`, *not*
`Java.Lang.Object._members`.  This would allow `n` to refer to
`DefaultMethodsInterface.foo()`, which *does* exist, and *can* be
invoked via `JniEnvironment.InstanceMethods.CallNonvirtualIntMethod()`.

Add the following `JniPeerMembers` constructor overload:

	partial class JniPeerMembers {
	    public JniPeerMembers (string jniPeerTypeName, Type managedPeerType, bool isInterface);
	}

Update `JniPeerMembers.GetPeerMembers()` so that if `isInterface` is
true, we return `this` (the *interface*s `JniPeerMembers` instance).

Update `generator` so that `_members` construction within interfaces
sets the `isInterface` parameter to true, resulting in:

	partial interface IDefaultMethodsInterface {
	    static readonly JniPeerMembers _members = new JniPeerMembers (
	            "java/DefaultMethodsInterface",
	            typeof (IDefaultMethodsInterface),
	            isInterface: true);
	}

This allows `(over as IDefaultMethodsInterface).Foo()` to work
without an exception being thrown.
jonpryor pushed a commit that referenced this pull request Aug 29, 2019
@jpobst @jonpryor
Bump to xamarin/Java.Interop/master@3fee05c (#3557)
c80676b 
Changes: dotnet/java-interop@29f9707...3fee05c

Context: #3533

Fix `generator` output of Java default-interface-methods so that
*non*-overrides are dispatched properly to the Java method (and
presumably `base` method invocations as well?).
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Aug 29, 2019
@jpobst @jonpryor
[Java.Interop, generator] Fix GetPeerMembers for interfaces (#486)
f0c28a4 
Context: xamarin/xamarin-android#3533

Consider the following Java interface which contains a default
interface method:

	// Java
	package com.xamarin.android;

	public interface DefaultMethodsInterface
	{
	    default int foo () { return 0; }
	}

This is bound as:

	// C# Binding
	public partial interface IDefaultMethodsInterface : IJavaPeerable {
	    static readonly JniPeerMembers _members = new JniPeerMembers (
	            "java/DefaultMethodsInterface",
	            typeof (IDefaultMethodsInterface));

	    [Register ("foo", "()V", "…")]
	    virtual unsafe int Foo ()
	    {
	        return _members.InstanceMethods.InvokeVirtualInt32Method ("foo.()V", this, null);
	    }
	}

If we "implement" the interface from C# *without* implementing
`IDefaultMethodsInterface.Foo()`:

	// C#
	class ManagedOverrideDefault : Java.Lang.Object, IDefaultMethodsInterface
	{
	}

Then invoke the default interface method:

	var over = new ManagedOverrideDefault ();
	(over as IDefaultMethodsInterface).Foo ();

The attempt to invoke the default implementation of
`IDefaultMethodsInterface.Foo()` causes a crash:

	Java.Lang.NoSuchMethodError : no non-static method "Ljava/lang/Object;.foo()I"

The cause of the crash is as follows: `IDefaultMethodsInterface.Foo()`
calls `JniPeerMembers.JniInstanceMethods.InvokeVirtualInt32Method()`
tries to determine if the method is overridden, and if the method is
*not* overridden -- as is the case here -- then we hit the non-virtual
invocation path:

	// src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods_Invoke.cs
	var j = Members.GetPeerMembers (self);
	var n = j.InstanceMethods.GetMethodInfo (encodedMember);
	return JniEnvironment.InstanceMethods.CallNonvirtualIntMethod (self.PeerReference, j.JniPeerType.PeerReference, n, parameters);

`JniInstanceMethods.Members` will be the
`IDefaultMethodsInterface._members` field, and
`Members.GetPeerMembers()` would return `self.JniPeerMembers`.
Because this is a C# class which inherits `Java.Lang.Object`,
`Java.Lang.Object._members` is returned.  We then attempt to resolve
`foo()` from `java.lang.Object`, which *does not exist*, which is why
the `NoSuchMethodError` is thrown.

The fix is to "intercept" the `Members.GetPeerMembers()` invocation
so that `j` refers to `IDefaultMethodsInterface._members`, *not*
`Java.Lang.Object._members`.  This would allow `n` to refer to
`DefaultMethodsInterface.foo()`, which *does* exist, and *can* be
invoked via `JniEnvironment.InstanceMethods.CallNonvirtualIntMethod()`.

Add the following `JniPeerMembers` constructor overload:

	partial class JniPeerMembers {
	    public JniPeerMembers (string jniPeerTypeName, Type managedPeerType, bool isInterface);
	}

Update `JniPeerMembers.GetPeerMembers()` so that if `isInterface` is
true, we return `this` (the *interface*s `JniPeerMembers` instance).

Update `generator` so that `_members` construction within interfaces
sets the `isInterface` parameter to true, resulting in:

	partial interface IDefaultMethodsInterface {
	    static readonly JniPeerMembers _members = new JniPeerMembers (
	            "java/DefaultMethodsInterface",
	            typeof (IDefaultMethodsInterface),
	            isInterface: true);
	}

This allows `(over as IDefaultMethodsInterface).Foo()` to work
without an exception being thrown.
@jpobst
Copy link
Contributor Author

jpobst commented Aug 30, 2019

I'm not aware of any benefit to Xamarin.Android.DefaultInterfaceMethods-Lib. I merely ported it from #1939. I suspect it was needed at the time because it required a custom compiler with DIM support. Now that DIM is built into shipping compilers and runtimes that isn't needed. Moved tests to McwGen and JcwGen.

steveisok pushed a commit to dotnet/java-interop that referenced this pull request Aug 30, 2019
@jpobst
[Java.Interop, generator] Fix GetPeerMembers for interfaces (#486)
f462752 
Context: xamarin/xamarin-android#3533

Consider the following Java interface which contains a default
interface method:

	// Java
	package com.xamarin.android;

	public interface DefaultMethodsInterface
	{
	    default int foo () { return 0; }
	}

This is bound as:

	// C# Binding
	public partial interface IDefaultMethodsInterface : IJavaPeerable {
	    static readonly JniPeerMembers _members = new JniPeerMembers (
	            "java/DefaultMethodsInterface",
	            typeof (IDefaultMethodsInterface));

	    [Register ("foo", "()V", "…")]
	    virtual unsafe int Foo ()
	    {
	        return _members.InstanceMethods.InvokeVirtualInt32Method ("foo.()V", this, null);
	    }
	}

If we "implement" the interface from C# *without* implementing
`IDefaultMethodsInterface.Foo()`:

	// C#
	class ManagedOverrideDefault : Java.Lang.Object, IDefaultMethodsInterface
	{
	}

Then invoke the default interface method:

	var over = new ManagedOverrideDefault ();
	(over as IDefaultMethodsInterface).Foo ();

The attempt to invoke the default implementation of
`IDefaultMethodsInterface.Foo()` causes a crash:

	Java.Lang.NoSuchMethodError : no non-static method "Ljava/lang/Object;.foo()I"

The cause of the crash is as follows: `IDefaultMethodsInterface.Foo()`
calls `JniPeerMembers.JniInstanceMethods.InvokeVirtualInt32Method()`
tries to determine if the method is overridden, and if the method is
*not* overridden -- as is the case here -- then we hit the non-virtual
invocation path:

	// src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods_Invoke.cs
	var j = Members.GetPeerMembers (self);
	var n = j.InstanceMethods.GetMethodInfo (encodedMember);
	return JniEnvironment.InstanceMethods.CallNonvirtualIntMethod (self.PeerReference, j.JniPeerType.PeerReference, n, parameters);

`JniInstanceMethods.Members` will be the
`IDefaultMethodsInterface._members` field, and
`Members.GetPeerMembers()` would return `self.JniPeerMembers`.
Because this is a C# class which inherits `Java.Lang.Object`,
`Java.Lang.Object._members` is returned.  We then attempt to resolve
`foo()` from `java.lang.Object`, which *does not exist*, which is why
the `NoSuchMethodError` is thrown.

The fix is to "intercept" the `Members.GetPeerMembers()` invocation
so that `j` refers to `IDefaultMethodsInterface._members`, *not*
`Java.Lang.Object._members`.  This would allow `n` to refer to
`DefaultMethodsInterface.foo()`, which *does* exist, and *can* be
invoked via `JniEnvironment.InstanceMethods.CallNonvirtualIntMethod()`.

Add the following `JniPeerMembers` constructor overload:

	partial class JniPeerMembers {
	    public JniPeerMembers (string jniPeerTypeName, Type managedPeerType, bool isInterface);
	}

Update `JniPeerMembers.GetPeerMembers()` so that if `isInterface` is
true, we return `this` (the *interface*s `JniPeerMembers` instance).

Update `generator` so that `_members` construction within interfaces
sets the `isInterface` parameter to true, resulting in:

	partial interface IDefaultMethodsInterface {
	    static readonly JniPeerMembers _members = new JniPeerMembers (
	            "java/DefaultMethodsInterface",
	            typeof (IDefaultMethodsInterface),
	            isInterface: true);
	}

This allows `(over as IDefaultMethodsInterface).Foo()` to work
without an exception being thrown.
@jpobst jpobst force-pushed the dim-2019 branch 3 times, most recently from 2736e5b to d76b104 Compare September 4, 2019 15:02
@jpobst jpobst marked this pull request as ready for review September 5, 2019 21:31
jonathanpeppers
jonathanpeppers reviewed Sep 6, 2019
View reviewed changes
src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets Outdated
@@ -494,6 +494,8 @@ Copyright (C) 2012 Xamarin Inc. All rights reserved.
ToolPath="$(MonoAndroidToolsDirectory)"
ToolExe="$(BindingsGeneratorToolExe)"
UseShortFileNames="$(UseShortGeneratorFileNames)"
LangVersion="$(LangVersion)"
EnableInterfaceMembersPreview="$(EnableInterfaceMembersPreview)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to document this new $(EnableInterfaceMembersPreview) property yet? https://github.com/xamarin/xamarin-android/blob/master/Documentation/guides/BuildProcess.md

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I forgot about documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpobst: please update this section: https://github.com/xamarin/xamarin-android/blob/master/Documentation/guides/BuildProcess.md#binding-project-build-properties

To document the new $(EnableInterfaceMembersPreview) property.

Additionally, convention is that public properties begin with Android.

The Preview suffix is...disturbing? What's the longevity for this property?

Should this instead be a $(_EnableInterfaceMembers) property? A _ prefix is "private"/"subject to change without notice" designator, so maybe that's what we should use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is to explicitly denote that this field is temporary and not something we are going to support for the rest of time. I am fine with a "private" property if that conveys the same idea. :)

@jonpryor
Copy link
Member

jonpryor commented Sep 6, 2019

Xamarin.Android.Bcl_Tests-NUnit crashed, but the associated logical doesn't really mention a "crash":

09-06 22:32:49.779  3242  5090 I NUnit   : Passed: 22173, Failed: 0, Skipped: 419, Inconclusive: 1, Total: 0, Filtered: 0
09-06 22:32:49.788  1954  1973 I ActivityManager: Force stopping Xamarin.Android.Bcl_Tests appid=10103 user=0: finished inst
09-06 22:32:49.789  1954  1973 I ActivityManager: Killing 3242:Xamarin.Android.Bcl_Tests/u0a103 (adj 0): stop Xamarin.Android.Bcl_Tests
...
09-06 22:32:49.881  1651  1651 I Zygote  : Process 3242 exited due to signal 9 (Killed)

It was force-stopped, for some reason, which was detected as a crash.

Ignoring.

@jonpryor jonpryor merged commit 37a75ad into master Sep 6, 2019
@jpobst jpobst deleted the dim-2019 branch September 7, 2019 14:19
jonpryor pushed a commit that referenced this pull request Sep 9, 2019
@jpobst @jonpryor
[Xamarin.Android.Build.Tasks] MSBuild support for C#8 DIM emission (#…
a6facb4 
…3533)

Context: dotnet/java-interop#25

Use [C#8 Default Interface Members][0] to improve bindings of Java 8
interface constructs, such as [default interface methods][1] and
static methods.

This allows the following Java type:

	// Java
	publc interface Example {
	    void requiredMethod();

	    default void optionalMethod() {
	    }

	    public static void staticMethod() {
	    }
	}

to be bound as:

	// C#
	public partial interface IExample : IJavaPeerable {
	    [Register("requiredMethod", "()V", …)]
	    void RequiredMethod ();
	    
	    [Register("optionalMethod", "()V", …)]
	    public void OptionalMethod()
	    {
		    /* … */
	    }
	    
	    [Register("staticMethod", "()V", …)]
	    public static void StaticMethod()
	    {
		    /* … */
	    }
	}

To enable use of C#8 default interface member generation, the
following two MSBuild properties must be set:

  * `$(LangVersion)`, which controls the C# compiler
    [language syntax version][2], must be set to `preview`.

  * `$(_EnableInterfaceMembers)` must be set to `True`.

For example:

	<PropertyGroup>
	  <LangVersion>preview</LangVersion>
	  <_EnableInterfaceMembers>True</_EnableInterfaceMembers>
	</PropertyGroup>

In the future, this will key entirely off of the `$(LangVersion)`
property and the `$(_EnableInterfaceMembers)` property will be removed.

[0]: https://github.com/dotnet/csharplang/blob/f7952cdddf85316a4beec493a0ecc14fcb3241c8/proposals/csharp-8.0/default-interface-methods.md
[1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
[2]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/langversion-compiler-option
@jonpryor jonpryor mentioned this pull request Sep 20, 2019
Closed
4 tasks
@jpobst jpobst mentioned this pull request Oct 1, 2019
Closed
mattleibow added a commit to xamarin/AndroidX that referenced this pull request Dec 10, 2019
@mattleibow
Enable default interface members
6a218d6 
xamarin/xamarin-android#3533
@mattleibow mattleibow mentioned this pull request Dec 10, 2019
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonpryor jonpryor jonpryor left review comments

@dellis1972 dellis1972 dellis1972 approved these changes

@jonathanpeppers jonathanpeppers jonathanpeppers left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jpobst @jonpryor @dellis1972 @jonathanpeppers