Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add C# 8 nullability annotations to bindings #468

Closed
cosminstirbu opened this issue Aug 13, 2019 · 3 comments
Closed

Add C# 8 nullability annotations to bindings #468

cosminstirbu opened this issue Aug 13, 2019 · 3 comments
Assignees
@jpobst
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Milestone
11.0 (d16-7)

Comments

@cosminstirbu
Copy link

Hello,

Are there any plans to update the Xamarin.Android bindings (and the generator it self) to support C# 8 nullability?

Since Kotlin adoption by Google the Java Android SDK and the AndroidX libraries have been enhanced to expose the @nullable annotation to help Kotlin indicate if it a value can be null or not.

The bindings generator could also leverage the @nullable annotation to expose C# 8 nullable types.

On Xamarin.iOS there is an issue already opened to capture this xamarin/xamarin-macios#6154 and since I didn't find one for Xamarin.Android I've decided to open one myself.

Thank you,
Cosmin
@jpobst
Copy link
Contributor

jpobst commented Aug 13, 2019

It's something we've started discussing over the past few weeks. Currently we are working on bringing C# 8's default interface methods to Xamarin.Android, as it's a large gap compared to Java we've needed for a long time.

Nullable reference types is one of the things we are considering after that. It's good to know that the Android SDK has been annotated in a way we should be able to use. That was one of the questions we had that we hadn't explored yet. Obviously that makes the feature much more valuable instead of just marking everything as nullable because we don't actually know.

@jpobst jpobst transferred this issue from dotnet/android Aug 13, 2019
@jonpryor
Copy link
Member

Related: in order to reasonably map the @nullable annotation -- do we know the exact type involved here? Kotlin appears to use org.jetbrains.annotations.NotNull -- we'll need to add support for parsing annotations -- PR #467 is a start, but it only handles types, not parameters -- but also propagate that info into the XML description.

We'd presumably need RuntimeVisibleParameterAnnotations and RuntimeInvisibleParameterAnnotations parsing support.

@mattleibow
Copy link
Member

@jonpryor There is this SO post with a list: https://stackoverflow.com/questions/4963300/which-notnull-java-annotation-should-i-use

@jpobst jpobst removed their assignment Aug 28, 2019
@jpobst jpobst added this to the d16-6 milestone Nov 22, 2019
@jpobst jpobst mentioned this issue Nov 25, 2019
Merged
jonpryor pushed a commit that referenced this issue Dec 2, 2019
@jpobst @jonpryor
[Xamarin.Android.Tools.Bytecode] Add @not-null support (#526)
18c29b7 
Context: #468

Add support for reading `@NotNull`/`@NonNull` annotations from Java
bytecode and inserting them into `api.xml` as `//*/@not-null` and
`//*/@return-not-null` attribute values: `@not-null` is for
parameters and fields, while `@return-not-null` is emitted for method
return values.

Support for `generator` to consume the annotations will be done later.

For example, given the Java code:

        // Java:
        public class Example {
            @nonnull
            public static final Creator<DirectAction> CREATOR = /* ... */ ;

            public void foo(@nonnull android.os.Handler handler) { /* ... */ }

            @nonnull
            public Context getContext() { /* ... */ }
        }

then `Example.CREATOR` would contain:

        <field 
          deprecated="not deprecated" 
          final="true" 
          name="CREATOR" 
          jni-signature="Landroid/os/Parcelable$Creator;" 
          not-null="true" 
          static="true" 
          transient="false" 
          type="android.os.Parcelable.Creator" 
          type-generic-aware="android.os.Parcelable.Creator&lt;android.app.DirectAction&gt;" 
          visibility="public" 
          volatile="false" />

The `handler` parameter for `Example.foo()` would contain:

        <parameter 
          name="handler" 
          type="android.os.Handler" 
          jni-type="Landroid/os/Handler;" 
          not-null="true" />

And the `Example.getContext()` method would contain:

        <method 
          abstract="false" 
          deprecated="not deprecated" 
          final="true" 
          name="getContext" 
          jni-signature="()Landroid/content/Context;" 
          bridge="false" 
          native="false" 
          return="android.content.Context" 
          jni-return="Landroid/content/Context;" 
          static="false" 
          synchronized="false" 
          synthetic="false" 
          visibility="public" 
          return-not-null="true" />

We look for the following annotations:

  * Android:
      * `android/annotation/NonNull`
      * `androidx/annotation/RecentlyNonNull`

  * Java:
      * `android/support/annotation/NonNull`
      * `edu/umd/cs/findbugs/annotations/NonNull`
      * `javax/annotation/Nonnull`
      * `javax/validation/constraints/NotNull`
      * `lombok/NonNull`
      * `org/eclipse/jdt/annotation/NonNull`
      * `org/jetbrains/annotations/NotNull`

List of annotation types partially taken from:

         https://stackoverflow.com/questions/4963300/which-notnull-java-annotation-should-i-use
@jpobst jpobst mentioned this issue Feb 4, 2020
Merged
@jpobst jpobst modified the milestones: d16-6, d16-7 Apr 6, 2020
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Apr 6, 2020
@jpobst jpobst self-assigned this Apr 8, 2020
jonpryor pushed a commit that referenced this issue Apr 22, 2020
@jpobst @jonpryor
[generator] Add nullable reference types (NRT) support. (#563)
59d86de 
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
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jpobst jpobst

Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Milestone
11.0 (d16-7)
Development

No branches or pull requests
4 participants
@jonpryor @jpobst @mattleibow @cosminstirbu