-
Notifications
You must be signed in to change notification settings - Fork 53
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
[Java.Interop] Add JniRuntime.JniValueManager.GetPeer() #1295
Conversation
Context: https://github.com/dotnet/android/pull/9630/files#r1891090085 Context: dotnet/android#9716 As part of NativeAOT prototyping within dotnet/android, we need to update `Java.Lang.Object.GetObject<T>()` so that it uses `Java.Interop.JniRuntime.JniValueManager` APIs instead of its own `TypeManager.CreateInstance()` invocation, as `TypeManager.CreateInstance()` hits P/Invokes which don't currently work in the NativeAOT sample environment. However, updated it to *what*? The obvious thing to do would be to use `JniRuntime.JniValueManager.GetValue()`: partial class Object { internal static IJavaPeerable? GetObject (IntPtr handle, JniHandleOwnershipt ransfer, Type? type = null) { var r = PeekObject (handle, type); if (r != null) { JNIEnv.DeleteRef (handle, transfer); return r; } var reference = new JniObjectReference (handle); r = (IJavaPeerable) JNIEnvInit.ValueManager.GetValue ( ref reference, JniObjectReferenceOptions.Copy, type); JNIEnv.DeleteRef (handle, transfer); return r; } } The problem is that this blows up good: <System.InvalidCastException: Arg_InvalidCastException at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type ) at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type ) at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index) at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] ) at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()> because somewhere in that stack trace we have a `java.lang.Integer` instance, and `.GetValue(Integer_ref, …)` returns a `System.Int32` containing the underling value, *not* an `IJavaPeerable` value for the `java.lang.Integer` instance. Consider: var i_class = new JniType ("java/lang/Integer"); var i_ctor = i_class.GetConstructor ("(I)V"); JniArgumentValue* i_args = stackalloc JniArgumentValue [1]; i_args [0] = new JniArgumentValue (42); var i_value = i_class.NewObject (i_ctor, i_args); var v = JniEnvironment.Runtime.ValueManager.GetValue (ref i_value, JniObjectReferenceOptions.CopyAndDispose, null); Console.WriteLine ($"v? {v} {v?.GetType ()}"); which prints `v? 42 System.Int32`. This was expected and desirable, until we try to use `GetValue()` for `Object.GetObject<T>()`; the semantics don't match. Add a new `JniRuntime.JniValueManager.GetPeer()` method, which better matches the semantics that `Object.GetObject<T>()` requires, allowing: partial class Object { internal static IJavaPeerable? GetObject (IntPtr handle, JniHandleOwnershipt ransfer, Type? type = null) { var r = JNIEnvInit.ValueManager.GetPeer (new JniObjectReference (handle)); JNIEnv.DeleteRef (handle, transfer); return r; } } Finally, add a new `JniRuntimeJniValueManagerContract` unit test, so that we have "more formalized" semantic requirements on `JniRuntime.JniValueManager` implementations.
Does It Build™?
Fixes build failure: /Users/runner/work/1/s/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs(16,3): error CS0246: The type or namespace name 'NonParallelizableAttribute' could not be found (are you missing a using directive or an assembly reference?) /Users/runner/work/1/s/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs(16,3): error CS0246: The type or namespace name 'NonParallelizable' could not be found (are you missing a using directive or an assembly reference?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (3)
- src/Java.Interop/PublicAPI.Unshipped.txt: Language not supported
- tests/Java.Interop-Tests/Java.Interop-Tests.csproj: Language not supported
- tests/TestJVM/TestJVM.csproj: Language not supported
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
public IJavaPeerable? GetPeer ( | ||
JniObjectReference reference, | ||
[DynamicallyAccessedMembers (Constructors)] | ||
Type? targetType = null) | ||
{ | ||
if (disposed) { | ||
throw new ObjectDisposedException (GetType ().Name); | ||
} | ||
|
||
if (!reference.IsValid) { | ||
return null; | ||
} | ||
|
||
var peeked = PeekPeer (reference); | ||
if (peeked != null && | ||
(targetType == null || | ||
targetType.IsAssignableFrom (peeked.GetType ()))) { | ||
return peeked; | ||
} | ||
return CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to update Java.Lang.Object.GetObject()
to use this method:
var p = JNIEnvInit.ValueManager!.GetPeer (new JniObjectReference (handle), type);
JNIEnv.DeleteRef (handle, transfer);
return p;
After merging this, we can try it in a new version of:
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Changes: dotnet/java-interop@9b1d878...e288589 * dotnet/java-interop@e288589d: [Java.Interop] Add JniRuntime.JniValueManager.GetPeer() (dotnet/java-interop#1295) * dotnet/java-interop@bbb15b71: [ci] Add dev/* branches to CI trigger (dotnet/java-interop#1297)
Changes: dotnet/java-interop@9b1d878...e288589 * dotnet/java-interop@e288589d: [Java.Interop] Add JniRuntime.JniValueManager.GetPeer() (dotnet/java-interop#1295) * dotnet/java-interop@bbb15b71: [ci] Add dev/* branches to CI trigger (dotnet/java-interop#1297)
Context: #9630 Context: dotnet/java-interop#1295 As an alternative to #9630... For NativeAOT support, implement `Java.Lang.Object.GetObject()` using the new `JniRuntime.JniValueManager.GetPeer()` method. This also cleans up a few things if `Java.Lang.Object` introduces an internal `DynamicallyAccessedMemberTypes Constructors` field.
Context: #9630 Context: dotnet/java-interop#1295 As an alternative to #9630... For NativeAOT support, implement `Java.Lang.Object.GetObject()` using the new `JniRuntime.JniValueManager.GetPeer()` method. This also cleans up a few things if `Java.Lang.Object` introduces an internal `DynamicallyAccessedMemberTypes Constructors` field.
Context: https://github.com/dotnet/android/pull/9630/files#r1891090085
Context: dotnet/android#9716
As part of NativeAOT prototyping within dotnet/android, we need to update
Java.Lang.Object.GetObject<T>()
so that it usesJava.Interop.JniRuntime.JniValueManager
APIs instead of its ownTypeManager.CreateInstance()
invocation, asTypeManager.CreateInstance()
hits P/Invokes which don't currently work in the NativeAOT sample environment.However, updated it to what? The obvious thing to do would be to use
JniRuntime.JniValueManager.GetValue()
:The problem is that this blows up good:
because somewhere in that stack trace we have a
java.lang.Integer
instance, and.GetValue(Integer_ref, …)
returns aSystem.Int32
containing the underling value, not anIJavaPeerable
value for thejava.lang.Integer
instance. Consider:which prints
v? 42 System.Int32
.This was expected and desirable, until we try to use
GetValue()
forObject.GetObject<T>()
; the semantics don't match.Add a new
JniRuntime.JniValueManager.GetPeer()
method, which better matches the semantics thatObject.GetObject<T>()
requires, allowing:Finally, add a new
JniRuntimeJniValueManagerContract
unit test, so that we have "more formalized" semantic requirements onJniRuntime.JniValueManager
implementations.