search

dotnet / java-interop

Java.Interop provides open-source bindings of Java's Java Native Interface (JNI) for use with .NET managed languages such as C#
Other
201 stars 53 forks source link

`managedType` parameter overrides don't influence `override` detection #586

Open moljac opened 4 years ago

moljac commented 4 years ago

When the <attr/> element is used to update managedType, the new value should be used to determine if the method that the parameter is one is overrides a base class method or is instead a new virtual method.

See: https://github.com/xamarin/java.interop/issues/586#issuecomment-591572465

That said, the other problem is that we shouldn't need an <attr/> to update managedType in the first place. This itself is a bug, but I'm less certain how to describe it, other than an issue seemingly related to "contravariant method parameters due to the use of Java generics".

-- original report follows --

Problem

Changing type (java) with managed FQCVlassName works:

https://github.com/moljac/Samples.AndroidX/blob/master/source/bindings/type-vs-managedType/source/androidx.leanback/leanback-preference/transforms/Metadata.xml#L6-L7

    <attr path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterMulti']/method[@name='onBindViewHolder']/parameter[1]" name="type">AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder</attr>
    <attr path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterSingle']/method[@name='onBindViewHolder']/parameter[1]" name="type">AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder</attr>

while changing managedType causes build errors (does not change managed type):

https://github.com/moljac/Samples.AndroidX/blob/master/source/bindings/type-vs-managedType/source/androidx.leanback/leanback-preference/transforms/Metadata.xml#L26-L36

    <attr 
        path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterMulti']/method[@name='onBindViewHolder']/parameter[1]" 
        name="managedType"
        >
        AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder</attr>
    <attr 
        path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterSingle']/method[@name='onBindViewHolder']/parameter[1]" 
        name="managedType"
        >
        AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder
    </attr>

Errors:

    generated/androidx.leanback.leanback-preference/obj/Debug/monoandroid90/generated/src/AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.cs(24,24): 
    Error CS0534: 
        'LeanbackListPreferenceDialogFragment.AdapterMulti' 
    does not implement inherited abstract member 
        'RecyclerView.Adapter.OnBindViewHolder(RecyclerView.ViewHolder, int)'

    generated/androidx.leanback.leanback-preference/obj/Debug/monoandroid90/generated/src/AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.cs(24,24):
    Error CS0534: 
        'LeanbackListPreferenceDialogFragment.AdapterSingle' 
    does not implement inherited abstract member 
        'RecyclerView.Adapter.OnBindViewHolder(RecyclerView.ViewHolder, int)'

References Links

Solution (minimal repro) is zipped/archived in:

https://github.com/moljac/Samples.AndroidX/blob/master/source/bindings/type-vs-managedType.zip

api.xml

https://github.com/moljac/Samples.AndroidX/blob/master/source/bindings/type-vs-managedType/api.xml

jonpryor commented 4 years ago

Background: changing type is usually considered Bad™, because the type value is used to compute the JNI signature for member lookups, and if it changes it could prevent the member from being found. Thus, changing managedType is preferred, as it shouldn't impact the JNI signatures.

The diff in the generated code between the "working yet wrong" version which changes type and the "broken yet 'correct' (?)" version which changes managedType is interesting:

--- ../androidx.leanback.leanback-preference-obj/Debug/monoandroid90/generated/src/AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.cs 2020-02-26 13:10:39.000000000 -0500
+++ androidx.leanback.leanback-preference/obj/Debug/monoandroid90/generated/src/AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.cs    2020-02-26 13:12:37.000000000 -0500
@@ -132,28 +132,28 @@
                }
            }

-           static Delegate cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I;
+           static Delegate cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I;
 #pragma warning disable 0169
-           static Delegate GetOnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_IHandler ()
+           static Delegate GetOnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_IHandler ()
            {
-               if (cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I == null)
-                   cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr, IntPtr, int>) n_OnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I);
-               return cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I;
+               if (cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I == null)
+                   cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr, IntPtr, int>) n_OnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I);
+               return cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I;
            }

-           static void n_OnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I (IntPtr jnienv, IntPtr native__this, IntPtr native_holder, int position)
+           static void n_OnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I (IntPtr jnienv, IntPtr native__this, IntPtr native_holder, int position)
            {
                global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.AdapterMulti __this = global::Java.Lang.Object.GetObject<global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.AdapterMulti> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
-               global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder holder = global::Java.Lang.Object.GetObject<global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder> (native_holder, JniHandleOwnership.DoNotTransfer);
+               global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.ViewHolder holder = global::Java.Lang.Object.GetObject<global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.ViewHolder> (native_holder, JniHandleOwnership.DoNotTransfer);
                __this.OnBindViewHolder (holder, position);
            }
 #pragma warning restore 0169

-           // Metadata.xml XPath method reference: path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterMulti']/method[@name='onBindViewHolder' and count(parameter)=2 and parameter[1][@type='AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder'] and parameter[2][@type='int']]"
-           [Register ("onBindViewHolder", "(Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V", "GetOnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_IHandler")]
-           public override unsafe void OnBindViewHolder (global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder holder, int position)
+           // Metadata.xml XPath method reference: path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterMulti']/method[@name='onBindViewHolder' and count(parameter)=2 and parameter[1][@type='androidx.leanback.preference.LeanbackListPreferenceDialogFragment.ViewHolder'] and parameter[2][@type='int']]"
+           [Register ("onBindViewHolder", "(Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;I)V", "GetOnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_IHandler")]
+           public virtual unsafe void OnBindViewHolder (global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder holder, int position)
            {
-               const string __id = "onBindViewHolder.(Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V";
+               const string __id = "onBindViewHolder.(Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;I)V";
                try {
                    JniArgumentValue* __args = stackalloc JniArgumentValue [2];
                    __args [0] = new JniArgumentValue ((holder == null) ? IntPtr.Zero : ((global::Java.Lang.Object) holder).Handle);
@@ -351,28 +351,28 @@
                }
            }

-           static Delegate cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I;
+           static Delegate cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I;
 #pragma warning disable 0169
-           static Delegate GetOnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_IHandler ()
+           static Delegate GetOnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_IHandler ()
            {
-               if (cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I == null)
-                   cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr, IntPtr, int>) n_OnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I);
-               return cb_onBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I;
+               if (cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I == null)
+                   cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I = JNINativeWrapper.CreateDelegate ((Action<IntPtr, IntPtr, IntPtr, int>) n_OnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I);
+               return cb_onBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I;
            }

-           static void n_OnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_I (IntPtr jnienv, IntPtr native__this, IntPtr native_holder, int position)
+           static void n_OnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_I (IntPtr jnienv, IntPtr native__this, IntPtr native_holder, int position)
            {
                global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.AdapterSingle __this = global::Java.Lang.Object.GetObject<global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.AdapterSingle> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
-               global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder holder = global::Java.Lang.Object.GetObject<global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder> (native_holder, JniHandleOwnership.DoNotTransfer);
+               global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.ViewHolder holder = global::Java.Lang.Object.GetObject<global::AndroidX.Leanback.Preference.LeanbackListPreferenceDialogFragment.ViewHolder> (native_holder, JniHandleOwnership.DoNotTransfer);
                __this.OnBindViewHolder (holder, position);
            }
 #pragma warning restore 0169

-           // Metadata.xml XPath method reference: path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterSingle']/method[@name='onBindViewHolder' and count(parameter)=2 and parameter[1][@type='AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder'] and parameter[2][@type='int']]"
-           [Register ("onBindViewHolder", "(Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V", "GetOnBindViewHolder_Landroidx_recyclerview_widget_RecyclerView_ViewHolder_IHandler")]
-           public override unsafe void OnBindViewHolder (global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder holder, int position)
+           // Metadata.xml XPath method reference: path="/api/package[@name='androidx.leanback.preference']/class[@name='LeanbackListPreferenceDialogFragment.AdapterSingle']/method[@name='onBindViewHolder' and count(parameter)=2 and parameter[1][@type='androidx.leanback.preference.LeanbackListPreferenceDialogFragment.ViewHolder'] and parameter[2][@type='int']]"
+           [Register ("onBindViewHolder", "(Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;I)V", "GetOnBindViewHolder_Landroidx_leanback_preference_LeanbackListPreferenceDialogFragment_ViewHolder_IHandler")]
+           public virtual unsafe void OnBindViewHolder (global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder holder, int position)
            {
-               const string __id = "onBindViewHolder.(Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V";
+               const string __id = "onBindViewHolder.(Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;I)V";
                try {
                    JniArgumentValue* __args = stackalloc JniArgumentValue [2];
                    __args [0] = new JniArgumentValue ((holder == null) ? IntPtr.Zero : ((global::Java.Lang.Object) holder).Handle);

Of particular note is the OnBindViewHolder() changed from override to virtual.

It appears that virtual-vs-override detection should be based on the managed name, and not based on the Java name, so that Metadata can be used to coerce generator to emit overrides when necessary.

jonpryor commented 4 years ago

Also of note is that the JNI signature changed, which is to be expected, e.g.

-               const string __id = "onBindViewHolder.(Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V";
+               const string __id = "onBindViewHolder.(Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;I)V";

(I guess that generator must lookup the Java type from the managed type somewhere/somehow, which is why AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder becomes androidx.recyclerview.widget.RecyclerView.ViewHolder? 'Cause otherwise, that doesn't make sense to me.)

This change is also weird, but not a bug "per-se"; Turns Out™, LeanbackListPreferenceDialogFragment.AdapterMulti has two onBindViewHolder() methods, one of which matches the "wrong" JNI signature, meaning it's not "wrong".

Digging further, I'm starting to think that this is in part an "api fixup" bug. Again, there are two onBindViewHolder() methods:

% javap -classpath externals/androidx.leanback/leanback-preference/classes.jar 'androidx/leanback/preference/LeanbackListPreferenceDialogFragment$AdapterSingle' | grep onBind
  public void onBindViewHolder(androidx.leanback.preference.LeanbackListPreferenceDialogFragment$ViewHolder, int);
  public void onBindViewHolder(androidx.recyclerview.widget.RecyclerView$ViewHolder, int);

api.xml.class-parse output contains both:

      <method
        abstract="false"
        deprecated="not deprecated"
        final="false"
        name="onBindViewHolder"
        native="false"
        return="void"
        jni-return="V"
        static="false"
        synchronized="false"
        visibility="public"
        bridge="false"
        synthetic="false"
        jni-signature="(Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;I)V">
        <parameter
          name="holder"
          type="androidx.leanback.preference.LeanbackListPreferenceDialogFragment.ViewHolder"
          jni-type="Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;" />
        <parameter
          name="position"
          type="int"
          jni-type="I" />
      </method>
      <method
        abstract="false"
        deprecated="not deprecated"
        final="false"
        name="onBindViewHolder"
        native="false"
        return="void"
        jni-return="V"
        static="false"
        synchronized="false"
        visibility="public"
        bridge="true"
        synthetic="true"
        jni-signature="(Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V">
        <parameter
          name="p0"
          type="androidx.recyclerview.widget.RecyclerView.ViewHolder"
          jni-type="Landroidx/recyclerview/widget/RecyclerView$ViewHolder;" />
        <parameter
          name="p1"
          type="int"
          jni-type="I" />
      </method>

But api.xml does not!

      <method abstract="false" deprecated="not deprecated" final="false" name="onBindViewHolder" jni-signature="(Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;I)V" bridge="false" native="false" return="void" jni-return="V" static="false" synchronized="false" synthetic="false" visibility="public">
        <parameter name="holder" type="androidx.leanback.preference.LeanbackListPreferenceDialogFragment.ViewHolder" jni-type="Landroidx/leanback/preference/LeanbackListPreferenceDialogFragment$ViewHolder;">
        </parameter>
        <parameter name="position" type="int" jni-type="I">
        </parameter>
      </method>

Additionally, note that it's the overload which has synthetic="true" which is removed.

Related: https://developer.android.com/reference/androidx/leanback/preference/LeanbackListPreferenceDialogFragment.AdapterSingle#onBindViewHolder(androidx.leanback.preference.LeanbackListPreferenceDialogFragment.ViewHolder,%20int)

So here's what's going on, I think: Java generics & parameter covariance, oh my!

Something like this is the presumed Java code:

package androidx.leanback.preference;

public /* partial */ class LeanbackListPreferenceDialogFragment {
    public /* partial */ class AdapterMulti extends RecyclerView.Adapter<LeanbackListPreferenceDialogFragment.ViewHolder> {
        @Override
        public void onBindViewHolder(LeanbackListPreferenceDialogFragment.ViewHolder holder, int position) {
        }
    }
}

However, because RecyclerView.Adapter<VH> is generic, and RecyclerView.Adapter<VH>.onBindViewHolder(VH, int) takes that generic type as a parameter, this is where Java generic type erasure enters the picture:

RecylerView.Adapter<VH>.onBindViewHolder()'s Java byte code only contains a declaration that matches the constraint on VH, which is RecyclerView.ViewHolder:

% javap -s -classpath 'externals/androidx.recyclerview/recyclerview/classes.jar' 'androidx/recyclerview/widget/RecyclerView$Adapter' | grep -1 onBindV

  public abstract void onBindViewHolder(VH, int);
    descriptor: (Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V

Along comes LeanbackListPreferenceDialogFragment.AdapterMulti. It declares onBindViewHolder(LeanbackListPreferenceDialogFragment.ViewHolder, int), so that must exist, but in the world of Java generics, how does an onBindViewHolder() invocation on an RecyclerView.Adapter variable get routed to LeanbackListPreferenceDialogFragment.AdapterMulti? That's where the "synthetic method" comes in: the Java compiler also emits an override with the "synthetic" attribute set, so the Java byte code is equivalent to if the Author wrote:

package androidx.leanback.preference;

public /* partial */ class LeanbackListPreferenceDialogFragment {
    public /* partial */ class AdapterMulti extends RecyclerView.Adapter<LeanbackListPreferenceDialogFragment.ViewHolder> {
        @Override
        public void onBindViewHolder(LeanbackListPreferenceDialogFragment.ViewHolder holder, int position) {
            /* ... */
        }
        // "synthetic method"!
        @Override
        public void onBindViewHolder(RecyclerView.ViewHolder holder, int position) {
            onBindViewHolder ((LeanbackListPreferenceDialogFragment.ViewHolder) holder, position);
        }
    }
}

It's because this synthetic method exists, and that the "wrong" JNI method uses the synthetic method signature, that the bindings work at runtime.

jonpryor commented 4 years ago

See also: Issue #587.