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 - string function signature doesn't hold #17742

Open
1 of 7 tasks
Lanayx opened this issue Sep 15, 2024 · 3 comments · May be fixed by #17809
Open
1 of 7 tasks

Nullness issue - string function signature doesn't hold #17742

Lanayx opened this issue Sep 15, 2024 · 3 comments · May be fixed by #17809
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

@Lanayx
Copy link
Contributor

Lanayx commented Sep 15, 2024

Issue description

This started in #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).
  • 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
@Lanayx Lanayx added Area-Nullness Issues related to handling of Nullable Reference Types Bug Needs-Triage labels Sep 15, 2024
@github-actions github-actions bot added this to the Backlog milestone Sep 15, 2024
@T-Gro
Copy link
Member

T-Gro commented Sep 15, 2024

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

@abonie abonie added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. and removed Needs-Triage labels Sep 24, 2024
@T-Gro T-Gro linked a pull request Sep 27, 2024 that will close this issue
@T-Gro T-Gro linked a pull request Sep 27, 2024 that will close this issue
@brianrourkeboll
Copy link
Contributor

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 dotnet/roslyn#23268 in F#.

@T-Gro
Copy link
Member

T-Gro commented Sep 30, 2024

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.

@T-Gro T-Gro modified the milestones: Backlog, October-2024 Oct 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

Successfully merging a pull request may close this issue.

4 participants