fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Nullness issue - Compiler-generated member overrides should change nullness

Open kerams opened this issue 1 year ago • 4 comments

Issue description

ToString on a record or union returns string | null. The method body is generated by the compiler and the result is known to be not null.

Choose one or more from the following categories of impact

  • [X] 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

9.0.0-preview.7.24405.7

Reproducible code snippet and actual behavior

type X = A | B

let x: string = A.ToString ()

Possible workarounds

No response

kerams avatar Aug 14 '24 08:08 kerams

Thanks for the finding, this makes sense.

The workaround/recommendation until then would likely be to use the string function.

T-Gro avatar Aug 14 '24 08:08 T-Gro

C# discussions of the same thing:

  • https://github.com/dotnet/roslyn/issues/23268
  • https://github.com/dotnet/roslyn/issues/35227
  • https://github.com/dotnet/coreclr/pull/23466

brianrourkeboll avatar Aug 14 '24 09:08 brianrourkeboll

I (think I) did support for consumption of "nullable-affecting overrides", they are present in the BCL and I encountered it already. The consuming side is there. This issue is right that producing side for F# was not updated, and the generated .ToString() members make a lot of sense.

Nullable-contravariance was also done, this was needed to e.g. subsume IEqualityComparer<string | null> (the StringComparer.*.. statics are like that) into a parameter which accepts IEqualityComparer<string>. The interface must be however marked as contravariant in IL, which in F# you cannot author.

T-Gro avatar Aug 14 '24 09:08 T-Gro

@kerams :

I am trying to publish a valspec for modified ToString (returning a string without null), but the possibility of overrides coming after usage makes this trickier than I thought.

If you have any suggestion, please let me know, I would be glad for any ideas. A particular idea, which would be however breaking, would be to strongly disallow a hand-written .ToString() returning nullable data. That way, the problem would be simplified (would mean that all F# records and DUs would always be typechecked as having a non-nullable ToString).

open System
type XX = A | B
    with member x.TextLength = x.ToString().Length   // warn or not? This depends on the existence of a possible override

module SomeOtherCode = 
    let plentyOfOtherTypes = () // ...

type XX with
    override x.ToString() = null   // only after we process this, we know for sure that .ToString().Length should actually warn

T-Gro avatar Aug 16 '24 10:08 T-Gro