[generator] Import NRT data from managed assemblies.

[jpobst]

Fixes: #857

Implement enough of decoding C# NRT attributes to support our use case.

This will allow us to better generate members that match nullability of members they are overriding/implementing when the base member is coming from a managed assembly.

[jonpryor]

Since this involves nullable reference types, should we enable nullable reference types on Java.Interop.Tools.Cecil.dll? :-)

[jonpryor]

A "concern" here is that this doesn't check for assembly name, so if any yahoo adds System.Void to their assembly, it will be treated the same as System.Void, mscorlib or System.Void, System.Private.CoreLib or System.Void, netstandard.

I'm not sure of a good way to say "this is a core assembly", so this concern is likely ignorable, but it is something to be aware of.

[jpobst]

I think we can live with this. I'm not comfortable trying to enumerate every "core" assembly that they might have put System.Void in, or will in the future. 😉

[jonpryor]

Should we get our net472 tests onto C# 8? We've done that for other assemblies which target netstandard or net472, e.g. PR #913.

[jpobst]

Yeah, that was easy enough. Done!

[jonpryor]

At least for commit message purposes, an elaboration on "our use case doesn't really need them" would be good. Is it not possible for Java code to express the difference between String?[]? vs. String[]? (possibly nullable array containing possibly null items vs. possibly null array which won't contain any null items)? That's my assumption.

At the same time, I now wonder if we're binding arrays & collections wrong; should all arrays permit null items?

As a "spot-check", we're binding DatabaseUtils.longForQuery(SQLiteDatabase, String, String[]) as:

public static unsafe long LongForQuery (Android.Database.Sqlite.SQLiteDatabase? db, string? query, string[]? selectionArgs);

And in this case, that looks correct; null items aren't allowed:

• https://github.com/aosp-mirror/platform_frameworks_base/blob/ec84453381ef4a772e37d3bbf8c9d4da560ab19c/core/java/android/database/DatabaseUtils.java#L1002-L1009
• https://github.com/aosp-mirror/platform_frameworks_base/blob/ec84453381ef4a772e37d3bbf8c9d4da560ab19c/core/java/android/database/DatabaseUtils.java#L1016
• https://github.com/aosp-mirror/platform_frameworks_base/blob/ec84453381ef4a772e37d3bbf8c9d4da560ab19c/core/java/android/database/sqlite/SQLiteProgram.java#L201-L207
• https://github.com/aosp-mirror/platform_frameworks_base/blob/ec84453381ef4a772e37d3bbf8c9d4da560ab19c/core/java/android/database/sqlite/SQLiteProgram.java#L166-L171

Compare to Arrays.hashCode(Object[]), which we similarly bind as:

public static unsafe int HashCode (Java.Lang.Object[]? a);

i.e. array can be null, but elements can't be. Compare to the source code, which does allow for null elements:

• https://android.googlesource.com/platform/libcore/+/fe39951/luni/src/main/java/java/util/Arrays.java#1249

implying that we should have bound this as Object?[]?, not Object[]?.

I'm not sure there's a "good" answer here, particularly since the few Java annotations I've seen around nullability work at an "outermost type" level.

However, Kotlin appears to support it:

class E {
    fun m1(v : Array<String?>?)
    {
    }

    fun m2(v : Array<String>?)
    {
    }
}

…but that said, our //@not-null attribute (01d0684) only works on the "outermost type" level as well, meaning we can't differentiate Array<String?> from Array<String?>? anyway…

[jpobst]

Technically some of the annotations we look for do support string?[]?, like Lorg/jetbrains/annotations/NotNull;:

package org.jetbrains.annotations;

@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})
public @interface NotNull { ... }

(source)

(My understanding is it's the ElementType.TYPE_USE that allows specifying string?[].)

However the Android ones we primarily use only support string[]?:

package androidx.annotation;

@Target({METHOD, PARAMETER, FIELD, LOCAL_VARIABLE, ANNOTATION_TYPE, PACKAGE})
public @interface NonNull { ... }

(source)

As you pointed out, our tools do not support specifying the array element nullability.

While we probably wouldn't gain much by adding support to our tools since the primary Android annotations don't support it, we probably should update generator to always emit nullable array elements, since the default should be nullable unless specified @NotNull.

So we should always emit string?[] or string?[]?.

[jpobst]

Since this involves nullable reference types, should we enable nullable reference types on Java.Interop.Tools.Cecil.dll? :-)

Let's keep our PR's focused. 😉

I'm happy to do it in a separate PR. I should probably look at which assemblies in this repo still need NRT added.

[jonpryor]

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

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

Imagine you have a `Mono.Android.dll` build which has nullable
reference types enabled, with a type declaration of:

	namespace Java.Lang {
	    public partial interface IIterable {
	        Java.Util.IIterator Iterator ();
	    }
	}

Then imagine that you bind a Java library which contains an interface
which implements `java.lang.Iterable`, resulting in a binding:

	namespace My.Example {
	    partial interface IMyIterable : Java.Lang.IIterable {
	    }
	}

`generator` also implements `IMyIterableInvoker`, to support
invoking the `IMyIterable` interface.  Unfortunately, because
`IIterable` comes from IL, `generator` doesn't know about nullability,
and thus assumes that *everything* can be nullable:

	namespace My.Example {
	    partial class IMyIterableInvoker : Java.Lang.Object, IMyIterable {
	        public Java.Util.IIterator? Iterator() {…}
	    }
	}


This results in a CS8766 warning:

	warning CS8766: Nullability of reference types in return type of
	'IIterator? IMyIterableInvoker.Iterator()' doesn't match implicitly implemented member
	'IIterator IIterable.Iterator()' (possibly because of nullability attributes).

Fix this use-case by updating `Java.Interop.Tools.Cecil.dll` to know
enough about the encoding of nullability information within
[`NullableAttribute` and C# Nullable Metadata][0] to determine if a
parameter type or return type can be null.  This allows `generator`
to appropriately emit:

	namespace My.Example {
	    partial class IMyIterableInvoker : Java.Lang.Object, IMyIterable {
	        public Java.Util.IIterator Iterator() {…}
	    }
	}

Note that our import doesn't support "nested nullability"; it only
handles nullability of the "outermost type".  It cannot differentiate
between `java.lang.String[]?` and `java.lang.String[]?` (possibly
null array that can have possibly null elements vs. possibly null
array that can only have non-null elements).  We don't currently
consider this to be a problem because most of the Java annotations
*also* don't support this form of differentiation.  Kotlin *can*;
`Array<String?>` differs from `Array<String?>?`.  However, that's
apparently not handled via Java annotations, but instead via the
Kotlin Protobuf data.

We don't feel a need to support nested nullability at this time.

TODO: All the above said… we think we made a mistake in 01d06845
around the binding of arrays, and other "nested generic-like" types.
We currently bind an annotation-less Java method such as
`example(Object[])` as `Example(Java.Lang.Object[]?)`, but we should
*actually* bind this as `Example(Java.Lang.Object?[]?)`!
This is tracked in xamarin/java.interop#917

[0]: https://github.com/dotnet/roslyn/blob/2da789af0f404e15871b6e6e17706185a2707ab4/docs/features/nullable-metadata.md