[jnimarshalmethod-gen] Do not miss registered methods

[radekdoulik]

Do not miss registered methods with custom attributes without named
arguments. Which can happen, when we load assemblies without symbols.

Our current register attribute detection logic was looking for named arguments only and missed register attributes in the assemblies without symbols. The fix fallbacks to arguments order in case we don't find the names. Like we do in other places, here for example https://github.com/xamarin/xamarin-android/blob/master/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/Extensions.cs#L159-L161

Fixes #356

For some types we miss some of the registered methods. That leads to problems during registration and later crashes, like here with CellAdapter type:

I/mono-stdout( 2532): Registering JNI marshal methods in Xamarin.Forms.Platform.Android.CellAdapter
W/art     ( 2532): Attempt to remove index outside index area (6 vs 7-7)
W/art     ( 2532): JNI WARNING: DeleteLocalRef(0x1bf00019) failed to find entry

Java side of things describes 10 methods, while jnimarshalmethod-gen finds and generates just 6. (note that there are methods with the same method name and different signature. so these are not duplicates and it is just coincidence. it happens in other cases as well, where we don't have same method names)

Java side:

__md_methods =
                        "n_onItemLongClick:(Landroid/widget/AdapterView;Landroid/view/View;IJ)Z:GetOnItemLongClick_Landroid_widget_AdapterView_Landroid_view_View_IJHandler:Android.Widget.AdapterView/IOnItemLongClickListenerInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
                        "n_onActionItemClicked:(Landroid/view/ActionMode;Landroid/view/MenuItem;)Z:GetOnActionItemClicked_Landroid_view_ActionMode_Landroid_view_MenuItem_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
                        "n_onCreateActionMode:(Landroid/view/ActionMode;Landroid/view/Menu;)Z:GetOnCreateActionMode_Landroid_view_ActionMode_Landroid_view_Menu_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
                        "n_onDestroyActionMode:(Landroid/view/ActionMode;)V:GetOnDestroyActionMode_Landroid_view_ActionMode_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
                        "n_onPrepareActionMode:(Landroid/view/ActionMode;Landroid/view/Menu;)Z:GetOnPrepareActionMode_Landroid_view_ActionMode_Landroid_view_Menu_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
                        "n_onItemClick:(Landroid/widget/AdapterView;Landroid/view/View;IJ)V:GetOnItemClick_Landroid_widget_AdapterView_Landroid_view_View_IJHandler:Android.Widget.AdapterView/IOnItemClickListenerInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
                        "n_onActionItemClicked:(Landroid/support/v7/view/ActionMode;Landroid/view/MenuItem;)Z:GetOnActionItemClicked_Landroid_support_v7_view_ActionMode_Landroid_view_MenuItem_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
                        "n_onCreateActionMode:(Landroid/support/v7/view/ActionMode;Landroid/view/Menu;)Z:GetOnCreateActionMode_Landroid_support_v7_view_ActionMode_Landroid_view_Menu_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
                        "n_onDestroyActionMode:(Landroid/support/v7/view/ActionMode;)V:GetOnDestroyActionMode_Landroid_support_v7_view_ActionMode_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
                        "n_onPrepareActionMode:(Landroid/support/v7/view/ActionMode;Landroid/view/Menu;)Z:GetOnPrepareActionMode_Landroid_support_v7_view_ActionMode_Landroid_view_Menu_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
                        "";

C# side:

[JniAddNativeMethodRegistration]
public static void __RegisterNativeMembers (JniNativeMethodRegistrationArguments args)
{
	Console.WriteLine ("Registering JNI marshal methods in Xamarin.Forms.Platform.Android.CellAdapter");
	args.AddRegistrations (new JniNativeMethodRegistration[6] {
		new JniNativeMethodRegistration ("n_onActionItemClicked", "(Landroid/view/ActionMode;Landroid/view/MenuItem;)Z", new Func<IntPtr, IntPtr, IntPtr, IntPtr, bool> (__<$>_jni_marshal_methods.n_onActionItemClicked_Landroid_view_ActionMode_Landroid_view_MenuItem_)),
		new JniNativeMethodRegistration ("n_onCreateActionMode", "(Landroid/view/ActionMode;Landroid/view/Menu;)Z", new Func<IntPtr, IntPtr, IntPtr, IntPtr, bool> (__<$>_jni_marshal_methods.n_onCreateActionMode_Landroid_view_ActionMode_Landroid_view_Menu_)),
		new JniNativeMethodRegistration ("n_onDestroyActionMode", "(Landroid/view/ActionMode;)V", new Action<IntPtr, IntPtr, IntPtr> (__<$>_jni_marshal_methods.n_onDestroyActionMode_Landroid_view_ActionMode_)),
		new JniNativeMethodRegistration ("n_onPrepareActionMode", "(Landroid/view/ActionMode;Landroid/view/Menu;)Z", new Func<IntPtr, IntPtr, IntPtr, IntPtr, bool> (__<$>_jni_marshal_methods.n_onPrepareActionMode_Landroid_view_ActionMode_Landroid_view_Menu_)),
		new JniNativeMethodRegistration ("n_onItemClick", "(Landroid/widget/AdapterView;Landroid/view/View;IJ)V", new Action<IntPtr, IntPtr, IntPtr, IntPtr, IntPtr, IntPtr> (__<$>_jni_marshal_methods.n_onItemClick_Landroid_widget_AdapterView_Landroid_view_View_IJ)),
		new JniNativeMethodRegistration ("n_onItemLongClick", "(Landroid/widget/AdapterView;Landroid/view/View;IJ)Z", new Func<IntPtr, IntPtr, IntPtr, IntPtr, float, float, bool> (__<$>_jni_marshal_methods.n_onItemLongClick_Landroid_widget_AdapterView_Landroid_view_View_IJ))
	});

[jonpryor]

The analysis doesn't make sense to me: why does it fail in the first place? All of these methods use the same [Register(name, signature, connector)] syntax, so I don't see why this code would fail.

(Aside: instead, I see how code like this would fail: [Register(Name="method", Signature="signature", Connector="connector")], because we don't check property initializers. This oversight should be fixed.)

So how is the referenced code failing?

Is it actually failing?

Instead, what I see are duplicates, or rather, the lack thereof. If we sort the Java side for clarity, we see:

"n_onActionItemClicked:(Landroid/support/v7/view/ActionMode;Landroid/view/MenuItem;)Z:GetOnActionItemClicked_Landroid_support_v7_view_ActionMode_Landroid_view_MenuItem_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
"n_onActionItemClicked:(Landroid/view/ActionMode;Landroid/view/MenuItem;)Z:GetOnActionItemClicked_Landroid_view_ActionMode_Landroid_view_MenuItem_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
"n_onCreateActionMode:(Landroid/support/v7/view/ActionMode;Landroid/view/Menu;)Z:GetOnCreateActionMode_Landroid_support_v7_view_ActionMode_Landroid_view_Menu_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
"n_onCreateActionMode:(Landroid/view/ActionMode;Landroid/view/Menu;)Z:GetOnCreateActionMode_Landroid_view_ActionMode_Landroid_view_Menu_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
"n_onDestroyActionMode:(Landroid/support/v7/view/ActionMode;)V:GetOnDestroyActionMode_Landroid_support_v7_view_ActionMode_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
"n_onDestroyActionMode:(Landroid/view/ActionMode;)V:GetOnDestroyActionMode_Landroid_view_ActionMode_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
"n_onItemClick:(Landroid/widget/AdapterView;Landroid/view/View;IJ)V:GetOnItemClick_Landroid_widget_AdapterView_Landroid_view_View_IJHandler:Android.Widget.AdapterView/IOnItemClickListenerInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
"n_onItemLongClick:(Landroid/widget/AdapterView;Landroid/view/View;IJ)Z:GetOnItemLongClick_Landroid_widget_AdapterView_Landroid_view_View_IJHandler:Android.Widget.AdapterView/IOnItemLongClickListenerInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
"n_onPrepareActionMode:(Landroid/support/v7/view/ActionMode;Landroid/view/Menu;)Z:GetOnPrepareActionMode_Landroid_support_v7_view_ActionMode_Landroid_view_Menu_Handler:Android.Support.V7.View.ActionMode/ICallbackInvoker, Xamarin.Android.Support.v7.AppCompat\n" +
"n_onPrepareActionMode:(Landroid/view/ActionMode;Landroid/view/Menu;)Z:GetOnPrepareActionMode_Landroid_view_ActionMode_Landroid_view_Menu_Handler:Android.Views.ActionMode/ICallbackInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +

Notice that there are two n_onActionItemClicked, n_onCreateActionMode, n_onDestroyActionMode, and n_onPrepareActionMode entries.

In the jnimarshalmethod-gen.exe output, there are no duplicates.

I do not understand how or why [Register] parsing would result in the lack of duplicates, and the fact that there are duplicates isn't even mentioned, or explored, or explained in any way. This is troubling.

[jonpryor]

My previous comments aside, I think we should remove the "erroneous" lines you referred to, replacing them with the contents of this PR: https://github.com/xamarin/java.interop/blob/f4c953fac21d5d829ef92a9010eb48a2b534d53d/tools/jnimarshalmethod-gen/App.cs#L479-L495

I don't think that this PR will fix Issue #356 -- or at least I don't understand how it would fix it -- but the change itself is a good idea, and in line with how Constructors are handled elsewhere. (But we still need to properly handle Name property setting, etc.)

[radekdoulik]

Notice that there are two n_onActionItemClicked, n_onCreateActionMode, n_onDestroyActionMode, and n_onPrepareActionMode entries.

In the jnimarshalmethod-gen.exe output, there are no duplicates.

I probably didn't choose the best example. Note that these are not duplicates. They have different signature and are implementations of 2 different interfaces.

Lets look at the first of them, the OnActionItemClicked methods: https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.Android/CellAdapter.cs#L83-L97

I do not understand how or why [Register] parsing would result in the lack of duplicates, and the fact that there are duplicates isn't even mentioned, or explored, or explained in any way. This is troubling.

I have updated the beginning of the PR description to better explain how we missed to detect the attributes.

[radekdoulik]

I don't think that this PR will fix Issue #356 -- or at least I don't understand how it would fix it -- but the change itself is a good idea, and in line with how Constructors are handled elsewhere. (But we still need to properly handle Name property setting, etc.)

It fixes the issue, hopefully the explanation is clear now. Good news is that with that change it works with XA/XForms applications :-)

For using the properties in the constructor, I will have to try that. I guess it might go to another PR, as we might need to fix it in other places too.