Skip to content
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.

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

Generic members of package-private base class not properly bound in public derived class #1083

Open
jonpryor opened this issue Feb 15, 2023 · 0 comments
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

Context: #1080

Consider the following Java code:

package p;

class ExampleBase<V> {
    public void m1(V value) {}
    public <T> void m2(T value) {}
}

public class Example extends ExampleBase<String> {
}

Build it, bind it:

mkdir bin
javac -d bin Example.java
mono path/to/class-parse.exe $(find bin -iname \*.class) > api.xml
mkdir gsrc
mono path/to/generator.exe api.xml \
    -o gsrc --codegen-target=XAJavaInterop1 \
    -r /Library/Frameworks/Xamarin.Android.framework/Versions/Current//lib/xamarin.android/xbuild-frameworks/MonoAndroid/v13.0/Mono.Android.dll \
    -L /Library/Frameworks/Xamarin.Android.framework/Versions/Current//lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0

…elicits lots of warnings; of interest to #1080 is:

api.xml(11,10): warning BG8800: Unknown parameter type 'V' for member 'P.Example.M1 (V)'.

Of interest is gsrc/P.Example.cs, which binds ExampleBase.m2() but not ExampleBase.m1():

namespace P {
	// Metadata.xml XPath class reference: path="/api/package[@name='p']/class[@name='Example']"
	[global::Android.Runtime.Register ("p/Example", DoNotGenerateAcw=true)]
	public partial class Example : Java.Lang.Object {
		// …
		[Register ("m2", "(Ljava/lang/Object;)V", "GetM2_Ljava_lang_Object_Handler")]
		[global::Java.Interop.JavaTypeParameters (new string [] {"T"})]
		public virtual unsafe void M2 (Java.Lang.Object p0) =>}
}

Thus, the bug/request: ExampleBase.m1() should also be bound in Example, using the type parameters used in the extends clause to resolve type parameters:

namespace P {
	public partial class Example : Java.Lang.Object {
		[Register ("m1", "(Ljava/lang/Object;)V", "GetM1_Ljava_lang_Object_Handler")]
		public virtual unsafe void M1 (string p0) =>}
}

Problem: This is apparently another case where Java type erasure is "fun".

Specifically, consider javap:

% javap -classpath bin p.Example
Compiled from "Example.java"
public class p.Example extends p.ExampleBase<java.lang.String> {
  public p.Example();
  public static void main(java.lang.String[]);
  public void m2(java.lang.Object);
  public void m1(java.lang.Object);
}

This matches class-parse.exe --dump and api.xml: Example.class contains an m1 method, but the parameter type for m1 is java.lang.Object, not java.lang.String! (Aside: when generator runs, it creates api.xml.adjusted, which removes the <method/> elements from api.xml…)

This suggests that I should be able to call new Example().m1(new Object()) in Java, but that doesn't compile!

error: incompatible types: Object cannot be converted to String

So how is it that Example.class has m1(java.lang.Object), and does not have m1(java.lang.String), and yet the Java compiler requires that a String parameter be passed to m1()?

The answer is that the Java compiler requires that the entire inheritance chain be present at compile-time; Example.class is not a "stand-alone" type! For example, if I "muck around" with things and remove bin/p/ExampleBase.class while leaving bin/p/Example.class behind, types attempting to use p.Example do not build as p.ExampleBase is still required! (Though the resulting Java bytecode doesn't reference ExampleBase, so if Example changes or removes it's base class in the future, existing code won't break. @jonpryor finds this fascinating.)

Thus, Example.class itself doesn't have enough information to tell the Java compiler that Example.m1() only accepts String parameters; it's Example.class in combination with ExampleBase.class which allows the compiler to know that only String parameters are allowed.

What this means in terms of binding is that while the parameter type needs to be a String (System.String for bindings), the JNI signature will instead be invoking Object, a'la:

namespace P {
	public partial class Example : Java.Lang.Object {
		[Register ("m1", "(Ljava/lang/Object;)V", "GetM1_Ljava_lang_Object_Handler")]
		public virtual unsafe void m1 (string p0)
		{
			const string __id = "m1.(Ljava/lang/Object;)I";
			IntPtr native_p0 = JNIEnv.NewString((string?) p0);
			try {
				JniArgumentValue* __args = stackalloc JniArgumentValue [1];
				__args[0] = new JniArgumentValue (native_p0);
				_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
			} finally {
				JNIEnv.DeleteLocalRef (native_p0);
				global::System.GC.KeepAlive (p0);
			}
		}
	}
}

Problem Then we have the problem of subclassing (which is probably a separate bug somewhere, which I'm not bothering to look up right now); consider this C# subclass:

public class MyExample : Example {
    public override void M1(string value) {}
}

This will eventually require a Java Callable Wrapper, which uses the [Register] custom attribute information, which is all Object, not String, which means we'll emit:

public class MyExample extends Example {
    public void m1(Object value) {…}
}

…which won't compile.

Question/suggestion: does the [Register] custom attribute need to match the __id value within the binding method? If not, we can "fake" things and have [Register] use the "generic parameter replaced" version, while __id uses the actual correct version:

namespace P {
	public partial class Example : Java.Lang.Object {
		[Register ("m1", "(Ljava/lang/String;)V", "GetM1_Ljava_lang_String_Handler")]
		public virtual unsafe void m1 (string p0)
		{
			const string __id = "m1.(Ljava/lang/Object;)I";

This would allow Java Callable Wrapper generation to work as-is, but I'm not sure offhand if generator can have separate values for [Register] vs. __id.

jonpryor pushed a commit that referenced this issue Feb 15, 2023
Context: 4ec5d4e
Context: #1083
Context: `generator` crash while binding Google Material 1.6.1

Imagine:

 1. A `public` class that inherits from a *package-private* class, and
 2. The *package-private* class contains `public` or `protected`
    members which should be exposed on (1).

For example:

	/* package-private*/ class BaseClass<V> {
	    public void doThing(V value) {}
	}

	public class MyClass extends BaseClass<Object> {
	}

We won't bind `BaseClass<V>` as it is *package-private*, but we want
(need) it's publicly accessible members to be available to users of
the `public` type `MyClass`; that is, *in Java*,
`MyClass.doThing(Object)` will be an accessible method, and our
binding of `MyClass` should likewise have a `MyClass.DoThing(Object)`
method binding.

In 4ec5d4e we supported this scenario by copying members from the
*package-private* base class into the derived class.

Or at least, that's what the commit says it does.  It does not actually
create a *copy* of the `Method`; rather, it simply adds the existing
`Method` instance to the public derived class, meaning that the same
`Method` instance is referenced by both the base & derived types!

In general, this is fine.  However consider `BaseClass<V>`, above:
the derived type `MyClass` doesn't have a generic type argument `V`,
and thus "copying" `BaseClass<V>.doThing(V)` as-is into `MyClass`
is a non-sequitur; when we later `Validate()` the `Method` instance,
this incongruity is detected and a warning is emitted:

	warning BG8800: Unknown parameter type 'V' for member 'MyClass.DoThing(V)'.

As part of validation, the `Method` instance is also marked as
invalid, by setting the `MethodBase.IsValid` property to `false`, and
the instance is removed from `GenBase.Methods` for `MyClass`.

Unfortunately that's not the end of it, because the `Method` instance
is shared and thus is also in the `GenBase.Methods` collection for
`BaseClass<V>`.  This instance is expected/assumed to be valid, but
was invalidated by `MyClass` validation.

`generator` assumes that after validation, everything within
`GenBase.Methods` is still valid.  This assumption is no longer true
for `BaseClass<V>`.  The result is that if we later attempt to
process `BaseClass<V>`, things blow up when trying to use the parameter
types on `BaseClass<V>.doThing(V)`:

	System.NullReferenceException: Object reference not set to an instance of an object.
	   at MonoDroid.Generation.Parameter.get_JniType() in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\Parameter.cs:line 103
	   at MonoDroid.Generation.ParameterList.get_JniSignature() in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\ParameterList.cs:line 203
	   at MonoDroid.Generation.Method.get_JniSignature() in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\Method.cs:line 210
	   at MonoDroid.Generation.GenBase.ContainsMethod(Method method, Boolean check_ifaces, Boolean check_base_ifaces) in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\GenBase.cs:line 225
	   at MonoDroid.Generation.GenBase.ContainsMethod(Method method, Boolean check_ifaces) in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\GenBase.cs:line 208
	   at MonoDroid.Generation.GenBase.FixupMethodOverrides(CodeGenerationOptions opt) in C:\code\xamarin-android\external\Java.Interop\tools\generator\Java.Interop.Tools.Generator.ObjectModel\GenBase.cs:line 341


The fix is "simple": do what 4ec5d4e said it was doing and make a
*copy* of the `Method` instance using a new `Clone()` method.  With
this fix, the copied method will be invalidated and removed without
corrupting the original `Method` instance.

*Note*: As seen in the unit test changes, the XPath comments are
updated to point to the public type rather than the original package-
private type.  While this isn't "correct", neither was the previous
XPath specification.  After discussion, we have decided that the
needed change is to not add XPath comments on "created" methods (that
is, API that is created by `generator` and is not part of the Java
public API).  However, the cost of doing this change is higher than we
currently wish to spend, so we will live with this minor issue for now.

TODO: #1083 suggests a "complete fix" which allows
`BaseClass<V>.doThing(V)` to be bound as `MyClass.DoThing(Object)`.
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Feb 15, 2023
@jpobst jpobst changed the title members of package-private base class not properly bound in public derived class Generic members of package-private base class not properly bound in public derived class Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

No branches or pull requests

2 participants