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

Using string obj vs obj.ToString() leads to problems: the type parameter can escape its scope #7958

Closed
abelbraaksma opened this issue Dec 12, 2019 · 9 comments · Fixed by #9549

Comments

@abelbraaksma
Copy link
Contributor

I was refactoring some old code, replacing x.ToString() with string x for readability and, let's face it, best practices. But much to my surprise, this didn't work in a number of cases, though I always thought the two were interchangeable (with the exception of null, which is treated as "" for string null and as NRE for null.ToString(), hence the preference for string vs .ToString()).

It can lead to this:

image

But I don't think anything is "escaping its scope" here...

Repro steps

It is thrown by this code:

type Foo<'T> =
    | Bar of 'T
    override this.ToString() =    // FS0670
        match this with
        | Bar x -> string x

Expected behavior

It should compile. The string function is so basic to the language, it should be used whenever x.ToString() can be used.

Actual behavior

Using string can raise the following compile error:

FS0670 This code is not sufficiently generic. The type variable ^T could not be generalized because it would escape its scope.

Known workarounds

I found two workarounds:

  1. Don't use string and stick to ToString(), but that's a PITA
  2. Write your own function (inline or not) that doesn't expose this problem:
    /// Shadowing Core.string function.
    /// Uses obj.ReferenceEquals instead of isNull to prevent null-constraint
    let string x = if obj.ReferenceEquals(x, null) then "" else x.ToString()

Related information

Seen on any recent F# version. I did not (yet) test whether this is a regression.

@BentTranberg
Copy link

I have the habit of using string x as opposed to x.ToString(), and have had several cases where string x gave me a compile time error. Certainly a surprise. Just wanted to let you know, so you don't think it's rare. Anybody else that want to say the same, then how about you just give me a thumbs up here so that we don't fill up this issue with a (possibly) long trail of "me too".

@brianrourkeboll
Copy link
Contributor

The same error shows up if you use ToString but mark the type parameter as statically resolved (^T):

type Foo< ^T > =
    | Bar of ^T
    override this.ToString () = // FS0670
        match this with
        | Bar x -> x.ToString ()

Not sure if this is relevant, but if you hide obj.ToString with a new inline member of the same name while using string in the body, it compiles (albeit with a warning that you should override instead, of course, although you couldn't actually use inline along with override):

type Baz< ^U > =
    | Qux of ^U
    member inline this.ToString () =
        match this with
        | Qux q -> string q

@abelbraaksma
Copy link
Contributor Author

Not sure if this is relevant, but if you hide obj.ToString with a new inline member of the same name while using string in the body, it compiles

I think it's relevant, because it suggests at why the error is happening in the first place. ToString() is virtual. If a base class calls it, it must determine what generic instantiation to choose. This is a runtime resolution. Since string is marked inline, it cannot decide statically what generic types to resolve it to.

What you did was creating a new non-virtual member that cannot be overridden. Hence the compiler knows statically what types to compile string for.

I'm not 100% sure my assumptions are correct (@dsyme, any thoughts?). But if so, one resolution might be to treat string specially in the compiler, such that it falls back to a normal generic version (not inlined) that otherwise behaves the same.

I think that we somehow should make it possible that string is a true null-safe version of ToString(). It'll go a long way in F# adoption and, of course, less surprises for the experienced programmer. Oh, and we can finally get rid of all ToString()occurrences.

@FunctionalFirst
Copy link

Another workaround:

type Foo<'T> =
    | Bar of 'T
    override this.ToString() =
        match this with
        | Bar x -> string (box x)

@dsyme
Copy link
Contributor

dsyme commented Dec 13, 2019

This is by design, since string uses static type resolution.

This must have been because there can in theory be differences between the behavior of string when statically resolved to be working on a specific numeric type v when operating on type obj. Forcing the programmer to add box as in the code above is declaring the resolution of this (pinning the behavior to string<obj>, which is another way to write this)

The implementation of string is below. Note string x is not the same as x.ToString() for various cases, e.g. culture invariance rather than current culture. I'm not 100% certain of the exact case where box >> string<obj> differs from, say, string<int> or string<DayOfWeek> or string<float> but looking at the code I guess there might be cases where there is a difference.

        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 : float      = (# "" value : float      #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : float32    = (# "" value : float32    #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : int64      = (# "" value : int64      #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : int32      = (# "" value : int32      #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : int16      = (# "" value : int16      #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : nativeint  = (# "" value : nativeint  #).ToString()
             when ^T : sbyte      = (# "" value : sbyte      #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : uint64     = (# "" value : uint64     #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : uint32     = (# "" value : uint32     #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : int16      = (# "" value : int16      #).ToString("g",CultureInfo.InvariantCulture)
             when ^T : unativeint = (# "" value : unativeint #).ToString()
             when ^T : byte       = (# "" value : byte       #).ToString("g",CultureInfo.InvariantCulture)

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Dec 20, 2019

@dsyme, thanks for the insights. From the docs (for any numeric type):

The ToString() method formats a Double value in the default ("G", or general) format of the current culture. If you want to specify a different format, precision, or culture, use the other overloads of the ToString method, as follows:

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

In other words, these all do exactly the same:

string someFloat
someFloat.ToString(null, CultureInfo.InvariantCulture)  // IFormattable
someFloat.ToString(CultureInfo.InvariantCulture)    // overload, not IFormattable
(someFloat :> IFormattable).ToString(null, CultureInfo.InvariantCulture);;

That leaves nativeint and unativeint. These are not IFormattable, so they'll stay as they are.

We could, therefor, simplify the code as follows without backward compat issues, and that'll allow the F# to continue using InvariantCulture:

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

Wrt to enums, they will still print their symbolic value. I.e., after this change, string NumberStyles.AllowCurrencySymbol will give "AllowCurrencySymbol".

Would such a PR be accepted? It will solve the problem with string not being allowed to be used in certain contexts. Obviously, you'd lose inlining, but I reckon that JIT will inline it anyway, so that's not a big loss.

@dsyme
Copy link
Contributor

dsyme commented Jan 7, 2020

That leaves nativeint and unativeint. These are not IFormattable, so they'll stay as they are.

How about the other integer cases? Do they default to "g" as well? I suppose so?

Would such a PR be accepted?

Yes I think so - it's definitely a simpler experience if this is not statically resolved. That said, I don't recall us making a change from statically-resolved to non-statically resolved for such a basic function before, so there may be some subtlety. Perhaps push a PR and run it through tests as a first indication?

@abelbraaksma
Copy link
Contributor Author

How about the other integer cases? Do they default to "g" as well? I suppose so?

@dsyme Yes, this is true for all,except the two native integers (but that's irrelevant, they already use the default).

so there may be some subtlety

I had the same feeling, but there's a way to find out.

Perhaps push a PR and run it through tests as a first indication?

That's the way ;)

If tests check the generated IL they may fail, we'll see. Obviously, the compiled IL will differ.

I'll make a PR and see what happens.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 22, 2020

The solution I chose in the referenced PR was eventually to remove the bug that the optimization paths were never chosen, and to improve and extend the optimization paths. In the top of the PR, you can find the performance benefit charts. Basically: it's between 10% gain and 900% gain,depending on input type.

The error doesn't rise anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants