[Java.Interop] Avoid `Type.GetType()` in `ManagedPeer`

[jonpryor]

Fixes: https://github.com/xamarin/java.interop/issues/1165

Context: https://github.com/xamarin/java.interop/pull/1153 Context: https://github.com/xamarin/java.interop/issues/1157

When building for NativeAOT (#1153) or when building .NET Android apps with -p:IsAotcompatible=true (#1157), we get IL2057 warnings from ManagedPeer.cs:

    ManagedPeer.cs(93,19,93,112): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type.
    ManagedPeer.cs(156,18,156,65): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type.
    ManagedPeer.cs(198,35,198,92): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type.

These warnings are because ManagedPeer.Construct() and ManagedPeer.RegisterNativeMembers() use Type.GetType() on string values provided from Java code, and thus the IL trimmer does not have visibility into those strings, and thus cannot reliably determine which types need to be preserved:

// Java Callable Wrapper
/* partial */ class ManagedType
{
  public static final String __md_methods;
  static {
    __md_methods =
      "n_GetString:()Ljava/lang/String;:__export__\n" +
      "";
    net.dot.jni.ManagedPeer.registerNativeMembers (
        /* nativeClass */             ManagedType.class,
        /* assemblyQualifiedName */   "Example.ManagedType, Hello-NativeAOTFromJNI",
        /* methods */                 __md_methods);
  }

  public ManagedType (int p0)
  {
    super ();
    if (getClass () == ManagedType.class) {
      net.dot.jni.ManagedPeer.construct (
          /* self */                  this,
          /* assemblyQualifiedName */ "Example.ManagedType, Hello-NativeAOTFromJNI",
          /* constructorSignature */  "System.Int32, System.Runtime",
          /* arguments */             new java.lang.Object[] { p0 });
    }
  }
}

ManagedPeer.construct() passes two sets of assembly-qualified type names: assemblyQualifiedName contains the type to construct, while constructorSignature contains a :-separated list of assembly-qualified type names for the constructor parameters. Each of these are passed to Type.GetType().

ManagedPeer.registerNativeMembers() passes an assembly-qualified type name to ManagedPeer.RegisterNativeMembers(), which passes the assembly-qualified type name to Type.GetType() to find the type to register native methods for.

If we more strongly rely on JNI signatures, we can remove the need for Java Callable Wrappers to contain assembly-qualified type names entirely, thus removing the need for ManagedPeer to use Type.GetType(), removing the IL2057 warnings.

For ManagedPeer.construct(), assemblyQualifiedName can be replaced with getting the JNI type signature from self.getClass(), and constructorSignature can be replaced with a JNI method signature of the calling constructor.

For ManagedPeer.registerNativeMembers(), assemblyQualifiedName can be replaced with getting the JNI type signature from nativeClass.

// Java Callable Wrapper
/* partial */ class ManagedType
{
  public static final String __md_methods;
  static {
    __md_methods =
      "n_GetString:()Ljava/lang/String;:__export__\n" +
      "";
    net.dot.jni.ManagedPeer.registerNativeMembers (
        /* nativeClass */             ManagedType.class,
        /* methods */                 __md_methods);
  }

  public ManagedType (int p0)
  {
    super ();
    if (getClass () == ManagedType.class) {
      net.dot.jni.ManagedPeer.construct (
          /* self */                  this,
          /* constructorSignature */  "(I)V",
          /* arguments */             new java.lang.Object[] { p0 });
    }
  }
}

Furthermore, if we add [DynamicallyAccessedMembers] to JniRuntime.JniTypeManager.GetType(), we can fix some IL2075 warnings which appeared after fixing the IL2057 warnings.

Aside: Excising assembly-qualified type names from Java Callable Wrappers had some "interesting" knock-on effects in the unit tests, requiring that more typemap information be explicitly provided. (This same information was implicitly provided before, via the provision of assembly-qualified type names everywhere…)

[jonpryor]

There is a remaining problem with this approach: there is no requirement of a 1:1 mapping between Java types and managed types. There are already scenarios in which the mapping is ambiguous, and the pre-existing solution of using assembly-qualified type names resolved the ambiguity: arrays.

Consider Java int[]: this has a JNI signature of [I, and can be bound as either int[] or Java.Interop.JavaInt32Array. The approach suggested in this PR does not handle that ambiguity, and will arbitrarily pick one (not sure which at the moment), preventing the other from being usable in constructor signatures.

This will be addressed in a future commit to this PR.

[jpobst]

Should we run an XA test PR on this before merging?

[jonpryor]

My general questions about performance depend on how often these would be called at runtime by a MAUI app?

Right now, nothing; this codepath is only hit by "JavaInterop1" Java Callable Wrappers, not Xamarin.Android "XAJavaInterop1" Java Callable Wrappers. (The codepath is present in .NET Android; it's just that only our unit tests will actually exercise it in practice.) Rephrased: the only way to hit it is for Java code to call ManagedPeer.construct(), and .NET Android apps use TypeManager.Activate(). No "real" .NET Android apps currently use ManagedPeer.construct().

This is thus a more "exploratory quest" to answer the question: so what about Type.GetType() (IL2057), and what can we do instead? (TL;DR: yes, it's possible. It's also fugly. I have no idea if it's actually worth it.) That question needs to be "appropriately answered" in order to support NativeAOT, if that is ever to happen.

[jonpryor]

@jpobst asked

Should we run an XA test PR on this before merging?

Yes, but given how we have a unit test failing here, I'll need to fix that first. We might be able to consider merging by end-of-year? November is out.

[jonpryor]

@jpobst, @jonathanpeppers: @Suchiman on https://libera.chat / #csharp mentioned an alternate way to make Type.GetType() work in a NativeAOT context:

1. Create a new method that contains all the Type.GetType() invocations that are needed.
2. The method in (1) needs to be called, but doesn't need to actually "do" anything; it can early return (but it the early return needs to be conditioned on a parameter):

   void TellNativeAotAboutTypes(bool loadTypes)
   {
       if (!loadTypes)
           return;
       Type.GetType ("System.Int32, System.Runtime");
       // …
   }

3. You can then Type.GetType() a non-const value and the type will be found and loaded.

For example:

using System.Text;

TellNativeAotAboutTypes ();

var t = Type.GetType (GetTypeName (), throwOnError: true);

Console.WriteLine (t);

string GetTypeName() =>
    new StringBuilder()
    .Append ("System.")
    .Append ("Int32")
    .Append (", System.Runtime")
    .ToString ();

void TellNativeAotAboutTypes (bool load = false)
{
    if (!load)
        return;
    Type.GetType ("System.Int32, System.Runtime");
}

The above app works at runtime. If you remove the TellNativeAotAboutTypes() invocation, or remove the bool load=false parameter while keeping the early return, then execution fails with:

Unhandled Exception: System.IO.FileNotFoundException: Could not resolve assembly 'System.Runtime'.
   at System.Reflection.TypeNameParser.ResolveAssembly(String) + 0x97
   at System.Reflection.TypeNameParser.GetType(String, ReadOnlySpan`1, String) + 0x32
   at System.Reflection.TypeNameParser.NamespaceTypeName.ResolveType(TypeNameParser&, String) + 0x17
   at System.Reflection.TypeNameParser.GetType(String, Func`2, Func`4, Boolean, Boolean, Boolean, String) + 0x99
   at Program.<Main>$(String[] args) + 0x3d
   at native-aot-with-Suchiman-workaround!<BaseAddress>+0x120dbc

However, compilation still generates an IL2057 warning:

% dotnet publish -c Release -r osx-x64
…
…/Program.cs(8,9): warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'. It's not possible to guarantee the availability of the target type. 

So the good news is that we have a more plausible workaround in Java.Interop that doesn't require changing TypeManager.Activate() to do what was done in this PR, and further suggests that we could revert #1168 (this PR).

The bad news is that this described workaround means we have IL2057 trim warnings, but we could [UnconditionalSuppressMessage]/etc. the IL2057 warning in that context.

@jonathanpeppers, @jpobst: what do you think we should do? Revert #1168 so that ManagedPeer is semantically closer to TypeManager and require this "pre-register types" workaround? Or keep #1168?

[MichalStrehovsky]

Instead of TellNativeAotAboutTypes, could you replace the uses of Type.GetType with an enum? For example:

Type GetTheType(TypeId id) =>
    id switch
    {
        Id.Int32 => typeof(int),
        // or Id.Int32 => Type.GetType("System.Int32, System.Runtime"),
    }

This way you don't have to do any suppressions, it's trim safe by construction.

Alternatively:

var dict = new Dictionary<string, Type>
{
    { "System.Int32, System.Runtime", typeof(int) },
    ...
}

I assume you're going to do something with the type (like Activator.CreateInstance it), so you'll probably actually need something along the lines of this:

struct DictionaryOfConstructableTypes
{
    Dictionary<string, Type> _inner;

    public void Add(string s, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type t) => _inner.Add(s, t);

    // We need to do a suppression here because static analysis cannot guarantee
    // whatever comes out of the dictionary has the constructors kept, but it's easy to audit
    // that it is in fact the case manually because the method that adds them is annotated to keep them.
    [UnconditionalSuppressMessage(...)]
    [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]
    public Type Get(string s) => _inner[s];
}

This way you can pass anything that comes out of this dictionary to Activator.CreateInstance because the static analysis can guarantee both the type and the constructor are available.

[jonpryor]

(The perils of comments after closing the PR. I didn't see @MichalStrehovsky 's comment until just now…)

@MichalStrehovsky: I lack the imagination to see how replacing Type.GetType() with an enum would work, in practice.

Firstly, there's architecture: "core" assemblies like Java.Interop.dll need this "alternate mechanism", but your hypothetical enum cannot be constructed until app build time, when all assemblies in the app are known. By definition, Java.Interop.dll cannot reference such an enum, unless the enum is empty, at which point it's basically an int. "Trim-safe by construction" it's not.

Secondly, generated Java code needs to be able to refer to C# types, and this mapping is done at packaging time. With enough hand-waving one can imagine a world where at app packaging time we create an int::type mapping, hard-coding these int values into the Java code.

Thirdly, this isn't incremental: change an assembly, add a new type, and all your previous int::type mappings are potentially wrong. Rebuilding the world is slow, and something we need to avoid in Debug builds. This in turn might be solvable, but it doesn't look easy in the 5s of mental energy I gave it.

It also doesn't quite consider the sheer number of entries we're looking at. The Mono.Android-Tests unit tests (debug builds, unlinked) contains 8274 typemap entries, and that isn't a MAUI app, nor does it reference AndroidX, either/both of which will explode the number of types involved. (A default MAUI app, debug build, contains 14309 typemap entries.)

The Dictionary<string, Type> approach appears viable, except for scale. Again, 8274 entries, and this dict needs to be initialized during app startup in order for anything to work properly. That's an actually significant amount of startup overhead, which is scary. (Related.)

The approach committed in https://github.com/xamarin/java.interop/commit/005c914170a0af9069ff18fd4dd9d45463dd5dc6 (which may or may not need to be reverted) builds atop the typemap infrastructure which already exists, and needs to exist, and has been optimized a fair bit. The alternate approach has the benefit of needing fewer changes overall, and avoids the need for me to construct a gigantic multi-thousand entry dict mapping strings to types. (Native AOT will presumably need to create such a mapping, somehow, but that's Not My Problem™ {until it is, due to perf hits}.)