[Java.Interop] Add JniTypeSignatureAttribute.InvokerType

[jonpryor]

Context: #1263
Context: #1263 (comment)

We want the default trimmer infrastructure to be able to automatically preserve the *Invoker types which are required for interacting with abstract classes and interfaces.

The most straightforward way to do this is to add a new InvokerType property to JniTypeSignatureAttribute (and eventually RegisterAttribute):

partial class JniTypeSignatureAttribute {
    [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors |
        DynamicallyAccessedMemberTypes.NonPublicConstructors)]
    public Type? InvokerType {get; set;}
}

Update generator so that generator --codegen-target=JavaInterop1 output sets this new property on abstract classes and interfaces:

namespace Java.Lang {

    [Java.Interop.JniTypeSignatureAttribute("java/lang/Runnable", GenerateJavaPeer=false, InvokerType=typeof(Java.Lang.IRunnableInvoker))]
    public partial interface IRunnable {
    }
    internal partial class IRunnableInvoker {
    }

    [Java.Interop.JniTypeSignatureAttribute("java/lang/Number", GenerateJavaPeer=false, InvokerType=typeof(Java.Lang.NumberInvoker))]
    public abstract partial class Number {
    }
    internal partial class NumberInvoker {
    }
}

This allows the default trimmer to automatically preserve the *Invoker type and constructors.

Update Java.Interop.JniRuntime.JniValueManager to no longer look for *Invoker types "by string", and instead require use of the JniTypeSignatureAttribute.InvokerType property.

Update unit tests and expected output so that everything works.

[jpobst]

Note: XAJavaInterop1 output is impacted, but it shouldn't matter, as it changes the RegisterAttribute connector on interfaces, which isn't used by anything:

Further note that the old value ("Xamarin.Test.NotificationCompatBase/Action/IFactoryInvoker") is non-sensical.

I am concerned that this might actually be a problem. The non-sensical format is the format used by Mono.Cecil.TypeDefinition.FullName, and it appears we use the connector for at least marshal methods:

https://github.com/dotnet/android/blob/7aa0d59ff76e97f9c77632f2584f44c3fc614d86/src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs#L672-L692

At the very least we should run a dotnet/android test PR to verify this.

[jonpryor]

@jpobst wrote:

it appears we use the connector for at least marshal methods:
…
At the very least we should run a dotnet/android test PR to verify this.

And dotnet/android#9596 says that it doesn't even build, because it can't get past the src/Mono.Android API compatibility check:

/Users/builder/azdo/_work/4/s/xamarin-android/src/Mono.Android/Mono.Android.targets(220,5): error : CannotChangeAttribute : Attribute 'Android.Runtime.RegisterAttribute' on 'Java.Security.KeyStore.IEntryAttribute' changed from '[RegisterAttribute("java/security/KeyStore$Entry$Attribute", "", "Java.Security.KeyStore/IEntryAttributeInvoker", ApiSince=26)]' in the contract to '[RegisterAttribute("java/security/KeyStore$Entry$Attribute", "", "Java.Security.KeyStore.IEntryAttributeInvoker", ApiSince=26)]' in the implementation.

(among many many others)

The change from Java.Security.KeyStore/IEntryAttributeInvoker to Java.Security.KeyStore.IEntryAttributeInvoker breaks it.

[jonathanpeppers]

It's good to remove this, it's probably not called on Android anyway, and this is used instead:

• https://github.com/dotnet/android/blob/63f1e6c709c4e66601042ac6dcefeefa195c7a8f/src/Mono.Android/Android.Runtime/AndroidRuntime.cs#L612

[jonathanpeppers]

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?

• Trimmer decides IFoo is used, and wants to keep the type
• InvokerType=typeof(FooInvoker) preserves the invoker
• InvokerType=typeof(FooInvoker) then could also preserve all interface implementations of IFoo?

[jonpryor]

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.

Related:

I would also consider not using All unless we absolutely have to. All is kind of weird in that it will keep really everything about the type, so for example if the type implements an interface it will force all the methods on that interface to be kept (for everybody) and so on.

Part of the issue is that this InvokerType property is used for both abstract classes and interfaces.

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 LauncherActivity: as it's an Context+Activity subclass, it contains dozens of members through the inheritance chain. We almost certainly do not need most of that infrastructure; we only need the infrastructure used by the method override!

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:

• InterfaceInvokerType vs. AbstractClassInvokerType properties! Which is possible, but ugh.
• Go with the least restrictive yet useful constraint (preserve constructors), and rely on "stuff elsewhere" to ensure things work and we get closer to our ideal semantic.

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!

[jonathanpeppers]

Yeah, I was just suggesting interfaces, not All, but yeah that doesn't make sense for abstract classes.

[jpobst]

I will defer to others on what the correct linker semantics are, but how we are generating the change looks good to me. 😉