From 205040b6c049ae7d42f30c67d3f4ac65a22fb77f Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Thu, 19 Jan 2023 19:04:26 -0500 Subject: [PATCH] [Java.Interop] Support Desugar + interface static methods Context: 1f27ab552d03aeb74cdc6f8985fcffbfdb9a7ddf Context: https://github.com/xamarin/xamarin-android/commit/f6f11a5a797cd8602854942dccb0f2b1db57c473 Context: https://github.com/xamarin/xamarin-android/issues/7663#issuecomment-1376324814 Commit 1f27ab55 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 1f27ab55 in concert with xamarin/xamarin-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 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 1f27ab55 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 --- .../JniPeerMembers.JniStaticMethods.cs | 34 ++++++++++---- .../Java.Interop/JniPeerMembers.cs | 2 +- .../Java.Interop-Tests.csproj | 8 ++-- .../Java.Interop-Tests.targets | 21 +++++++++ .../Java.Interop/JavaVMFixture.cs | 4 +- .../Java.Interop/JniPeerMembersTests.cs | 47 +++++++++++++++++++ .../com/xamarin/interop/AndroidInterface.java | 14 ++++++ .../interop/DesugarAndroidInterface$_CC.java | 8 ++++ 8 files changed, 120 insertions(+), 18 deletions(-) create mode 100644 tests/Java.Interop-Tests/Java.Interop-Tests.targets create mode 100644 tests/Java.Interop-Tests/java/com/xamarin/interop/AndroidInterface.java create mode 100644 tests/Java.Interop-Tests/java/com/xamarin/interop/DesugarAndroidInterface$_CC.java diff --git a/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs b/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs index d27920a58..9ce77fbe3 100644 --- a/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs +++ b/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); } }} } diff --git a/src/Java.Interop/Java.Interop/JniPeerMembers.cs b/src/Java.Interop/Java.Interop/JniPeerMembers.cs index 9e445be47..d2ffd424b 100644 --- a/src/Java.Interop/Java.Interop/JniPeerMembers.cs +++ b/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) { } diff --git a/tests/Java.Interop-Tests/Java.Interop-Tests.csproj b/tests/Java.Interop-Tests/Java.Interop-Tests.csproj index bed263368..4afa21e4e 100644 --- a/tests/Java.Interop-Tests/Java.Interop-Tests.csproj +++ b/tests/Java.Interop-Tests/Java.Interop-Tests.csproj @@ -53,14 +53,12 @@ + + - - - - - + diff --git a/tests/Java.Interop-Tests/Java.Interop-Tests.targets b/tests/Java.Interop-Tests/Java.Interop-Tests.targets new file mode 100644 index 000000000..e3910c046 --- /dev/null +++ b/tests/Java.Interop-Tests/Java.Interop-Tests.targets @@ -0,0 +1,21 @@ + + + + + + <_Source Include="@(JavaInteropTestJar->Replace('%5c', '/'))" /> + + + + + + + + diff --git a/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs b/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs index 855f52990..f289a1566 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs +++ b/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs @@ -91,8 +91,8 @@ IEnumerable 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", }; } diff --git a/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs b/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs index 788f5e61e..875a67a0b 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs +++ b/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 + // 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 } diff --git a/tests/Java.Interop-Tests/java/com/xamarin/interop/AndroidInterface.java b/tests/Java.Interop-Tests/java/com/xamarin/interop/AndroidInterface.java new file mode 100644 index 000000000..dda252a47 --- /dev/null +++ b/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"; + // } +} diff --git a/tests/Java.Interop-Tests/java/com/xamarin/interop/DesugarAndroidInterface$_CC.java b/tests/Java.Interop-Tests/java/com/xamarin/interop/DesugarAndroidInterface$_CC.java new file mode 100644 index 000000000..503814d73 --- /dev/null +++ b/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"; + } +}