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

[generator] Support default interface methods. #459

Merged
merged 10 commits into from
Aug 23, 2019
Merged

[generator] Support default interface methods. #459

merged 10 commits into from
Aug 23, 2019

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Aug 9, 2019

Code primarily from #341.

Adds binding support for Java default interface methods (which can become C# default interface methods or properties). Specifically, these Java cases are supported:

public interface MyInterface
{
    default String FindValue ()
    {
        return "MyInterface.GetValue ()";
    }

    default int getFoo ()
    {
        return 8;
    }

    default void setFoo (int newFoo)
    {
    }
}

This is enabled by passing the flag --lang-features=default-interface-methods.

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Aug 15, 2019
Changes: dotnet/java-interop@be58159...2abfc1e

Context: dotnet/java-interop#459

Updates `generator` so that all bound Java interfaces also implement
`IJavaPeerable` in addition to `IJavaObject`, for eventual future
C#8 Default Interface Member support.

[generator] Remove extraneous slash when creating .projitems.

[generator] Always use XAPeerMembers for XAJavaInterop1

Drop dependency on DylibMono when building for Xamarin.Android

[jnienv-gen] fix p/invoke usage for .NET framework

Add `jnimarshalmethod-gen.exe -r ASSEMBLY` option.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Aug 16, 2019
Changes: dotnet/java-interop@be58159...5fe28cd

Context: dotnet/java-interop#459

Updates `generator` so that all bound Java interfaces also implement
`IJavaPeerable` in addition to `IJavaObject`, for eventual future
C#8 Default Interface Member support.

[generator] Remove extraneous slash when creating .projitems.

[generator] Always use XAPeerMembers for XAJavaInterop1

Drop dependency on DylibMono when building for Xamarin.Android

[jnienv-gen] fix p/invoke usage for .NET framework

Add `jnimarshalmethod-gen.exe -r ASSEMBLY` option.

Improve support for binding package-private interfaces.

Parse EnclosingMethod, SourceFile annotation blobs.

Emit events for addListener(Listener,Handler) pattern

Fix `jnimarshalmethod-gen.exe`-related build error:

	Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	    at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)

Fix DylibMono build issues which prevented `src/monodroid` from
building.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Aug 16, 2019
Changes: dotnet/java-interop@be58159...39a3b87

Context: dotnet/java-interop#459

Updates `generator` so that all bound Java interfaces also implement
`IJavaPeerable` in addition to `IJavaObject`, for eventual future
C#8 Default Interface Member support.

[generator] Remove extraneous slash when creating .projitems.

[generator] Always use XAPeerMembers for XAJavaInterop1

Drop dependency on DylibMono when building for Xamarin.Android

[jnienv-gen] fix p/invoke usage for .NET framework

Add `jnimarshalmethod-gen.exe -r ASSEMBLY` option.

Improve support for binding package-private interfaces.

Parse EnclosingMethod, SourceFile annotation blobs.

Emit events for addListener(Listener,Handler) pattern

Fix `jnimarshalmethod-gen.exe`-related build error:

	Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	    at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)

Fix DylibMono build issues which prevented `src/monodroid` from
building.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Aug 18, 2019
Changes: dotnet/java-interop@be58159...7228af0

Context: dotnet/java-interop#459

Updates `generator` so that all bound Java interfaces also implement
`IJavaPeerable` in addition to `IJavaObject`, for eventual future
C#8 Default Interface Member support.

[generator] Remove extraneous slash when creating .projitems.

[generator] Always use XAPeerMembers for XAJavaInterop1

Drop dependency on DylibMono when building for Xamarin.Android

[jnienv-gen] fix p/invoke usage for .NET framework

Add `jnimarshalmethod-gen.exe -r ASSEMBLY` option.

Improve support for binding package-private interfaces.

Parse EnclosingMethod, SourceFile annotation blobs.

Emit events for addListener(Listener,Handler) pattern

Fix `jnimarshalmethod-gen.exe`-related build error:

	Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	    at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)

Fix DylibMono build issues which prevented `src/monodroid` from
building.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Aug 18, 2019
Changes: dotnet/java-interop@be58159...f7d97c2

Context: dotnet/java-interop#459

Updates `generator` so that all bound Java interfaces also implement
`IJavaPeerable` in addition to `IJavaObject`, for eventual future
C#8 Default Interface Member support.

[generator] Remove extraneous slash when creating .projitems.

[generator] Always use XAPeerMembers for XAJavaInterop1

Drop dependency on DylibMono when building for Xamarin.Android

[jnienv-gen] fix p/invoke usage for .NET framework

Add `jnimarshalmethod-gen.exe -r ASSEMBLY` option.

Improve support for binding package-private interfaces.

Parse EnclosingMethod, SourceFile annotation blobs.

Emit events for addListener(Listener,Handler) pattern

Fix `jnimarshalmethod-gen.exe`-related build error:

	Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	    at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)

Fix DylibMono build issues which prevented `src/monodroid` from
building.
@jpobst jpobst marked this pull request as ready for review August 19, 2019 15:28
jonpryor added a commit to dotnet/android that referenced this pull request Aug 19, 2019
Changes: dotnet/java-interop@be58159...60e85b0

Context: dotnet/java-interop#459

Updates `generator` so that all bound Java interfaces also implement
`IJavaPeerable` in addition to `IJavaObject`, for eventual future
C#8 Default Interface Member support.

[generator] Remove extraneous slash when creating `.projitems`.

[generator] Always use `XAPeerMembers` for `XAJavaInterop1`

Drop dependency on DylibMono when building for Xamarin.Android (#3223)

[jnienv-gen] fix p/invoke usage for .NET framework

Add `jnimarshalmethod-gen.exe -r ASSEMBLY` option.

Improve support for binding package-private interfaces.

Parse `EnclosingMethod`, `SourceFile` attribute blobs.

Emit events for `addListener(Listener,Handler)` pattern.

Fix `jnimarshalmethod-gen.exe`-related build error introduced by
having bound interfaces implement `IJavaPeerable`:

	Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
	Parameter name: propertyName
	    at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)
	    at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue)

Added `external/Java.Interop/build-tools/jnienv-gen.csproj` to
`Xamarin.Android.sln` so that it builds properly.
bool get_virt = prop.Getter.IsVirtual;
bool set_virt = prop.Setter == null ? false : prop.Setter.IsVirtual;
prop.Getter.IsVirtual = !isFinal && get_virt;
if (prop.Setter != null)
Copy link
Member

Choose a reason for hiding this comment

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

Question/test case: what should happen if the getter & setter differ in their "virtual-ness"?

In the case of Java default interface methods, this shouldn't be possible: https://stackoverflow.com/questions/23453287/why-is-final-not-allowed-in-java-8-interface-methods

However, if this method can be called for class methods, that certainly is possible...and has actually happened in Android, via Activity's getTitle() and setTitle() methods:

<class abstract="false" deprecated="not deprecated" extends="android.view.ContextThemeWrapper" extends-generic-aware="android.view.ContextThemeWrapper" jni-extends="Landroid/view/ContextThemeWrapper;" final="false" name="Activity" static="false" visibility="public" jni-signature="Landroid/app/Activity;">
  <method abstract="false" deprecated="not deprecated" final="true" name="getTitle" jni-signature="()Ljava/lang/CharSequence;" bridge="false" native="false" return="java.lang.CharSequence" jni-return="Ljava/lang/CharSequence;" static="false" synchronized="false" synthetic="false" visibility="public" />
  <method abstract="false" deprecated="not deprecated" final="false" name="setTitle" jni-signature="(Ljava/lang/CharSequence;)V" bridge="false" native="false" return="void" jni-return="V" static="false" synchronized="false" synthetic="false" visibility="public">
    <parameter name="title" type="java.lang.CharSequence" jni-type="Ljava/lang/CharSequence;" />
  </method>
</class>

(Above XML fragments from xamarin-android/bin/BuildDebug/api/api-28.xml.in.)

Which is to say, consider:

public class Activity {
    public final CharSequence getTitle() {return null;}
    public void setTitle(CharSequence value) {}
}

Currently -- arguably a bug! -- we remove the "virtual-ness", and emit:

partial class Activity {
  public unsafe Java.Lang.ICharSequence TitleFormatted {
    get { /*performs non-virtual dispatch */}
    set { /* performs virtual dispatch! */ }
  }
}

Which is admittedly "wonky", but... that's what we've done.

That doesn't mean we should continue this bizarro functionality for default interface methods. (Which, again, we arguably can't, because you can't have final interface methods.)

Rephrased: maybe perhaps we should reconsider this code idiom?

That said, in "reconsidering" we also need to maintain ABI compatibility with existing Mono.Android.dll, but there are various ways to work around that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was renamed from WriteClassProperties to WriteImplementedProperties and is now used by by both classes and interfaces, so no logic changes were made. Due to merge conflicts the old method was not removed. Added a commit that removes the old method.

I think if this is something we want to tackle we should do so in a separate issue. I'm not sure how it is solvable other than not converting the methods into a property, or generating the property as we do today (without virtual) and additionally adding a virtual SetTitle (CharSequence) method.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be a separate issue/PR.

As for a fix, i think the get methods should become a read-only property, and the set method should remain a separate method:

class Activity {
    public ICharSequence TitleFormatted {
      get {...}
    }
    public virtual void SetTitle (ICharSequence value) {...}
}

Which, again, would result in API breakage in Mono.Android.dll, but that can be "solved"/"worked around" via Metadata. The above is a saner default.

@@ -476,7 +499,7 @@ public void WriteInterface (InterfaceGen @interface, string indent, GenerationIn
// For each interface, generate either an abstract method or an explicit implementation method.
public void WriteInterfaceAbstractMembers (InterfaceGen @interface, ClassGen gen, string indent)
{
foreach (Method m in @interface.Methods.Where (m => !m.IsInterfaceDefaultMethod && !m.IsStatic)) {
foreach (var m in @interface.Methods.Where (m => (!opt.SupportDefaultInterfaceMethods || !m.IsInterfaceDefaultMethod) && !m.IsStatic)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the addition of !opt.SupportDefaultInterfaceMethods || here. Why would generator --lang-features=default-interface-methods alter what statement does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not seem to have any effect, removed.

@jonpryor
Copy link
Member

Suggested commit message:

Fixes: https://github.com/xamarin/java.interop/issues/25

Context: https://github.com/xamarin/java.interop/pull/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

@jonpryor jonpryor merged commit 29f9707 into master Aug 23, 2019
@jonpryor jonpryor deleted the dim-2019 branch August 23, 2019 19:17
@jpobst jpobst mentioned this pull request Aug 23, 2019
2 tasks
steveisok pushed a commit that referenced this pull request Aug 30, 2019
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
@brendanzagaeski
Copy link
Contributor

Release status update

A new Preview version of Xamarin.Android has now been published on Windows that includes the new capabilities from this item. See the release notes for additional details.

The new functionality is not yet available on macOS. I will update this item again when a Preview version with the new functionality is available on macOS.

Feature included in Xamarin.Android 10.0.99.100.

Feature included on Windows in Visual Studio 2019 version 16.4 Preview 1. To try the Preview version that includes the fix, check for the latest updates in Visual Studio Preview.

Feature not yet available on macOS.

@brendanzagaeski
Copy link
Contributor

Release status update

A new Release version of Xamarin.Android has now been published on Windows that includes the new capabilities related to this item. See the release notes for additional details.

The new functionality is not yet published in a Release version on macOS. I will update this item again when a Release version with the new functionality is available on macOS.

Feature included in Xamarin.Android 10.1.0.30.

Feature included on Windows in Visual Studio 2019 version 16.4. To get the new version that includes the feature, check for the latest updates or install the latest version from https://visualstudio.microsoft.com/downloads/.

(Feature also included on macOS in Visual Studio 2019 for Mac version 8.4 Preview 2.1 and higher. To try the Preview version that includes the feature, check for the latest updates on the Preview updater channel.)

@brendanzagaeski
Copy link
Contributor

Release status update

A new Release version of Xamarin.Android has now been published on macOS that includes the new capabilities related to this item. See the release notes for additional details.

Feature included in Xamarin.Android 10.1.1.0.

Feature included on macOS in Visual Studio 2019 for Mac version 8.4. To get the new version that includes the feature, check for the latest updates on the Stable updater channel.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants