-
Notifications
You must be signed in to change notification settings - Fork 54
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
Labels
enhancement
Proposed change to current functionality
generator
Issues binding a Java library (generator, class-parse, etc.)
Comments
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)`.
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.)
Context: #1080
Consider the following Java code:
Build it, bind it:
…elicits lots of warnings; of interest to #1080 is:
Of interest is
gsrc/P.Example.cs
, which bindsExampleBase.m2()
but notExampleBase.m1()
:Thus, the bug/request:
ExampleBase.m1()
should also be bound inExample
, using the type parameters used in theextends
clause to resolve type parameters:Problem: This is apparently another case where Java type erasure is "fun".
Specifically, consider
javap
:This matches
class-parse.exe --dump
andapi.xml
:Example.class
contains anm1
method, but the parameter type form1
isjava.lang.Object
, notjava.lang.String
! (Aside: whengenerator
runs, it createsapi.xml.adjusted
, which removes the<method/>
elements fromapi.xml
…)This suggests that I should be able to call
new Example().m1(new Object())
in Java, but that doesn't compile!So how is it that
Example.class
hasm1(java.lang.Object)
, and does not havem1(java.lang.String)
, and yet the Java compiler requires that a String parameter be passed tom1()
?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 removebin/p/ExampleBase.class
while leavingbin/p/Example.class
behind, types attempting to usep.Example
do not build asp.ExampleBase
is still required! (Though the resulting Java bytecode doesn't referenceExampleBase
, so ifExample
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 thatExample.m1()
only accepts String parameters; it'sExample.class
in combination withExampleBase.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 invokingObject
, a'la: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:
This will eventually require a Java Callable Wrapper, which uses the
[Register]
custom attribute information, which is allObject
, notString
, which means we'll emit:…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: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
.The text was updated successfully, but these errors were encountered: