The 'string' operator produces suboptimal code when the object's type is known.

[teo-tsirpanis]

This simple code snippet:

let f() = string 4

produces this IL code that involves a redundant boxing and type checking:

public static string f()
{
    object obj = 4;
    if (obj != null)
    {
        IFormattable formattable = obj as IFormattable;
        if (formattable != null)
        {
            IFormattable formattable2 = formattable;
            return formattable2.ToString(null, CultureInfo.InvariantCulture);
        }
        object obj2 = obj;
        return obj2.ToString();
    }
    return "";
}

The compiler knows that 4 is an integer and implements IFormattable. For value types and sealed classes known to implement it, emitting a direct call to ToString with the invariant culture passed (and the constrained. IL prefix) would be much better. Same with the eligible types that are known to not implement it.

For all other cases (non-sealed reference types that do not implement IFormattable), the existing behavior is acceptable (no boxings) and backwards-compatible. Performance-concerned developers can always directly call ToString().

There might be a problem with types that might stop implementing IFormattable at a later version of the library, but this is a breaking change that should require recompilation and we should not be concerned with such scenarios.

[charlesroddie]

Why not the simpler approach: to compile string x where x has type 'a.

If 'a is IFormattable then compile to (x:>IFormattable).ToString(null, CultureInfo.InvariantCulture).
Otherwise compile to x.ToString().

If 'a:null (NRT doc) then a null check will also be needed to preserve existing behaviour.

[teo-tsirpanis]

@charlesroddie it might be possible that a subclass of 'a implements IFormattable while 'a itself does not. That's why I restricted the optimization you said to only value types or sealed classes: it is certain whether they implement it or not. This behavior preserves backwards compatibility, but I have no problem breaking it here. And keep in mind that types like F# records and DUs are automatically sealed. And you can always directly call ToString().

[charlesroddie]

If x:'b where b:>'a, and 'b implements IFormattable and 'a does not, then string x should use IFormattable and string(x:>'a) should not. I understand that with all this casting to obj you may be right about the current behaviour. But it seems like a bug to me.

This would not make string a well-behaved function, but it would move it in that direction. (If F# allowed overloading functions and had not in type constraints you could define it as a function in F# with an IFormattable overload and a not IFormattable overload.)

[abelbraaksma]

@charlesroddie, your proposal is pretty much what @dsyme and I researched a few months ago, see #7958 (I forgot about creating the pr, this thread reminded me again).

Though the reasons to want to change string are different: my motivation was that presently it cannot always be used in (sometimes very trivial) code. Bottom line in that issue is: get rid of "inline" so that the dreaded "cannot escape its scope" error doesn't hit us anymore.

[charlesroddie]

Existing code from #7958 (comment)

let inline anyToString nullStr x = 
    match box x with 
    | null -> nullStr
    | :? System.IFormattable as f -> f.ToString(null,System.Globalization.CultureInfo.InvariantCulture)
    | obj ->  obj.ToString()

let inline string (value: ^T) = 
     anyToString "" value
     // since we have static optimization conditionals for ints below, we need to special-case Enums.
     // This way we'll print their symbolic value, as opposed to their integral one (Eg., "A", rather than "1")
     when ^T struct = anyToString "" value
     ...
     when ^T : int32      = (# "" value : int32      #).ToString("g",CultureInfo.InvariantCulture)
     ...

I don't understand the compiler-specific syntax. But the intention seems to be to use the code from lowest down case as the most specific case. So string 4 should give 4.ToString("g",CultureInfo.InvariantCulture) But this issue implies that anyToString is used instead.

Your suggestion @abelbraaksma removes the static attempts completely. That would entrench the problem in this issue, wouldn't it? I think using anyToString at any time is an inefficiency so we should eliminate use of this function if possible. I don't know if something like this is possible:

let inline string (value: ^T) = 
    value.ToString()
    when ^T struct = anyToString "" value
    when ^T : null = match (# "" value : ^T      #) with | null -> "" | NonNull v -> v.ToString()
    when ^T :> IFormattable = (# "" value : IFormattable      #).ToString(CultureInfo.InvariantCulture)

[abelbraaksma]

@charlesroddie, the compiler-specific syntax is used to create specific compile paths for certain statically known types, and emit different IL for those. It's a bit more powerful than plain SRTP (and personally, I'd love it to be available to the general public, but that's another issue).

The special-casing is, however, redundant. Each path creates a cast to System.IFormattable (not visible here, that's part of the "g" modifier for ToString). Ergo, the boxing is inevitable, since on the "unknown" path we already specifically check for System.IFormattable anyway, and ToString does it for the known paths.

The proposed simpler code removes this redundant compile time checking, and removes inline, leading to a function string that can be used in all circumstances, without loss of performance (though it should be noted that ToString is very slow, and losing one boxing or cast wouldn't really make a measurable difference).

[abelbraaksma]

Quoting myself (#7958 (comment)):

The overload ToString(IFormatProvider), i.e. without the "g", calls Number.FormatDouble(...) with null internally for the format string. This then defaults to "g".

[abelbraaksma]

I think using anyToString at any time is an inefficiency so we should eliminate use of this function if possible. I don't know if something like this is possible:

@charlesroddie I don't think it's inefficient, the call should be eliminated by the JIT, and certainly in my proposed code.

Your suggested code makes sense (if the syntax allows it), but it doesn't eliminate any boxing, and it still requires inline. Also, the statically unknown type test should probably be added, including a runtime test for System.IFormattable. Ultimately, all these checks are redundant, and simpler, and without inline is better.

The proposed simplified code would become (#7958 (comment)):

let anyToString nullStr x = 
    match box x with 
    | null -> nullStr
    | :? System.IFormattable as f -> f.ToString(null,System.Globalization.CultureInfo.InvariantCulture)
    | obj ->  obj.ToString()

let string value = 
    anyToString "" value

[abelbraaksma]

I've researched this a bit further for an up and coming PR. It turns out that the original code contained "dead code", anything after the struct test is never reached. These are static type tests done during compilation, but since when ^T struct = anyToString "" value comes first, the special paths for int etc are redundant.

This means that the current code is already equal to let string x = anyToString "" value after compiling, which, since it is inlined, is just the same as anyToString.

This made we wonder about the double null-check as originally reported here by @teo-tsirpanis. It turns out that writing this:

match box x with
| null -> do something
| :? ISomething as foo -> foo.CallInterface()

already leads to the double null-check:

• The first | null -> ... is compiled into if x != null then...
• Then the type-test is done
• Then another null-check is done, a type-test is like as in C#
• Then the call on the interface is done

Since this means that you don't need to check for null when you do a match-with-type-test, I wondered if we could see anything if we put the null check at the bottom instead, like this:

let inline anyToString nullStr x = 
    match box x with 
    | :? System.IFormattable as f -> f.ToString(null,System.Globalization.CultureInfo.InvariantCulture)
    | null -> nullStr
    | obj ->  obj.ToString()

let inline string value = 
    anyToString "" value

This leads to the following decompiled C# code:

public string newString1()
{
	object obj = MixString.get(1);   // this is from my test-setup
	object obj2 = obj;
	IFormattable formattable = obj2 as IFormattable;
	if (formattable != null)   // only one null-test now
	{
		IFormattable formattable2 = formattable;
		return formattable2.ToString(null, CultureInfo.InvariantCulture);
	}
	if (obj2 != null)   // and another one if it is not IFormattable
	{
		object obj3 = obj2;
		return obj3.ToString();
	}
	return "";
}

I was wondering if this would lead to any measurable performance gain or loss. I set the max-relative-error extra tight, but still there's no measurable difference, the timings are all inside the error range, and different runs give different results.

First run:

Second run:

Since the inlining of string leads to it not being usable inside virtual methods with generics, and since the timings show now significant downside of removing inline here, I propose to make it a normal generic function.

Example of what's currently impossible:

type Du<'T> = 
    | Foo of 'T 
    override this.ToString() =   // FS0670 error shown here
        match this with
        | Foo x -> string x    // caused by this

[teo-tsirpanis]

Since the inlining of string leads to it not being usable inside virtual methods with generics, and since the timings show now significant downside of removing inline here, I propose to make it a normal generic function.

@abelbraaksma I suppose you are talking about the string operator on generics (maybe even those not constrained to implement IFormattable) and plain objects. In other cases where the object's type is known, I believe we should directly call the appropriate overload of ToString depending on whether the type implements IFormattable. There is no reason to insert an FSharp.Core function call when it's not needed. And the thing we definitely must avoid is boxing a value type in a core library function like string.

Other than that, you proposal seems good. Still I haven't understood what is the difference between improved and noinline. Could you show us the three functions for comparison? And did I correctly understand that we are going to use improved?