Add C# 8 nullability annotations to bindings

[spouliot]

Xamarin.iOS|Mac bindings already support [NullAllowed]. This controls if null checks are generated inside (the body of) API bindings.

We often talked about doing more tooling but it did not yet materialize. Now the upcoming C# 8 supports nullability annotations so our attributes are even more useful - with even less work on our side (tooling wise).

Steps

[x] Add nullability support to xtrospection tests - so we know where our bindings are missing (or extra) [NullAllowed]. The results can be ignore until we're ready for the next step;
[ ] Fix bindings (taking care not to create a merge hell with other existing/upcoming work). This will also eliminate recurrent bugs when we're missing [NullAllowed] (since using null is not possible).
[x] Add C#8 nullability annotations to the generator

Example

This binding

[Export ("alertControllerWithTitle:message:preferredStyle:")]
UIAlertController Create ([NullAllowed] string title, [NullAllowed] string message, UIAlertControllerStyle preferredStyle);

currently generate

[Export ("alertControllerWithTitle:message:preferredStyle:")]
[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]
public static UIAlertController Create (string title, string message, UIAlertControllerStyle preferredStyle)
{
	global::UIKit.UIApplication.EnsureUIThread ();
	var nstitle = NSString.CreateNative (title);
	var nsmessage = NSString.CreateNative (message);
	...  
}

but would now generate

[Export ("alertControllerWithTitle:message:preferredStyle:")]
[BindingImpl (BindingImplOptions.GeneratedCode | BindingImplOptions.Optimizable)]
public static UIAlertController Create (string? title, string? message, UIAlertControllerStyle preferredStyle)
{
	global::UIKit.UIApplication.EnsureUIThread ();
	var nstitle = NSString.CreateNative (title);
	var nsmessage = NSString.CreateNative (message);
	...  
}

note the extra ? on the parameter types

[rolfbjarne]

Question: is adding/removing the nullability annotation from an existing binding a breaking change?

[spouliot]

@rolfbjarne I don't think it is (but need to confirm [1])
else there are breaking changes in the BCL

[1] guessing it's a custom attribute

[cosminstirbu]

Hello,

I know you guys are busy with Xcode 11 and iOS 13 stuff but I was wondering if you have a timeline in mind for this feature, giving that .NET Core 3.0 (and C# 8) release date is getting closer - https://www.dotnetconf.net/

Thank you,
Cosmin

[rolfbjarne]

@cosminstirbu we don't have a timeline yet, but I think it's safe to say it won't happen before our Xcode 11 / iOS 13 support is out. After that we'll probably have a look at what we'll do and how to prioritize it.

[Therzok]

I want to leave this doc link for brevity, as it can enhance nullability annotation information:
https://docs.microsoft.com/en-us/dotnet/csharp/nullable-attributes

[cosminstirbu]

Now that Xcode 11 / iOS 13 support is fairly stable, are there plans to move this forward?

[spouliot]

There's a work-in-progress PR
#7570

[spouliot]

PR #7570 has landed in master and being backported to d16-7

[spouliot]

PR #8335 adds and enable xtro rule
Also includes a bunch of (but not all) fixes. Remaining fixes to be scheduled later

[spouliot]

xtro rule and some (partial) updates to bindings are merged with #8335

[cosminstirbu]

I've recently cloned https://github.com/xamarin/net5-samples and noticed that it references a Xamarin.iOS version that already includes some NRT work.

However I find it a bit confusing that I receive a warning (Dereference of a possibly null reference.) when accessing the View property of a UIViewController like in the snippet below (View.BackgroundColor = UIColor.Red;).

On the Swift side, the property is defined as var view: UIView! { get set } (docs) and on Objective-C as @property(nonatomic, strong) UIView *view; (docs).

Also, I couldn't tell from the C# docs what is the C# 8 equivalent of Swift's var view: UIView! { get set } would it maybe be

[NotNull]
public UIView? View { get; set; }

This is the sample

#nullable enable

using System;
using UIKit;

public class MyViewController : UIViewController
{
    protected internal MyViewController(IntPtr handle) : base(handle)
    {
    }

    public override void ViewDidLoad()
    {
        base.ViewDidLoad();

        View.BackgroundColor = UIColor.Red;
    }
}

If this is something that you were planning to address then please excuse my eagerness on poking around with this feature 😄

[spouliot]

If this is something that you were planning to address

@cosminstirbu short answer: yes, that's the step "Fix the bindings" in the issue's description.

Long answer

Xamarin.iOS/Mac added nullability many years before Apple did. At the time there was very little and often incorrect documentation (web or headers) if null was allowed or not for an API.

Eventually Apple added their own annotations (and they was quite a bit of errors and changes since then). Xamarin.iOS/Mac started to use them for new bindings but we kept our own for the older ones.

In PR #8335 you can see three things:

1. an xtro rule that detect mismatches between Apple and Xamarin nullability attributes. That ensure all new bindings will be up-to-date with Apple (additions or changes)
2. a lot of nullability attributes updates for several (but not all) frameworks. Why not all ? too time consuming (right now) but some amount was needed to debug the xtro rule properly
3. the .ignore files have been updated (for mismatches) so the xtro rule would not report errors in new PR (that are not binding related). Life must go on until this is completed.

I can't say when this will (backlog) be completed... However it's something that is quite easy to contribute. It's largely (90%) editing the bindings and updating (deleting) the .ignore files. In some cases (10%) there might be some tests to add/update and a bit more validation (e.g. when Apple docs and headers contradict themselves).

[cosminstirbu]

Thanks for providing more context.

I'd like to take a stab at it (ideally maybe on a smaller framework just to get familiar with the process) if you could point me to a guide / documentation on how we could contribute to add missing nullability annotations.

[spouliot]

Look at the .ignore files that my PR updated https://github.com/xamarin/xamarin-macios/pull/8335/files?file-filters%5B%5D=.ignore

There's a common file (prefix) and OS specific files for each framework. If a file does not exists then it's because all the work was done. Note that xtro is used for many other things (not just nullability).

Smaller ones means less works :)

E.g. you might want to start with tests/xtro-sharpie/common-CoreLocation.ignore
and the normal workflow would be

1. open tests/xtro-sharpie/*-CoreLocation.ignore
2. open src/corelocation.cs
3. for each nullability entry in the .ignore file adjust the bindings .cs file
4. remove entries from .ignore file as you go
5. there's a corresponding .ignore file in the api-annotations-dotnet subdirectory (tests/xtro-sharpie/api-annotations-dotnet/*-CoreLocation.ignore for this example), you'll have to remove the same entries from that file too
6. build (make && make install)
7. run xtro tests, cd tests/xtro-sharpie/, make, open reports/index.html

Once complete (and when xtro does not report errors) then you can submit a PR for review.

note: the above steps assume you have cloned xamarin-macios and built it

ping us on gitter #macios-community if you have issues with the steps above

[rpendleton]

I use Rider for my Xamarin.iOS IDE and I've started to notice warnings related to nullability that seem to be incorrect.

For example:

public class MessageViewController : UIViewController
{
    public override void ViewDidLoad()
    {
        base.ViewDidLoad();

        if (View.Window != null) // warning: Expression is always true
        {
            ...
        }

        if (PresentedViewController == null) // warning: Expression is always false
        {
            ... // warning with dimmed code: Code is heuristically unreachable
        }
    }
}

In that snippet, it's perfectly valid for View.Window to be null, and the same is true for PresentedViewController, but warnings are shown for both. Apple's documentation for each of those properties indicate that the properties are nullable.

I assume these are ReSharper (opposed to Roslyn) warnings, but do you happen to know if these warnings are being driven by incorrect annotations in Xamarin.iOS? If so, is there an easy way for me to fix these as I find them? Would it be okay for me to just open PRs as I run into individual properties that are wrong, or would you prefer an entire framework to be corrected in a single PR?

[rpendleton]

I just looked at the ignore file for UIKit, and although it's a few hundred lines of missing-null-allowed and extra-null-allowed, it's actually quite a bit shorter than I was expecting for a framework as large as UIKit.

In my previous message I asked if it would be okay to update properties individually as I run into them, but it seems like it would probably be best to just fix all of the attributes in one pass. I'd be willing to do that assuming nobody else has started yet. (And perhaps after fixing a smaller framework first, such as Photos.)

If that's something you're okay with me doing, is there a particular branch I should branch off of? Or would it be best to wait until the Xcode 12 SDK changes get merged back into main?

[rolfbjarne]

In my previous message I asked if it would be okay to update properties individually as I run into them, but it seems like it would probably be best to just fix all of the attributes in one pass.

We'll be happy for any contribution, so either way works, but doing them all in one pass is probably going to be less work for all of us if you plan on doing them all anyway.

If that's something you're okay with me doing

Of course!

I'd be willing to do that assuming nobody else has started yet. (And perhaps after fixing a smaller framework first, such as Photos.)

There's nobody working on this that we know of, so you can pick any framework you want.

is there a particular branch I should branch off of?

The main branch.

Or would it be best to wait until the Xcode 12 SDK changes get merged back into main?

Yes, this would be best to avoid merge conflicts, but it shouldn't be long until it's done (a few days at most, this is the PR where we merge the Xcode 12 changes in to main: #9692)

[rolfbjarne]

I'm moving this to the .NET 7 milestone, we've done a lot of the annotations, but it's still a work in progress, and there will be some left over for .NET 7.

[rolfbjarne]

There has been a lot of work done here already, but we're not done yet, so I'm moving this to .NET 8.