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

jpobst · 2019-08-21T20:21:24Z

Adds MSBuild support for DIM to Android Bindings projects.

To enable, add the following to a binding project:

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

jonpryor · 2019-08-27T01:29:55Z

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 · 2019-08-27T01:31:32Z

tests/CodeGen-Binding/Xamarin.Android.DefaultInterfaceMethods-Tests/DimBindingTests.cs

+		}
+
+		[Test]
+		public void TestOverriddenDefaultInterfaceMethods ()


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

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

Context: dotnet/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.

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

Context: dotnet/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 · 2019-08-30T14:56:46Z

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.

Context: dotnet/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.

jonathanpeppers · 2019-09-06T13:18:47Z

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets

@@ -494,6 +494,8 @@ Copyright (C) 2012 Xamarin Inc. All rights reserved.
      ToolPath="$(MonoAndroidToolsDirectory)"
      ToolExe="$(BindingsGeneratorToolExe)"
      UseShortFileNames="$(UseShortGeneratorFileNames)"
+      LangVersion="$(LangVersion)"
+      EnableInterfaceMembersPreview="$(EnableInterfaceMembersPreview)"


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

Good catch, I forgot about documentation.

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

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 · 2019-09-06T23:21:10Z

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.

…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

dotnet/android#3533

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

jonpryor reviewed Aug 27, 2019

View reviewed changes

jpobst force-pushed the dim-2019 branch from e3e3b86 to 1cd7b4d Compare August 29, 2019 14:56

jpobst force-pushed the dim-2019 branch 3 times, most recently from 2736e5b to d76b104 Compare September 4, 2019 15:02

jpobst marked this pull request as ready for review September 5, 2019 21:31

jpobst requested review from dellis1972 and jonathanpeppers as code owners September 5, 2019 21:31

dellis1972 approved these changes Sep 6, 2019

View reviewed changes

jonathanpeppers reviewed Sep 6, 2019

View reviewed changes

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

44911e0

jpobst force-pushed the dim-2019 branch from d76b104 to 44911e0 Compare September 6, 2019 19:44

jonpryor merged commit 37a75ad into master Sep 6, 2019

jpobst deleted the dim-2019 branch September 7, 2019 14:19

jonpryor mentioned this pull request Sep 20, 2019

default interface methods support #1939

Closed

4 tasks

jpobst mentioned this pull request Oct 1, 2019

Support default interface methods #1883

Closed

mattleibow added a commit to dotnet/android-libraries that referenced this pull request Dec 10, 2019

Enable default interface members

6a218d6

dotnet/android#3533

mattleibow mentioned this pull request Dec 10, 2019

Enable default interface members dotnet/android-libraries#26

Closed

github-actions bot locked and limited conversation to collaborators Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jpobst commented Aug 21, 2019 •

edited

Loading

jonpryor commented Aug 27, 2019

jonpryor Aug 27, 2019

jpobst Aug 30, 2019

jpobst commented Aug 30, 2019

jonathanpeppers Sep 6, 2019 •

edited

Loading

dellis1972 Sep 6, 2019

jonpryor Sep 6, 2019

jpobst Sep 6, 2019

jonpryor commented Sep 6, 2019

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

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

Conversation

jpobst commented Aug 21, 2019 • edited Loading

jonpryor commented Aug 27, 2019

jonpryor Aug 27, 2019

Choose a reason for hiding this comment

jpobst Aug 30, 2019

Choose a reason for hiding this comment

jpobst commented Aug 30, 2019

jonathanpeppers Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

dellis1972 Sep 6, 2019

Choose a reason for hiding this comment

jonpryor Sep 6, 2019

Choose a reason for hiding this comment

jpobst Sep 6, 2019

Choose a reason for hiding this comment

jonpryor commented Sep 6, 2019

jpobst commented Aug 21, 2019 •

edited

Loading

jonathanpeppers Sep 6, 2019 •

edited

Loading