Numeric format strings silently ignoring unsupported values or producing wrong results

[kerams]

let x = 100000
x.ToString "#,#" |> printfn "%s"
x.ToString "N0" |> printfn "%s"
printfn $"{x:N0}"

In fsi, this prints 3 identical lines based on the current culture.

In Fable, the result is

100,000
100000
%P(N0)

• [x] x.ToString "#,#" produces the correct string, but I'd appreciate a Fable warning saying that invariant culture will be used (I think I've seen something like that in other places).
• [x] x.ToString "N0" silently produces an incorrect string. An unsupported format error is warranted in my opinion.
• [ ] $"{x:N0}" creates the compiled representation of the interpolation string, which is complete nonsense. An unsupported format error is warranted in my opinion.
• [ ] Also $"{1000:``#,#``} items" results in %P(#,#) items.

[MangelMaxime]

x.ToString "#,#" produces the correct string, but I'd appreciate a Fable warning saying that invariant culture will be used (I think I've seen something like that in other places).

I didn't know Fable was doing that in some cases, but I found this example:

TimeSpan.Zero.ToString("g") |> ignore

Will make Fable compilation fails with this error:

FABLE: TimeSpan.ToString with one argument is not supported, because it depends of local culture, please add CultureInfo.InvariantCulture

@kerams Is it what you are referring too?

[kerams]

I think so. Guess it's not a warning but an error.

[MangelMaxime]

The warning one that I know about is something like if you pass a CultureInfo parameter Fable says, that it will just be ignored.

I am just discovering the error one today thanks to you.

[MangelMaxime]

I am looking at fixing this issue and I am able to fix the first 2 issues.

Looking more into these issues, I have the feeling that we are having a different behavior / expectation depending on what API we are calling.

If we call (5592405).ToString("x") or TimeSpan.ToString("g"), we expect to generate an error asking the user to provide CultureInfo.InvariantCulture.

But if we call (5592405).ToString() or TimeSpan.ToString(), we don't generate the error even if this API is culture dependent (I believe). Perhaps, we don't generate the error for keeping the user code simpler as he doesn't need to do TimeSpan.ToString(CultureInfo.InvariantCulture) everywhere and we state on the docs that Fable is behaving as if CultureInfo.InvariantCulture was passed globally.

Finally, if we call DateTime.Now.ToString("O") then Fable don't generate the error, perhaps we should?

I think it would be nice to coming up with a convention and try to apply it everywhere. To do so we need to decide what is the balance we want?

1. Do we want to enforce the same behavior between .NET and Fable? And prevent potential differences?

   Then we probably want to force the user to pass CultureInfo.InvariantCulture. Do we want to force it only when using custom format or all the time?

2. Do we prefer a lighter API, and consider that CultureInfo.InvariantCulture is enforced globally ?

   Then we don't want to generate errors when not passing CultureInfo.InvariantCulture

@alfonsogarciacaro @ncave

[MangelMaxime]

For reference, while we wait for a decision to be taken.

To generate an error if no CultureInfo is passed for number formatting here is the code that can be user:

    | "ToString", [ ExprTypeAs(String, _) ] ->
        $"%s{i.DeclaringEntityFullName}.ToString with one argument is not supported, because it depends on local culture, please add CultureInfo.InvariantCulture"
        |> addError com ctx.InlinePath r

        None
    | "ToString", [ ExprTypeAs(String, format); _culture ] ->
        let format = emitExpr r String [ format ] "'{0:' + $0 + '}'"

        Helper.LibCall(
            com,
            "String",
            "format",
            t,
            [ format; thisArg.Value ],
            [ format.Type; thisArg.Value.Type ],
            ?loc = r
        )
        |> Some

To be applied here:

https://github.com/fable-compiler/Fable/blob/8273959e13080e9d8fa074d9a431d223d204d652/src/Fable.Transforms/Replacements.fs#L2499-L2511

[ncave]

@MangelMaxime Considering our limited resources, IMO all we can realistically shoot for is supporting CultureInfo.InvariantCulture. I don't think we should be throwing errors, but warnings are fine. We should be able to produce warnings on unsupported formats when the format is a const string, and otherwise just silently do the best rendering we can do for that unsupported format. Again, this is just IMO.

[kerams]

Does Fable respect TreatWarningsAsErrors? I don't think it does, in which case a warning for an usupported format is insufficient for me. I really don't want it to go unnoticed.

[MangelMaxime]

Does Fable respect TreatWarningsAsErrors?

No it doesn't but this is perhaps possible to support it.

We can probably retrieve the value of TreatWarningsAsErrors when cracking the projects and then when calling addWarn we check the value of TreatWarningsAsErrors and instead generate an error or a warning and error I don't remember the behavior in .NET. The idea is to have a simple implementation IHMO.

Considering our limited resources, IMO all we can realistically shoot for is supporting CultureInfo.InvariantCulture.

I fully agree with that.

Which leads me to think that things like:

FABLE: TimeSpan.ToString with one argument is not supported, because it depends of local culture, please add CultureInfo.InvariantCulture

should not be generated by Fable.

We should allow user to write myTimeSpan.ToString("g"), just like he can write myDateTime.ToString("yyyyy") which generates an output equivalent to myDateTime.ToString("yyyyy", CultureInfo.InvariantCulture) in .NET or if CultureInfo.InvariantCulture is set at the assembly level.

[MangelMaxime]

Just want to add that if a format is not supported, then generating an error independently of TreatWarningsAsErrors should be the expected behavior.

This is perhaps what you were was referring to @kerams.

The discussion became messy because we have 2 topics happening at once in this issue I think. Sorry for that

[kerams]

myDateTime.ToString("yyyyy") which generates an output equivalent to myDateTime.ToString("yyyyy", CultureInfo.InvariantCulture) in .NET

In .NET, CurrentCulture, which is not a thing in Fable, is used by default. The warning you referenced thus points out a difference in behavior between the platforms, but on the other hand, not all format strings are culture-dependent, and I get that having to specify InvariantCulture everywhere would be annoying.

[MangelMaxime]

In .NET, CurrentCulture, which is not a thing in Fable, is used by default. The warning you referenced thus points out a difference in behavior between the platforms, but on the other hand, not all format strings are culture-dependent, and I get that having to specify InvariantCulture everywhere would be annoying.

Right for CurrentCulture and Fable always considered it to be set to InvariantCulture because we don't support globalization.

I will make sure to check that the documentation mention this.

[MangelMaxime]

Looking back at this issue, I think we are trying to do too much.

What I mean by that is the behavior in .NET when passing an invalid format is not to generate a compile time error but instead an exception at runtime.

Furthermore trying to detect the format provided by the user is not 100% reliable for example:

let myFormat (cond : bool) =
    if cond then
        "D"
    else
        "B"

(10).ToString(myFormat true) |> printfn "%A"

In this code, trying to find "D" from at the replacement call is not easy because this is not a direct string but instead a function.

[ncave]

@MangelMaxime

Furthermore trying to detect the format provided by the user is not 100% reliable

Yes, we can only examine the .ToString(format) argument and issue an unsupported warning if the format is a const string argument supplied directly (which it is most of the time).

Still, that covers the majority of the cases. My only point against a runtime error on unsupported formats was that a working F# code (on .NET) will be throwing a hard error on Fable, personally I would rather have some value back (and a compile-time warning) vs a hard runtime error from a code that works in .NET. But that's a personal preference, of course, so I understand other people might prefer the hard error.

For the (minority of) cases where the format is not a string const and we cannot examine whether we can support it or not, we can just issue a general warning that we only support certain formats. This way we can have 100% "warning" coverage. As long as we have the warning, whether we throw an error on unsupported formats, or just the best value we can, is a matter of preference.