Bug 9979 - Regression: java.lang.Void parameters are not supported.
Summary: Regression: java.lang.Void parameters are not supported.
Status: RESOLVED FIXED
Alias: None
Product: Android
Classification: Xamarin
Component: Bindings ()
Version: 4.5.x
Hardware: PC Mac OS
: Normal normal
Target Milestone: ---
Assignee: Atsushi Eno
URL:
Depends on:
Blocks:
 
Reported: 2013-02-01 09:38 UTC by Jonathan Pryor
Modified: 2013-06-21 08:39 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Jonathan Pryor 2013-02-01 09:38:41 UTC
When attempting to bind a .jar file which contains a .class which contains the following method:

	public void whyOhWhy (java.lang.Void p)
	{
	}

The resulting api.xml replaces the `java.lang.Void` parameter with `void`:

> <method abstract="false" deprecated="not deprecated" final="false" name="whyOhWhy" native="false" return="void" static="false" synchronized="false" visibility="public">
> <parameter name="p0" type="void">
> </parameter>
> </method>

This API description is wrong.

When generating the C# wrapper for this method, we then get:

>	static IntPtr idwhyOhWhy_V;
>	[Register ("wikitudesdkWhatAreYouDoing", "(V)V", "GetWikitudesdkWhatAreYouDoing_VHandler")]
>	public virtual void WikitudesdkWhatAreYouDoing (void p0)
>	{
>		if (idwhyOhWhy_V == IntPtr.Zero)
>			idwhyOhWhy_V = JNIEnv.GetMethodID (class_ref, "wikitudesdkWhatAreYouDoing", "(V)V");
>
>		if (GetType () == ThresholdType)
>			JNIEnv.CallVoidMethod  (Handle, idwhyOhWhy_V, new JValue (p0));
>		else
>			JNIEnv.CallNonvirtualVoidMethod  (Handle, ThresholdClass, idwhyOhWhy_V, new JValue (p0));
>	}

This has two problems:

1. It won't compile:
> error CS1547: Keyword `void' cannot be used in this context

2. Even if it did compile, it would fail at runtime because the JNI signature is incorrect; the JNI signature is "(Ljava/lang/Void;)V", not "(V)V".

The fundamental problem is that `jar2xml` is too aggressive in replacing `java.lang.Void` with `void`.
Comment 2 Atsushi Eno 2013-02-01 10:38:44 UTC
This actually happens when generic parameters contain void (e.g. in AsyncTask<,,>). Since they are mapped to Java.Lang.Object, they have to be manually replaced by JLO in metadata fixup so far.

I'm not sure if we can change that situation.
Comment 3 Atsushi Eno 2013-02-01 10:39:22 UTC
OK, this *actually* looks like the AsyncTask case I mentioned ;)
Comment 4 Jonathan Pryor 2013-02-01 10:50:57 UTC
> This actually happens when generic parameters contain void.

I don't follow. You mean when a generic parameter is used which in turn uses Void, such as android.os.AsyncTask<Void,Void,Void>?

That shouldn't apply; android.os.AsyncTask<Void,Void,Void> should still be bound as non-generic android.os.AsyncTask (type erasure); there is no Void in there anymore.
Comment 6 Atsushi Eno 2013-02-01 13:21:02 UTC
jar2xml/963c6478 is never a regression. This only fixes the issue that some Java reflection API could return "java.lang.Void" instead of "void" which our generator expects (if our generator doesn't expect that, this fix wasn't required).

Actually nothing is regression. This kind of use of void in AsyncTask has been taken care in metadata fixup. It could have happened by any chance.

This is important difference to understand: java allows such use of void in applied generic parameters. C# does not.

Regardless of whether generic information goes away or not, the implemented methods that embodies concrete types remain in the api.xml.

Take a look at this class from new Facebook Android SDK:

https://github.com/facebook/facebook-android-sdk/blob/master/facebook/src/com/facebook/RequestAsyncTask.java

It specifies the type signature as:

> public class RequestAsyncTask extends AsyncTask<Void, Void, List<Response>>

That means, its doInBackground() becomes: doInBackground(Void...)

https://github.com/facebook/facebook-android-sdk/blob/master/facebook/src/com/facebook/RequestAsyncTask.java#L161

It is precisely reflected in api.xml.

The effective workaround is to remove that overload from the api.xml and let generator automatically implement the "missing" doInBackground() using "params Java.Lang.Object []" (if it doesn't then it should be fixed. But to my understanding, it does, as Facebook SDK binding builds with such metadata fixups - see bug #9348, there I privately attached my updated project that requires nonpublic mfa build).
Comment 7 Jonathan Pryor 2013-02-01 13:50:39 UTC
> jar2xml/963c6478 is never a regression. This only fixes the issue that some
> Java reflection API could return "java.lang.Void

Which makes it sound like it's Java reflection which is broken, but never mind...

Please, look at the original description:

    public void whyOhWhy (java.lang.Void p)
    {
    }

There are no generic types involved. There are no generics involved. Just "normal" use of java.lang.Void as a parameter. Nothing more, nothing less, no AsyncTask, no generics. Completely valid Java.

This fails. Binding projects that used to work no longer work. This is a regression. (Even if the API is in poor taste, it's still valid Java.)

Furthermore, Java generics are a herring; a distraction. They Do Not Exist (99% of the time, and it's that remaining 1% which makes a javap-based tool problematic [0]).

What we (normally) care about is the JNI representation of things, because we're generating JNI-based wrappers, so whatever we generate needs to be kosher with JNI. Just s/java.lang.Void/void/g everywhere will result in INVALID JNI signatures, as mentioned in the original report.

> Take a look at this class from new Facebook Android SDK:
> https://github.com/facebook/facebook-android-sdk/blob/master/facebook/src/com/facebook/RequestAsyncTask.java
...
> It is precisely reflected in api.xml.

HOW is it reflected in api.xml? (Without me needing to build that .jar then bind it...).

Because I'm quite sure that in JNI-land doInBackground() will have TWO method declarations, equivalent to:

> java.util.List doInBackground(java.lang.Void[] params) ;
> java.lang.Object doInBackground(java.lang.Object[] params);

Because...Java.

Furthermore, I'm quite sure that with 963c6478 the JNI signature for the first overload will be "(V)Ljava/util/List;"), which is WRONG (it should be "([Ljava/lang/Void;)Ljava/util/List;").

Fortunately it should be moot here; we should detect that doInBackground() is an override, and thus not emit it in jar2xml (and/or generator; not sure), so when C# code invokes RequestAsyncTask.DoInBackground(), it'll hit the AsyncTask version, not a "new" declaration.

If we did have a new C# JNI declaration, and we invoked it, and the JNI signature was wrong... BOOM. NoSuchMethodError (or whatever).

[0]: The 1%: Implementing generic interfaces which implement Java interfaces, e.g. trying to implement the android.content.EntityIterator interface w/o using generics, which is impossible.
Comment 8 Atsushi Eno 2013-02-01 14:18:45 UTC
Hmm, now I sorta got what you meant aggressive replacement...


>     public void whyOhWhy (java.lang.Void p)

My explanation could also explain this, if the container type D is derived from SomeType<T> and whyOhWhy() method is defined as whyOhWhy(T), then D is like "extends SomeType<java.lang.Void>" (or any indirect derivation).


> HOW is it reflected in api.xml? (Without me needing to build that .jar then
bind it...).

Well, RequestAsyncTask only contains doInBackground(java.lang.Void...) :

<method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="java.util.List&lt;com.facebook.Response&gt;" static="false" synchronized="false" visibility="protected">
<parameter name="p0" type="java.lang.Void...">
</parameter>
</method>

This does not always apply. Session.AuthorizationRequest.4 (obfuscated type!) is different, it has both:

<method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="java.lang.Object" static="false" synchronized="false" visibility="protected">
<parameter name="p0" type="java.lang.Object[]">
</parameter>
</method>
<method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="java.lang.Void" static="false" synchronized="false" visibility="protected">
<parameter name="p0" type="java.lang.Void...">
</parameter>
</method>

This may have two methods in the implementation.

> Fortunately it should be moot here; we should detect that doInBackground() is
an override

It cannot be done in that optimistic way. I think we rather gave up that. Java overrides could be too flexible variant and we don't really track them (i.e. we *don't* track those method overrides that don't match signatures).

Anyways that's another problem. If we need to deal with java.lang.Void in different manner than void, it rather need to be taken care in generator, and I have no idea what could get broken if we revert 963c6478 (it was introduced certainly to "fix" some build error for some library).

This does not sound like something we can fix immediately, and it's not a regression (even though it likely broke something, that's just to convert something that did not build to something that does not run). It should be "fixed" later.
Comment 9 Jonathan Pryor 2013-02-01 14:34:18 UTC
So I built the facebook-android-sdk project; my guessed JNI signatures are correct:

> $ javap -s -classpath bin/classes.jar com/facebook/RequestAsyncTask
> protected java.util.List doInBackground(java.lang.Void[]);
>   Signature: ([Ljava/lang/Void;)Ljava/util/List;
> protected java.lang.Object doInBackground(java.lang.Object[]);
>   Signature: ([Ljava/lang/Object;)Ljava/lang/Object;

So I create an Android Java Bindings Library project, bind my classes.jar, see 11 errors (whatever), but most important is obj\Debug\api.xml:

> <method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="java.util.List&lt;com.facebook.Response&gt;" static="false" synchronized="false" visibility="protected">
> <parameter name="p0" type="void...">
> </parameter>
> </method>

And...BOOM, there we go, the type is `void...`, which isn't valid.

Unsurprisingly that also results in a CS1547 compilation error for RequestAsyncTask.cs (`void[]` isn't valid):

> 		static IntPtr n_DoInBackground_arrayV (IntPtr jnienv, IntPtr native__this, IntPtr native_p0)
> 		{
> 			global::Com.Facebook.RequestAsyncTask __this = global::Java.Lang.Object.GetObject<global::Com.Facebook.RequestAsyncTask> (native__this, JniHandleOwnership.DoNotTransfer);
> 			void[] p0 = (void[]) JNIEnv.GetArray (native_p0, JniHandleOwnership.DoNotTransfer, typeof (void));
> 			IntPtr __ret = global::Android.Runtime.JavaList<global::Com.Facebook.Response>.ToLocalJniHandle (__this.DoInBackground (p0));
> 			if (p0 != null)
> 				JNIEnv.CopyArray (p0, native_p0);
> 			return __ret;
> 		}

What is surprising is that the above XML declaration for doInBackground() is the only doInBackground() method for RequestAsyncTask.

The Session.AutoPublishAsyncTask type, though, does have two:

> <method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="java.lang.Object" static="false" synchronized="false" visibility="protected">
> <parameter name="p0" type="java.lang.Object[]">
> </parameter>
> </method>
> <method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="void" static="false" synchronized="false" visibility="protected">
> <parameter name="p0" type="void...">
> </parameter>
> </method>

Nicely, with the values I expected. ;-)

Unfortunately, that means it'll break too. :-/

Finally, what's the javap for Session.AutoPublishAsyncTask?

> $ javap -s -classpath bin/classes.jar com/facebook/Session.AutoPublishAsyncTask
...
> protected java.lang.Void doInBackground(java.lang.Void[]);
>   Signature: ([Ljava/lang/Void;)Ljava/lang/Void;
> protected java.lang.Object doInBackground(java.lang.Object[]);
>   Signature: ([Ljava/lang/Object;)Ljava/lang/Object;

Note carefully: AutoPublishAsyncTask inherits from AsyncTask<Void, Void, Void>. This means that doInBackground() has a java.lang.Void return type, not `void`, and this shows up in the JNI signature: ([Ljava/lang/Void;)Ljava/lang/Void;

Yet again, massive s/java.lang.Void/void/g results in an invalid JNI signature.
Comment 10 Jonathan Pryor 2013-02-01 14:37:34 UTC
Comparing Comment #8 to Comment #9, what's going on between us?

Comment #8:

> <method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="java.util.List&lt;com.facebook.Response&gt;" static="false" synchronized="false" visibility="protected">
> <parameter name="p0" type="java.lang.Void...">
> </parameter>
> </method>

Comment #9:

> <method abstract="false" deprecated="not deprecated" final="false" name="doInBackground" native="false" return="java.util.List&lt;com.facebook.Response&gt;" static="false" synchronized="false" visibility="protected">
> <parameter name="p0" type="void...">
> </parameter>
> </method>

This is for the SAME METHOD, yet our api.xml's differ!

Note: this is with 4.7.0-8 (4.7.0.43102243). I don't think master has diverged significantly from 4.6, has it?
Comment 11 Jonathan Pryor 2013-02-01 14:46:36 UTC
From Comment #8:
> Anyways that's another problem. If we need to deal with java.lang.Void in
> different manner than void, it rather need to be taken care in generator, and I
> have no idea what could get broken if we revert 963c6478 (it was introduced
> certainly to "fix" some build error for some library).

We ABSOLUTELY need to deal with `java.lang.Void` in a different manner than `void`, as generator MUST know the proper Java-side types in order to construct a proper, valid, JNI signature. Take Session.AutoPublishAsyncTask.doInBackground() as an example, again; as per the api.xml I'm getting, the return type is being detected as `void`. The result is that the C# method will be `void` (okay, fine) but the JNI signature is "V" instead of "Ljava/lang/Void;".

This will prevent JNI from finding the method, rendering it un-invokable.

This is bad.

It looks like the proper place for this fix is in generator, not jar2xml.
Comment 12 Atsushi Eno 2013-02-01 15:55:15 UTC
quick response: my api.xml could be somewhat older, as I'm in the middle of inspecting .__override__ issue.
Comment 13 Atsushi Eno 2013-03-06 14:42:37 UTC
I have reverted jar2xml change so far.
Comment 14 Atsushi Eno 2013-06-21 08:39:50 UTC
So far bindings and use of it is fine with Java.Lang.Void type (we use it in some code), so I mark this as fixed.