support default interface methods #341
Conversation
atsushieno
changed the title
|
So, currently generator is implemented to generate some DIM-bound code that does NOT compile. Let's see why. Taking this interfaces: The generator in this branch generates C# code like this (partially): csc reports this as: Note that it simply generates code in the same way as ClassGen generates code for class (plus, I have made changes to InterfaceGen so that What is this
The is no documentation for that class! Let's google this:
It leads to #17 , which says IJavaPeerable should not be implemented(!) What should DIMs do here then? |
|
So far I'm going to add cast to IJavaPeerable (which should be almost all the case, right?). And simply making that code generation change resulted in unit tests failing ALL RED :| |
|
Looking at the previous comment, this suggests that for default interface methods we should start having interfaces implement public partial interface ITheInterface : IJavaPeerable, IJavaObject {
}A problem with this idea is that we'll want to do this with everything in However, I don't think this is an actual problem because everybody should be inheriting from Then we hit the next problem: the body of the property: public partial interface ITheInterface : IJavaPeerable, IJavaObject {
virtual unsafe int Bar {
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/interface[@name='TheInterface']/method[@name='getBar' and count(parameter)=0]"
[Register ("getBar", "()I", "GetGetBarHandler")]
get {
const string __id = "getBar.()I";
try {
var __rm = _members.InstanceMethods.InvokeVirtualInt32Method (__id, this, null);
return __rm;
} finally {
}
}
}
}
This could be short-term worked around by instead using public partial interface ITheInterface : IJavaPeerable, IJavaObject {
virtual unsafe int Bar {
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/interface[@name='TheInterface']/method[@name='getBar' and count(parameter)=0]"
[Register ("getBar", "()I", "GetGetBarHandler")]
get {
const string __id = "getBar.()I";
try {
var __rm = JniPeerMembers.InstanceMethods.InvokeVirtualInt32Method (__id, this, null);
return __rm;
} finally {
}
}
}
}The above should compile. What I'm less certain about is the runtime effects; I'm fairly sure that this will not work right. Normal generation has namespace Android.App {
public partial class Activity {
internal new static readonly JniPeerMembers _members = new XAPeerMembers ("android/app/Activity", typeof (Activity));
}
}
In particular/in pseudo-code, we have: if (Members.UsesVirtualDispatch (self, declaringType)) {
// This performs *virtual method dispatch*
var m = GetMethodInfo (encodedMember);
return JniEnvironment.InstanceMethods.CallIntMethod (self.PeerReference, m, parameters);
}
// Perform *non-virtual* method dispatch
...By using namespace Java.Util {
public interface IList : IJavaPeerable {
[Register (...)]
void ReplaceAll (Java.Util.Function.IUnaryOperator operator) {
this.JniPeerMembers.InstanceMethods.InvokeVirtualVoidMethod (...);
}
}
}Then if a user has types: class MyList : Java.Lang.Object, Java.Util.IList {
// Doesn't implement ReplaceAll()
}and the user later does: Java.Util.IList list = new MyList();
list.ReplaceAll (...);what will happen is that we'll hit the body of Things go "bang" shortly afterward. Thus, what I think we'll need to do is: internal static class TheInterfaceDefaultInterfacePeerMembers {
public static JniPeerMembers _members = new XAPeerMembers ("xamarin/test/TheInterface", typeof (TheInterface));
}
public partial interface ITheInterface : IJavaPeerable, IJavaObject {
virtual unsafe int Bar {
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/interface[@name='TheInterface']/method[@name='getBar' and count(parameter)=0]"
[Register ("getBar", "()I", "GetGetBarHandler")]
get {
const string __id = "getBar.()I";
try {
var __rm = TheInterfaceDefaultInterfacePeerMembers._members.InstanceMethods.InvokeVirtualInt32Method (__id, this, null);
return __rm;
} finally {
}
}
}
} |
|
Possible alteration on the solution in the previous comment: if C#8 supports static fields in interfaces, we can instead do: public partial interface ITheInterface : IJavaPeerable, IJavaObject {
static JniPeerMembers _members = new XAPeerMembers ("xamarin/test/TheInterface", typeof (TheInterface));
virtual unsafe int Bar {
// Metadata.xml XPath method reference: path="/api/package[@name='xamarin.test']/interface[@name='TheInterface']/method[@name='getBar' and count(parameter)=0]"
[Register ("getBar", "()I", "GetGetBarHandler")]
get {
const string __id = "getBar.()I";
try {
var __rm = _members.InstanceMethods.InvokeVirtualInt32Method (__id, this, null);
return __rm;
} finally {
}
}
}
} |
BTW, every method in IJavaPeerable can become a default interface method :-) |
I'm not sure how |
|
So far those "how to implement IJavaPeerable" can be ignored. Any instance of Java interface callable wrapper must be IJavaObject and IJavaPeerable. |
|
It turned out that we don't have any runnable Java.Lang.Object and therefore current approach to simply add run verification on generator-Tests is unlikely successful / useful. A set of binding project builder (msbuild-alike) and executor would be useful here. |
context: dotnet#341 (comment) So far, it runs javac and jar to create input jars.
|
It turned out that getting a "runnable" unit test within the new binding-integration-Tests (or with any of the existing tests) is not really easy as we guessed. Here is what I wrote in the latest commit |
|
After grepping around java.interop source code, I'm concluding that there is no existing working code for desktop interop, as the actual Java.Lang.Object implementation seems to be inside xamarin-android. Therefore it is impossible to test run time behavior here. If run time tests are really important, there should have already been such working Java.Lang.Object code in this repository, regardless of DIMs. So far it looks simply impossible. |
context: dotnet#341 (comment) So far, it runs javac and jar to create input jars.
|
This is the latest code that the generator generates: https://gist.github.com/atsushieno/eb37a3d13e4049f9f04a6e841769ba5d It looks about right[*1], yet AbstractMethodError is thrown:
[*1] according to https://gist.github.com/atsushieno/d8c00bd1910dc3a130f6b13a20278e26 |
|
Just made sure that JNI for DIMs worked as expected on Android too. https://gist.github.com/atsushieno/9f963cae6ac89b4b96ec9e0c1866b910 |
atsushieno
changed the title
context: dotnet#341 (comment) So far, it runs javac and jar to create input jars.
- Added simple test case that contains a DIM method and a DIM-based property. - CodeGenerator takes SupportDefaultInterfaceMethods property. - command line option --default-interface-methods is added. - DIMs in interface generates implementation, not declaration. - DIM-based properties in interface generates implementation, not declaration.
The idea is in the same mindset as existing Windows Roslyn support in terms of access to package contents (dll/exe). I couldn't enable this for ALL projects as java.lang.Object test somehow failed. We only need it only if DIM is required, so limit the usage to it.
CodeGenerator.WriteMethodBody() now takes CodeGen so that it can detect InterfaceGen and if it is for interface then it adds missing casts. It is also possible to simply add cast everywhere which makes code generation simple, but it will involve ALL the unit tests' expected output rewritten. It will be annoying work especially the branch development is going to take a while until merged, so I skip that for now.
…class.
It was generating code like this:
```
public static partial class IAdapterExtensions {
public static string[] GetAutofillOptions (this Android.Widget.IAdapter self, )
```
DIMs were not correctly marked as expected - they should be marking only implementors of DIMs. What it was trying instead was just marking anything that matches the signature, in the interfaces. First this change expands the target of marking to class methods, therefore anything that implements DIMs are covered. Then limited the marking targets to only DIMs, not all the iface methods. (The change result for expanding them to classes was disasterous because of this problem.)
For an DIM that overrides another method from its implementing interface, it must be declared explicitly overriding the base interface method (which can be either DIM or non-DIM). To achieve that, we need to store the name of the declaring type of the base method. Also, we cannot "override" static methods, so skip checking that.
java.util.Set#spliterator() overrides java.util.Collection#spliterator(),
which in turn overrides java.util.Iterator#spliterator().
What should ISet.Spliterator() be like? It used to be:
`Java.Util.ISpliterator Java.Util.ICollection.Spliterator() { ... }`
It is wrong. ICollection declares Spliterator() like:
`Java.Util.ISpliterator Java.Util.IIterable.Spliterator() { ... }`
... so, ISet also has to declare it like this. That is, we have to
track all the override chain up to non-overriding declaration.
Unlike class methods that "implement" interface methods, interface methods that "override" their implementing interface methods need to be "explicitly" implemented. java.util.PrimitiveIterator.OfDouble#next() is a DIM and implements java.util.Iterator#next(), which then need to be generated as an explicit implementation of Iterator#next(), otherwise its Implementor will not have Iterator#next() implementation (the one from PrimitiveIterator.OfDouble is regarded as an irrelevant method).
Marking everything that overrides any base method AND is declared from an interface was too much. AsynchronousFileChannel#close() was regarded as a DIM and therefore its Invoker did not get any Close() method generated, resulting in missing IAsynchronousChannel.Close() implementation.
java.util.concurrent.CopyOnWriteArraySet#removeIf() was not marked as overriding Collection#removeIf() because we didn't check the interfaces of all the base types, but only checking the declaring type's. By iterating all those relevant interfaces, now it is correctly marked as an override.
The previous change introduced a stupid regression in method lookup loop (which will "break" in the middle, when iterating types for each method it's fine, but when iterating methods for each type it's not.)
… Invokers. There were cases that an abstract class generated abstract methods and properties for DIMs, which later caused missing implemented members in their Invokers. In Spliterators.AbstractSpliterator, `Comparator` and `ExactSizeIfKnown` properties were regarded as missing in the invoker.
There were handful of ABI compatibility regressions that DIM overrides are marked as final. They are intended to be kept virtual, so explicitly mark them as virtual. Although they should be virtual (DIM specification-wise).
We were marking interface methods too much as non-public. Static methods in the interfaces are generated in C# classes and they should remain public!
|
Any idea when this is going to land? More and more java libraries use Java 8 features, and making bindings for them is really hard without these changes, as also requested in #25 |
|
Hey guys, Any chance that we will have that fixed to the 2019 official release on April. I try to use the VS 2019 RC, but the WEBRTC framework crashes with the error below. It compiles, something that the 2017 was not able to. But there is a static class, that doesn't work no matter what. 03-06 10:29:46.712 I/MonoDroid(14650): UNHANDLED EXCEPTION: |
|
Any update on default interface methods? |
Context: #341 Context: #341 (comment) Context: #25 Java 8 introduced support for [interface default methods][0]: // Java public interface Example { int defaultInterfaceMethod() { return 42; } } The question is, how should we bind them? Currently, we *don't* bind interface default methods: // C# public interface IExample : IJavaObject { // No `DefaultInterfaceMethod()` method } This means that C# types which implement `IExample` don't need to implement `Example.defaultInterfaceMethod()`, reducing the work that needs to be done: // C# public class MyExample : Java.Lang.Object, IExample { // No need to implement Example.defaultInterfaceMethod()! } However, if a C# type *does* wish to implement `Example.defaultInterfaceMethod()`, they *can* do so by using [`ExportAttribute`][1], but this can be painful, as it requires ensuring that the Java side and C# sides are consistent, without compiler assistance: // C# partial class MyExample : Java.Lang.Object, IExample { [Java.Interop.Export ("defaultInterfaceMethod")] // If the above string is wrong, e.g. via typo, the method isn't overridden! public int DefaultInterfaceMethod() { return 42*2; } } We want to improve support for this scenario by binding Java interface default methods as [C#8 Default Interface Members][2], as C#8 Default Interface Methods have similar semantics as Java 8 interface default methods, and means that `[ExportAttribute]` would no longer be necessary to override them: // C#8? partial class MyExample : Java.Lang.Object, IExample { // Just Works™, and the compiler will complain if there are typos. public override int DefaultInterfaceMethod() { return 42*2; } } However, a question arises: how do we *do* that? // C#8 public interface IExample : IJavaObject { [Register ("defaultInterfaceMethod", "()I", ...)] int DefaultInterfaceMethod() { // How do we call `Example.defaultInterfaceMethod()` here? } } The desirable thing would be for `IExample.DefaultInterfaceMethod()` to have the same implementation as any other method binding, e.g. when using `generator --codegen-target=XAJavaInterop1`: // C#8 partial interface IExample { [Register ("defaultInterfaceMethod", "()I", ...)] void DefaultInterfaceMethod() { const string __id = "defaultInterfaceMethod.()I"; return _members.InstanceMethods.InvokeVirtualInt32Method (__id, this, null); } } The problem is twofold: 1. There is no `_members` field to access here, and 2. Even if there were a `_members` field, `JniPeerMembers.JniInstanceMethods.InvokeVirtualInt32Method()` requires an `IJavaPeerable` instance, and `IExample` doesn't implement `IJavaPeerable`. (1) is straight forwardly solvable, as C#8 Default Interface Members also adds support for static members of all sorts, so it should be possible to add a static `_members` field, if deemed necessary. (2) is the holdup, and has a simple solution: "Just" have every bound interface *also* implement `IJavaPeerable`! - public partial interface IExample : IJavaObject + public partial interface IExample : IJavaObject, IJavaPeerable "But!", someone cries, "What about API compatibility?!" Isn't adding `IJavaPeerable` to the implements list of an interface a Breaking Change? In theory and *most* practice, *Yes*, this *would* be a Breaking Change, and thus something to be avoided. *However*, Xamarin.Android is *not* "most" practice. It has been an XA4212 error since ab3c2b2 / Xamarin.Android 8.0 to implement an interface that implements `IJavaObject` *without* inheriting from `Java.Lang.Object` or `Java.Lang.Throwable`: // elicits XA4212 error class MyBadClass : IExample { // Implements IJavaObject.Handle public IntPtr Handle { get {return IntPtr.Zero;} } } Meanwhile, both `Java.Lang.Object` and `Java.Lang.Throwable` have implemented `IJavaPeerable` since Xamarin.Android *6.1*. *Because* it has been an error to manually implement `IJavaObject` for nearly two years now, it should be Reasonably Safe™ to update interfaces to implement *both* `IJavaObject` *and* `IJavaPeerable`. Doing so should not break any code -- unless they've overridden the `$(AndroidErrorOnCustomJavaObject)` MSBuild property to False, in which case they deserve what happens to them. (It's not really possible to implement `IJavaObject` in a sane manner, and the "straightforward" approach means that passing instances of e.g. `MyBadClass` to Java code will result in passing *`null`* to Java, which almost certainly is NOT what is intended, hence XA4212!) [0]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html [1]: https://docs.microsoft.com/en-us/xamarin/android/platform/java-integration/working-with-jni#exportattribute-and-exportfieldattribute [2]: https://github.com/dotnet/csharplang/blob/f7952cdddf85316a4beec493a0ecc14fcb3241c8/proposals/csharp-8.0/default-interface-methods.md
|
I am currently working on finishing up this work so that |
Context: #341 When generating the `_members` field when using `generator --codegen-target=XAJavaInterop1`, in one instance we emit: static JniPeerMembers _members = new XAPeerMembers ("android/text/foo", typeof (ISpannableInvoker)); while in the 3 other instances we generate: static JniPeerMembers _members = new JniPeerMembers ("android/text/foo", typeof (ISpannableInvoker)); Refactor all the instances of writing this line to a single method which correctly uses `XAPeerMembers` when needed.
Fixes: #25 Context: #341 Java 8 supports [interface default methods][0]: public interface HelloJava8 { public void a (); public default int getFoo () { return 8; } public default void setFoo (int newValue) { throw new UnsupportedOperationException(); } } With C#8, C# also supports [default interface members][1]. Add a new `generator --lang-features=default-interface-methods` flag which takes advantage of C#8 default interface members to bind Java 8 default interface methods: // C# binding of HelloJava8 public partial interface IHelloJava8 : IJavaObject, IJavaPeerable { static new readonly JniPeerMembers _members = new JniPeerMembers ("HelloJava8", typeof (IHelloJava8)); void A(); virtual unsafe int Foo { [Regsiter ("getFoo", "()I", "…")] get { return _members.InstanceMethods.InvokeVirtualInt32Method ("getFoo.()I", this, null); } [Regsiter ("setFoo", "(I)V", "…")] set { JniArgumentValue* __args = stackalloc JniArgumentValue [1]; __args [0] = new JniArgumentValue (value); return _members.InstanceMethods.InvokeVirtualVoidMethod ("setFoo.(I)V", this, __args); } } } C#8 Default Interface Members cannot be used with legacy `generator --codegen-target=XamarinAndroid`, as they require the `IJavaPeerable` infrastructure in order to work. Connector Methods are emitted within the interface binding, and not within the corresponding `*Invoker` type. If a Java default interface method is "invalid", we just skip binding the method instead of invalidating the entire interface, just as we do with classes and non-`abstract` methods. Finally, the default interface method implementation uses `virtual` dispatch, *not* non-`virtual` dispatch, in order to support Java-side versioning. For example, imagine `exampele.jar` v1: // Java public interface Fooable { default void foo() { System.out.println ("Fooable.foo"); } } public class Example implements Fooable { } In v1, `Example` does *not* contain an `Example.foo()` method, though `foo()` can be invoked on it, because of `Fooable.foo()`: Fooable value = new Example(); value.foo(); // Invokes Fooable.foo() In v2, `Example` overrides `Fooable.foo`: public class Example implements Fooable { public void foo() { System.out.println ("Example.foo"); } } If our binding used non-`virtual` dispatch for `IFooable.Foo()`, and bound `example.jar` v1, then if we updated `example.jar` to v2 *without* producing a new binding -- and why should a new binding be required? -- then we would continue invoking `Fooable.foo()` when we *should* be invoking `Example.foo()`. Use of `virtual` dispatch thus ensures we support Java-side versioning. [0]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html [1]: https://github.com/dotnet/csharplang/blob/f7952cdddf85316a4beec493a0ecc14fcb3241c8/proposals/csharp-8.0/default-interface-methods.md
|
The finalized code from this PR has been rebased and committed in PR #459. Closing out this PR. |
Fixes: #25 Context: #341 Java 8 supports [interface default methods][0]: public interface HelloJava8 { public void a (); public default int getFoo () { return 8; } public default void setFoo (int newValue) { throw new UnsupportedOperationException(); } } With C#8, C# also supports [default interface members][1]. Add a new `generator --lang-features=default-interface-methods` flag which takes advantage of C#8 default interface members to bind Java 8 default interface methods: // C# binding of HelloJava8 public partial interface IHelloJava8 : IJavaObject, IJavaPeerable { static new readonly JniPeerMembers _members = new JniPeerMembers ("HelloJava8", typeof (IHelloJava8)); void A(); virtual unsafe int Foo { [Regsiter ("getFoo", "()I", "…")] get { return _members.InstanceMethods.InvokeVirtualInt32Method ("getFoo.()I", this, null); } [Regsiter ("setFoo", "(I)V", "…")] set { JniArgumentValue* __args = stackalloc JniArgumentValue [1]; __args [0] = new JniArgumentValue (value); return _members.InstanceMethods.InvokeVirtualVoidMethod ("setFoo.(I)V", this, __args); } } } C#8 Default Interface Members cannot be used with legacy `generator --codegen-target=XamarinAndroid`, as they require the `IJavaPeerable` infrastructure in order to work. Connector Methods are emitted within the interface binding, and not within the corresponding `*Invoker` type. If a Java default interface method is "invalid", we just skip binding the method instead of invalidating the entire interface, just as we do with classes and non-`abstract` methods. Finally, the default interface method implementation uses `virtual` dispatch, *not* non-`virtual` dispatch, in order to support Java-side versioning. For example, imagine `exampele.jar` v1: // Java public interface Fooable { default void foo() { System.out.println ("Fooable.foo"); } } public class Example implements Fooable { } In v1, `Example` does *not* contain an `Example.foo()` method, though `foo()` can be invoked on it, because of `Fooable.foo()`: Fooable value = new Example(); value.foo(); // Invokes Fooable.foo() In v2, `Example` overrides `Fooable.foo`: public class Example implements Fooable { public void foo() { System.out.println ("Example.foo"); } } If our binding used non-`virtual` dispatch for `IFooable.Foo()`, and bound `example.jar` v1, then if we updated `example.jar` to v2 *without* producing a new binding -- and why should a new binding be required? -- then we would continue invoking `Fooable.foo()` when we *should* be invoking `Example.foo()`. Use of `virtual` dispatch thus ensures we support Java-side versioning. [0]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html [1]: https://github.com/dotnet/csharplang/blob/f7952cdddf85316a4beec493a0ecc14fcb3241c8/proposals/csharp-8.0/default-interface-methods.md
context: dotnet/android#1883
@monojenkins skip
TODOs:
See also dotnet/android#1939