[Java.Interop] ignore remaining trimming warnings

[jonathanpeppers]

Fixes: #1157

The remaining trimmer warnings are around usage of System.Reflection such as:

• Create an Invoker type from an original type via Type.Assembly.GetType()

• Create a Type based on a ParameterInfo.ParameterType and/or create instances

• Use Type.MakeGenericType()

• Use Type.MakeArrayType()

The trimming warnings themselves all indicate that these types "might be trimmed", in that nothing explicitly preserves them. However, we have a plethora of custom trimmer steps that work to preserve these types involved in Java interop, such as:

• https://github.com/xamarin/xamarin-android/blob/main/src/Microsoft.Android.Sdk.ILLink/PreserveApplications.cs
• https://github.com/xamarin/xamarin-android/blob/main/src/Microsoft.Android.Sdk.ILLink/PreserveExportedTypes.cs
• https://github.com/xamarin/xamarin-android/blob/main/src/Microsoft.Android.Sdk.ILLink/PreserveJavaExceptions.cs
• https://github.com/xamarin/xamarin-android/blob/main/src/Microsoft.Android.Sdk.ILLink/PreserveJavaInterfaces.cs
• https://github.com/xamarin/xamarin-android/blob/main/src/Microsoft.Android.Sdk.ILLink/PreserveRegistrations.cs

One day, in a perfect world, we could remove these trimmer steps and completely rely on the trimmer's warnings & attributing mechanisms. That day doesn't have to be today -- as our goal is begin surfacing trimmer warnings to users in their own code and dependencies.

With these warnings ignored, we can fully enable analyzers with:

<IsTrimmable>true</IsTrimmable>
<EnableAotAnalyzer>true</EnableAotAnalyzer>

We can then remove:

[assembly: AssemblyMetadata ("IsTrimmable", "True")]

As the MSBuild property does this for us, in addition to enabling analyzers.

I don't think we should enable $(IsAotCompatible) quite yet, as Java.Interop.dll likely won't work yet on NativeAOT, and this places metadata that says an assembly is supported. We should just use the $(EnableAotAnalyzer) analyzers for now, so we don't introduce new issues.

[jonathanpeppers]

/cc @simonrozsival @vitek-karas

[vitek-karas]

General guidance for suppressions - make then as specific as possible. Since it's not possible to add an attribute to a single line of code, we typically do that by extracting the problematic call into a local function and annotate the local function. That way the suppression is very precise and if somebody changes other code in the method it will not accidentally hide other unrelated problems.

I see two places here which should produce warnings:
Activator.CreateInstance
This is fixable with just annotations.
Annotate the attribute with DAM to preserve the constructor on the type specified to the attribute .ctor: https://github.com/jonathanpeppers/java.interop/blob/07c73009571d3b5d36ae79d0d4d69a02062dd6e8/src/Java.Interop/Java.Interop/JniValueMarshalerAttribute.cs#L16 - and then annotate the property MarshalerType.

The second is the usage of ParameterType - that one is probably not fixable with annotations in any way. If we have custom steps which preserve these, I would add a description here pointing to the custom step which does that and why it applies to all cases where this can happen. Ideally we would also add a test validating that behavior, but that might be pretty tricky to do.

Another general rule: We really, really.... really... try to avoid suppressions. There are just too many cases where we thought we got it right, and learned that we still got it wrong. And so if there are cases where we can't avoid them, we try to compensate for our inability to think it through by adding some kind of test to validate the assumptions. In places we added tests which:

• Run a specially crafted app through the trimmer and validate that certain things are kept in the output and thus the assumptions of the suppression hold.
• Add a test which using reflection validates that certain things have certain shapes (for example we have a case where we assume that a given class has properties of only primitive types, so we write a test which validates that)

The goal of these tests is twofold - validate the assumption we're making with the suppression, try to guard against breaks in the future, where somebody modifies something, breaks the assumption and is not aware of it.

In this case we might for example create a test which validate the behavior of the linker custom steps (if we don't already have one like that).

[jonathanpeppers]

• Run a specially crafted app through the trimmer and validate that certain things are kept in the output and thus the assumptions of the suppression hold.
• Add a test which using reflection validates that certain things have certain shapes (for example we have a case where we assume that a given class has properties of only primitive types, so we write a test which validates that)

For these two, my plans were to do this type of thing in the xamarin-android repo. We have existing tests that likely can toggle TrimMode=full (if not already) to verify these work.

The problem with trying to add tests in this repo, I guess we'd run them on desktop CoreCLR? We'd dotnet publish a NUnit test project with the trimmer enabled?

I'm going to take another look at this one today, I will see what we can do to have less UnconditionalSuppressMessage and link to the trimmer step as I go.

[vitek-karas]

Sorry for the late review:
This is potentially a trimming problem as well.
The trimming side of this is if you do something like:

class MyType
{
     public static void GenericMethod<[DAM(PublicMethods)] T>() {
          typeof(T).GetMethod("Foo").Invoke(...);
     }
}

class TestType
{
     public static void Foo() { }
}

public void Test()
{
     // This will crash at runtime, beacuse TestType.Foo is going to be trimmed away.
     // The only warning this code generates is below
     CallT(typeof(MyType).GetMethod("GenericMethod"), typeof(TestType));
}

private void CallT(MethodInfo genericMethod, Type typearg)
{
     // IL2060 (or something like it)
     genericMethod.MakeGenericMethod(typearg).Invoke(...);
}

The problem is that the trimmer sees that you're instantiating a generic method, but since it doesn't know which method, it must warn in the case the generic method would have an annotation on one of its type parameters.

It is also an AOT problem, that one is if the elementType is a value type, then this might fail since there might not be code for the value type instantiation of the generic method.