String operator always returning nonNull string

[T-Gro]

This fixes https://github.com/dotnet/fsharp/issues/17742 .

F#-originating code has always been against implicit nulls. Even when null literal was passed into the string function, the null was still replaced with an empty string.

What was missing was checking for object types overriding ToString and returning null out of their implementation - this has not been historically checked, very likely due to this pattern not being heavily used and possibly overlooked in the implementation of the string operator.

This PR fixes it by making sure that all code paths of the statically optimized string call will not return an empty string.

The alternative has been evaluated and changing the signature to let inline string x : string | null = ... would flood all legitimate usages of this standard function and enforce unnecessary null checks (under the assumption that legitimate F# code does not return null from .ToString, which is even a new warning now)

[abonie]

This PR fixes it by making sure that all code paths of the statically optimized string call will not return an empty string.

This was supposed to say "will now return an empty string" (and "all code paths" by implication meant "all code paths that returned null before"), right? Otherwise I don't get it

[T-Gro]

Just to be clear — this is an actual change in behavior, not just a change in nullness inference, right?

Yes, this bugfix (or handling a forgotten scenario) is a breaking change (I guess technically, every bugfix is). Will have to be documented as such.

I'd also be curious what the perf implications are

We first have to make it correct. I assume there is limited potential in detecting certain types whose ToString is guaranteed to by not null at JIT time (at generic instantiation), even though even the F# types could have been returning null as well.

[vzarytovskii]

Just to be clear — this is an actual change in behavior, not just a change in nullness inference, right?

Yes, this bugfix (or handling a forgotten scenario) is a breaking change (I guess technically, every bugfix is). Will have to be documented as such.

Do we know if this is described in any rfc or spec?

I'd also be curious what the perf implications are

We first have to make it correct.

I assume there is limited potential in detecting certain types whose ToString is guaranteed to by not null at JIT time (at generic instantiation), even though even the F# types could have been returning null as well.

What the rough IL we will be generating here?

[T-Gro]

I did not find any mention of the behavior for the string function, a promise about exactly returning .ToString() is not there.

The IL will look like this:

        IL_0001: callvirt instance string [System.Runtime]System.Object::ToString()
        IL_0006: stloc.0
        IL_0007: ldloc.0
        IL_0008: brtrue.s IL_0010

        IL_000a: ldstr ""
        IL_000f: ret

        IL_0010: ldloc.0
        IL_0011: ret

For all normally-behaving objects, branch prediction will be stable for the brtrue.s check.

[vzarytovskii]

I did not find any mention of the behavior for the string function, a promise about exactly returning .ToString() is not there.

The IL will look like this:

        IL_0001: callvirt instance string [System.Runtime]System.Object::ToString()

        IL_0006: stloc.0

        IL_0007: ldloc.0

        IL_0008: brtrue.s IL_0010



        IL_000a: ldstr ""

        IL_000f: ret



        IL_0010: ldloc.0

        IL_0011: ret

For all normally-behaving objects, branch prediction will be stable for the brtrue.s check.

Yeah, I would think so as well. Will be curious to hear from @EgorBo as well

[T-Gro]

C# does it like this for o.ToString() ?? "" :

        IL_0001: callvirt instance string [System.Runtime]System.Object::ToString()
        IL_0006: dup
        IL_0007: brtrue.s IL_000f

        IL_0009: pop
        IL_000a: ldstr ""

        IL_000f: ret

Which uses less instructions for the happy path (ToString result not being null).

[github-actions[bot]]

:heavy_exclamation_mark: Release notes required

---

:white_check_mark: Found changes and release notes in following paths:

Change path	Release notes path	Description
src/FSharp.Core	docs/release-notes/.FSharp.Core/9.0.200.md

[psfinaki]

The fix is clear, but I think it would be nice to add some IL baselines for this behavior.

[T-Gro]

Suggestion approved now: https://github.com/fsharp/fslang-suggestions/issues/1386

[T-Gro]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).