java.interop icon indicating copy to clipboard operation
java.interop copied to clipboard

Make `IJavaPeerable` implementable

Open jonpryor opened this issue 10 years ago • 6 comments
trafficstars

https://twitter.com/sh4na/status/600662831017676800

If a user of your API complains of breakage and you reply “oh, you shouldn’t be doing that”, it’s a sign that your API is badly designed.

What's the problem with IJavaPeerable? It shouldn't be implemented by developers. Ever. (In theory it could be properly implemented. In practice, I wouldn't want to try to do so.)

We should nuke the IJavaPeerable interface from the public API.

Problem: JniPeerMethods and other types make use of IJavaObject, which would need to be fixed. Perhaps we could refactor things to remove the problematic members, e.g. IJavaPeerable.PeerReference?

jonpryor avatar May 19 '15 18:05 jonpryor

A closely related problem is Xamarin.Android's tools/generator, which generates binding assemblies, which would be using Java.Interop.JniPeerMembers. JniPeerMembers.JniInstanceMethods, meanwhile, uses IJavaPeerable and IJavaPeerable.PeerReference extensively; how can it possibly work without an IJavaPeerable type?

The only way I can think of to make it work without an IJavaPeerable type is to instead emit overloads for e.g. JniPeerMembers.JniInstanceMethods.InvokeAbstractVoidMethod() which take both a JavaObject and a JavaException...which ~immediately breaks the current Xamarin.Android integration strategy because those types won't exist in the Cycle 7 Java.Interop.dll that Xamarin.Android 6.1 will be using.

So that's a bit of a non-starter.

jonpryor avatar Dec 31 '15 02:12 jonpryor

At present, I can't think of a way to satisfy the goal of having a sanely designed API (no interfaces that can't actually be implemented) which is also usable and fulfills known compatibility and integration points.

Keeping this open (for now) should a brain storm occur, but I don't see how it can happen.

jonpryor avatar Dec 31 '15 02:12 jonpryor

Related: https://bugzilla.xamarin.com/show_bug.cgi?id=37560

People see an interface, and try to manually implement, and shit breaks. Film at 11.

Even if the interface were implementable...they'd probably implement it wrong, because they're clearly already trying to implement the interface even when the documentation explicitly says not to.

jonpryor avatar Jan 14 '16 20:01 jonpryor

Closing, as this seems unlikely at this point.

jpobst avatar Apr 16 '20 19:04 jpobst

After discussion with @jpobst, we can probably:

  1. Provide default implementations for all members on IJavaPeerable, and
  2. Update jcw-gen to look for IJavaPeerable as an implemented interface, and generate Java Callable Wrappers when found.

These changes would allow this to compile and work as expected:

class MyRunnable : Java.Lang.IRunnable {
    public void Run() { /* … */ }
}

Doing this would require use of ConditionalWeakTable<TKey, TValue> to implement IJavaPeerable.PeerReference/etc., which might not be "ideal", but should be possible.

jonpryor avatar Mar 28 '22 19:03 jonpryor

After some further thought…

Can we implement IJavaPeerable via ConditionalWeakTable<TKey, TValue>? Kinda. We can implement the C# side, in a way that works:

var list = new Java.Util.ArrayList ();
lits.Add (new MyRunnable());
var r = (MyRunnable) list.Get (0);

The problem is, this overlooks the GC bridge:

  • https://github.com/xamarin/java.interop/blob/ed9c2abfd9db0638703ffee72068bd495db3ce51/src/Java.Runtime.Environment/Java.Interop/MonoRuntimeValueManager.cs#L41-L44
  • https://github.com/xamarin/java.interop/blob/ed9c2abfd9db0638703ffee72068bd495db3ce51/src/java-interop/java-interop-gc-bridge-mono.cc#L353-L379

On Mono, the GC bridge currently requires that the type provide 4 fields (handle, handle_type, refs_added, weak_handle), and as an interface cannot provide instance fields, this is a problem:

var list = new Java.Util.ArrayList ();
lits.Add (new MyRunnable());

/* Do Something to cause the `MyRunnable` to be collected; GC.Collect(), etc. */

var r = (MyRunnable) list.Get (0);
// throws a `NotSupportedException` as there isn't an "activation constructor"
// for `MyRunnable`

This can be "solved" by not using the existing GC bridge, a'la https://github.com/jonpryor/Java.Interop/commits/jonp-registration-scope , or by otherwise rethinking how cross-VM object references work.

jonpryor avatar Mar 29 '22 14:03 jonpryor