[Mono.Android] Add NRT annotations. #4227

jpobst · 2020-02-06T21:38:53Z

Companion JI PR: dotnet/java-interop#563

This PR annotates all levels of Mono.Android.dll with nullable reference type (NRT) annotations, pulled directly from the Java source.

Additionally, our hand written code has been audited and updated to include NRT annotations.

Notes

There are 8 new warnings caused by the change of the form:

Android.Runtime/JavaDictionary.cs(655,24): warning CS8714: The type 'K' cannot be used as type parameter 'TKey' in the generic type or method 'IDictionary<TKey, TValue>'. Nullability of type argument 'K' doesn't match 'notnull' constraint.

This is due to the BCL being improperly annotated. The change has since been reverted, but it has not made it into various distribution packs: dotnet/runtime#793.

There are a considerable amount of warnings (~400) caused by mismatched annotations when members are overridden, like this:

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

While it may be desirable to fix these, it is a non-trivial job that can be prioritized separately if desired.

In the mean time, this set of warnings has been disabled:

<NoWarn>8609;8610;8614;8617;8613;8764;8765;8766;8767</NoWarn>

Release Notes (draft)

Mono.Android.dll Nullable Reference Types

Mono.Android.dll assemblies of all platform levels are now annotated with C#8's nullable reference types (NRT). Users who opt their applications into this feature with <Nullable>enable</Nullable> will receive warnings if their code does not properly account for possible null values.

General documentation for the NRT feature is available here: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references.

Note:
The majority of Mono.Android.dll is automatically generated from the Android Java source, including these new annotations. As such, we will not be manually fixing places where the Android source code is not annotated correctly.

If there is an error regarding nullability for any of the Mono.Android API's that Xamarin adds to the Android source (such as JavaList or InputStreamAdapter), please file a bug so we can properly annotate our additions.

Context: dotnet/android#4227 Our generated code *may* predate `var`, or at least predate `var` becoming widely accepted. As such, we generate code like this: java.code.MyClass __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer); string key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer); Migrate this code to `var` as it simplifies `generator` and reasoning about behavior when "cross-compiling" code between C#7 and C#8-with- [Nullable Reference Type][0] toolchains: var __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer); var key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer); [0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references

@OverRide

Fixes: #468 Context: dotnet/android#4227 Add [C#8 nullable reference type][0] (NRT) support to `generator` when given `generator -lang-features=nullable-reference-types`. This uses a variety of Java annotations to infer nullable information (18c29b7) via the `//method/@return-not-null` and `//parameter/@not-null` attribute values within `api.xml` to "forward" nullability information to the generated C# binding. ~~ Goals ~~ `generator` should be able to interpret the nullable annotations provided by an input `.jar` file (via `class-parse`). It should use this information to generate bindings that expose similar nullable annotations on the produced public C# API. For example, this Java: // Java public class Foo { public void bar (@NotNull Object baz, String value) { … } } Should generate this C# API: // C# Binding public class Foo : Java.Lang.Object { public void Bar (Java.Lang.Object baz, string? value) { … } } Additionally, 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 ~~ There exists cases in our generated plumbing code that do not play nicely with the provability of C#8 nullable reference types. 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 forgiving operator (`!`)][1] 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. Additionally, there are cases where Java nullable annotations can create scenarios that will produce warnings in C#, particularly around inheritance. For example: // Java public class Base { public void m (@NotNull Object baz) { … } } public class Derived extends Base { @OverRide public void m (Object baz) { … } } This would produce a C# warning such as: CS8610: Nullability of reference types in type of parameter 'M' 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` to override the `//@not-null` attribute 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> ~~ Unit Test Changes ~~ Several of the unit test "expected output" files changed their property type from `java.lang.String` to `string`. This occurred due to a related refactoring of parameter & return type generation code. This change shouldn't be "user visible" because the unit tests don't go through a "complete" pipeline which would involve ensuring that get- and set-method pairs have consistent parameter & return types. [0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references [1]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-forgiving

Context: dotnet/android#4227 Our generated code *may* predate `var`, or at least predate `var` becoming widely accepted. As such, we generate code like this: java.code.MyClass __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer); string key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer); Migrate this code to `var` as it simplifies `generator` and reasoning about behavior when "cross-compiling" code between C#7 and C#8-with- [Nullable Reference Type][0] toolchains: var __this = global::Java.Lang.Object.GetObject<java.code.MyClass> (jnienv, native__this, JniHandleOwnership.DoNotTransfer); var key = JNIEnv.GetString (native_key, JniHandleOwnership.DoNotTransfer); [0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references

@OverRide

Fixes: #468 Context: dotnet/android#4227 Add [C#8 nullable reference type][0] (NRT) support to `generator` when given `generator -lang-features=nullable-reference-types`. This uses a variety of Java annotations to infer nullable information (18c29b7) via the `//method/@return-not-null` and `//parameter/@not-null` attribute values within `api.xml` to "forward" nullability information to the generated C# binding. ~~ Goals ~~ `generator` should be able to interpret the nullable annotations provided by an input `.jar` file (via `class-parse`). It should use this information to generate bindings that expose similar nullable annotations on the produced public C# API. For example, this Java: // Java public class Foo { public void bar (@NotNull Object baz, String value) { … } } Should generate this C# API: // C# Binding public class Foo : Java.Lang.Object { public void Bar (Java.Lang.Object baz, string? value) { … } } Additionally, 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 ~~ There exists cases in our generated plumbing code that do not play nicely with the provability of C#8 nullable reference types. 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 forgiving operator (`!`)][1] 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. Additionally, there are cases where Java nullable annotations can create scenarios that will produce warnings in C#, particularly around inheritance. For example: // Java public class Base { public void m (@NotNull Object baz) { … } } public class Derived extends Base { @OverRide public void m (Object baz) { … } } This would produce a C# warning such as: CS8610: Nullability of reference types in type of parameter 'M' 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` to override the `//@not-null` attribute 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> ~~ Unit Test Changes ~~ Several of the unit test "expected output" files changed their property type from `java.lang.String` to `string`. This occurred due to a related refactoring of parameter & return type generation code. This change shouldn't be "user visible" because the unit tests don't go through a "complete" pipeline which would involve ensuring that get- and set-method pairs have consistent parameter & return types. [0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references [1]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-forgiving

…ing" changes.

The `UpdateRegistrationSwitch` generates `CallRegisterMethodByIndex` method during linking. The signature of this method changed, where second argument has `int?` type now. Handle `int?` by calling `System.Nullable`1<int32>::GetValueOrDefault()` and passing its return value to the switch. New IL code looks like this: .method private hidebysig static bool CallRegisterMethodByIndex([Java.Interop]Java.Interop.JniNativeMethodRegistrationArguments arguments, [mscorlib]System.Nullable`1<int32> typeIdx) cil managed { // Code size 9285 (0x2445) .maxstack 2 .locals init (bool V_0, [mscorlib]System.Nullable`1<int32> V_1) IL_0000: ldarg.1 IL_0001: stloc.1 IL_0002: ldloca.s V_1 IL_0004: call instance !0 [mscorlib]System.Nullable`1<int32>::GetValueOrDefault() IL_0009: switch ( ...

We don't want to fail silently when the signature changes

Before creating `CallRegisterMethodByIndex` body. In some builds there was `bool` variable already defined, while in CI builds there was none. Thus clear the collection, add our `int?` variable and use it. Newly generated IL looks like: .method private hidebysig static bool CallRegisterMethodByIndex([Java.Interop]Java.Interop.JniNativeMethodRegistrationArguments arguments, [mscorlib]System.Nullable`1<int32> typeIdx) cil managed { // Code size 9285 (0x2445) .maxstack 2 .locals init ([mscorlib]System.Nullable`1<int32> V_0) IL_0000: ldarg.1 IL_0001: stloc.0 IL_0002: ldloca.s V_0 IL_0004: call instance !0 [mscorlib]System.Nullable`1<int32>::GetValueOrDefault() IL_0009: switch ( ...

Context: dotnet/java-interop@01d0684 Annotates all API levels of `Mono.Android.dll` with [C#8 Nullable Reference Type][0] (NRT) annotations, pulled directly from the Java `.jar` files. Additionally, our hand written code has been audited and updated to include NRT annotations. ~~ Notes ~~ There are 8 new warnings caused by this change of the form: Android.Runtime/JavaDictionary.cs(655,24): warning CS8714: The type 'K' cannot be used as type parameter 'TKey' in the generic type or method 'IDictionary<TKey, TValue>'. Nullability of type argument 'K' doesn't match 'notnull' constraint. This is due to the BCL being improperly annotated. The change has since been reverted, but it has not made it into various distribution packs: dotnet/runtime#793 There are a considerable number of warnings (~400) caused by mismatched annotations when members are overridden, such as: CS8610: Nullability of reference types in type of parameter 'baz' doesn't match overridden member. While it may be desirable to fix these, it is a non-trivial job that will be prioritized separately. In the mean time, this set of warnings has been disabled: <NoWarn>8609;8610;8614;8617;8613;8764;8765;8766;8767</NoWarn> Finally, when `$(AndroidGenerateJniMarshalMethods)`=True and `jnimarshalmethod-gen.exe` is executed, the resulting app was crashing because `MagicRegistrationMap.CallRegisterMethodByIndex()` had a parameter change from `int` to `int?`. Fix this by calling `Nullable<int>.GetValueOrDefault()` with the `switch`. `MonoDroidMarkStep.UpdateRegistrationSwitch()` now emits IL like: .method private hidebysig static bool CallRegisterMethodByIndex([Java.Interop]Java.Interop.JniNativeMethodRegistrationArguments arguments, [mscorlib]System.Nullable`1<int32> typeIdx) cil managed { // Code size 9285 (0x2445) .maxstack 2 .locals init ([mscorlib]System.Nullable`1<int32> V_0) IL_0000: ldarg.1 IL_0001: stloc.0 IL_0002: ldloca.s V_0 IL_0004: call instance !0 [mscorlib]System.Nullable`1<int32>::GetValueOrDefault() IL_0009: switch ( … Co-authored-by: Radek Doulik <[email protected]> [0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references

jpobst force-pushed the nrt-monoandroid branch 5 times, most recently from 3df0bce to 486dc51 Compare April 3, 2020 14:34

jpobst force-pushed the nrt-monoandroid branch 2 times, most recently from 59af5d1 to 6acc45f Compare April 8, 2020 18:25

jpobst mentioned this pull request Apr 10, 2020

[generator] Add nullable reference types (NRT) support. dotnet/java-interop#563

Merged

jpobst changed the title ~~[WIP] [Mono.Android] Batch of changes to enable NRT in Mono.Android.dll.~~ [Mono.Android] Add NRT annotations. Apr 10, 2020

jpobst marked this pull request as ready for review April 17, 2020 14:09

jpobst requested review from grendello, jonpryor and radekdoulik as code owners April 17, 2020 14:09

grendello approved these changes Apr 17, 2020

View reviewed changes

jpobst and others added 12 commits April 23, 2020 09:56

[Mono.Android] Batch of changes to enable NRT in Mono.Android.dll.

fe8dd5e

Fix merge conflict booboo.

f5958ec

More nullability fixes.

c2574bc

[XAT.JavadocImporter] Update LangVersion and add NullableAttributes.cs.

f75bd3c

Disable ApiCompat check for now, since nullable attributes are "break…

00453b6

…ing" changes.

[Mono.Android] Enable NRT generator flag.

2bdf86d

More fixes.

53c1802

[Mono.Android] Fix NRT warnings caused by an annotated BCL.

8c60f45

Tweaks.

4bc8a16

[api-compat] Exclude NRT attributes.

684461a

Get rid of the test which I added in previous commit

070f318

We don't want to fail silently when the signature changes

radekdoulik and others added 2 commits April 23, 2020 09:58

Release notes.

ed50703

jpobst force-pushed the nrt-monoandroid branch from 4e9be13 to ed50703 Compare April 23, 2020 15:02

jpobst requested a review from brendanzagaeski as a code owner April 23, 2020 15:02

Bump to JI master.

e1f451b

jonpryor merged commit d471b4a into master Apr 23, 2020

jonpryor deleted the nrt-monoandroid branch April 23, 2020 22:39

jpobst mentioned this pull request Apr 24, 2020

Nullable Reference Types support #2042

Closed

brendanzagaeski added this to the In Preview: 10.4 (d16-7) milestone May 19, 2020

github-actions bot locked and limited conversation to collaborators Jan 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mono.Android] Add NRT annotations. #4227

[Mono.Android] Add NRT annotations. #4227

jpobst commented Feb 6, 2020 •

edited

Loading

[Mono.Android] Add NRT annotations. #4227

[Mono.Android] Add NRT annotations. #4227

Conversation

jpobst commented Feb 6, 2020 • edited Loading

Notes

Release Notes (draft)

Mono.Android.dll Nullable Reference Types

jpobst commented Feb 6, 2020 •

edited

Loading