Expose nullability annotations (and enable nullability warnings internally in production code)

[blairconrad]

We'll need to support .NET Core 3.0 and C# 8.0.
I fear there may be challenges with the PublicApiGenerator. At least there were a few months ago when I started looking at this and eventually dropped it.

[blairconrad]

I thought I'd start out by submitting a few PRs that clean up some our internal code. Nothing that requires a language or framework change at this point, but that would reduce the pain we have in the future.

[thomaslevesque]

What exactly do you have in mind for nullable ref types?

[blairconrad]

I wasn't anticipating a question of this type! Now I fear I do not understand the feature. I guess technically I'm more interested in non-nullable reference types.

I'd hoped to enable the nullable annotation context as well as the nullable warning context. Clients would know which arguments must not be null (and which return values won't be) and we would know the same about our own methods.

Do you think this is a bad idea? Not worthwhile?

[thomaslevesque]

Do you think this is a bad idea?

Not at all. I was just wondering if you just wanted to enable it for internal use, or actually expose the nullability annotations to package consumers (both are useful, of course).

[blairconrad]

Ah, sorry. If I had to choose, the second. However.

[thomaslevesque]

Both, yes. Anyway, I don't think we could do the first without the second (for public types)

[blairconrad]

I think I've gone about as far as I can without taking advantage of the new language features. Or at least without a discussion. Next steps, as I see them, not necessarily in this order:

• enable nullable warnings and annotations. This is a breaking change, but we're already making breaking changes
• conduct a public API review. We will need to use some nullable reference types in public members
• address warnings
• potentially remove Guard.Against calls for internal methods where we know we'll not have incoming nulls

Addressing warnings will be interesting. As I type, there are about 90 warnings, although #1680 will take care of about 20 of those. There are at least 4 major classes of remaining warnings:

• legitimate places where we need to support null. We'll likely have to ? those
• TryGet style members that return a bool and set an out parameter. When we return false, usually the value is null. Sometimes it has the potential to be null even in a truthy situation. We can either rewrite to return a Maybe or just add ?s. I don't mind the ?, since it's obvious what the method is doing. For situations where the out is not null when we return true, we can potentially apply [MaybeNullWhen(false)]
• *Result style classes where we return something that sort of acts like an Either. We can accept that we might have null reasons for failure or null results and sprinkle in ?s or try to implement these classes via a formal Either, like Either<object, String> for something that represents a creation result or a reason for failure. I'm leaning toward ?s.
• stuff around unconstrained typeparams. Honestly, I get lost here.

I haven't looked closely at the analyzer warnings. Most of them come from our calls to external methods that might return null. Maybe ?s are the way to go.

My overall feelings are that the more "private" a usage is, the more likely I am to be happy with a ?. Local variables and privates? Sure, if avoiding them isn't too hard. On protected and public members, I prefer to work harder, as it just introduces. Is this unreasonable?

At some point, we may find that we want to introduce a standalone Maybe or Either. If so, we'll have to consider whether we want to roll our own or take a package dependency, but I think we can hold off on that. I may just be buying trouble.

[thomaslevesque]

* potentially remove `Guard.Against` calls for internal methods where we know we'll not have incoming `null`s

For internal methods, yes. For public methods, we have to keep them. And we can probably decorate Guard.AgainstNull with an attribute that tells the compiler it's a null check (I don't remember the name)

* Sometimes it has the potential to be `null` even in a truthy situation

Oh? Examples?

* We can either rewrite to return a `Maybe` or just add `?`s. I don't mind the `?`, since it's obvious what the method is doing.

Yes, ? is fine in those cases I think.

* For situations where the `out` is not `null` when we return `true`, we can potentially apply `[MaybeNullWhen(false)]`

👍

* `*Result` style classes where we return something that sort of acts like an Either. We can accept that we might have null reasons for failure or null results and sprinkle in `?`s or try to implement these classes via a formal Either, like `Either<object, String>` for something that represents a creation result or a reason for failure. I'm leaning toward `?`s.

I like the idea of an either-style type. Something like creationResult.IsSuccess(out object createdObject, out string failureReason), maybe. (with [MaybeNullWhen(true/false)]). If it doesn't work out, we can always fall back to nullables.

* stuff around unconstrained typeparams. Honestly, I get lost here.

Yeah, it's a mess...

I haven't looked closely at the analyzer warnings. Most of them come from our calls to external methods that might return null. Maybe ?s are the way to go.

Did you check again after the upgrade to .NET Standard 2.0? I updated to the latest Roslyn packages, they might have nullability annotations.

My overall feelings are that the more "private" a usage is, the more likely I am to be happy with a ?. Local variables and privates? Sure, if avoiding them isn't too hard. On protected and public members, I prefer to work harder, as it just introduces. Is this unreasonable?

Sounds good to me. Nullables aren't that bad, since the compiler will force us to check them before use.

At some point, we may find that we want to introduce a standalone Maybe or Either. If so, we'll have to consider whether we want to roll our own or take a package dependency, but I think we can hold off on that. I may just be buying trouble.

Even though I wrote my own Option type, I think we should avoid taking a dependency for this. It's easy to roll our own, and we can tailor it to our needs.

[blairconrad]

And we can probably decorate Guard.AgainstNull with an attribute that tells the compiler it's a null check (I don't remember the name)

Of course

• Sometimes it has the potential to be null even in a truthy situation
  Oh? Examples?

Maybe I spoke too soon. But it's something to keep our eyes out for!

• *Result style classes where we return something that sort of acts like an Either. We can accept that we might have null reasons for failure or null results and sprinkle in ?s or try to implement these classes via a formal Either, like Either<object, String> for something that represents a creation result or a reason for failure. I'm leaning toward ?s.

I like the idea of an either-style type. Something like creationResult.IsSuccess(out object createdObject, out string failureReason), maybe. (with [MaybeNullWhen(true/false)]). If it doesn't work out, we can always fall back to nullables.

I don't understand that usage. But let's worry about it when we get to it

I haven't looked closely at the analyzer warnings. Most of them come from our calls to external methods that might return null. Maybe ?s are the way to go.

Did you check again after the upgrade to .NET Standard 2.0? I updated to the latest Roslyn packages, they might have nullability annotations.

I've been building the world (all TFMs) and accumulating unique warnings, like this:

.\build.cmd build | 
  Select-String -CaseSensitive \bwarning\b |
  Sort-Object -Unique |
  Out-File -Encoding utf8 -Width 1000 ../nullwarnings/HEAD
copy ..\nullwarnings\HEAD "../nullwarnings/$(git branch --show-current)"
copy ..\nullwarnings\HEAD "../nullwarnings/$(git rev-parse HEAD)"

I didn't notice a drop in the number of warnings. But maybe I wasn't paying attention. There are only about 17 in there now.
Here's my current list of warnings: https://docs.google.com/spreadsheets/d/1RRv0aQJgcjGQBMs74QCIY42v0nMi8lH14b38lopb4aM/edit?usp=sharing

[afakebot]

This change has been released as part of FakeItEasy 6.0.0-beta.1.