dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.87k stars 781 forks source link

Better diagnostics for mixing different nullness pattern matching ways #17432

Open psfinaki opened 1 month ago

psfinaki commented 1 month ago

There are 2 ways to do null-related pattern matching now:

let len1 (str: string | null) =
    match str with
    | Null -> -1
    | NonNull s -> s.Length

let len2 (str: string | null) =
    match str with
    | null -> -1
    | s -> s.Length

Both produce no warnings, all good. What I don't like is the behavior when we start mixing those two approaches:

let len3 (str: string | null) =
    match str with
    | null -> -1
    | NonNull s -> s.Length 

warning FS3262: Value known to be without null passed to a function meant for nullables: You can remove this |Null|NonNull| pattern usage.

let len4 (str: string | null) =
    match str with
    | Null -> -1
    | s -> s.Length 

warning FS3261: Nullness warning: The types 'string' and 'string | null' do not have compatible nullability.


As a user I just want consistent diags here in the spirit of "hey, pick one way or another, don't mix things up" - ideally with exact pointers what to change but that's an extra.

I think such errors can happen really often because of fatfingering or copypasting - and casing differences are not even very noticeable, so there is accessibility in the game as well.

_Originally posted by @psfinaki in https://github.com/dotnet/fsharp/pull/15181#discussion_r1677921389_

T-Gro commented 1 month ago

Related to https://github.com/dotnet/fsharp/issues/17409

T-Gro commented 1 month ago

Might be implicitely solved by contextual information to nullness diag (... "possible derefence"..)

T-Gro commented 1 month ago

Idea: Make the active pattern |Null|NonNull| to an module which needs explicit opening or explicit prefixing.

T-Gro commented 1 month ago

Idea: RQA for the active pattern:

[<RequireQualifiedAccess>]
module Nullable = 

    let inline (|Null|NonNull|) (x: 'T) : Choice<unit,'T> = 
        match x with null -> Null | v -> NonNull v
smoothdeveloper commented 1 month ago

What is the main reason for those patterns to exist?

I find matching on literal null is most natural thing to do.

I'd favour explicit opening but no RQA attribute as it makes it really inconvenient to use for those that prefer it.

T-Gro commented 1 month ago

The active pattern can eliminated nullness from a type (i.e. what you match as values will be known to be non-nullable) even in deeply nested scenarios. Imagine a string | null field within record, being wrapped in voption, within another record.

The the classical null literal this only eliminated nullness on first level values, but not from nested members of types.

From it's signature, NonNull will give you a value known to be not null.