[generator] Add nullable reference types (NRT) support.

[jpobst]

Context: #468
XA Companion PR: dotnet/android#4227

Adds nullable reference types (NRT) support in generator, when passed the flag -lang-features=nullable-reference-types, using Java's @NotNull annotations to annotate the C# public API.

Goals

Surface Java Provided Annotations

generator should be able to interpret the nullable annotations provided by an input jar (technically the info provided by class-parse). It should use this information to generate bindings that expose the same nullable annotations on the produced public C# API.

For example, this Java:

class Foo {
  void Bar (@NotNull object baz, string value) { ... }
}

Should generate this C# API:

class Foo {
  void Bar (object baz, string? value) { ... }
}

No Additional Infrastructure Warnings

The generated binding code should not produce any additional warnings on its own. That is, the internal plumbing code itself should not create warnings.

Non-Goals

Generated Binding Code Audit

There exists cases in our generated plumbing code that do not play nicely with the provability of NRT. For example, we may generate code like this:

int Java.Lang.IComparable.CompareTo (Java.Lang.Object o)
{
  return CompareTo (global::Java.Interop.JavaObjectExtensions.JavaCast<Android.Util.Half>(o));
}

Technically JavaCast<> can return null, which cannot be passed to CompareTo (object o) because it does not accept null. In these cases we liberally use the null forgiveness operator (!) to suppress warnings. It may be desirable to change how this code is structured to be better provably null-safe, however this PR does not attempt to make those modification. It is assumed that the code is currently working, so null is prevented here via other mechanisms. No functional changes are made to generated code.

Fixing "Wrong" Java Annotations

There are cases where Java nullable annotations can create scenarios that will produce warnings in C#, particularly around inheritance. For example:

class Foo {
  void Bar (@NotNull object baz) { ... }
}

class Foo2 : Foo {
  @override void Bar (object baz) { ... }
}

This would produce a C# warning such as:

CS8610: Nullability of reference types in type of parameter 'baz' doesn't match overridden member.  

generator will not attempt to resolve this error, it is an exercise for the user. This can be accomplished by fixing the Java code or using metadata such as:

<attr path="/api/package[@name='blah']/class[@name='Foo2']/method[@name='Bar' and count(parameter)=1 and parameter[1][@type='object']]/parameter" name="not-null">true</attr>

Release Notes

I don't think this change needs to be specifically called out, this is just a reminder to call it out in the 2 places it will be visible to users:

• Adding NRT annotations to Mono.Android.dll: [Mono.Android] Add NRT annotations. android#4227
• Enabling NRT annotations in Java Bindings Projects: no PR yet.

[jonpryor]

Do we have commit message info for why the introduction of [MaybeNull]? Why wasn't this needed before?

Additionally, there should be a corresponding change to src/Java.Interop/Java.Interop/JniBuiltinMarshalers.tt. Visual Studio (for Mac?) should update JniBuiltinMarshalers.cs based on JniBuiltinMarshalers.tt.

[jonpryor]

Probably "too late", but did we need the Java.Interop.dll changes intermixed with the generator changes?

[jpobst]

The method it is overriding specifies [MaybeNull], so you get a "nullability mismatch warning":

https://github.com/xamarin/java.interop/blob/master/src/Java.Interop/Java.Interop/JniValueMarshaler.cs#L221

I can't find an instance of it right now, from what I can tell this warning was still being tweaked by the Roslyn team, and it only appears on newer versions of the C# compiler.

[jpobst]

I removed these changes for now, as they'll need to be done in the .tt files. There's about 60 warnings that I am seeing now:

https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3665517&view=logs&j=f31c9f97-4411-58e7-49ac-fc73f645e6b6&t=cbeb8522-7b64-5cc0-7bee-eff0e164a409

I don't know how I missed these before. I suggest we go ahead and continue getting this PR merged and then we can go back and clean up those warnings.

[jonpryor]

These changes should likewise prompt a change to src/Java.Interop/Java.Interop/JavaPrimitiveArrays.tt.

[jonpryor]

This change concerns me: why did it occur? For that matter, how was it java.lang.String in the first place?

Fortunately this doesn't trigger any API breakage in Mono.Android.dll, as per https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3648625&view=results

I just find this really weird. What change to generator is triggering this? Was it deliberate? What's the rationale?

I don't see this mentioned in the commit message.

[jpobst]

The change is that previously we were writing this type from property.Type, which is Setter != null ? Setter.Parameters [0].Type : Getter.ReturnType.

https://github.com/xamarin/java.interop/blob/master/tools/generator/Java.Interop.Tools.Generator.CodeGeneration/CodeGenerator.cs#L1690

This was refactored and combined with other places that were writing property.Getter.ReturnType, to a new method which always prefers Getter.ReturnType:

https://github.com/xamarin/java.interop/pull/563/files#diff-18357c6193fe262963d21bb1d0bab9feR99-R105

The test is a unit test and does not go through the full pipeline of resolving all the types, thus its Setter.Parameters [0].Type isn't changing from java.lang.String to string, but its Getter.ReturnType is getting resolved to string.

That is, the unit test code is incorrectly this:

• Setter.Parameters [0].Type = java.lang.String
• Getter.ReturnType = string

In the real world, Setter.Parameters [0].Type and Getter.ReturnType should always refer to the same symbol, so this should not affect real code.