[Xamarin.Android.Build.Tasks] Invokers after abstracts in typemaps (#9545)

Fixes: #9535

Context: a017561
Context: 35f41dc

Create a new .NET for Android project (`dotnet new android`), add
a `@(PackageReference)` to [Xamarin.AndroidX.Media3.ExoPlayer][0],
and add the following code:

	var builder = new AndroidX.Media3.ExoPlayer.ExoPlayerBuilder(this);
	var player = builder.Build();

Build and run in *Debug* configuration, and it crashes:

	System.ArgumentException: Could not determine Java type corresponding to `AndroidX.Media3.ExoPlayer.IExoPlayerInvoker, Xamarin.AndroidX.Media3.ExoPlayer, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null`. (Parameter 'targetType')
	   at Java.Interop.TypeManager.CreateInstance(IntPtr handle, JniHandleOwnership transfer, Type targetType) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Interop/TypeManager.cs:line 323
	   at Java.Lang.Object.GetObject(IntPtr handle, JniHandleOwnership transfer, Type type) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Lang/Object.cs:line 302
	   at Java.Lang.Object._GetObject[IExoPlayer](IntPtr handle, JniHandleOwnership transfer) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Lang/Object.cs:line 288
	   at Java.Lang.Object.GetObject[IExoPlayer](IntPtr handle, JniHandleOwnership transfer) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Lang/Object.cs:line 280
	   at AndroidX.Media3.ExoPlayer.ExoPlayerBuilder.Build() in D:\a\_work\1\s\generated\androidx.media3.media3-exoplayer\obj\Release\net8.0-android\generated\src\AndroidX.Media3.ExoPlayer.IExoPlayer.cs:line 749
	   at media3.MainActivity.OnCreate(Bundle savedInstanceState) in /Users/moljac/Downloads/1036/media3/MainActivity.cs:line 16
	   at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_savedInstanceState) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/net9.0/android-35/mcw/Android.App.Activity.cs:line 3196
	   at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(_JniMarshal_PPL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:line 121

☹️

The problem, as often appears to be the case these days, is an
unexpected interaction between different things.

Firstly, the *immediate* cause of the crash is 35f41dc, which
updated `TypeManager.CreateInstance()` to attempt to lookup the Java
type that corresponds to a possible `*Invoker` type:

	// `type` is `typeof(AndroidX.Media3.ExoPlayer.IExoPlayerInvoker)`
	var typeSig  = JNIEnvInit.androidRuntime?.TypeManager.GetTypeSignature (type) ?? default;
	if (!typeSig.IsValid || typeSig.SimpleReference == null) {
	        throw new ArgumentException ($"Could not determine Java type corresponding to `{type.AssemblyQualifiedName}`.", nameof (targetType));
	}

The underlying cause of the crash, and what 35f41dc uncovered, is
that *there is no type mapping for `IExoPlayerInvoker`*.  This is
(not quite) readily seen by examining the generated file:

	% grep IExoPlayerInvoker obj/Debug/net*android*/android/typemaps.arm64-v8a.ll
	# no match

This is also true on .NET 8: there is no type mapping for
`IExoPlayerInvoker`.

There *are* type mappings for *other* interface types, e.g.
`IIntSupplier`:

	% grep IIntSupplier obj/Debug/net*android*/android/typemaps.arm64-v8a.ll
	@.TypeMapEntry.16871_from = private unnamed_addr constant [47 x i8] c"Java.Util.Functions.IIntSupplier, Mono.Android\00", align 1
	@.TypeMapEntry.16873_from = private unnamed_addr constant [54 x i8] c"Java.Util.Functions.IIntSupplierInvoker, Mono.Android\00", align 1

A bit more digging around, and the "problem" is as follows: type map
generation had a *type ordering* requirement, and needed the
`Invoker` types to *follow* their corresponding abstract types.

	# Mono.Android.dll: IIntSupplier is before IIntSupplierInvoker
	% monodis --typedef obj/Debug/net9.0-android/android/assets/arm64-v8a/Mono.Android.dll | grep IIntSupplier
	7676: Java.Util.Functions.IIntSupplier (flist=57465, mlist=135759, flags=0x1000a1, extends=0x0)
	7677: Java.Util.Functions.IIntSupplierInvoker (flist=57465, mlist=135760, flags=0x100000, extends=0x7c28)

	# Xamarin.AndroidX.Media3.ExoPlayer.dll: IExoPlayerInvoker is *before* IExoPlayer
	% monodis --typedef obj/Debug/net9.0-android/android/assets/arm64-v8a/Xamarin.AndroidX.Media3.ExoPlayer.dll| grep IExoPlayer
	139: AndroidX.Media3.ExoPlayer.IExoPlayerInvoker (flist=20, mlist=567, flags=0x100000, extends=0x85)
	…
	166: AndroidX.Media3.ExoPlayer.IExoPlayer (flist=598, mlist=1671, flags=0x1000a1, extends=0x0)

Reasonable aside: *why* is `IExoPlayerInvoker` before `IExoPlayer`?
Because it's a ["real" checked-in copy of generated code][1], not
generated code that follows the `IExoPlayer` generated declaration.

The reason why ordering matters comes from a017561:

> Update typemap generation code in `Xamarin.Android.Build.Tasks.dll` so
> that all the duplicate Java type names will point to the same managed
> type name.  Additionally, make sure we select the managed type in the
> same fashion the old typemap generator in `Java.Interop` worked: prefer
> base types to the derived ones if the type is an interface or an
> abstract class.  The effect of this change is that no matter which
> entry `EmbeddedAssemblies::binary_search()` ends up selecting it will
> always return the same managed type name for all aliased Java types.

The `TypeMapGenerator.HandleDebugDuplicates()` method from a017561
implicitly preferred the first TypeDefinition encountered for a given
Java type.  When the `*Invoker` came first, it attempted to treat it
as a duplicate, but wound up *removing* it entirely:

	oldEntry.TypeDefinition = td;
	oldEntry.ManagedName = GetManagedTypeName (td);

`oldEntry.TypeDefinition` would refer to `IExoPlayerInvoker`, and
setting it to `td` would instead set it to `IExoPlayer`.
`IExoPlayerInvoker` is lost.

Fix this by updating `HandleDebugDuplicates()` to be clearer about
intent:

	// oldEntry == typeof(IExoPlayerInvoker)
	// td == typeof(IExoPlayer)
	if ((td.IsAbstract || td.IsInterface) &&
			!oldEntry.TypeDefinition.IsAbstract &&
			!oldEntry.TypeDefinition.IsInterface &&
			td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
		// We found the `Invoker` type *before* the declared type
		// Fix things up so the abstract type is first, and the `Invoker` is considered a duplicate.
		duplicates.Insert (0, entry);
		oldEntry.SkipInJavaToManaged = false;
	}

Now, when the "new" TypeDefinition is:

 1. An abstract class or interface type, and
 2. The original entry is *neither* an abstract class nor interface, &
 3. The old entry can be assigned to the new entry,

then we assume that this is a `*Invoker` scenario, ensure things are
consistent with the "expected" { IExoPlayer, IExoPlayerInvoker } order:

 1. Inserting the new TypeDefinition *first* in `duplicates`.

    This ensures expected ordering.

 2. Setting `oldEntry.SkipInJavaToManaged` to `false`.

    This ensures that `IExoPlayerInvoker` is emitted in the typemaps.

***WORKAROUND***: Building the app in Release configuration avoids
the crash.  (Then cry about inner dev loop performance.)

"But what about tests?"  Originally, @jonpryor thought that
`JavaObjectExtensionsTests.JavaCast_InterfaceCast()` was a sufficient
test.  This was wrong, because it didn't test what it thought it did:

	IntPtr g;
	using (var n = new Java.Lang.Integer (42)) {
		g = JNIEnv.NewGlobalRef (n.Handle);
	}
	// We want a Java.Lang.Object so that we create an IComparableInvoker
	// instead of just getting back the original instance.
	using (var o = Java.Lang.Object.GetObject<Java.Lang.Object> (g, JniHandleOwnership.TransferGlobalRef)) {
		var c = JavaObjectExtensions.JavaCast<Java.Lang.IComparable> (o);
		c.Dispose ();
	}

The comment is not quite right: while it *says* we want a
`Java.Lang.Object`, that's not the runtime type `o.GetType()`.
The runtime type `o.GetType()` would be `Java.Lang.Integer` (!),
*because* the first thing that `TypeManager.CreateInstance()` tries
to do is see if we have an existing type mapping for the runtime type
of the JNI handle.  As the runtime type is `java.lang.Integer`, we
*do* have such a binding, *and* that bound type is implicitly
convertible to `Java.Lang.Object`.

Meaning this test never actually tested an interface cast!

So for starters, we need to fix this to something that *ensures* we
get an unbound type in the base class hierarchy, so that we can
actually test interface checking behavior.

Enter `@(AndroidJavaSource)`, `ValueProvider.java`, and
`Example.java`.  `Example.getValueProvider()` returns an anonymous
inner class that implements `ValueProvider`, ensuring that the only
bound type that will work is `Java.Lang.Object`.  We then can use
`JavaObjectExtensions.JavaCast<T>()` as originally intended.

We could have used any interface type instead of introducing a new
`ValueProvider` interface, but introducing a new type is the only way
to *attempt* to reproduce the TypeDefinition ordering issue, by
adding a new `partial` class for `IValueProviderInvoker`.

*Locally*, we get the intended "wrong" ordering of Invoker before
abstract type:

	% monodis --typedef tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/obj/Debug/net9.0-android/Mono.Android.NET-Tests.dll | grep IValueProvider
	5: Net.Dot.Android.Test.IValueProviderInvoker (flist=10, mlist=20, flags=0x100000, extends=0x89)
	7: Net.Dot.Android.Test.IValueProvider (flist=13, mlist=37, flags=0x1000a1, extends=0x0)

and the test passes as desired.

[0]: https://www.nuget.org/packages/Xamarin.AndroidX.Media3.ExoPlayer/1.4.1
[1]: https://github.com/dotnet/android-libraries/blob/3c21f4643c5dfe74e64fec54e3679baf4dc8d067/source/androidx.media3/media3-exoplayer/Additions/AndroidX.Media3.ExoPlayer.IExoPlayer.cs#L10

Author: jonpryor

src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs

@@ -77,6 +77,11 @@ internal sealed class TypeMapDebugEntry
 			public TypeDefinition TypeDefinition;
 			public bool SkipInJavaToManaged;
 			public TypeMapDebugEntry DuplicateForJavaToManaged;
+
+			public override string ToString ()
+			{
+				return $"TypeMapDebugEntry{{JavaName={JavaName}, ManagedName={ManagedName}, JavaIndex={JavaIndex}, ManagedIndex={ManagedIndex}, SkipInJavaToManaged={SkipInJavaToManaged}, DuplicateForJavaToManaged={DuplicateForJavaToManaged}}}";
+			}
 		}
 
 		// Widths include the terminating nul character but not the padding!
@@ -305,13 +310,18 @@ void HandleDebugDuplicates (Dictionary<string, List<TypeMapDebugEntry>> javaDupl
 			if (!javaDuplicates.TryGetValue (entry.JavaName, out duplicates)) {
 				javaDuplicates.Add (entry.JavaName, new List<TypeMapDebugEntry> { entry });
 			} else {
-				duplicates.Add (entry);
 				TypeMapDebugEntry oldEntry = duplicates[0];
-				if (td.IsAbstract || td.IsInterface || oldEntry.TypeDefinition.IsAbstract || oldEntry.TypeDefinition.IsInterface) {
-					if (td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
-						oldEntry.TypeDefinition = td;
-						oldEntry.ManagedName = GetManagedTypeName (td);
-					}
+				if ((td.IsAbstract || td.IsInterface) &&
+						!oldEntry.TypeDefinition.IsAbstract &&
+						!oldEntry.TypeDefinition.IsInterface &&
+						td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
+					// We found the `Invoker` type *before* the declared type
+					// Fix things up so the abstract type is first, and the `Invoker` is considered a duplicate.
+					duplicates.Insert (0, entry);
+					oldEntry.SkipInJavaToManaged = false;
+				} else {
+					// ¯\_(ツ)_/¯
+					duplicates.Add (entry);
 				}
 			}
 		}

tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs

@@ -29,13 +29,14 @@ public void JavaCast_BaseToGenericWrapper ()
 		public void JavaCast_InterfaceCast ()
 		{
 			IntPtr g;
-			using (var n = new Java.Lang.Integer (42)) {
-				g = JNIEnv.NewGlobalRef (n.Handle);
+			using (var vp = global::Net.Dot.Android.Test.Example.ValueProvider) {
+				g = JNIEnv.NewGlobalRef (vp.Handle);
 			}
-			// We want a Java.Lang.Object so that we create an IComparableInvoker
+			// We want a Java.Lang.Object so that we create an IIValueProviderInvoker
 			// instead of just getting back the original instance.
 			using (var o = Java.Lang.Object.GetObject<Java.Lang.Object> (g, JniHandleOwnership.TransferGlobalRef)) {
-				var c = JavaObjectExtensions.JavaCast<Java.Lang.IComparable> (o);
+				var c = JavaObjectExtensions.JavaCast<global::Net.Dot.Android.Test.IValueProvider> (o);
+				Assert.AreEqual (42, c.Value);
 				c.Dispose ();
 			}
 		}
@@ -111,3 +112,7 @@ public MyObject (IntPtr handle, JniHandleOwnership transfer)
 	}
 }
 
+namespace Net.Dot.Android.Test {
+	partial class IValueProviderInvoker {
+	}
+}

tests/Mono.Android-Tests/Mono.Android-Test.Shared.projitems

@@ -120,6 +120,12 @@
       <Link>Assets\subfolder\asset2.txt</Link>
     </AndroidAsset>
   </ItemGroup>
+
+  <ItemGroup>
+    <AndroidJavaSource Include="$(MSBuildThisFileDirectory)java/net/dot/android/test/ValueProvider.java" />
+    <AndroidJavaSource Include="$(MSBuildThisFileDirectory)java/net/dot/android/test/Example.java" />
+  </ItemGroup>
+
   <ItemGroup>
     <Folder Include="$(MSBuildThisFileDirectory)LinkedAssets\" />
   </ItemGroup>

tests/Mono.Android-Tests/java/net/dot/android/test/Example.java

@@ -0,0 +1,12 @@
+package net.dot.android.test;
+
+public class Example {
+	public static ValueProvider getValueProvider() {
+		return new ValueProvider() {
+			@Override
+			public int getValue() {
+				return 42;
+			}
+		};
+	}
+}

tests/Mono.Android-Tests/java/net/dot/android/test/ValueProvider.java

@@ -0,0 +1,5 @@
+package net.dot.android.test;
+
+public interface ValueProvider {
+	int getValue();
+}