-
Couldn't load subscription status.
- Fork 63
[Java.Interop] Support Desugar + interface static methods #1077
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
Merged
jonpryor
merged 1 commit into
dotnet:main
from
jonpryor:jonp-use-right-class-for-remapped-static-method-invocations
Feb 22, 2023
Merged
[Java.Interop] Support Desugar + interface static methods #1077
jonpryor
merged 1 commit into
dotnet:main
from
jonpryor:jonp-use-right-class-for-remapped-static-method-invocations
Feb 22, 2023
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ed3326e to
3b68eb2
Compare
jpobst
approved these changes
Jan 26, 2023
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Feb 16, 2023
Context: dotnet/java-interop#1077 Context: dotnet/java-interop@1f27ab5 Context: f6f11a5 [Desugaring][0] is the process of rewriting Java bytecode so that Java 8+ constructs can be used on Android pre-7.0 (API-24), as API-24 is the Android version which added native support for Java 8 features such as [interface default methods][1]. One of the implications of desugaring is that methods can "move"; consider this Java interface: package example; public interface StaticMethodsInterface { static int getValue() { return 3; } } Java.Interop bindings will attempt to invoke the `getValue().I` method on the type `example/StaticMethodsInterface`: public partial interface IStaticMethodsInterface : IJavaObject, IJavaPeerable { private static readonly JniPeerMembers _members = new XAPeerMembers ("example/StaticMethodsInterface", typeof (IStaticMethodsInterface), isInterface: true); static unsafe int Value { get { const string __id = "getValue.()I"; var __rm = _members.StaticMethods.InvokeInt32Method (__id, null); return __rm; } } } The problem is that, after Desugaring, the Java side *actually* looks like this: package example; public interface StaticMethodsInterface { } public class StaticMethodsInterface$-CC { public static int getValue() {return 3;} } Commits dotnet/java-interop@1f27ab55 and f6f11a5 added partial runtime support for this scenario via `AndroidTypeManager.GetStaticMethodFallbackTypesCore()`, which would attempt to lookup types with a `$-CC` suffix. While this was a good start, it wasn't ever actually *tested* end-to-end. Consequently, instead of *working*, this would instead cause the process to *abort*: JNI DETECTED ERROR IN APPLICATION: can't call static int example.StaticMethodsInterface$-CC.getValue() with class java.lang.Class<example.StaticMethodsInterface> in call to CallStaticIntMethodA from void crc….MainActivity.n_onCreate(android.os.Bundle) Oops. dotnet/java-interop#1077 improves our runtime support for invoking *`static`* methods on interfaces. Add a new `XASdkDeployTests.SupportDesugaringStaticInterfaceMethods()` test which performs an on-device, end-to-end invocation of a static method on a Java interface, to ensure that things *actually* work. *Note*: if `$(SupportedOSPlatformVersion)` is 24 or higher, this test will work even without dotnet/java-interop#1077, as Desugaring is *disabled* in that case. [0]: https://developer.android.com/studio/write/java8-support#library-desugaring [1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
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
3b68eb2 to
205040b
Compare
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Feb 16, 2023
Context: dotnet/java-interop#1077 Context: dotnet/java-interop@1f27ab5 Context: f6f11a5 [Desugaring][0] is the process of rewriting Java bytecode so that Java 8+ constructs can be used on Android pre-7.0 (API-24), as API-24 is the Android version which added native support for Java 8 features such as [interface default methods][1]. One of the implications of desugaring is that methods can "move"; consider this Java interface: package example; public interface StaticMethodsInterface { static int getValue() { return 3; } } Java.Interop bindings will attempt to invoke the `getValue().I` method on the type `example/StaticMethodsInterface`: public partial interface IStaticMethodsInterface : IJavaObject, IJavaPeerable { private static readonly JniPeerMembers _members = new XAPeerMembers ("example/StaticMethodsInterface", typeof (IStaticMethodsInterface), isInterface: true); static unsafe int Value { get { const string __id = "getValue.()I"; var __rm = _members.StaticMethods.InvokeInt32Method (__id, null); return __rm; } } } The problem is that, after Desugaring, the Java side *actually* looks like this: package example; public interface StaticMethodsInterface { } public class StaticMethodsInterface$-CC { public static int getValue() {return 3;} } Commits dotnet/java-interop@1f27ab55 and f6f11a5 added partial runtime support for this scenario via `AndroidTypeManager.GetStaticMethodFallbackTypesCore()`, which would attempt to lookup types with a `$-CC` suffix. While this was a good start, it wasn't ever actually *tested* end-to-end. Consequently, instead of *working*, this would instead cause the process to *abort*: JNI DETECTED ERROR IN APPLICATION: can't call static int example.StaticMethodsInterface$-CC.getValue() with class java.lang.Class<example.StaticMethodsInterface> in call to CallStaticIntMethodA from void crc….MainActivity.n_onCreate(android.os.Bundle) Oops. dotnet/java-interop#1077 improves our runtime support for invoking *`static`* methods on interfaces. Add a new `XASdkDeployTests.SupportDesugaringStaticInterfaceMethods()` test which performs an on-device, end-to-end invocation of a static method on a Java interface, to ensure that things *actually* work. *Note*: if `$(SupportedOSPlatformVersion)` is 24 or higher, this test will work even without dotnet/java-interop#1077, as Desugaring is *disabled* in that case. [0]: https://developer.android.com/studio/write/java8-support#library-desugaring [1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Feb 16, 2023
Context: dotnet/java-interop#1077 Context: dotnet/java-interop@1f27ab5 Context: f6f11a5 [Desugaring][0] is the process of rewriting Java bytecode so that Java 8+ constructs can be used on Android pre-7.0 (API-24), as API-24 is the Android version which added native support for Java 8 features such as [interface default methods][1]. One of the implications of desugaring is that methods can "move"; consider this Java interface: package example; public interface StaticMethodsInterface { static int getValue() { return 3; } } Java.Interop bindings will attempt to invoke the `getValue().I` method on the type `example/StaticMethodsInterface`: public partial interface IStaticMethodsInterface : IJavaObject, IJavaPeerable { private static readonly JniPeerMembers _members = new XAPeerMembers ("example/StaticMethodsInterface", typeof (IStaticMethodsInterface), isInterface: true); static unsafe int Value { get { const string __id = "getValue.()I"; var __rm = _members.StaticMethods.InvokeInt32Method (__id, null); return __rm; } } } The problem is that, after Desugaring, the Java side *actually* looks like this: package example; public interface StaticMethodsInterface { } public class StaticMethodsInterface$-CC { public static int getValue() {return 3;} } Commits dotnet/java-interop@1f27ab55 and f6f11a5 added partial runtime support for this scenario via `AndroidTypeManager.GetStaticMethodFallbackTypesCore()`, which would attempt to lookup types with a `$-CC` suffix. While this was a good start, it wasn't ever actually *tested* end-to-end. Consequently, instead of *working*, this would instead cause the process to *abort*: JNI DETECTED ERROR IN APPLICATION: can't call static int example.StaticMethodsInterface$-CC.getValue() with class java.lang.Class<example.StaticMethodsInterface> in call to CallStaticIntMethodA from void crc….MainActivity.n_onCreate(android.os.Bundle) Oops. dotnet/java-interop#1077 improves our runtime support for invoking *`static`* methods on interfaces. Add a new `XASdkDeployTests.SupportDesugaringStaticInterfaceMethods()` test which performs an on-device, end-to-end invocation of a static method on a Java interface, to ensure that things *actually* work. *Note*: if `$(SupportedOSPlatformVersion)` is 24 or higher, this test will work even without dotnet/java-interop#1077, as Desugaring is *disabled* in that case. [0]: https://developer.android.com/studio/write/java8-support#library-desugaring [1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Feb 17, 2023
Context: dotnet/java-interop#1077 Context: dotnet/java-interop@1f27ab5 Context: f6f11a5 [Desugaring][0] is the process of rewriting Java bytecode so that Java 8+ constructs can be used on Android pre-7.0 (API-24), as API-24 is the Android version which added native support for Java 8 features such as [interface default methods][1]. One of the implications of desugaring is that methods can "move"; consider this Java interface: package example; public interface StaticMethodsInterface { static int getValue() { return 3; } } Java.Interop bindings will attempt to invoke the `getValue().I` method on the type `example/StaticMethodsInterface`: public partial interface IStaticMethodsInterface : IJavaObject, IJavaPeerable { private static readonly JniPeerMembers _members = new XAPeerMembers ("example/StaticMethodsInterface", typeof (IStaticMethodsInterface), isInterface: true); static unsafe int Value { get { const string __id = "getValue.()I"; var __rm = _members.StaticMethods.InvokeInt32Method (__id, null); return __rm; } } } The problem is that, after Desugaring, the Java side *actually* looks like this: package example; public interface StaticMethodsInterface { } public class StaticMethodsInterface$-CC { public static int getValue() {return 3;} } Commits dotnet/java-interop@1f27ab55 and f6f11a5 added partial runtime support for this scenario via `AndroidTypeManager.GetStaticMethodFallbackTypesCore()`, which would attempt to lookup types with a `$-CC` suffix. While this was a good start, it wasn't ever actually *tested* end-to-end. Consequently, instead of *working*, this would instead cause the process to *abort*: JNI DETECTED ERROR IN APPLICATION: can't call static int example.StaticMethodsInterface$-CC.getValue() with class java.lang.Class<example.StaticMethodsInterface> in call to CallStaticIntMethodA from void crc….MainActivity.n_onCreate(android.os.Bundle) Oops. dotnet/java-interop#1077 improves our runtime support for invoking *`static`* methods on interfaces. Add a new `XASdkDeployTests.SupportDesugaringStaticInterfaceMethods()` test which performs an on-device, end-to-end invocation of a static method on a Java interface, to ensure that things *actually* work. *Note*: if `$(SupportedOSPlatformVersion)` is 24 or higher, this test will work even without dotnet/java-interop#1077, as Desugaring is *disabled* in that case. [0]: https://developer.android.com/studio/write/java8-support#library-desugaring [1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Feb 17, 2023
Context: dotnet/java-interop#1077 Context: dotnet/java-interop@1f27ab5 Context: f6f11a5 [Desugaring][0] is the process of rewriting Java bytecode so that Java 8+ constructs can be used on Android pre-7.0 (API-24), as API-24 is the Android version which added native support for Java 8 features such as [interface default methods][1]. One of the implications of desugaring is that methods can "move"; consider this Java interface: package example; public interface StaticMethodsInterface { static int getValue() { return 3; } } Java.Interop bindings will attempt to invoke the `getValue().I` method on the type `example/StaticMethodsInterface`: public partial interface IStaticMethodsInterface : IJavaObject, IJavaPeerable { private static readonly JniPeerMembers _members = new XAPeerMembers ("example/StaticMethodsInterface", typeof (IStaticMethodsInterface), isInterface: true); static unsafe int Value { get { const string __id = "getValue.()I"; var __rm = _members.StaticMethods.InvokeInt32Method (__id, null); return __rm; } } } The problem is that, after Desugaring, the Java side *actually* looks like this: package example; public interface StaticMethodsInterface { } public class StaticMethodsInterface$-CC { public static int getValue() {return 3;} } Commits dotnet/java-interop@1f27ab55 and f6f11a5 added partial runtime support for this scenario via `AndroidTypeManager.GetStaticMethodFallbackTypesCore()`, which would attempt to lookup types with a `$-CC` suffix. While this was a good start, it wasn't ever actually *tested* end-to-end. Consequently, instead of *working*, this would instead cause the process to *abort*: JNI DETECTED ERROR IN APPLICATION: can't call static int example.StaticMethodsInterface$-CC.getValue() with class java.lang.Class<example.StaticMethodsInterface> in call to CallStaticIntMethodA from void crc….MainActivity.n_onCreate(android.os.Bundle) Oops. dotnet/java-interop#1077 improves our runtime support for invoking *`static`* methods on interfaces. Add a new `XASdkDeployTests.SupportDesugaringStaticInterfaceMethods()` test which performs an on-device, end-to-end invocation of a static method on a Java interface, to ensure that things *actually* work. *Note*: if `$(SupportedOSPlatformVersion)` is 24 or higher, this test will work even without dotnet/java-interop#1077, as Desugaring is *disabled* in that case. [0]: https://developer.android.com/studio/write/java8-support#library-desugaring [1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Feb 22, 2023
Changes: dotnet/java-interop@9e0a469...bbaeda6 * dotnet/java-interop@bbaeda6f: [Java.Interop] Support Desugar + interface static methods (dotnet/java-interop#1077)
jonpryor
added a commit
to dotnet/android
that referenced
this pull request
Feb 23, 2023
Changes: dotnet/java-interop@9e0a469...bbaeda6 * dotnet/java-interop@bbaeda6f: [Java.Interop] Support Desugar + interface static methods (dotnet/java-interop#1077) Context: dotnet/java-interop@bbaeda6 Context: dotnet/java-interop@1f27ab5 Context: f6f11a5 [Desugaring][0] is the process of rewriting Java bytecode so that Java 8+ constructs can be used on Android pre-7.0 (API-24), as API-24 is the Android version which added native support for Java 8 features such as [interface default methods][1]. One of the implications of desugaring is that methods can "move"; consider this Java interface: package example; public interface StaticMethodsInterface { static int getValue() { return 3; } } Java.Interop bindings will attempt to invoke the `getValue().I` method on the type `example/StaticMethodsInterface`: public partial interface IStaticMethodsInterface : IJavaObject, IJavaPeerable { private static readonly JniPeerMembers _members = new XAPeerMembers ("example/StaticMethodsInterface", typeof (IStaticMethodsInterface), isInterface: true); static unsafe int Value { get { const string __id = "getValue.()I"; var __rm = _members.StaticMethods.InvokeInt32Method (__id, null); return __rm; } } } The problem is that, after Desugaring, the Java side *actually* looks like this: package example; public interface StaticMethodsInterface { } public class StaticMethodsInterface$-CC { public static int getValue() {return 3;} } Commits dotnet/java-interop@1f27ab55 and f6f11a5 added partial runtime support for this scenario via `AndroidTypeManager.GetStaticMethodFallbackTypesCore()`, which would attempt to lookup types with a `$-CC` suffix. While this was a good start, it wasn't ever actually *tested* end-to-end. Consequently, instead of *working*, this would instead cause the process to *abort*: JNI DETECTED ERROR IN APPLICATION: can't call static int example.StaticMethodsInterface$-CC.getValue() with class java.lang.Class<example.StaticMethodsInterface> in call to CallStaticIntMethodA from void crc….MainActivity.n_onCreate(android.os.Bundle) Oops. dotnet/java-interop@bbaeda6f improves our runtime support for invoking *`static`* methods on interfaces. Add a new `XASdkDeployTests.SupportDesugaringStaticInterfaceMethods()` test which performs an on-device, end-to-end invocation of a static method on a Java interface, to ensure that things *actually* work. *Note*: if `$(SupportedOSPlatformVersion)` is 24 or higher, this test will work even without dotnet/java-interop#1077, as Desugaring is *disabled* in that case. The test `JniPeerMembersTests.DesugarInterfaceStaticMethod()` added in dotnet/java-interop@bbaeda6f attempts to "fake" a Desugar environment for testing on Desktop Java, and this "fakery" doesn't work in the Android environment. Fix execution on Android by updating `AndroidRuntime.GetStaticMethodFallbackTypesCore()` to support type remapping (f6f11a5) -- which was overlooked/not considered -- such that the types returned are *after* calling `AndroidRuntime.GetReplacementTypeCore()`, which looks up `<replace-type/>` values. This allows us to remap `AndroidInterface` to `DesugarAndroidInterface$_CC`, allowing the `DesugarInterfaceStaticMethod()` test to pass. Update the `BuildTestJarFile` target so that it properly builds files that contain `$` on Unixy platforms. [0]: https://developer.android.com/studio/write/java8-support#library-desugaring [1]: https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: 1f27ab5
Context: dotnet/android@f6f11a5
Context: dotnet/android#7663 (comment)
Commit 1f27ab5 added partial support for desugaring, which
rewrites Java bytecode so that default interface methods and
static methods in interfaces 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(), allowingGetMethodInfo()toinstead resolve the static method from the fallback type, allowing
the static method invocation to work.
What actually happened? Not enough testing happened, such that
when it was actually attempted, it blew up bigly:
Oops.
What went wrong?
The problem is that
JNIEnv::CallStatic*Method()requires thatwe provide the
jclass clazzvalue for the type that declares thestatic method, but we're providing the wrong class!
Specifically, consider this Java interface:
In a desugared environment, this is transformed into the pair of
types:
GetStaticMethodFallbackTypes("StaticMethodsInterface")returnsStaticMethodsInterface$-CC, allowingGetMethodInfo()to lookupStaticMethodsInterface$-CCand resolve the methodgetValue.()I. However, whenJniPeerMembers.JniStaticMethods.Invoke*Method()is later invoked,the
jclassvalue forStaticMethodsInterfaceis provided, notthe value for
StaticMethodsInterface$-CC. It's as if we do:The problem is that we need to use
to.PeerReference, notfrom.PeerReference!Fix
GetMethodInfo()so that when a method is resolved from afallback type, the type instance is stored in the
JniMethodInfo.StaticRedirectfield, and then update theInvoke*Method()methods so thatJniMethodInfo.StaticRedirectispassed to
JNIEnv::CallStatic*Method()if it is set, beforedefaulting to
JniPeerMembers.JniPeerType."Optimization opportunity": the approach taken does not attempt to
cache the
JniTypeinstances which correspond to the fallback types.Consequently, it is possible that multiple GREFs could be consumed
for the
JniMethodInfo.StaticRedirectinstances, as each instancewill 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)constructorso that it participates in type remapping. Without this change,
interface types cannot be renamed by the 1f27ab5 infrastructure.