[Java.Interop] Support Desugar + interface static methods (#1077)

Context: 1f27ab5
Context: dotnet/android@f6f11a5
Context: dotnet/android#7663 (comment)

Commit 1f27ab5 added partial support for [desugaring][0], which
rewrites Java bytecode so that [default interface methods][1] and
[static methods in interfaces][2] can be supported on pre-Android 7.0
(API-24) devices, as the pre-API-24 ART runtime does not directly
support those bytecode constructs.

The hope was that commit 1f27ab5 in concert with
dotnet/android@f6f11a5a would allow static methods on
interfaces to work, by having
`Java.Interop.JniPeerMembers.JniStaticMethods.GetMethodInfo()`
call `JniRuntime.JniTypeManager.GetStaticMethodFallbackTypes()`.
xamarin/xamarin-android would then override
`GetStaticMethodFallbackTypes()`, allowing `GetMethodInfo()` to
instead resolve the static method from the fallback type, allowing
the static method invocation to work.

> TODO: Update xamarin-android to override
> `GetStaticMethodFallbackTypes()`, to return
>  `$"{jniSimpleReference}$-CC"`.

What *actually* happened?  Not enough testing happened, such that
when it was *actually* attempted, it blew up bigly:

	JNI DETECTED ERROR IN APPLICATION: can't call static int com.xamarin.android.StaticMethodsInterface$-CC.getValue() with class java.lang.Class<com.xamarin.android.StaticMethodsInterface>
	    in call to CallStaticIntMethodA
	    from void crc641149b9fe658fbe8e.MainActivity.n_onCreate(android.os.Bundle)

Oops.

What went wrong?

The problem is that [`JNIEnv::CallStatic*Method()`][3] requires that
we provide the `jclass clazz` value for the type that declares the
static method, but we're providing the wrong class!

Specifically, consider this Java interface:

	public interface StaticMethodsInterface {
	    static int getValue() {
	        return 3;
	    }
	}

In a desugared environment, this is transformed into the *pair* of
types:

	public interface StaticMethodsInterface {
	}
	public class StaticMethodsInterface$-CC {
	    public static int getValue() {
	        return 3;
	    }
	}

`GetStaticMethodFallbackTypes("StaticMethodsInterface")` returns
`StaticMethodsInterface$-CC`, allowing `GetMethodInfo()` to lookup
`StaticMethodsInterface$-CC` and resolve the method
`getValue.()I`.  However, when
`JniPeerMembers.JniStaticMethods.Invoke*Method()` is later invoked,
the `jclass` value for `StaticMethodsInterface` is provided, *not*
the value for `StaticMethodsInterface$-CC`.  It's as if we do:

	var from = new Java.Interop.JniType ("com/xamarin/android/StaticMethodsInterface");
	var to   = new Java.Interop.JniType ("com/xamarin/android/StaticMethodsInterface$-CC");
	var m    = to.GetStaticMethod ("getValue", "()I");
	var r    = Java.Interop.JniEnvironment.StaticMethods.CallStaticIntMethod (from.PeerReference, m);

The problem is that we need to use `to.PeerReference`, *not*
`from.PeerReference`!

Fix `GetMethodInfo()` so that when a method is resolved from a
fallback type, the type instance is stored in the
`JniMethodInfo.StaticRedirect` field, and then update the
`Invoke*Method()` methods so that `JniMethodInfo.StaticRedirect` is
passed to `JNIEnv::CallStatic*Method()` if it is set, before
defaulting to `JniPeerMembers.JniPeerType`.

"Optimization opportunity": the approach taken does not attempt to
cache the `JniType` instances which correspond to the fallback types.
Consequently, it is possible that multiple GREFs could be consumed
for the `JniMethodInfo.StaticRedirect` instances, as each instance
will be unique.  This was done for expediency, and because @jonpryor
doesn't know if this will be an actual problem in practice.

Unrelatedly, fix the `JniPeerMembers(string, Type, bool)` constructor
so that it participates in type remapping.  Without this change,
interface types cannot be renamed by the 1f27ab5 infrastructure.

[0]: https://developer.android.com/studio/write/java8-support#library-desugaring
[1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
[2]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html#static
[3]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#CallStatic_type_Method_routines

Author: jonpryor

src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs

@@ -66,6 +66,18 @@ JniMethodInfo GetMethodInfo (string method, string signature)
 			return Members.JniPeerType.GetStaticMethod (method, signature);
 		}
 
+#pragma warning disable CA1801
+		JniType GetMethodDeclaringType (JniMethodInfo method)
+		{
+#if NET
+			if (method.StaticRedirect != null) {
+				return method.StaticRedirect;
+			}
+#endif  // NET
+			return Members.JniPeerType;
+		}
+#pragma warning restore CA1801
+
 #if NET
 		JniMethodInfo? FindInFallbackTypes (string method, string signature)
 		{
@@ -80,6 +92,8 @@ JniMethodInfo GetMethodInfo (string method, string signature)
 						continue;
 					}
 					if (t.TryGetStaticMethod (method, signature, out var m)) {
+						m.StaticRedirect    = t;
+						t                   = null;
 						return m;
 					}
 				}
@@ -94,61 +108,61 @@ JniMethodInfo GetMethodInfo (string method, string signature)
 		public unsafe void InvokeVoidMethod (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			JniEnvironment.StaticMethods.CallStaticVoidMethod (Members.JniPeerType.PeerReference, m, parameters);
+			JniEnvironment.StaticMethods.CallStaticVoidMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe bool InvokeBooleanMethod (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticBooleanMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticBooleanMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe sbyte InvokeSByteMethod (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticByteMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticByteMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe char InvokeCharMethod (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticCharMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticCharMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe short InvokeInt16Method (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticShortMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticShortMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe int InvokeInt32Method (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticIntMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticIntMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe long InvokeInt64Method (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticLongMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticLongMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe float InvokeSingleMethod (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticFloatMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticFloatMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe double InvokeDoubleMethod (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticDoubleMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticDoubleMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 
 		public unsafe JniObjectReference InvokeObjectMethod (string encodedMember, JniArgumentValue* parameters)
 		{
 			var m = GetMethodInfo (encodedMember);
-			return JniEnvironment.StaticMethods.CallStaticObjectMethod (Members.JniPeerType.PeerReference, m, parameters);
+			return JniEnvironment.StaticMethods.CallStaticObjectMethod (GetMethodDeclaringType (m).PeerReference, m, parameters);
 		}
 	}}
 }

src/Java.Interop/Java.Interop/JniPeerMembers.cs

@@ -12,7 +12,7 @@ public partial class JniPeerMembers {
 		private bool isInterface;
 
 		public JniPeerMembers (string jniPeerTypeName, Type managedPeerType, bool isInterface)
-			: this (jniPeerTypeName, managedPeerType, checkManagedPeerType: true, isInterface: isInterface)
+			: this (jniPeerTypeName = GetReplacementType (jniPeerTypeName), managedPeerType, checkManagedPeerType: true, isInterface: isInterface)
 		{
 		}
 

tests/Java.Interop-Tests/Java.Interop-Tests.csproj

@@ -53,14 +53,12 @@
     <JavaInteropTestJar Include="$(MSBuildThisFileDirectory)java\com\xamarin\interop\RenameClassBase1.java" />
     <JavaInteropTestJar Include="$(MSBuildThisFileDirectory)java\com\xamarin\interop\RenameClassBase2.java" />
     <JavaInteropTestJar Include="$(MSBuildThisFileDirectory)java\com\xamarin\interop\RenameClassDerived.java" />
+    <JavaInteropTestJar Include="$(MSBuildThisFileDirectory)java\com\xamarin\interop\AndroidInterface.java" />
+    <JavaInteropTestJar Include="$(MSBuildThisFileDirectory)java\com\xamarin\interop\DesugarAndroidInterface$_CC.java" />
     <JavaInteropTestJar Include="$(MSBuildThisFileDirectory)java\com\xamarin\interop\SelfRegistration.java" />
     <JavaInteropTestJar Include="$(MSBuildThisFileDirectory)java\com\xamarin\interop\TestType.java" />
   </ItemGroup>
 
-  <Target Name="BuildInteropTestJar" BeforeTargets="Build" Inputs="@(JavaInteropTestJar)" Outputs="$(OutputPath)interop-test.jar">
-    <MakeDir Directories="$(IntermediateOutputPath)it-classes" />
-    <Exec Command="&quot;$(JavaCPath)&quot; $(_JavacSourceOptions) -d &quot;$(IntermediateOutputPath)it-classes&quot; -classpath &quot;$(OutputPath)../$(Configuration)/java-interop.jar&quot; @(JavaInteropTestJar->'%(Identity)', ' ')" />
-    <Exec Command="&quot;$(JarPath)&quot; cf &quot;$(OutputPath)interop-test.jar&quot; -C &quot;$(IntermediateOutputPath)it-classes&quot; ." />
-  </Target>
+  <Import Project="Java.Interop-Tests.targets" />
 
 </Project>

tests/Java.Interop-Tests/Java.Interop-Tests.targets

@@ -0,0 +1,21 @@
+<Project>
+
+  <Target Name="BuildInteropTestJar"
+      BeforeTargets="Build"
+      Inputs="@(JavaInteropTestJar)"
+      Outputs="$(OutputPath)interop-test.jar">
+    <MakeDir Directories="$(IntermediateOutputPath)it-classes" />
+    <ItemGroup>
+      <_Source Include="@(JavaInteropTestJar->Replace('%5c', '/'))" />
+    </ItemGroup>
+    <WriteLinesToFile
+        File="$(IntermediateOutputPath)_java_sources.txt"
+        Lines="@(_Source)"
+        Overwrite="True"
+    />
+    <Exec Command="&quot;$(JavaCPath)&quot; $(_JavacSourceOptions) -d &quot;$(IntermediateOutputPath)it-classes&quot; -classpath &quot;$(OutputPath)../$(Configuration)/java-interop.jar&quot; &quot;@$(IntermediateOutputPath)_java_sources.txt&quot;" />
+    <Delete Files="$(IntermediateOutputPath)_java_sources.txt" />
+    <Exec Command="&quot;$(JarPath)&quot; cf &quot;$(OutputPath)interop-test.jar&quot; -C &quot;$(IntermediateOutputPath)it-classes&quot; ." />
+  </Target>
+
+</Project>

tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs

@@ -91,8 +91,8 @@ IEnumerable<string> CreateSimpleReferencesEnumerator (Type type)
 			// "potentially non-existent" types ensures that we don't throw
 			// from places we don't want to internally throw.
 			return new[]{
-				desugarType,
-				$"{jniSimpleReference}$-CC"
+				$"{desugarType}$_CC",           // For JniPeerMembersTests.DesugarInterfaceStaticMethod()
+				$"{jniSimpleReference}$-CC",
 			};
 		}
 

tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs

@@ -108,6 +108,38 @@ public void ReplaceInstanceMethodWithStaticMethod ()
 			// Shouldn't throw; should instead invoke ObjectHelper.getHashCodeHelper(Object)
 			o.remappedToStaticHashCode ();
 		}
+
+#if !__ANDROID__
+		// Note: this test looks up a static method from one class, then
+		// calls `JNIEnv::CallStaticObjectMethod()` passing in a jclass
+		// for a *different* class.
+		//
+		// This appears to work on Desktop JVM.
+		//
+		// On Android, this will ABORT the app:
+		//  JNI DETECTED ERROR IN APPLICATION: can't call static int com.xamarin.interop.DesugarAndroidInterface$_CC.getClassName() with class java.lang.Class<com.xamarin.interop.AndroidInterface>
+		//      in call to CallStaticObjectMethodA
+		//
+		// *Fascinating* the differences that can appear between JVM implementations
+		[Test]
+		public unsafe void DoesTheJmethodNeedToMatchDeclaringType ()
+		{
+			var iface   = new JniType ("com/xamarin/interop/AndroidInterface");
+			var desugar = new JniType ("com/xamarin/interop/DesugarAndroidInterface$_CC");
+			var m       = desugar.GetStaticMethod ("getClassName", "()Ljava/lang/String;");
+
+			var r = JniEnvironment.StaticMethods.CallStaticObjectMethod (iface.PeerReference, m, null);
+			var s = JniEnvironment.Strings.ToString (ref r, JniObjectReferenceOptions.CopyAndDispose);
+			Assert.AreEqual ("DesugarAndroidInterface$-CC", s);
+		}
+#endif  // !__ANDROID__
+
+		[Test]
+		public void DesugarInterfaceStaticMethod ()
+		{
+			var s = IAndroidInterface.getClassName ();
+			Assert.AreEqual ("DesugarAndroidInterface$-CC", s);
+		}
 #endif  // NET
 	}
 
@@ -208,4 +240,19 @@ public override unsafe int hashCode ()
 			return base.hashCode ();
 		}
 	}
+
+#if NET
+	[JniTypeSignature (JniTypeName)]
+	interface IAndroidInterface : IJavaPeerable {
+		internal            const       string          JniTypeName    = "com/xamarin/interop/AndroidInterface";
+
+		private static JniPeerMembers _members = new JniPeerMembers (JniTypeName, typeof (IAndroidInterface), isInterface: true);
+
+		public static unsafe string getClassName ()
+		{
+			var s = _members.StaticMethods.InvokeObjectMethod ("getClassName.()Ljava/lang/String;", null);
+			return JniEnvironment.Strings.ToString (ref s, JniObjectReferenceOptions.CopyAndDispose);
+		}
+	}
+#endif  // NET
 }

tests/Java.Interop-Tests/java/com/xamarin/interop/AndroidInterface.java

@@ -0,0 +1,14 @@
+package com.xamarin.interop;
+
+// When Android Desugaring is enabled -- the default when targeting API-25 and earlier --
+// certain Java constructs result in Java bytecode rewriting.
+// Interface static methods are *moved* into $-CC types.
+public interface AndroidInterface {
+
+	// When Desugaring is enabled, this is moved to `AndroidInterface$-CC.getClassName()`,
+	// and the original `AndroidInterface.getClassName()` *no longer exists*.
+
+	// public static String getClassName() {
+	// 	return "AndroidInterface";
+	// }
+}

tests/Java.Interop-Tests/java/com/xamarin/interop/DesugarAndroidInterface$_CC.java

@@ -0,0 +1,8 @@
+package com.xamarin.interop;
+
+public class DesugarAndroidInterface$_CC {
+
+	public static String getClassName() {
+		return "DesugarAndroidInterface$-CC";
+	}
+}