Skip to content

[J.I, generator] Fix GetPeerMembers for interfaces. #486

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
Merged
merged 1 commit into from
Aug 28, 2019
Merged

[J.I, generator] Fix GetPeerMembers for interfaces. #486

merged 1 commit into from
Aug 28, 2019

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Aug 27, 2019

The following DIM case currently fails:

Java:
package com.xamarin.android;

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

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

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

with:

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

This adds a parameter to the JniPeerMembers constructor denoting this type is an Interface rather than a Class, so that GetPeerMembers can correctly resolve the method.

@jpobst jpobst force-pushed the interface-peers branch 5 times, most recently from e988ee5 to b2b4cf5 Compare August 27, 2019 19:52
jonpryor
jonpryor reviewed Aug 27, 2019
View reviewed changes
jonpryor
jonpryor reviewed Aug 27, 2019
View reviewed changes
@jpobst
Copy link
Contributor Author

jpobst commented Aug 28, 2019

Updated to address review feedback.

@jonpryor jonpryor merged commit 3fee05c into master Aug 28, 2019
@jonpryor jonpryor deleted the interface-peers branch August 28, 2019 17:53
jonpryor pushed a commit that referenced this pull request Aug 29, 2019
@jpobst @jonpryor
[Java.Interop, generator] Fix GetPeerMembers for interfaces (#486)
f0c28a4 
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 jpobst mentioned this pull request Aug 30, 2019
Merged
steveisok pushed a commit that referenced this pull request Aug 30, 2019
@jpobst
[Java.Interop, generator] Fix GetPeerMembers for interfaces (#486)
f462752 
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.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonpryor jonpryor jonpryor left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jpobst @jonpryor