Nullness issue - `string` function signature doesn't hold

Lanayx commented 1 month ago

Issue description

This started in https://github.com/dotnet/fsharp/issues/17730, where I initially though that string null would return null, but then was corrected. However later it was found out that string function still can return null, so current signature is incorrect. There are 3 ways to fix it: 1) Just make return type string | null 2) Make return type string | null and fix cases like string null so they return null as expected 3) Leave return type to be string, but add more null checks so the result will be converted to empty string instead

Out of those three options while I like 2nd one, it seems to be impossible due to backwards compatibility, so I'd vote for the 3d option instead.

Choose one or more from the following categories of impact

[ ] Unexpected nullness warning (false positive in nullness checking, code uses --checknulls and langversion:preview).
[X] 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.0-rc.1.24431.7

Reproducible code snippet and actual behavior

type A() =
    override x.ToString() = null
let y = A() |> string // y is null

Possible workarounds

Leave as is, so if insightful user wants to do a null check, they should use `withNull:

type A() =
    override x.ToString() = null
let y = A() |> string |> withNull

T-Gro commented 1 month ago

Thanks for this.

Since string handles even null literal itself, it will be better to change it to return not-null. Before NRTs, it was not visible that some types are actually capable of returning null in their .ToString().

Funnily enough, I have used "string" to convert a potentially-nullable string to a non-nullable string many times myself when applying nullness to the codebase.

I would like to treat it as a bugfix or a historical overlook (of people being capable of returning null from ToString()), because FSharp.Core does not return surprising nulls elsewhere either.

cc @vzarytovskii

brianrourkeboll commented 1 month ago

Ideally, the string function's return type would take the nullness annotation of the actual resolved ToString implementation into account.

If someone writes this in C#:

public class C
{
    public override string ToString() => null!;
}

we should (and do) respect the annotation (string instead of string?) on the F# side and infer string instead of string | null when ToString is called directly:

let s = C().ToString() // Inferred as string.

Likewise, if someone wrote this in F#:

type C () =
    override _.ToString () : string = nonNull null

let s = string (C ())

we should respect the annotation and infer string, not string | null. We currently (as of RC1 anyway) don't even respect the annotation (of string instead of string | null) when directly calling ToString:

let s = C().ToString() // Inferred as string | null.

But I guess that's basically asking for the equivalent of https://github.com/dotnet/roslyn/issues/23268 in F#.

T-Gro commented 1 month ago

Since this would mean for inlined fslib code having different return type depending on the type argument (and not just in T-type-algrebra, but by inspecting its method's metadata), I think we can skip the "ideally" part of the proposal :)

The second part should have been done as part of https://github.com/dotnet/fsharp/pull/17547/files and did not make it to RC1 . Also, from that PR on, having a null-returnign ToString on an F# type will lead to a warning.

dotnet / fsharp