Skip to content

Better diagnostics for mixing different nullness pattern matching ways #17432

Open

Description

@psfinaki

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 #15181 (comment)

Activity

  1. added this to the Backlog milestone on Jul 23, 2024
  2. added
    Area-NullnessIssues related to handling of Nullable Reference Types
    Area-Diagnosticsmistakes and possible improvements to diagnostics
    on Jul 23, 2024
  3. T-Gro commented on Jul 31, 2024

    @T-Gro
    Member

    Related to #17409

  4. T-Gro commented on Jul 31, 2024

    @T-Gro
    Member

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

  5. T-Gro commented on Jul 31, 2024

    @T-Gro
    Member

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

  6. T-Gro commented on Jul 31, 2024

    @T-Gro
    Member

    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
  7. smoothdeveloper commented on Jul 31, 2024

    @smoothdeveloper
    Contributor

    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.

  8. T-Gro commented on Aug 1, 2024

    @T-Gro
    Member

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Area-Compiler-PatternMatchingpattern compilation, active patterns, performance, codegenArea-Diagnosticsmistakes and possible improvements to diagnosticsArea-NullnessIssues related to handling of Nullable Reference Types

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions