[DO-NOT-MERGE] [RFC FS-1060] Nullness checking (applied to codebase)

[dsyme]

This extends #15181 by applying it to the codebase itself. Getting this green is an exit criteria for #15181.

Keeping this separate from #15181 keeps us sane, as it's easiest to work on this with a Visual Studio installed with nullness checking added to the toolchain (using a build of a green-tick feature/nullness)

git checkout feature/nullness
.\build -deploy
git checkout feature/nullness-enabled
devenv VisualFSharp.sln /RootSuffix RoslynDev
.\build

This PR currently does selective enablement, hopefully by the end we enable it across the whole codebase apart from tests

[T-Gro]

In this branch, I locally added net8 to the list of target frameworks. Reason is to get a lot more nullness annotations, since BCL in its netstandard versions does not have them. A few added packages do have them even in their ns equivalents, but those are not updated anymore and those annotations have been shown wrong already for API we have used.

Therefore, adding the net8 moniker helps us a lot more with dogfooding and for seeing what kind of issues are expected at a larger codebase.

Basic

• [x] ToString() in the BCL returns string?, which is frequently not checked
• [x] ToString(), when overriden, can change the annotation with that override (it is the same thing in IL, so still considered an override) => we need to add support for this at import time (overriden e.g. for StringBuilder, boolean, Exception => right now not supported at F# import side, so causes a lot of false alarms)
• [x] Option.ofObj not being correctly detected, this is a bug which needs to be looked at. Common code utilizing this has form like this Type.GetType(..) |> Option.ofObj, where the GetType is annotated to return Type|null.

Branching

• [x] Missing flow analysis for match x with | null -> ... | shouldBeNotNullNow -> .. => this is work in progress now https://github.com/dotnet/fsharp/pull/16659/files#diff-b9def937a045fb1bfb790380a509648589c8a4fdd7399fd2141b25e00132a624
• [ ] Missing flow analysis for if-then-else blocks utilizing isNull. At a closer look, it is not just isNull, it is also isNotNull, not(isNull), String.IsNullOrEmpty and many others. Special casing a single one of them (isNull) is unlikely to have a big impact. Proposal is either for not solving it at all, or solving it via a general mechanism on metadata level.
• [x] Missing flow analysis for exception handling (if isNull(arg) then failwith "", code which follows wants to have not null). => proposal is to replace usage with the newly added nullArgCheck function to FSharp.Core

Metadata

• [ ] There is a class of BCL APIs, mainly seen on String and File operations, which "return null only if the input is null". C# supports an attribute for this , NotNullIfNotNull. The full scope of all metadata situations is too big, but this one in particular seems to be common

Isolated issues

• [x] We do have bigger chunk of code working with Directory.GetName(), e.g. in input processing or compiler imports. There is a single situation in which this returns null, and that is when being at the root of a drive. Fully propagating the potential nullness here might be a lot of hassle (at least in the compiler codebase, which also uses .fsi annotations and every nullness annotation has to be covered via an #ifdef or #ifdef implemented wrapper like MaybeNull. Proposal: Allow null forgiveness functions within the codebase (https://github.com/dotnet/fsharp/pull/16654/) + start using defaultIfNull (defacto equivalent to Option.defaultValue)

[T-Gro]

One new annoying thing noticed in recent addition I did (printfn %s supporting null).

Imagine code like this:

let process text =
     printfn "%s" text
     Call.Api_Which_Does_Not_Accept_Null(text)

It was safe before. But now, thanks to type inference first infering text to be string | null due to %s usage, it shows a warning.

=> A code which worked just fine without annotations and possibly never head any intention to work with null, starts to show warnings.

Is this worth tweaking inference rules?

[T-Gro]

New(er) wave of pain points after some of the above have been resolved in the nullness impl

Migration concerns

• [ ] New need to explicitly annotate when T: not null type parameters, which frequently bubbles up throughout a few types. E.g. an unconstrained Dictionary<'T, int> does not work anymore. I have to constrain 'T in the containing type, into MetadataTable<'T when 'T:not null>.

• [ ] This error was particularly painful because it did not recover. Basically at that point the type could not be typechecked to be a Dictionairy anymore, therefore any subsequent code needing to access known members of a dictionary was failing.

• [ ] It is possible to create Some(possiblyNullValue) which leads to a string | null option. This is not technically wrong, but is hardly what is expected by the user. e.g. the following code triggered it, simply because the API returns a nullable string: Some(Path.GetDirectoryName d.Location)

• [ ] Changing a common IDisposable like Activity to IDisposable | null leads to nullness warnings from use. The basic implementation is null safe, but one in computation expressions does not have to be . Can we change the .Using to force the author to be null-defensive for the incoming IDisposable ?

Painful but commonly used APIs

• [x] System.Diagnostics.Activity, used for Telemetry in dotnet, uses many callbacks in the form of Action<Activity | null> | null. (EDIT: the following complain only applied in case of multitargeting dotnet frameworks, see tooling-relevant pain point below: For some reason, the lambda parameter being | null works for typechecking, but not for tooling features like tooltips or intellisense. VS does show type of the Action<_ | null> | null correctly, but inference of the lambda argument is missing.)

• [ ] Checks using String.IsNullOrEmpty() and similar do not work for eliminating nullness. Practically this leads to either null ignore call, or duplicating nullness checks

• [x] Overrides which return a type without null are not supported in F# as of now. This is especially visible when consuming custom .ToString() overrides, which as of now always return string | null. The change needed to suport this scenario might not be that big, but it would also need to flow across assemblies and signature files for example.

• [x] Reflection search APIs naturally return null almost everywhere, which makes sense in the general case. For practical scenarios, this makes implementation quite bloated. Among others, this prevents using "fluent OO" style of chaining methods. Situations like this might lead to feature requests from users in the sort of: #ignore null pragma, disable warning per file or throwOnNull CE which would do an nullArgCheck on its Bind.

• [x] System.IO.File APIs not only return null (see end of the comment from February https://github.com/dotnet/fsharp/pull/15310#issuecomment-1931612236), but defensively accept it as arguments. Which breaks type inference by assuming a value to be nullable if this is used as the first expression of a block:

Tooling-relevant scenarios

• [ ] When multitargeting differently annotated runtimes (e.g. net8.0,netstandard2.0), "go to definition" and mouseover picks based on the dropdown on the top of VS for choosing frameworks - this is technically correct, but does not help with solving migration-related nullness warnings.

Clear bugs to be fixed in the feature/nullness branch

• [x] Eliminating nullness after pattern matching null (that is , for subsequent patterns) must visit contents of abbreviations as well. Otherwise it does not work with the Maybe<T> type whcih we use in the compiler.

• [x] The box operation inside FSharp.Core must be changed to return obj | null all time, even when the input is known to be without null. That comes from observation that box is used for values which were known to be unsafely created/modified.

• [x] Error on 'useless null checkwith nullness constraint propagation in code like this:let meTry = Option.ofObj (Path.GetDirectoryName "")`. The warning about 'useless Option.ofObj' points to the string literal, ignoring the string literal is first passed to an API which may return null

• [x] let mutable cache = null and later type inference does not automatically infer the type to be nulalble, leading to 'does not support null' warnings. This is the case in ilread.fs and having a mutable,potentially nullable, ConcurrentDictionary

• [x] C# extension methods which put "?" on the this argument are wrongly interpreted by moving the nullability elsewhere. See AsMemory<T> from System.Memory.dll , this treats byteArray.ToMemory() as resulting in a Memory<byte | null> which is clearly wrong.

• [x] C# import bug: LinkedList .Last .First etc. are clearly with possible nulls, yet current F# compiler infers them as non nullable. Might be caused by the annotation of the generic T parameter and nullableattribute nesting ( the operations return LinkedListNode<T>, where both the node itself and T can be null

• [x] Usage of upcast with Type instance leads to a false-alarm warning. The code typB.CreateTypeInfo() :> Type shows an error, despite the .NET API for CreateTypeInfo not returning nulls.

• [x] There is a mismatch in generic interfaces putting | null on arguments, and struct-based instances of such an interfaces. This leads to code which cannot be satisfied

• [x] obj cannot be passed to generic typars which require T: not null, such as the NonNull active pattern.

• [x] Type<_>, even when later specialized by left-hand side assignment, is typechecked as if _ = obj. This leads to issues esp. while the bug above is still active.

• [x] Events in NET are pretty much always declared as potentially nullable, since they are null if they do not have subscribers. However, the F# wrappers created via IEvent are safe and do not need explicit null checks by F# programmer. This at the moment gives warnings about Delegate and Delegate|null not being compatible, which also cannot be easily worked around because of a few layers of code for the event subscription. Solution: Remove delegate nullness during the rewrite from CLI event into the F# event type. Sample code from fsi.fs (produces false warnings as of now): AppDomain.CurrentDomain.ProcessExit |> Event.add (fun _ -> deleteScriptingSymbols ())

• [x] The snippet above is somewhat confused about applying an operation within a call, this is clearly wrong:

[T-Gro]

/run fantomas

[github-actions[bot]]

Failed to run fantomas: https://github.com/dotnet/fsharp/actions/runs/9547963274

[T-Gro]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[T-Gro]

/azp run

[azure-pipelines[bot]]

Pull request contains merge conflicts.

[github-actions[bot]]

:warning: Release notes required, but author opted out

[!WARNING] Author opted out of release notes, check is disabled for this pull request. cc @dotnet/fsharp-team-msft

[T-Gro]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).