Enable C# 8's nullable annotations

[ForNeVeR]

After #228 is merged, we should be able to enable the new nullable reference types all over our project. It's important so we could provide the nullability information through our API for the users.

Remaining points to do:

• [ ] fix this: https://github.com/ForNeVeR/wpf-math/blob/c8c6a5f4efc88ddbc0f6807693c7f09906470e20/src/WpfMath/Atoms/RowAtom.cs#L62
• [ ] BigOperatorAtom.BaseAtom should become non-nullable
• [ ] ScriptsAtom.BaseAtom should become non-nullable
• [ ] make StyledAtom's constructor parameter atom non-nullable, add asserts
• [ ] make TypedAtom.Atom non-nullable, add asserts
• [ ] make UnderlinedAtom.Atom non-nullable
• [ ] make VerticalCenteredAtom.Atom non-nullable
• [ ] make OverUnderBox.ScriptBox non-nullable
• [ ] PredefinedFormulaParser shouldn't return null? to fix predefinedFormula!.RootAtom! (TexFormulaParser.cs:648)
• [ ] make Radical.BaseAtom non-nullable
• [ ] remove TODO / fix the issue with RowAtom ctor (see RowAtom(SourceSpan? source, IEnumerable<Atom?> elements))
• [ ] fix an issue with RowAtom(SourceSpan? source, Atom? baseAtom) (see the comment there)
• [ ] review any remaining comments about // Nullable TODO and remove TODO or fix them
• [ ] review any remaining comments about // Nullable

[ForNeVeR]

Please also fix these TODOs:

• [x] https://github.com/ForNeVeR/wpf-math/blob/c8c6a5f4efc88ddbc0f6807693c7f09906470e20/src/WpfMath/Atoms/RowAtom.cs#L76
• [ ] https://github.com/ForNeVeR/wpf-math/blob/c8c6a5f4efc88ddbc0f6807693c7f09906470e20/src/WpfMath/Atoms/RowAtom.cs#L62

[ygra]

I thought I'd start on this issue. However, a few questions arose regarding the project in general. I haven't yet read through everything and mostly followed the warnings to fix nullability issues. But of course, doing this naïvely requires a lot of nullable types.

The project multi-targets .NET 4.5.2 and .NET Core 3.0. This means that attributes that can be used to aid the compiler regarding null can only be used in one of the target frameworks, e.g. NotNullWhen. One option would be something like:

        public static bool TryGetAtom(string name,
                                      SourceSpan? source,
#if NETCOREAPP3_0
                                      [NotNullWhen(true)]
#endif
                                      out SymbolAtom? atom)
        {
            ...
        }

The other option would be to just accept that things are not as good as they could be and add ! to usages where the compiler complains.

A few things that can be null are already spelled out in comments. However, some things, like basically all atoms, seem to be nullable in many instances, but seemingly not null in others. The contagious nature of nullable types, however, causes a lot of the nullable-ness to bleed into code that seemingly assumes things to never be null. I wonder whether there's a general idea as to whether those things can be null in general.

Is this issue intended to result in a warning-free build? A bug-free build? a complete overhaul of nullability in the whole library? As mentioned, there's a few places that are very ambiguous as to whether null is really expected and desired.

[ForNeVeR]

Thanks for your help!

I haven't yet read through everything and mostly followed the warnings to fix nullability issues.

Yep, that's what I had in mind when writing this issue.

The project multi-targets .NET 4.5.2 and .NET Core 3.0. This means that attributes that can be used to aid the compiler regarding null can only be used in one of the target frameworks, e.g. NotNullWhen.

You could conditionally include a NuGet package with the corresponding attributes, and they should be picked up properly. See also a request for the runtime team to provide such package: https://github.com/dotnet/runtime/issues/30801.

I wonder whether there's a general idea as to whether those things can be null in general.

Not quite. One of the purposes of this task is to find the places where the nullability is ambiguous, and possibly take a closer look a bit later. All I could remember from the top of my head is that Atom.SourceSpan is supposed to be nullable almost universally. Everything else is in the gray zone I'd say.

Is this issue intended to result in a warning-free build?

Yes.

A bug-free build?

I don't think it's possible (at least it's an impractical requirement as of now), even if we only talk about the NullReferenceExceptions which could be fixed by carefully fixing all the nullability warnings.

a complete overhaul of nullability in the whole library?

Definitely not (yet).

As mentioned, there's a few places that are very ambiguous as to whether null is really expected and desired.

Yes, that's expected from a code base that old.

Generally, as the result of this task, I'd like to enable "nullability warnings as errors" flag, annotate the whole code base and shut the ambiguous places with a few ! operators (there shouldn't be too much of them, right? I hope…). At least they will become explicit and more visible. We can decide on them later; fixing all the nullability bugs is out of scope of this task.

[ForNeVeR]

Also, please note that I'd also accept a partial migration (i.e. covering only some files in the project) if it's impossible/too tedious to migrate the whole project at once. One practice I'd recommend to consider:

1. Enable nullable reference types in the whole project (so any new files would automatically work with enabled nullability)
2. Add #nullable disable to every file in the project
3. Enable "nullability warnings as errors" flag
4. Incrementally drop the #nullable disable and fix the warnings/errors, while possibly sending reasonably-sized PRs

[ygra]

Ok, I've stashed the mishmash of fixes I've had to far (down to 47 warnings) and decided to start anew with a gradual approach (I can still pick changes from the stash to avoid duplicating work). This makes it easier to commit piecemeal enablings and fixes as well as making it easier to track what nullability inclusion now brought nullable types all over everything (there were a few very viral parts that could perhaps better be tackled at the source to not let them be null to begin with).

I'll probably issue the first partial PR today or tomorrow, since I'm offline for a week after that and building a WPF library on my Linux laptop won't work so great.

One thing I noticed is that with TargetFramework=net452 the analyzer seems less smart and ignores things like Debug.Assert and other things. This made a few ! necessary. I've commented all of them for now and if someone has a better idea to solve a particular instance we can fix them in another way (! is just the quickest fix to silence the diagnostic, of course ;-)).

[ForNeVeR]

One thing I noticed is that with TargetFramework=net452 the analyzer seems less smart and ignores things like Debug.Assert and other things. This made a few ! necessary.

This is to be expected: the standard library for .NET 4.5.2 isn't annotated by nullable types at all. So, probably, we shouldn't care about it, and only enable nullable warnings on .NET Core 3.

I want our libraries for 4.5.2 to include the nullable type annotations in the API, though. So it's okay to ignore errors on 4.5.2 completely, and only care about the new stuff, while spreading the resulting annotations for all the artifacts we produce.

[ygra]

This is to be expected: the standard library for .NET 4.5.2 isn't annotated by nullable types at all.

Aaah, that's a good point I haven't considered. But yeah, in that case I'll probably make the nullability conditional to .NET Core and can get rid of a few exclamation marks and comments.

[ForNeVeR]

That's alright; please do whatever unblocks the future work. We'll figure out how to proceed with the legacy framework later.

[ForNeVeR]

I've decided to keep this issue open even after merging #277. Remaining small pieces of work are enumerated in the opening post.