Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullness issue - Incorrect type inferred when pattern matching with primary branch inferred from a member call. #18015

Open
2 of 7 tasks
isaacabraham opened this issue Nov 16, 2024 · 3 comments
Assignees
Labels
Area-Nullness Issues related to handling of Nullable Reference Types Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@isaacabraham
Copy link
Contributor

isaacabraham commented Nov 16, 2024

Issue description

If in a pattern match, the first branch returns a non-nullable ref type, the overall expression is (correctly) inferred to be of that type. However, subsequent branches allow you to still return nulls.

Choose one or more from the following categories of impact

  • Unexpected nullness warning (false positive in nullness checking, code uses --checknulls and langversion:preview).
  • Missing nullness warning in a case which can produce nulls (false negative, code uses --checknulls and langversion:preview).
  • Breaking change related to older null constructs in code not using the checknulls switch.
  • Breaking change related to generic code and explicit type constraints (null, not null).
  • Type inference issue (i.e. code worked without type annotations before, and applying the --checknulls enforces type annotations).
  • C#/F# interop issue related to nullness metadata.
  • Other (none of the categories above apply).

Operating System

Windows (Default)

What .NET runtime/SDK kind are you seeing the issue on

.NET SDK (.NET Core, .NET 5+)

.NET Runtime/SDK version

.NET 9

Reproducible code snippet and actual behavior

let z = // inferred as a string
    match false with
    | true -> "Test".ToUpper()
    | false -> null // no warning here!

z.Length // no warning here, pop

z is (apparently) inferred to be string, yet the false branch is permitted.

Possible workarounds

Putting in an explicit annotation forces the compiler to give FS3261:

let z : string =
    match false with
    | true -> "Test".ToUpper()
    | false -> null // FS3261

Related to #18012 perhaps.

@isaacabraham isaacabraham added Area-Nullness Issues related to handling of Nullable Reference Types Bug Needs-Triage labels Nov 16, 2024
@github-actions github-actions bot added this to the Backlog milestone Nov 16, 2024
@T-Gro T-Gro removed their assignment Nov 18, 2024
@T-Gro
Copy link
Member

T-Gro commented Nov 18, 2024

Thank you for the report, this is an impactful regression - we will try to fix it until the next release's update.

@T-Gro T-Gro modified the milestones: Backlog, November-2024 Nov 25, 2024
@T-Gro
Copy link
Member

T-Gro commented Nov 26, 2024

This is basically a duplicate of #18012 (once for pattern matching, then for ifelse).
I will keep both to make sure they are covered as test cases though.

@T-Gro T-Gro self-assigned this Nov 28, 2024
@T-Gro
Copy link
Member

T-Gro commented Nov 28, 2024

This is the latest behavior for matching and if-then-else.
Good news is that unsafe code is not produced without warnings - this decreases the severity of the issue IMO.
If string and null are mixed in either matching of ifs, a warning about string not supporting null is raised. That's good.

What stopped working is inference of the overall result to be (string|null) in the case of first branch (for both "if" and "match") being an explicit null. Without an explicit annotation, this still gives a warning. With an explicit annotation forcing this to be a nullable type (plain : _|nul does the trick here).
Image

=> With that summary, it's a type inference issue, not a missed warning.

Answer applies to #18012 as well.

@T-Gro T-Gro modified the milestones: November-2024, December-2024 Nov 28, 2024
@0101 0101 added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. and removed Needs-Triage labels Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Nullness Issues related to handling of Nullable Reference Types Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
Status: New
Development

No branches or pull requests

3 participants