Nullness issue - NotNullWhenAttribute not taken into account for byref?

[cmeeren]

Issue description

System.Net.IPAddress.TryParse has this signature in Rider:

IPAddress.TryParse([<NotNullWhenAttribute (true)>] ipString: string, [<NotNullWhenAttribute (true)>] address: byref<IPAddress>) : bool

My understanding is that this should indicate that when the function returns true, then the byref IPAddress is not null. However, F# seems to think the ip value van be null in the code below:

let validIpAddress (str: string) : Result<IPAddress, string> =
    match IPAddress.TryParse str with
    | true, ip -> Ok ip
    | _ -> Error "The value must be a valid IP address"

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 SDK 9.0.100

Reproducible code snippet and actual behavior

Repro solution: Repro.zip

Possible workarounds

Add NonNullQuick:

let validIpAddress (str: string) : Result<IPAddress, string> =
    match IPAddress.TryParse str with
    | true, NonNullQuick ip -> Ok ip
    | _ -> Error "The value must be a valid IP address"

[vzarytovskii]

Essentially a duplicate of #18019, this will require introducing a full blown flow analysis "engine" and significant changes to type inference.

[T-Gro]

For this attribute specifically, I would recommend wrapping it into a custom active pattern which would do both the branching parsed/not parsed logic, as well as binding for the non-null values of ipString and address.

[T-Gro]

let (|InvalidIP|ValidIP|) (ipString:string|null) =
    match IPAddress.TryParse ipString with
    | true, NonNull ipAddress -> ValidIP ipAddress
    | _ -> InvalidIP

[cmeeren]

Thanks. I think it's beter to just use NonNullQuick in the original code, then:

let validIpAddress (str: string) : Result<IPAddress, string> =
    match IPAddress.TryParse str with
    | true, NonNullQuick ip -> Ok ip
    | _ -> Error "The value must be a valid IP address"

[T-Gro]

NonNullQuick will fail with an exception if the value turns out to be null at runtime.
NonNull active pattern will cause the value to be handled by the Null case (or _ wildcard case in this case).

If you can trust the API's contract, both should be fine.

[cmeeren]

Sure, and since this is the BCL, I believe the API contract can be trusted.

In any case, my main point was using the active pattern (either, really) inside the original function instead of rewriting the function to a separate active pattern, which is not needed and orthogonal to this issue.

[T-Gro]

Flow analysis is not part of F# NRTs , it is a Roslyn-only feature for C# as of now.
I will close the issue with it.

Adding flow analysis would be a separate (large) change for the compiler - not saying it will never happen, but is not like other bugs.