-
Notifications
You must be signed in to change notification settings - Fork 56
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 JniTypeSignatureAttribute.InvokerType #1284
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
#nullable enable | ||
|
||
using System; | ||
using System.Diagnostics.CodeAnalysis; | ||
|
||
namespace Java.Interop | ||
{ | ||
|
@@ -31,6 +32,9 @@ public int ArrayRank { | |
} | ||
|
||
public bool GenerateJavaPeer {get; set;} | ||
|
||
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)] | ||
public Type? InvokerType {get; set;} | ||
Comment on lines
+36
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The trimmer warnings might not require this, but should this also preserve interfaces? Just thinking generally, if it needs to. In theory, that would make every interface preserve its interface implementations?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the TL;DR is that "as-is", the NativeAOT linker cannot be made to do what we want it to do, and I'm not sure if it'll "ever" be supported in some "reasonable" timeframe.
Part of the issue is that this The semantic we "need" is "preserve all method overrides/interface implementations." We can't express that semantic right now (afaik). Trying to "fake" the above semantic with "okay, just preserve everything on the type" means that we'll preserve everything on the type. This is "fine" for interfaces (but see below), but is not fine for abstract classes. Consider Since, afaik, we can't actually get our desired semantic right now, and "preserve everything" looks like it would have terrible implications on app size when applied to abstract classes, I see only two solutions here:
Additionally, "preserve everything" isn't even a useful constraint on interfaces! It's mostly correct, for non-default instance methods. Pull in a default method, or static members, and we certainly should be able to trim those away! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I was just suggesting interfaces, not |
||
} | ||
} | ||
|
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.
It's good to remove this, it's probably not called on Android anyway, and this is used instead: