-
Notifications
You must be signed in to change notification settings - Fork 52
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 .JavaAs()
extension method
#1234
Conversation
Will remain a Draft until we create & review a corresponding dotnet/android PR for integration testing… |
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.
Love it, I think this will be a useful addition for users.
@@ -11,5 +12,40 @@ public static class JavaPeerableExtensions { | |||
JniPeerMembers.AssertSelf (self); | |||
return JniEnvironment.Types.GetJniTypeNameFromInstance (self.PeerReference); | |||
} | |||
|
|||
public static bool TryJavaCast< | |||
[DynamicallyAccessedMembers (JavaObject.ConstructorsAndInterfaces)] |
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.
The existing JavaCast<T>
only uses [DynamicallyAccessedMembers (Constructors)]
, should Interfaces
be added as well?
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'll leave that to @jonathanpeppers. 😅
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 added enough attributes to silence the analyzer's warnings on the original JavaCast<T>()
.
I would recommend just putting Constructors
for now, if no warnings appear.
} | ||
|
||
if (!self.PeerReference.IsValid) { | ||
throw new ObjectDisposedException (self.GetType ().FullName); |
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.
There are still cases where we throw exceptions (here and in JniRuntime.JniValueManager.cs
). I assume these are corner cases where the object is in a "bad state", but I think I would lean towards never throwing an exception under any circumstance. If the object is bad and we return null
, the user cannot use it anyways.
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 mentally split this into two "important" bits:
- Java type checking, and
- Things that could never succeed, and would indicate a fundamental error.
The exceptions are for (2). For example, what if someone wanted to use this to convert a Java type to System.ICloneable
:
var wat = new Java.Lang.Object().JavaAs<System.ICloneable>();
This would fail to compile right now, due to the generic constraints on JavaAs()
(yay), but if someone avoided those constraints (e.g. directly hit JniEnvironment.Runtime.ValueManager.CreatePeer()
, use MemberInfo.MakeGenericMethod()
at runtime to create an invalid instantiation {untested; maybe that'll throw?}), then I think it would be better to always throw, because the usage is bad.
Fixes: dotnet/android#9038 Context: 1adb796 Imagine the following Java type hierarchy: // Java public abstract class Drawable { public static Drawable createFromStream(IntputStream is, String srcName) {…} // … } public interface Animatable { public void start(); // … } /* package */ class SomeAnimatableDrawable extends Drawable implements Animatable { // … } Further imagine that a call to `Drawable.createFromStream()` returns an instance of `SomeAnimatableDrawable`. What does the *binding* `Drawable.CreateFromStream()` return? // C# var drawable = Drawable.CreateFromStream(input, name); The binding `Drawable.CreateFromStream()` look at the runtime type of the value returned, see that it's of type `SomeAnimatableDrawable`, and look for an existing binding of that type. If no such binding is found -- which will be the case here, as `SomeAnimatableDrawable` is package-private -- then we check the value's base class, ad infinitum, until we hit a type that we *do* have a binding for (or fail catastrophically when we can't find a binding for `java.lang.Object`). See also [`TypeManager.CreateInstance()`][0], which is similar to the code within `JniRuntime.JniValueManager.GetPeerConstructor()`. Any interfaces implemented by Java value are not consulted. Only the base class hiearchy. For the sake of discussion, assume that `drawable` will be an instance of `DrawableInvoker` (e.g. 1adb796), akin to: internal class DrawableInvoker : Drawable { // … } Further imagine that we want to invoke `Animatable` methods on `drawable`. How do we do this? This is where the [`.JavaCast<TResult>()` extension method][1] comes in: we can use `.JavaCast<TResult>()` to perform a Java-side type check the type cast, which returns a value which can be used to invoke methods on the specified type: var animatable = drawable.JavaCast<IAnimatable>(); animatable.Start(); The problem with `.JavaCast<TResult>()` is that it always throws on failure: var someOtherIface = drawable.JavaCast<ISomethingElse>(); // throws some exception… @mattleibow requests an "exception-free JavaCast overload" so that he can *easily* use type-specific functionality *optionally*. Add the following extension methods on `IJavaPeerable`: static class JavaPeerableExtensions { public static TResult? JavaAs<TResult>( this IJavaPeerable self); public static bool TryJavaCast<TResult>( this IJavaPeerable self, out TResult? result); } The `.JavaAs<TResult>()` extension method mirrors the C# `as` operator, returning `null` if the type coercion would fail. This makes it useful for one-off invocations: drawable.JavaAs<IAnimatable>()?.Start(); The `.TryJavaCast<TResult>()` extension method follows the `TryParse()` pattern, returning true if the type coercion succeeds and the output `result` parameter is non-null, and false otherwise. This allows "nicely scoping" things within an `if`: if (drawable.TryJavaCast<IAnimatable>(out var animatable)) { animatable.Start(); // … animatable.Stop(); } [0]: https://github.com/dotnet/android/blob/06bb1dc6a292ef5618a3bb6ecca3ca869253ff2e/src/Mono.Android/Java.Interop/TypeManager.cs#L276-L291 [1]: https://github.com/dotnet/android/blob/06bb1dc6a292ef5618a3bb6ecca3ca869253ff2e/src/Mono.Android/Android.Runtime/Extensions.cs#L9-L17
7dbb82b
to
27236f2
Compare
@@ -276,16 +276,45 @@ static Type GetPeerType ([DynamicallyAccessedMembers (Constructors)] Type type) | |||
if (disposed) | |||
throw new ObjectDisposedException (GetType ().Name); | |||
|
|||
if (!reference.IsValid) { |
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.
Though this check being first means I'm fibbing about "always checking for fundamental errors"; if the value were always null, for whatever reason, we'd never get around to "sanity checking" whether it's even possible. 🤔
so that dotnet/android can use it for tests.
Context: dotnet/java-interop#1234 Does It Build™?
I guess this PR fixes this issue #10 too |
If `IJavaPeerable.PeerReference` isn't valid, return null.
60334df
to
31045b8
Compare
Should have updated TryJavaCast<T>() alongside JavaAs<T>()…
[Test] | ||
public void JavaAs_InstanceThatDoesNotImplementInterfaceReturnsNull () | ||
{ | ||
using var v = new MyJavaInterfaceImpl (); | ||
Assert.AreEqual (null, JavaPeerableExtensions.JavaAs<IAndroidInterface> (v)); | ||
} |
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.
Do we need a test that tries to cast to a random, incorrect type like System.Guid
? (One that isn't even a Java thing) Would that case throw?
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 believe the case you are suggesting is compiler prevented by the generic constraint?
public static TResult? JavaAs<TResult> (this IJavaPeerable? self)
where TResult : class, IJavaPeerable
{ ... }
Or perhaps I misunderstand your comment.
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.
Otherwise, this looks good to me. I just wondered about the one case.
Context: dotnet/java-interop#1234 Does It Build™?
Fixes: #9038 Changes: dotnet/java-interop@ccafbe6...7a058c0 * dotnet/java-interop@7a058c0e: [Java.Interop] Add `IJavaPeerable.JavaAs()` extension method (dotnet/java-interop#1234) * dotnet/java-interop@6f9defa5: Bump to dotnet/android-tools@3debf8e0 (dotnet/java-interop#1236) * dotnet/java-interop@6923fb89: [ci] Add dependabot branches to build triggers (dotnet/java-interop#1233) * dotnet/java-interop@573028f3: [ci] Use macOS 13 Ventura build agents (dotnet/java-interop#1235) dotnet/java-interop@7a058c0e adds a new `IJavaPeerable.JavaAs<T>()` extension method, to perform type casts which return `null` when the cast will not succeed instead of throwing an exception. Update `AndroidValueManager.CreatePeer()` to check for Java-side type compatibility (by way of `TypeManager.CreateInstance()`). Previously, this would not throw: var instance = … var r = instance.PeerReference; var wrap_instance = JniRuntime.CurrentRuntime.ValueManager.CreatePeer ( reference: ref r, options: JniObjectReferenceOptions.Copy, targetType: typeof (SomeInterfaceThatInstanceDoesNotImplement)); // `wrap_instance` is not null, `.CreatePeer()` does not throw wrap_instance.SomeMethod(); // will throw, due to a type mismatch. `AndroidValueManager.CreatePeer()` will now return `null` if the Java instance referenced by the `reference:` parameter is not convertible to `targetType`, as per [`JNIEnv::IsAssignableFrom()`][0]. [0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#IsAssignableFrom
Fixes: #10
Fixes: dotnet/android#9038
Context: 1adb796
Imagine the following Java type hierarchy:
Further imagine that a call to
Drawable.createFromStream()
returnsan instance of
SomeAnimatableDrawable
.What does the binding
Drawable.CreateFromStream()
return?The binding
Drawable.CreateFromStream()
look at the runtime typeof the value returned, see that it's of type
SomeAnimatableDrawable
,and look for an existing binding of that type. If no such binding is
found -- which will be the case here, as
SomeAnimatableDrawable
ispackage-private -- then we check the value's base class, ad infinitum,
until we hit a type that we do have a binding for (or fail
catastrophically when we can't find a binding for
java.lang.Object
).See also
TypeManager.CreateInstance()
, which is similar to thecode within
JniRuntime.JniValueManager.GetPeerConstructor()
.Any interfaces implemented by Java value are not consulted. Only
the base class hiearchy.
For the sake of discussion, assume that
drawable
will be aninstance of
DrawableInvoker
(e.g. 1adb796), akin to:Further imagine that we want to invoke
Animatable
methods ondrawable
. How do we do this?This is where the
.JavaCast<TResult>()
extension method comesin: we can use
.JavaCast<TResult>()
to perform a Java-side typecheck the type cast, which returns a value which can be used to
invoke methods on the specified type:
The problem with
.JavaCast<TResult>()
is that it always throws onfailure:
@mattleibow requests an "exception-free JavaCast overload" so that he
can easily use type-specific functionality optionally.
Add the following extension methods on
IJavaPeerable
:The
.JavaAs<TResult>()
extension method mirrors the C#as
operator, returning
null
if the type coercion would fail.This makes it useful for one-off invocations:
The
.TryJavaCast<TResult>()
extension method follows theTryParse()
pattern, returning true if the type coercion succeedsand the output
result
parameter is non-null, and false otherwise.This allows "nicely scoping" things within an
if
: