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

Bump to xamarin/Java.Interop/main@bbaeda6f #7799

Merged
merged 3 commits into from
Feb 23, 2023

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Feb 16, 2023

Changes: dotnet/java-interop@9e0a469...bbaeda6

Context: dotnet/java-interop@bbaeda6
Context: dotnet/java-interop@1f27ab5
Context: f6f11a5

Desugaring 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.

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.

@jonpryor jonpryor force-pushed the jonp-try-ji-1077 branch 2 times, most recently from 55f8bf0 to 7f633b5 Compare February 17, 2023 16:57
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
Copy link
Member Author

"interestingly", the new xamarin-android SupportDesugaringStaticInterfaceMethods test passes, while the Java.Interop test DesugarInterfaceStaticMethod() test fails:

Java.Lang.NoSuchMethodError : no static method "Lcom/xamarin/interop/AndroidInterface;.getClassName()Ljava/lang/String;"
   at Java.Interop.JniEnvironment.StaticMethods.GetStaticMethodID(JniObjectReference , String , String )
   at Java.Interop.JniType.GetStaticMethod(String , String )
   at Java.Interop.JniPeerMembers.JniStaticMethods.GetMethodInfo(String , String )
   at Java.Interop.JniPeerMembers.JniStaticMethods.GetMethodInfo(String )
   at Java.Interop.JniPeerMembers.JniStaticMethods.InvokeObjectMethod(String , JniArgumentValue* )
   at Java.InteropTests.IAndroidInterface.getClassName()
   at Java.InteropTests.JniPeerMembersTests.DesugarInterfaceStaticMethod()
   at System.Reflection.MethodInvoker.InterpretedInvoke(Object , IntPtr* )
   at System.Reflection.MethodInvoker.Invoke(Object , IntPtr* , BindingFlags )
  --- End of managed Java.Lang.NoSuchMethodError stack trace ---
java.lang.NoSuchMethodError: no static method "Lcom/xamarin/interop/AndroidInterface;.getClassName()Ljava/lang/String;"
	at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
	at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:27)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)

$(SupportedOSPlatformVersion) for Mono.Android.NET-Tests.csproj is 21, which should enable real Desugar, so… what's wrong?

@jonpryor
Copy link
Member Author

jonpryor commented Feb 21, 2023

following up

If I look at base/dex/classes.dex within Mono.Android.NET_Tests-Signed.aab, I see:

% dexdump base/dex/classes.dex| grep 'Class desc.*AndroidInter'
  Class descriptor  : 'Lcom/xamarin/interop/AndroidInterface;'
  Class descriptor  : 'Lcom/xamarin/interop/DesugarAndroidInterface$_CC;'

i.e. DesugarAndroidInterface$_CC is included, but not DesugarAndroidInterface$-CC (note: _ vs. -).

This is because AndroidInterface is wrong for "Android with Desugar"; we need to instead have com.xamarin.android.AndroidInterface contain a static method so that Desugar has something to do.

It's thus somewhat disturbing that SupportDesugaringStaticInterfaceMethods passes at all!


Edited to add: SupportDesugaringStaticInterfaceMethods passes because it doesn't invoke com.xamarin.android.AndroidInterface, but instead example.StaticMethodsInterface.java, which is desugared.

This leaves two plausible ways to fix DesugarInterfaceStaticMethod:

  1. Use <TestJarEntry Remove="…\AndroidInterface*.java" /> and provide an Android implementation of AndroidInterface.java, one which will actually work with Android desugaring, or
  2. Update Remaps.xml to remap DesugarAndroidInterface$-CC to DesugarAndroidInterface$_CC.

(1) feels duplicative of SupportDesugaringStaticInterfaceMethods(), while (2) would make for an interesting additional test…


Edited to add: (2) makes for a somewhat interesting conundrum, as AndroidRuntime.GetStaticMethodFallbackTypesCore() doesn't currently support <replace-type/> (oops?), meaning that even if we added a type remapping from AndroidInterface$-CC to DesugarAndroidInterface$_CC, it wouldn't mean anything.

🤔

I guess adding type remap logic to AndroidRuntime.GetStaticMethodFallbackTypesCore() should work?

Context: dotnet#7799 (comment)

`DesugarInterfaceStaticMethod()` (from Java.Interop) fails:

	Java.Lang.NoSuchMethodError : no static method "Lcom/xamarin/interop/AndroidInterface;.getClassName()Ljava/lang/String;"
	   at Java.Interop.JniEnvironment.StaticMethods.GetStaticMethodID(JniObjectReference , String , String )
	   at Java.Interop.JniType.GetStaticMethod(String , String )
	   at Java.Interop.JniPeerMembers.JniStaticMethods.GetMethodInfo(String , String )
	   at Java.Interop.JniPeerMembers.JniStaticMethods.GetMethodInfo(String )
	   at Java.Interop.JniPeerMembers.JniStaticMethods.InvokeObjectMethod(String , JniArgumentValue* )
	   at Java.InteropTests.IAndroidInterface.getClassName()
	   at Java.InteropTests.JniPeerMembersTests.DesugarInterfaceStaticMethod()
	   at System.Reflection.MethodInvoker.InterpretedInvoke(Object , IntPtr* )
	   at System.Reflection.MethodInvoker.Invoke(Object , IntPtr* , BindingFlags )
	  --- End of managed Java.Lang.NoSuchMethodError stack trace ---
	java.lang.NoSuchMethodError: no static method "Lcom/xamarin/interop/AndroidInterface;.getClassName()Ljava/lang/String;"
		at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
		at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:27)
		at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)

This is because `AndroidInterface` isn't desugared, so there is no
`AndroidInterface$-CC` type.

`AndroidInterface` isn't desugared, because it's *empty*.  There are
thus two ways to make `DesugarInterfaceStaticMethod()` work:

 1. Provide an Android-specific `AndroidInterface` type, which contains
    a `static` interface method.  This will cause the type to be
    desugared, creating the `AndroidInterface$-CC` type, and the test
    should thus work.

 2. Use type remapping (f6f11a5) to remap `AndroidInterface$-CC` to
    `DesugarAndroidInterface$_CC`.

(1) is (somewhat?) "duplicative" of the new test
`SupportDesugaringStaticInterfaceMethods()`.  We could do that, but…
¯\_(ツ)_/¯

Which leaves (2), which is "more interesting".  It's more interesting
in part because it also presents a problem!
`AndroidRuntime.GetStaticMethodFallbackTypesCore()` doesn't support/use
type remapping!  Meaning that even if we have a `<replace-type/>`
to map `AndroidInterface` to `DesugarAndroidInterface$_CC`, that
remapping won't be used!

🧐

Update `AndroidRuntime.GetStaticMethodFallbackTypesCore()` so that
the returned types are *after* calling
`AndroidRuntime.GetReplacementTypeCore()`, which looks up
`<replace-type/>` values.  This (should?) allow use to remap
`AndroidInterface` with `DesugarAndroidInterface$_CC`, allowing the
`DesugarInterfaceStaticMethod()` test to pass. 🤞
@jonpryor jonpryor changed the title Try xamarin/java.interop#1077 Bump to xamarin/Java.Interop/main@bbaeda6f Feb 22, 2023
@jonpryor
Copy link
Member Author

Grand unified commit message:

Changes: https://github.com/xamarin/java.interop/compare/9e0a4690adb71c04cd88a5b54bea3c3f8da73cc0...bbaeda6f698369bdd48bfc2dd1a32a41c9d23229
    
  * xamarin/java.interop@bbaeda6f: [Java.Interop] Support Desugar + interface static methods (xamarin/java.interop#1077)

Context: https://github.com/xamarin/java.interop/commit/bbaeda6f698369bdd48bfc2dd1a32a41c9d23229
Context: https://github.com/xamarin/java.interop/commit/1f27ab552d03aeb74cdc6f8985fcffbfdb9a7ddf
Context: f6f11a5a797cd8602854942dccb0f2b1db57c473

[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 xamarin/java.interop@1f27ab55 and f6f11a5a 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.

xamarin/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 xamarin/java.interop#1077, as Desugaring is
*disabled* in that case.

The test `JniPeerMembersTests.DesugarInterfaceStaticMethod()` added
in xamarin/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 (f6f11a5a) -- 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

@jonpryor jonpryor merged commit 77678eb into dotnet:main Feb 23, 2023
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 23, 2023
* main:
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
  Localized file check-in by OneLocBuild Task (dotnet#7820)
  Add Unit Test for testOnly apps (dotnet#7637)
  [build] Only build the latest API level (dotnet#7786)
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 24, 2023
* main:
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
  Localized file check-in by OneLocBuild Task (dotnet#7820)
  Add Unit Test for testOnly apps (dotnet#7637)
  [build] Only build the latest API level (dotnet#7786)
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 24, 2023
* main:
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
  Localized file check-in by OneLocBuild Task (dotnet#7820)
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 24, 2023
* main:
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
  Localized file check-in by OneLocBuild Task (dotnet#7820)
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 24, 2023
* main:
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
  Localized file check-in by OneLocBuild Task (dotnet#7820)
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 24, 2023
* main:
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
  Localized file check-in by OneLocBuild Task (dotnet#7820)
grendello added a commit to grendello/xamarin-android that referenced this pull request Feb 27, 2023
* main:
  Localized file check-in by OneLocBuild (dotnet#7827)
  Revert "Bump to dotnet/installer@d25a3bb 8.0.100-preview.2.23105.6 (dotnet#7769)"
  LEGO: Merge pull request 7828
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants