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

managedType parameter overrides don't influence override detection #586

Open
moljac opened this issue Feb 26, 2020 · 3 comments
Open

managedType parameter overrides don't influence override detection #586

moljac opened this issue Feb 26, 2020 · 3 comments
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@moljac
Copy link

moljac commented Feb 26, 2020

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: #586 (comment)

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

See also: Issue #587.

@jonpryor jonpryor changed the title Weird changing type with Managed FQClassName works while managedType cause build errors managedType parameter overrides don't influence override detection Feb 26, 2020
@jpobst jpobst added generator Issues binding a Java library (generator, class-parse, etc.) bug Component does not function as intended labels Apr 6, 2020
jonpryor pushed a commit that referenced this issue Aug 14, 2020
`generator` is an amazingly powerful tool that has proven to be very
versatile over the past decade.  However, many of the remaining bugs
fall into a category that is hard to fix given `generator`'s current
design, which is:

 1. Read in Java model
 2. Make tweaks to Java model
 3. Open `<TYPE>.cs`
 4. Loop through Java model determining what C# to write

The issue with this is that we are deciding **what** to generate
on-the-fly to a write-only stream.  This means that once we've written
to the stream we cannot change it if future information suggests that
we should.

A good example is issue #461.

The Java model is: #461

Given:

	// Java:
	interface AnimatorListener {
	  onAnimationEnd (int p0);
	  onAnimationEnd (int p0, p1);
	}

Looping through this, we see we need to generate an `EventArgs` class for
`AnimatorListener.onAnimationEnd(int p0)`, so we do:

	public partial class OnAnimationEndEventArgs : global::System.EventArgs {
	    int p0;

	    public OnAnimationEndEventArgs (int p0) { this.p0= p0; }

	    public int P0 { get { return p0; } }
	}

Then we get to the second method
`AnimatorListener.onAnimationEnd(int p0, int p1)` and see that we need
to add additional parameters to `OnAnimationEndEventArgs`.  However we
cannot modify the already written class, so we generate a second
`OnAnimationEndEventArgs` with different parameters, which causes
CS0101 compilation errors in C#.

Another example is we can generate an empty `InterfaceConsts` class.
This is because we write the class opening because we think we have
members to place in it (`public static class InterfaceConsts {`), but
as we loop through the members we thought we were going to add they
all get eliminated.  At that point all we can do is close the class
with no members.

The solution to both examples above is "simple": we need to first loop
through the rest of the Java model and determine what we **actually**
need to generate before we start writing anything.  We can then merge
the two `OnAnimationEndEventArgs` classes, or forgo writing an empty
`InterfaceConsts` type.

However, rather than adding this additional logic in each instance
that needs it, there is enough benefit to convert the entire
`generator` architecture to work by first building a C# model of what
needs to be generated that can be tweaked before writing to a file,
resulting in a design of:

 1. Read in Java model
 2. Make tweaks to Java model
 3. ***Build C# model from Java model***
 4. ***Make tweaks to C# model***
 5. Open `<TYPE>.cs`
 6. Loop through C# model, writing code

Having a C# model to tweak should also help in a few other tricky
cases we fail at today, like determining `override`s (#367, #586) and
implementing `abstract` methods for `abstract` classes inheriting an
`interface` (#470).

Additionally, having distinct logic and source writing steps will make
unit testing better.  Currently, the only way to test the *logic* of if
we are going to generate something is to write out the source code and
compare it to "expected" source code.  This means tiny fixes can have
many "expected" files that need to change (#625).

With separate steps, we can have a set of unit tests that test what
code we are writing, and a set that tests the logic of determining what
to generate.  To test the above "2 `EventArgs` classes" example, we can
test a fix by looking at the created C# model to ensure that only a
single combined `OnAnimationEndEventArgs` class is created.  We would
not need to write out the generated source code and compare it to
expected source code.

To assist in this new design, add a new `Xamarin.SourceWriter.dll`
assembly which is responsible for generating the C# source code.
This eliminates a lot of hard to read and error prone code such as:

	writer.WriteLine ("{0}{1}{2}{3}{4} unsafe {5} {6}{7} ({8})",
	        indent,
	        visibility,
	        static_arg,
	        virt_ov,
	        seal,
	        ret,
	        is_explicit ? GetDeclaringTypeOfExplicitInterfaceMethod (method.OverriddenInterfaceMethod) + '.' : string.Empty,
	        method.AdjustedName,
	        method.GetSignature (opt));

and replaces it with:

	var m = new MethodWriter {
	    IsPublic = true,
	    IsUnsafe = true,
	    IsOverride = true,
	    Name = method.AdjustedName
	};

	m.Write (…);

which will emit:

	public unsafe override void DoSomething ()
	{
	}

`Xamarin.SourceWriter.CodeWriter` is a wrapper around a
`System.IO.Stream` that tracks the current indent level, rather than
needing to pass `indent` around to every method.

	cw.WriteLine ("{");
	cw.Indent ();
	// ... write block body ...
	cw.Unindent ();
	cw.WriteLine ("}");

~~ Testing ~~

Many existing unit tests call directly into methods like
`CodeGenerator.WriteProperties()`.  These methods are no longer
available when using `JavaInterop`/`XAJavaInterop`.  These tests were
either changed to use `CodeGenerator.WriteType()` or were moved to only
run for `XamarinAndroid` codegen target.

Tests were updated to ignore whitespace and line endings differences,
as the refactor did not preserve 100% identical whitespace.
jonpryor pushed a commit that referenced this issue Apr 19, 2022
Fixes: #967

Context: dotnet/android-libraries#504
Context: #424
Context: #586
Context: #918

Java generics continues to be a "difficulty" in creating bindings.
Consider [`ActivityResultContracts.RequestPermission`][0]:

	// Java
	public abstract /* partial */ class ActivityResultContract<I, O> {
	    public abstract Intent createIntent(Context context, I input);
	}
	public /* partial */ class /* ActivityResultContracts. */ RequestPermission
	    extends ActivityResultContract<String, Boolean>
	{
	    @OverRide public Intent createIntent(Context context, String input) {…}
	}

The JNI Signature for
`ActivityResultContracts.RequestPermission.createIntent()` is
`(Landroid/content/Context;Ljava/lang/String;)Landroid/content/Intent;`,
i.e. `class-parse` & `generator` believe that the `input` parameter
is of type `String`, which we bind by default as `string`.  Thus:

	// C#
	public abstract partial class ActivityResultContract {
	    public abstract Intent CreateIntent (Context? context, Java.Lang.Object? input) => …
	}
	public partial class /* ActivityResultContracts. */ RequestPermission {
	    public override Intent CreateIntent (Context? context, string? input) => …
	}

This fails to compile with a [CS0115][1]:

	'RequestPermission.CreateIntent(Context?, string?)': no suitable method found to override

as the `input` parameter of `RequestPermission.CreateIntent()` changes
from `Java.Lang.Object?` to `string?`.

We can attempt to address this via Metadata:

	<attr
	  path="/api/package[@name='androidx.activity.result.contract']/class[@name='ActivityResultContracts.RequestPermission']/method[@name='createIntent' and count(parameter)=2 and parameter[1][@type='android.content.Context'] and parameter[2][@type='java.lang.String']]"
	  name="managedType"
	>Java.Lang.Object</attr>

This fixes one error, as `generator` now emits:

	public partial class /* ActivityResultContracts. */ RequestPermission {
	    [Register ("createIntent", "(Landroid/content/Context;Ljava/lang/String;)Landroid/content/Intent;", "")]
	    public override unsafe global::Android.Content.Intent CreateIntent (global::Android.Content.Context context, global::Java.Lang.Object input)
	    {
	        const string __id = "createIntent.(Landroid/content/Context;Ljava/lang/String;)Landroid/content/Intent;";
	        IntPtr native_input = JNIEnv.NewString (input);
	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [2];
	            __args [0] = new JniArgumentValue ((context == null) ? IntPtr.Zero : ((global::Java.Lang.Object) context).Handle);
	            __args [1] = new JniArgumentValue (native_input);
	            var __rm = _members.InstanceMethods.InvokeAbstractObjectMethod (__id, this, __args);
	            return global::Java.Lang.Object.GetObject<global::Android.Content.Intent> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
	        } finally {
	            JNIEnv.DeleteLocalRef (native_input);
	            global::System.GC.KeepAlive (context);
	            global::System.GC.KeepAlive (input);
	        }
	    }
	}

The `override` method declaration is correct.

However, this introduces a [CS1503][2] error:

	IntPtr native_input = JNIEnv.NewString (input);
	// Error CS1503 Argument 1: cannot convert from 'Java.Lang.Object' to 'string?'

The workaround is to remove `createIntent()` ~entirely, and manually
bind it, as done in dotnet/android-libraries#512.

Fix this issue by always emitting a cast to `(string)` as
part of the `JNIEnv.NewString()` invocation, instead emitting:

	IntPtr native_input = JNIEnv.NewString ((string?) input);

This works because `Java.Lang.Object` defines an
[explicit conversion to `string?`][3], and if a `Java.Lang.String`
instance is provided to the `input` parameter, it's equivalent to
calling `.ToString()`.

This fix allows the original suggested Metadata solution to work.

[0]: https://developer.android.com/reference/androidx/activity/result/contract/ActivityResultContracts.RequestPermission
[1]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs0115
[2]: https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs1503
[3]: https://github.com/xamarin/xamarin-android/blob/a58d4e9706455227eabb6e5b5103b25da716688b/src/Mono.Android/Java.Lang/Object.cs#L434-L439
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Component does not function as intended generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

No branches or pull requests

3 participants