[java.interop] address some "easy" trimmer warnings

[jonathanpeppers]

Context: #1157

On the path to enabling IsAotCompatible, we can enable some settings
to get started:

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

This opts into the analyzers without declaring the assembly is fully AOT-compatible.

Starting out, I got 33 warnings: this is an attempt to address the ones that don't require too much thinking. Unfortunately, solving 1 warning likely will create dozens more -- as you have to update all callers.

This results in 24 warnings remaining. Since Release builds have $(TreatWarningsAsErrors), I will wait to enable the analyzers until all warnings are addressed.

Example Warnings

System.Linq.Expression usage:

src\Java.Interop\Java.Interop\JniRuntime.JniValueManager.cs(672,58):
warning IL2026: Using member 'System.Linq.Expressions.Expression.Property(Expression, String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed.

I decorated this one with:

[RequiresUnreferencedCode (ExpressionRequiresUnreferencedCode)]

Type.GetNestedType() usage:

src\Java.Interop\Java.Interop\JniRuntime.JniTypeManager.cs(447,28):
warning IL2070: 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicNestedTypes' in call to 'System.Type.GetNestedType(String, BindingFlags)'. The parameter 'type' of method 'Java.Interop.JniRuntime.JniTypeManager.TryLoadJniMarshalMethods(JniType, Type, String)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

Added:

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]

Activator.CreateInstance() usage:

src\Java.Interop\Java.Interop\JniRuntime.JniValueManager.cs(542,33): warning IL2072: 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Activator.CreateInstance(Type)'. The return value of method 'Java.Interop.JniValueMarshalerAttribute.MarshalerType.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

Added:

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]

Code that Actually Changed

As I enabled more attributes, these snowball into more and more warnings! I eventually had to make GetPeerType() look like:

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
static Type GetPeerType ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)] Type type)

The analyzer was not able to understand code like:

static  readonly    KeyValuePair<Type, Type>[]      PeerTypeMappings = new []{
    new KeyValuePair<Type, Type>(typeof (object),           typeof (JavaObject)),
    new KeyValuePair<Type, Type>(typeof (IJavaPeerable),    typeof (JavaObject)),
    new KeyValuePair<Type, Type>(typeof (Exception),        typeof (JavaException)),
};
//...
foreach (var m in PeerTypeMappings) {
    if (m.Key == type)
        return m.Value;
}

Simply removing the PeerTypeMappings array and using if statements solved the warnings. This may be the only real code change if any.

[jonathanpeppers]

Hmm, on CI some warnings I introduced with analyzers are errors:

src\Java.Interop\Java.Interop\ManagedPeer.cs(237,19): Error IL2072: 'targetType' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors'

Will look into why.

[jonathanpeppers]

Ok, building with -c Release these become errors???

[jpobst]

Yes, we use <TreatWarningsAsErrors> in JI for Release builds:

https://github.com/xamarin/java.interop/blob/07c73009571d3b5d36ae79d0d4d69a02062dd6e8/Directory.Build.props#L6

[jonpryor]

I have a random unpublished diff which has an alternate take at "fixing" these Expression.Property() warnings by moving those callsites into Java.Interop.Export.dll. It's incomplete, and I need to turn it into a proper branch -- it's currently based atop #1153 -- but it comes to mind as "maybe cleaner", if also more complicated. Incomplete, untested, needs much more work, but it's an idea…

https://gist.github.com/jonpryor/417294863fc9ff17c0521c04a607829e

[jonathanpeppers]

I have a random unpublished diff which has an alternate take at "fixing" these Expression.Property() warnings by moving those callsites into Java.Interop.Export.dll. It's incomplete, and I need to turn it into a proper branch -- it's currently based atop #1153 -- but it comes to mind as "maybe cleaner", if also more complicated. Incomplete, untested, needs much more work, but it's an idea…

https://gist.github.com/jonpryor/417294863fc9ff17c0521c04a607829e

We discussed this idea, and the problems were:

• We don't ship Java.Interop.Export.dll yet
• If we did, we'd have to mark it trimmable and fix warnings
• [RequiresUnreferencedCode] would still bubble up to all callers

I have a good idea on what's left here, maybe I can finish it today/tomorrow.

[jonpryor]

To prevent lines from looking like line noise, we should start requiring that all custom attributes take at least two lines: one (or more) for the custom attribute(s), and one for the thing being attributed.

The downside with that idea is the location of < and > for generics, and I'm just gonna decree:

1. < is on the line of the containing declaration
2. Contents should be indented an additional tab stop
3. > is on a new line vertically aligned with the start of the declaration.

And we should start consistently having { begin on a newline for class declarations. (We frequently did this anyway, just not consistently.)

Thus:

public abstract class JavaPrimitiveArray<
        [DynamicallyAccessedMembers (ConstructorsAndInterfaces)] // two tab stops
        T   // two tab stops
> // vertically aligned with start of declaration
    : JavaArray<T> // one tab stop
{
    // … one tab stop
}

If we assume/imagine that JavaArray<T>.ArrayCreator needed custom attributes, I'd want:

internal delegate TArray ArrayCreator<
        [DynamicallyAccessedMembers (…)] // 2 tab stops
        TArray
> (
        ref JniObjectReference reference,  // 2 tab stops
        JniObjectReferenceOptions transfer)
    where TArray : JavaArray<T>; // 1 tab stop

The > (, while fugly, provides a visual barrier between the end of the type parameters and the beginning of the parameter list.

This only needs to happen when custom attributes are used on type parameters. If custom attributes aren't required, this is quite compact and readable:

internal TArray CreateArray<TArray> (
        ref JniObjectReference reference, // 2 tab stops
        JniObjectReferenceOptions transfer) // 2 tab stops
    where TArray : JavaArray<T> // 1 tab stop
{
    // … 1 tab stop
}

[jonathanpeppers]

Ok, I think I've found & reformatted these: used VS's regex search to find them all.

[jonpryor]

I think we found a deficiency in VS's regex search: it doesn't appear to have found or fully fixed the .tt files, meaning if (when) we regenerate the .cs files form the .tt templates, changes are lost.

I think we've fixed all "mismatches" now.

We should probably consider a pre-build step which explicitly regenerates the .cs files from the .tt files, to ensure/require that the templates are the real source of truth. (Though I'm not quite sure how to make this work; the "obvious" route would be to use Target Inputs & Outputs, but if the .cs is manually changed w/o a corresponding .tt change, the Inputs & Outputs won't cause regeneration…)

[jonpryor]

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

We want to enable `$(IsAotCompatible)`=true, so we can identify and
fix trimmer warnings within `Java.Interop.dll`.  (Then later work our
way "up the stack", fixing trimmer warnings within `Mono.Android.dll`
and `Microsoft.Maui.dll` and…)

On the path to enabling `$(IsAotCompatible)`=true, we can enable some
settings to get started:

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

This opts into the analyzers without declaring that the assembly is
fully AOT-compatible.

Starting out, I got 33 warnings: this is an attempt to address the
ones that don't require too much thinking.  Unfortunately, solving
one warning likely will create dozens more -- as you have to update
all callers.

This results in 24 warnings remaining.  Since `Release` builds have
`$(TreatWarningsAsErrors)`, I will wait to enable the analyzers until
all warnings are addressed.

~~ Example Warnings ~~

**`System.Linq.Expression` usage:**
`JniRuntime.JniValueManager.CreateParameterFromManagedExpression()`

	/* 638 */ partial class JavaPeerableValueMarshaler {
	/* 667 */     public override Expression CreateParameterFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize)
	/* 668 */     {
	/* 669 */         var r = CreateIntermediaryExpressionFromManagedExpression (context, sourceValue);
	/* 670 */         var h = Expression.Variable (typeof (IntPtr), sourceValue.Name + "_handle");
	/* 671 */         context.LocalVariables.Add (h);
	/* 672 */         context.CreationStatements.Add (Expression.Assign (h, Expression.Property (r, "Handle")));
	/* 674 */         return h;
	/* 675 */     }
	/* 710 */ }

emits an IL2026:

	src\Java.Interop\Java.Interop\JniRuntime.JniValueManager.cs(672,58):
	warning IL2026: Using member 'System.Linq.Expressions.Expression.Property(Expression, String)'
	which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code.
	Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed.

I updated this with:

	partial class JniValueMarshaler {
	    internal const string ExpressionRequiresUnreferencedCode = "System.Linq.Expression usage may trim away required code.";

	    [RequiresUnreferencedCode (ExpressionRequiresUnreferencedCode)]
	    public virtual Expression CreateParameterFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize) => …
	}
	partial class JavaPeerableValueMarshaler /* indirectly inherits JniValueMarshaler */ {
	    [RequiresUnreferencedCode (ExpressionRequiresUnreferencedCode)]
	    public override Expression CreateParameterFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize) => …
	}

**`Type.GetNestedType()` usage:**
`JniRuntime.JniTypeManager.TryLoadJniMarshalMethods()`:

	/*  82 */ partial class JniRuntime.JniTypeManager {
	/* 445 */     bool TryLoadJniMarshalMethods (JniType nativeClass, Type type, string? methods)
	/* 446 */     {
	/* 447 */         var marshalType = type?.GetNestedType ("__<$>_jni_marshal_methods", BindingFlags.NonPublic);

emits an IL2070 warning:

	src\Java.Interop\Java.Interop\JniRuntime.JniTypeManager.cs(447,28):
	warning IL2070: 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicNestedTypes' in call to
	'System.Type.GetNestedType(String, BindingFlags)'.
	The parameter 'type' of method 'Java.Interop.JniRuntime.JniTypeManager.TryLoadJniMarshalMethods(JniType, Type, String)'
	does not have matching annotations.
	The source value must declare at least the same requirements as those declared on the target location it is assigned to.

I updated this with:

	partial class JniRuntime.JniTypeManager {
	    bool TryLoadJniMarshalMethods (
	            JniType nativeClass,
	            [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]
	            Type type,
	            string? methods) => …

**`Activator.CreateInstance()` usage:**
`JniRuntime.JniValueManager.GetValueMarshaler()`:

	/*  50 */ partial class JniRuntime.JniValueManager {
	/* 530 */     public JniValueMarshaler GetValueMarshaler (Type type)
	/* 531 */     {
	/* 541 */         if (marshalerAttr != null)
	/* 542 */             return (JniValueMarshaler) Activator.CreateInstance (marshalerAttr.MarshalerType)!;
	/* 591 */     }
	/* 612 */ }

emits an IL2072 warning:

	src\Java.Interop\Java.Interop\JniRuntime.JniValueManager.cs(542,33): warning IL2072:
	'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor'
	in call to 'System.Activator.CreateInstance(Type)'.
	The return value of method 'Java.Interop.JniValueMarshalerAttribute.MarshalerType.get' does not have matching annotations.
	The source value must declare at least the same requirements as those declared on the target location it is assigned to.

I updated this with:

	partial class JniRuntime.JniValueManager {
	    public JniValueMarshaler GetValueMarshaler (
	            [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.Interfaces)]
	            Type type)
	        => …
	}
	partial class JniValueMarshalerAttribute {
	    public Type MarshalerType {
	        [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
	        get;
	    }
	}


~~ Code that Actually Changed ~~

As I added more attributes, these snowballed into more and more
warnings!  I eventually had to make
`JniRuntime.JniValueManager.GetPeerType()` look like:

	partial class JniRuntime.JniValueManager {
	    internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

	    [return: DynamicallyAccessedMembers (Constructors)]
	    static Type GetPeerType ([DynamicallyAccessedMembers Constructors)] Type type) => …
	}

The analyzer was not able to understand code like:

	partial class JniRuntime.JniValueManager {
	    static  readonly    KeyValuePair<Type, Type>[]      PeerTypeMappings = new []{
	        new KeyValuePair<Type, Type>(typeof (object),           typeof (JavaObject)),
	        new KeyValuePair<Type, Type>(typeof (IJavaPeerable),    typeof (JavaObject)),
	        new KeyValuePair<Type, Type>(typeof (Exception),        typeof (JavaException)),
	    };
	    static Type GetPeerType (Type type)
	    {
	        foreach (var m in PeerTypeMappings) {
	            if (m.Key == type)
	                return m.Value;
	        }
	    }
	}

Simply removing the `PeerTypeMappings` array and using `if` statements
solved the warnings.  This may be the only real code change if any.

[vitek-karas]

Is there a reason this can't use Type.GetType("Java.Interop.MarshalMemberBuilder, Java.Interop.Export").
That is something both trimmer and AOT understand and will handle without warnings and correctly preserve the .ctor in the call to Activator.CreateInstance.

[jonathanpeppers]

This is kind of an odd codepath. We have a Java.Interop.Export.dll in this repo, but we don't actually ship it in the Android workload. Some of the projects in here could be used for Java interop on desktop, etc., but there isn't a real product using them.

Let me check if it's worth refactoring this yet.

[vitek-karas]

As a scenario this is not new - there are many places even in the core BCL where something similar happens. In fact there are a couple of places even in CoreLib where we use this to break circular references between assemblies. Meaning it's a pattern which trimming/AOT has to support well.

[vitek-karas]

/cc @simonrozsival as FYI