Separate out ABI from method bindings

[jonpryor]

Context: https://github.com/xamarin/xamarin-android/blob/6c6766de4db50fb43c42fccf442387a95f39086c/src/Mono.Android/Android.Widget/AbsListView.cs#L55-L64
Context: dotnet/android-libraries@5ce8b5f

Background:

Sometimes it is necessary to use partial classes to "coerce" generator output to be valid C# code. A not-entirely-uncommon use case around method overrides (related: Issue #681, fixed in 5a0e37e).

Consider dotnet/android-libraries@5ce8b5f: in it, NotificationCompat.Action.getRemoteInputs() was supposed to override a base class method, but -- for whatever reason -- generator couldn't properly determine that override was needed. As this was before 5a0e37e, the "workaround" was to rename the managedName of the Java method to _GetRemoteInputs(), then add a partial class declaration which could have the proper override logic.

While this works, it isn't obvious (hence #681). Additionally, such a solution should require two Metadata fixups: one to rename the method, and another to change the visibility of the method so that it isn't public:

	<attr path="/api/package[@name='android.support.v4.app']/class[@name='NotificationCompat.Action']/method[@name='getRemoteInputs' and count(parameter)=0]" name="managedName">_GetRemoteInputs</attr>
	<attr path="/api/package[@name='android.support.v4.app']/class[@name='NotificationCompat.Action']/method[@name='getRemoteInputs' and count(parameter)=0]" name="visibility">internal</attr>

(This wasn't done in the dotnet/android-libraries@5ce8b5f, and I don't understand why NotificationCompat.Action._GetRemoteInputs() isn't public…)

Another example is within Mono.Android.dll (and elsewhere), where it is too common to have generator emit output, copy that output into a new file, fixup that output, and then use Metadata to remove the original "bad" output. See e.g.

• https://github.com/xamarin/AndroidX/blob/b6d33b99255140f61700e98c1863afcdb7498a8b/source/androidx.appcompat/appcompat/Additions/Additions.cs
• https://github.com/xamarin/xamarin-android/blob/6c6766de4db50fb43c42fccf442387a95f39086c/src/Mono.Android/Android.Widget/AbsListView.cs#L55-L64

There should be no need for such hand-written JNI code, yet we have lots of it.

Proposal

I propose that we "refactor and cleanup" our bindings to separate out the code responsible for JNI invocations from the code responsible for "C# bindings".

Consider Android.App.Activity, where JNI code is interspersed with the C# API:

namespace Android.App {
	[global::Android.Runtime.Register ("android/app/Activity", DoNotGenerateAcw=true)]
	public partial class Activity {

	internal static readonly JniPeerMembers _members = new XAPeerMembers ("android/app/Activity", typeof (Activity));

		static Delegate? cb_createPendingResult_ILandroid_content_Intent_I;
#pragma warning disable 0169
		static Delegate GetCreatePendingResult_ILandroid_content_Intent_IHandler ()
		{
			if (cb_createPendingResult_ILandroid_content_Intent_I == null)
				cb_createPendingResult_ILandroid_content_Intent_I = JNINativeWrapper.CreateDelegate ((_JniMarshal_PPILI_L) n_CreatePendingResult_ILandroid_content_Intent_I);
			return cb_createPendingResult_ILandroid_content_Intent_I;
		}

		static IntPtr n_CreatePendingResult_ILandroid_content_Intent_I (IntPtr jnienv, IntPtr native__this, int requestCode, IntPtr native_data, int native_flags)
		{
			var __this = global::Java.Lang.Object.GetObject<Android.App.Activity> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
			var data = global::Java.Lang.Object.GetObject<Android.Content.Intent> (native_data, JniHandleOwnership.DoNotTransfer);
			var flags = (Android.App.PendingIntentFlags) native_flags;
			IntPtr __ret = JNIEnv.ToLocalJniHandle (__this.CreatePendingResult (requestCode, data!, flags!));
			return __ret;
		}
#pragma warning restore 0169

		// Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='Activity']/method[@name='createPendingResult' and count(parameter)=3 and parameter[1][@type='int'] and parameter[2][@type='android.content.Intent'] and parameter[3][@type='int']]"
		[Register ("createPendingResult", "(ILandroid/content/Intent;I)Landroid/app/PendingIntent;", "GetCreatePendingResult_ILandroid_content_Intent_IHandler")]
		public virtual unsafe Android.App.PendingIntent? CreatePendingResult (int requestCode, Android.Content.Intent data, [global::Android.Runtime.GeneratedEnum] Android.App.PendingIntentFlags flags)
		{
			const string __id = "createPendingResult.(ILandroid/content/Intent;I)Landroid/app/PendingIntent;";
			try {
				JniArgumentValue* __args = stackalloc JniArgumentValue [3];
				__args [0] = new JniArgumentValue (requestCode);
				__args [1] = new JniArgumentValue ((data == null) ? IntPtr.Zero : ((global::Java.Lang.Object) data).Handle);
				__args [2] = new JniArgumentValue ((int) flags);
				var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
				return global::Java.Lang.Object.GetObject<Android.App.PendingIntent> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
			} finally {
				global::System.GC.KeepAlive (data);
			}
		}
	}
}

I propose three changes:

1. Emit the "marshal methods" into a separate jnimarshalmethods.[Java-package-name].[Java-type] class.
2. Emit the "JNI" into a separate jniabi.[Java-package-name].[Java-type] class.
3. Implement C# "API descriptions" in terms of (1) and (2).

For example:

namespace Android.App {
	[global::Android.Runtime.Register ("android/app/Activity", DoNotGenerateAcw=true)]
	public partial class Activity {

		public override JniPeerMembers JniPeerMembers => jniabi.android.app.Activity._members;

		[Register ("createPendingResult", "(ILandroid/content/Intent;I)Landroid/app/PendingIntent;", "GetCreatePendingResult_ILandroid_content_Intent_IHandler")]
		public virtual unsafe Android.App.PendingIntent? CreatePendingResult (int requestCode, Android.Content.Intent data, [global::Android.Runtime.GeneratedEnum] Android.App.PendingIntentFlags flags)
		{
			var __rm = jniabi.android.app.Activity.createPendingResult (this, requestCode, data, (int) flags);
			return global::Java.Lang.Object.GetObject<Android.App.PendingIntent> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
		}

		// Ideally we wouldn't need this; would require updating `[Register]` to use the "interface-style" registration pattern w/ separate type.
		// May actually be *desirable* to "better bind" Java generics; see also:
		// https://discord.com/channels/732297728826277939/732297837953679412/906288298853556224
		static Delegate GetCreatePendingResult_ILandroid_content_Intent_IHandler () =>
			jnimarshalmethods.android.app.Activity.GetOnCreate_Landroid_os_Bundle_Handler ();
	}
}

namespace jniabi.android.app {
	partial static class Activity {
		internal static readonly JniPeerMembers _members = new XAPeerMembers ("android/app/Activity", typeof (Activity));

		public static JniObjectReference createPendingResult (IJavaPeerable self, int requestCode, IJavaPeerable data, int flags)
		{
			try {
				return createPendingResult (self, requestCode, data?.PeerReference ?? new JniPeerReference (), flags);
			}
			finally {
				GC.KeepAlive (self);
				GC.KeepAlive (data);
			}
		}

		public static JniObjectReference createPendingResult (IJavaPeerable self, int requestCode, JniObjectReference data, int flags)
		{
			JniArgumentValue* __args = stackalloc JniArgumentValue [3];
			__args [0] = new JniArgumentValue (requestCode);
			__args [1] = new JniArgumentValue ((data == null) ? IntPtr.Zero : ((global::Java.Lang.Object) data).Handle);
			__args [2] = new JniArgumentValue ((int) flags);
			var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
		}

	}
}

namespace jnimarshalmethods.android.app {

	partial static class Activity {

		static Delegate? cb_createPendingResult_ILandroid_content_Intent_I;
#pragma warning disable 0169
		static Delegate GetCreatePendingResult_ILandroid_content_Intent_IHandler ()
		{
			if (cb_createPendingResult_ILandroid_content_Intent_I == null)
				cb_createPendingResult_ILandroid_content_Intent_I = JNINativeWrapper.CreateDelegate ((_JniMarshal_PPILI_L) n_CreatePendingResult_ILandroid_content_Intent_I);
			return cb_createPendingResult_ILandroid_content_Intent_I;
		}

		static IntPtr n_CreatePendingResult_ILandroid_content_Intent_I (IntPtr jnienv, IntPtr native__this, int requestCode, IntPtr native_data, int native_flags)
		{
			var __this = global::Java.Lang.Object.GetObject<Android.App.Activity> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
			var data = global::Java.Lang.Object.GetObject<Android.Content.Intent> (native_data, JniHandleOwnership.DoNotTransfer);
			var flags = (Android.App.PendingIntentFlags) native_flags;
			IntPtr __ret = JNIEnv.ToLocalJniHandle (__this.CreatePendingResult (requestCode, data!, flags!));
			return __ret;
		}
#pragma warning restore 0169
	}
}

Key to the idea here is that jniabi.… will contain all methods in the api.xml for the declaring type. That way a partial class can always access the "ABI" methods in partial classes/etc.

This may result in larger binding assemblies (all declared methods get "JNI glue code", not just those appearing within the bindings). This assumes that the linker will be able to remove such unused methods, which it should be able to.

---

Related design points on the jniabi type:

• It should ideally be identical to a future "JavaInterop1" ABI, so no use of Android-specific types like IJavaObject.

[AmrAlSayed0]

This would be even better
Activity.cs

public partial class Activity {

	// Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='Activity']/method[@name='onCreate' and count(parameter)=1 and parameter[1][@type='android.os.Bundle']]"
	partial protected virtual unsafe void OnCreate (Android.OS.Bundle? savedInstanceState);

}

Activity._Binding.cs

[global::Android.Runtime.Register ("android/app/Activity", DoNotGenerateAcw=true)]
public partial class Activity {

	static Delegate GetOnCreate_Landroid_os_Bundle_Handler () => _JniJavaCaller.GetOnCreate_Landroid_os_Bundle_Handler ();

	// Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='Activity']/method[@name='onCreate' and count(parameter)=1 and parameter[1][@type='android.os.Bundle']]"
	[Register ("onCreate", "(Landroid/os/Bundle;)V", "GetOnCreate_Landroid_os_Bundle_Handler")]
	partial protected virtual unsafe void OnCreate (Android.OS.Bundle? savedInstanceState)
	{
		try {
			_JniMethods.onCreate (this, savedInstanceState?.Handle ?? IntPtr.Zero);
		} finally {
			global::System.GC.KeepAlive (savedInstanceState);
		}
	}

	internal static class _JniJavaCaller {
		static Delegate? cb_onCreate_Landroid_os_Bundle_;
#pragma warning disable 0169
		public static Delegate GetOnCreate_Landroid_os_Bundle_Handler ()
		{
			if (cb_onCreate_Landroid_os_Bundle_ == null)
				cb_onCreate_Landroid_os_Bundle_ = JNINativeWrapper.CreateDelegate ((_JniMarshal_PPL_V) n_OnCreate_Landroid_os_Bundle_);
			return cb_onCreate_Landroid_os_Bundle_;
		}

		static void n_OnCreate_Landroid_os_Bundle_ (IntPtr jnienv, IntPtr native__this, IntPtr native_savedInstanceState)
		{
			var __this = global::Java.Lang.Object.GetObject<Android.App.Activity> (jnienv, native__this, JniHandleOwnership.DoNotTransfer)!;
			var savedInstanceState = global::Java.Lang.Object.GetObject<Android.OS.Bundle> (native_savedInstanceState, JniHandleOwnership.DoNotTransfer);
			__this.OnCreate (savedInstanceState);
		}
#pragma warning restore 0169
	}

	internal static class _JniMethods {
		internal static readonly JniPeerMembers _members = new XAPeerMembers ("android/app/Activity", typeof (Activity));

		public static void onCreate (IJavaPeerable self, IntPtr savedInstanceState)
		{
			const string __id = "onCreate.(Landroid/os/Bundle;)V";
			JniArgumentValue* __args = stackalloc JniArgumentValue [1];
			__args [0] = new JniArgumentValue (savedInstanceState);
			_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
		}
	}
}

This would help external developers with fixing the bindings in the Metadata file. Most of the time developers don't care about the implementation, they just care about the interface. If the interface is separated out in it's own file, finding the issue by just scrolling will be easier, especially issue that aren't build errors (e.g JLO instead of generic constraint return).

But maybe this could be a separate issue.

[jpobst]

One issue with this proposal is that our "internal" binding infrastructure is a kind of "public" API due to our use of partial types. Making changes to it can break people's Additions code.

For example, I tried to make the trivial change to turn this generated code:

int foo;
public int Foo { get { return foo; } }

Into an auto-property:

public int Foo { get; }

However this broke Additions code that was accessing the field foo. 😢

While I would love all of our generated code to be cleaner, I don't know if we can do much without creating a new $(AndroidCodegenTarget) and maintaining the new and old one ~forever. Unless there's another way to handle this, I don't think it's worth making this change.