[WIP] [RFC FS-1060] Nullness checking

[dsyme]

Continuation of #5790 and #6804

This is a prototype implementation of RFC FS-1060 nullable reference types

See tests\adhoc\nullness for testing and samples including baselines of outputs from

• existing compiler
• updated compiler
• updated compiler with /langversion:preview
• updated compiler with /langversion:preview /checknulls

TODO:

• [ ] revise RFC and resolve all unresolved issues (@dsyme)
• [x] implement syntax string | null and : not null (@T-Gro )
• [x] implement import of .NET metadata (@T-Gro )
• [x] implement emit of .NET metadata (@T-Gro )
• [x] Import+Export TODOs (to test and adjust if needed)
• [x] Nullness used in inheritance (e.g. inherit System.Collections.Generic.List<string | null>)
• [x] Soundness of import of CLI EventHandlers (possibly overly defensive)
• [ ] Nullness and import of TypeProvider-generated types + In general the overall TP scenario whet it comes to nullness support - do in separate PR
• [x] Object overrides can change nullness (e.g. boolean.ToString() does or StringBuilder.ToString()), make it work to avoid false alarms
• [x] Type.GetType(..) |> Option.ofObj produces a false alarm, fix that
• [x] printfn "%s" someNullableString produces a false alarm, check the printing pattern annotations
• [ ] (+) operator for strings gives false alarms on nullable strings, should allow nullables since String.Concat also does
• [x] Pattern matching flow analysis on match x with | null -> Support this as a scenario
• [x] Allow incoming arrays to be null (this causes false alarms as of now, it's a bug)
• [ ] resolve all // TODO NULLNESS (@dsyme)
• [ ] use it in the codebase and get it green
• [x] there's a case in tests\adhoc\nullness we're getting The types 'System.String (...)' and 'System.String (...)' do not have compatible nullability. which is wrong - either the nullability aren't shown for some reason or the types should be considered compatible

Testing:

• [ ] add proper testing, moving tests across from tests\adhoc\nullness
• [ ] test and check signature compatibility. While integrating master I noticed a case in the compiler where a signature file had a non-nullable string type for ther return of a function and an implementation had a nullable string type (and a later soundness problem arose)

To be moved to RFC and resolved, then tested here:

• [ ] should nullness be supported on ref tuples and anon tuples
• [ ] work out what to do about compat of Option.ofObj and others
• [ ] the "type equiv" relation is currently directional only giving a nullness warning if "actual" has a null and "expected" is non-null. This includes a dubious change of direction in SolveFunType thing - for the contravariance of inputs. This has caused a few outputs to change in tests. This is both unsound in the general case (nested cases of equivalence should demand true equivalence) and is dubious for functions because the "MatchingOnly" case of type equivalence is also directional.
• [ ] assess nullness checking in conditionals, e..g if x then null else "" and if x then "" else null

[dsyme]

@vzarytovskii @T-Gro I squahed this and also fixed a nasty bug with nullness inference that I spotted while walking through with Vlad on Thursday

[dsyme]

@vzarytovskii @T-Gro This is basically green now :) And much, much trimmed down, with some bugs fixed. I've also revised the TODO list to what would be needed for bringing into preview

(It's possible we could even ship preview using the string __withnull syntax then incrementally improve after)

[vzarytovskii]

@vzarytovskii @T-Gro This is basically green now :) And much, much trimmed down, with some bugs fixed. I've also revised the TODO list to what would be needed for bringing into preview

I'm going to take care of metadata part

[edgarfgp]

Will https://github.com/dotnet/fsharp/issues/711 be revisited as part of this feature implementation?

[vzarytovskii]

Will #711 be revisited as part of this feature implementation?

Short answer is: No, we don't want to mix anything more into the implementation.

Detailed answer is: this implements a specific RFC for nullabillity, and unless the issue in question is covered in RFC directly or indirectly, it should be looked at separately.

[edgarfgp]

Will #711 be revisited as part of this feature implementation?

Short answer is: No, we don't want to mix anything more into the implementation.

Detailed answer is: this implements a specific RFC for nullabillity, and unless the issue in question is covered in RFC directly or indirectly, it should be looked at separately.

Thanks. I was curious as this comment https://github.com/dotnet/fsharp/issues/711#issuecomment-460344789 mentions the RFC in regards of making progress on this long standing bug

[vzarytovskii]

Will #711 be revisited as part of this feature implementation?

Short answer is: No, we don't want to mix anything more into the implementation. Detailed answer is: this implements a specific RFC for nullabillity, and unless the issue in question is covered in RFC directly or indirectly, it should be looked at separately.

Thanks. I was curious as this comment #711 (comment) mentions the RFC in regards of making progress on this long standing bug

I would say when it's merged, it should be looked at, unless it's going to need an implicit fix during this PR

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[kerams]

Why does the trimmed assembly size change? Do we need to preserve nullness (and sig, opt) metadata? I would have thought no.

[T-Gro]

Why does the trimmed assembly size change? Do we need to preserve nullness (and sig, opt) metadata? I would have thought no.

It is new part of a signature and goes with sigdata, therefore metadata increases in size.

[vzarytovskii]

Why does the trimmed assembly size change? Do we need to preserve nullness (and sig, opt) metadata? I would have thought no.

I would say so, since if Roslyn consumes it, they need to read nullable attributes to know nullabillity.

[kerams]

Why would Roslyn consume a trimmed F# assembly?

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[T-Gro]

Why would Roslyn consume a trimmed F# assembly?

You are right, will remove the IL attributes in case of aggressive trimming.

[vzarytovskii]

@kerams

Why would Roslyn consume a trimmed F# assembly?

If you publish a trimmed library, and then consume from C# perhaps?

[kerams]

I don't think libraries are meant to be trimmed (on their own), can you even do that?. Maybe we should formalize usage rules for trimmed F# assemblies?

[vzarytovskii]

I don't think libraries are meant to be trimmed (on their own), can you even do that?.

Oh, really? TIL. I thought it's a legit use case (now thinking about it, if it is legit, then a weird one).

Maybe we should formalize usage rules for trimmed F# assemblies?

Yeah, it's a good idea.

[github-actions[bot]]

:heavy_exclamation_mark: Release notes required

---

:white_check_mark: Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.100.md
LanguageFeatures.fsi	docs/release-notes/.Language/preview.md

[isaacabraham]

Just a TLDR for the above linked issue (#16731) - I believe that Option.ofObj should only be valid for non-nullable ref types i.e 'T | null - it is semantically incorrect to use it on a non-nullable value and inconsistent with the behaviour of Option.ofNullable.

In fact, along the same lines, I suppose that Option.toObj now needs to be changed to return a 'T | null - it can't return 'T any longer once this goes in, surely?

[T-Gro]

Just a TLDR for the above linked issue (#16731) - I believe that Option.ofObj should only be valid for non-nullable ref types i.e 'T | null - it is semantically incorrect to use it on a non-nullable value and inconsistent with the behaviour of Option.ofNullable.

In fact, along the same lines, I suppose that Option.toObj now needs to be changed to return a 'T | null - it can't return 'T any longer once this goes in, surely?

I am thinking about introducing a general mechanism to it here. In other function call conditions, passing a 'T where 'T | null is expected is fine, the nullness is subsumed and conversion in this direction works.

Maybe the .ofObj function could be marked with an attribute with the semantics WarnOnNullConversion which would raise a warning for passing a 'T in it?

(and user code would be allowed to utilize the same attribute)

cc: @dsyme , @vzarytovskii

[vzarytovskii]

passing a 'T where 'T | null is expected is fine, the nullness is subsumed and conversion in this direction works

Yep, that definitely should be the case, otherwise it will be pain.

[dsyme]

@isaacabraham

Just a TLDR for the above linked issue (https://github.com/dotnet/fsharp/issues/16731) - I believe that Option.ofObj should only be valid for non-nullable ref types i.e 'T | null - it is semantically incorrect to use it on a non-nullable value and inconsistent with the behaviour of Option.ofNullable.

It's an interesting point. I don't think it's a huge problem to allow this through is it? I guess this would be arguing for this signaure here: https://github.com/dotnet/fsharp/pull/15181/files#diff-ba9a2ffeee4daae6454aeea56b816bdc81b2b8755db142f6725959d684ffc463R444, which demands null on the type - but somehow stripping the null on return.

@T-Gro I'm happy for you to decide what to do here.

In fact, along the same lines, I suppose that Option.toObj now needs to be changed to return a 'T | null - it can't return 'T any longer once this goes in, surely?

I think that's the case, seeing https://github.com/dotnet/fsharp/pull/15181/files#diff-ba9a2ffeee4daae6454aeea56b816bdc81b2b8755db142f6725959d684ffc463R467, though note the TODO about assessing whether this is ever a breaking change (I think it isn't but needs to be double-checked)

[isaacabraham]

@T-Gro although the build is showing green, it looks like it's actually broken (see https://dev.azure.com/dnceng-public/public/_build/results?buildId=631736&view=logs&j=119f12ab-97c9-53f0-7ea6-00e6f97fda11&t=5269ef62-afc8-5810-682c-8c493ccd0e61).

[vzarytovskii]

@T-Gro although the build is showing green, it looks like it's actually broken (see https://dev.azure.com/dnceng-public/public/_build/results?buildId=631736&view=logs&j=119f12ab-97c9-53f0-7ea6-00e6f97fda11&t=5269ef62-afc8-5810-682c-8c493ccd0e61).

It's plain build, and unsupported scenario, will be fixed at the very end.

[isaacabraham]

It's plain build, and unsupported scenario, will be fixed at the very end.

I saw the issue locally when building today. But a full git clean -xdf seemed to resolve it.

[vzarytovskii]

It's plain build, and unsupported scenario, will be fixed at the very end.

I saw the issue locally when building today. But a full git clean -xdf seemed to resolve it.

If you built it with just dotnet build, that's unsupported in current state. If with build scripts, then yes, you need to clean build it, since things are rapidly changing and both bootstrap compiler and product one need to be built from scratch.

[T-Gro]

If this happens to be a really big pain point, I can have a look a #ifdef my way out of it - should be doable I think. But as of now, the compiler uses new constructs which were added by FSharp.Core, and therefore needs the 2-phase bootstrap process via build scripts.

[ken-okabe]

I came across an article about F# Nullness support . As someone who's eagerly awaiting Nullable types including reference types in F#, I want to thank you for this contributions.

https://github.com/ken-okabe/vanfs?tab=readme-ov-file#nullable https://github.com/ken-okabe/vanfs/blob/main/README-whatisNull.md

[vzarytovskii]

I came across an article about F# Nullness support . As someone who's eagerly awaiting Nullable types including reference types in F#, I want to thank you for this contributions.

https://github.com/ken-okabe/vanfs?tab=readme-ov-file#nullable

https://github.com/ken-okabe/vanfs/blob/main/README-whatisNull.md

Thank you for kind words. We hoped to release it earlier, but feature turned out to be much more complex and big than we anticipated :(

[github-actions[bot]]

[!CAUTION] Repository is on lockdown for maintenance, all merges are on hold.

[Lanayx]

Nice work! One note - as the amount of null checks will be dramatically increased, null propagation operator will be very useful. @dsyme closed the issue as covered by NRT, but it doesn't look like it's covered in this PR https://github.com/fsharp/fslang-suggestions/issues/14

[T-Gro]

Nice work! One note - as the amount of null checks will be dramatically increased, null propagation operator will be very useful. @dsyme closed the issue as covered by NRT, but it doesn't look like it's covered in this PR fsharp/fslang-suggestions#14

I think the null propagation operator suggestion can be reopened and potentially approved as a separate item, if @dsyme agrees. It is not part of this implementation nor the nullness RFC, and the operator has value of its own (since it could be made to work on other types)

Full disclosure, when applying the nullness feature on the compiler codebase, I created an operator with the semantics of a nullable map function for internal(= not part of fslib) usage:

    let inline (^) (a: 'a | null) ([<InlineIfLambda>] b: 'a -> 'b) : ('b | null) =
        match a with
        | Null -> null
        | NonNull v -> b v

For intended usage like this with the lambda shorthand, inspired by the null-coalescing operator.

                let currentProcess = Process.GetCurrentProcess()
                let mainModule = currentProcess.MainModule
                let processFileName = mainModule ^ _.FileName

And in the end I almost never used it and favored explicit pattern matching instead, or early conversion to option/voption and then working with their respective combinators.

[Lanayx]

Full disclosure, when applying the nullness feature on the compiler codebase, I created an operator with the semantics of a nullable map function for internal(= not part of fslib) usage:

    let inline (^) (a: 'a | null) ([<InlineIfLambda>] b: 'a -> 'b) : ('b | null) =
        match a with
        | Null -> null
        | NonNull v -> b v

For intended usage like this with the lambda shorthand, inspired by the null-coalescing operator.

                let currentProcess = Process.GetCurrentProcess()
                let mainModule = currentProcess.MainModule
                let processFileName = mainModule ^ _.FileName

And in the end I almost never used it and favored explicit pattern matching instead, or early conversion to option/voption and then working with their respective combinators.

We've just discussed the operator in F# chat in telegram :) And didn't find a way to reuse the same operator for null, option and voption without HKT (at least from user code)

As for particularly null context - it becomes extremely useful when working with large generated C# classes with deep (4-5 levels) nesting