Reconsider (remove?) ApiXmlAdjuster

[jonpryor]

Background: In The Beginning, Google provided an API XML file, and we used that as input to generator to produce Mono.Android.dll. It Was Good...except we couldn't bind anything else. Then API-H/Honeycomb came out and didn't have an API XML file, so we created jar2xml to create an API XML description which we could continue feeding to generator, and It Was Good.

...well, mostly. jar2xml had problems if any types were unresolvable, which wasn't that uncommon in practice, so class-parse was written to create an API XML description...

...which couldn't be fed to generator. class-parse emitted the same XML dialect as jar2xml (which was similar to the original Google dialect), as it read data based on what was in .class files, which differs from what Java reflection "sees", it was sufficiently different that trying to use class-parse xml with generator would result in Pain and Suffering. (@dellis1972 likely remembers some of that...)

The fix was Xamarin.Android.Tools.ApiXmlAdjuster, which would read class-parse XML and mutate it to be "more like" jar2xml XML, which could then be fed into generator for bindings:

https://github.com/xamarin/xamarin-android/blob/2722719279df2665e82ff772f14b779c551bb2ea/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets#L394-L411

The Problem: We want generator to be Robust and Bug Free™ (arguably impossible, but go with it!), so enter Issue #586 and https://github.com/xamarin/java.interop/issues/586#issuecomment-591597212

One of the problems in Issue #586 is that generator doesn't take managedType overrides into consideration when determining whether a method should be an override or not.

The real problem, though, is that any fixup was required at all. The "current fix" in #586 is to change //@type (?!) so that the resulting C# code compiles, while arguably managedType should be usable, but isn't.

BUT NO CHANGES SHOULD BE REQUIRED AT ALL.

Why are any changes needed to bind androidx.leanback?

The Solution (?):

One of the members that ApiXmlAdjuster removes are "synthetic" methods, which are methods emitted by the compiler and not written by the developer. This makes sense as they're compiler-generated, yet...

In the case of Issue #586, the synthetic method matches C# requirements! Because of type erasure, RecylerView.Adapter<VH>.onBindViewHolder(VH, int) is really RecylerView.Adapter.onBindViewHolder(RecyclerView.ViewHolder, int), which Turns Out™ to be exactly what C# needs for override resolution purposes!

Thus: can we remove ApiXmlAdjuster, and use the original data within class-parse to emit more reliable bindings? Could we rely on synthetic methods as part of our binding strategy, as those may be closer to C# API semantics than what we're currently trying to do?

[jonpryor]

After a night's sleep, there's a problem with the "let's just use synthetic methods!" approach suggested above: overrides and base method invocations.

Consider this Java code:

class MyExampleAdapter extends LeanbackListPreferenceDialogFragment.AdapterMulti {
    @Override
    public void onBindViewHolder(LeanbackListPreferenceDialogFragment.ViewHolder holder, int position) {
        /* ... */
        super.onBindViewHolder(holder, position);
    }
}

The compiler emits overrides for both the synthetic and non-synthetic methods, a'la

class MyExampleAdapter extends LeanbackListPreferenceDialogFragment.AdapterMulti {
    @Override
    public void onBindViewHolder(LeanbackListPreferenceDialogFragment.ViewHolder holder, int position) {
        /* ... */
        super.onBindViewHolder(holder, position);
    }
    // compiler-emitted synthetic method
    @Override
    public void onBindViewHolder(RecyclerView.ViewHolder holder, int position) {
        /* ... */
        super.onBindViewHolder(holder, position);
    }
}

but if we rely exclusively on the Synthetic method -- "skipping" the LeanbackListPreferenceDialogFragment.ViewHolder parameter -- then the code does not compile:

class MyExampleAdapter extends LeanbackListPreferenceDialogFragment.AdapterMulti {
    // If we try to manually write method with a "synthetic method"-compatible signature...
    @Override
    public void onBindViewHolder(RecyclerView.ViewHolder holder, int position) {
        /* ... */
        super.onBindViewHolder(holder, position);
        // error: incompatible types: RecyclerView.ViewHolder cannot be converted to LeanbackListPreferenceDialogFragment.ViewHolder
    }
}

Thus, removing the synthetic method -- as ApiXmlAdjuster does -- is very likely the sane thing to do.

[jonpryor]

The above explanation actually brings to light the problem with altering the type attribute, as was originally done in Issue #586. Recall, by changing type, we got:

[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)
{
    const string __id = "onBindViewHolder.(Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V";
    try {
        JniArgumentValue* __args = stackalloc JniArgumentValue [2];
        __args [0] = new JniArgumentValue ((holder == null) ? IntPtr.Zero : ((global::Java.Lang.Object) holder).Handle);
        __args [1] = new JniArgumentValue (position);
        _members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
    } finally {
    }
}

This "works", to the extent that the binding compiles, and to the extent that you can call the OnBindViewHolder() method:

var instance = ...
instance.OnBindViewHolder (holder, position);

But you cannot override OnBindViewHolder()! (Or so I assume, based on testing a smaller example.). This code should fail to build, as the Java Callable Wrapper should fail to compile:

public class MyFragment : LeanbackListPreferenceDialogFragment {
    public class MyAdapter : LeanbackListPreferenceDialogFragment.AdapterMulti {
        public override void OnBindViewHolder (global::AndroidX.RecyclerView.Widget.RecyclerView.ViewHolder holder, int position)
        {
            base.OnBindViewHolder (holder, position);
        }
    }
}

Because the [Register] attribute on OnBindViewHolder() is (Landroidx/recyclerview/widget/RecyclerView$ViewHolder;I)V, the parameter of the Java Callable Wrapper will be androidx.recyclerview.widget.RecyclerView.ViewHolder, which is the signature of the synthetic method, resulting in the javac compiler error.

[jonpryor]

My minimal example code for exploring synthetic methods:

package synthetic_example;

interface Fooable<T extends Runnable> {
    void foo(T value);
}

class MyRunnable implements Runnable {
    public void run() {
    }
}

class MyRunnableFooable implements Fooable<MyRunnable> {
    public void foo(MyRunnable value) {
    }
}

class MyExtendedRunnableFooable extends MyRunnableFooable {
    @Override
    public void foo(MyRunnable value) {
    }
    /* trying to explicitly use the following method fails to compile
    @Override
    public void foo(Runnable value) {
        super.foo(value);
    }
    */
}

[jonpryor]

We may want to remove ApiXmlAdjuster for other reasons, but the reasons set forth here do not hold up. Closing the issue.