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

String interpolation and culture sensitivity lead to tests failing depending on culture #10030

Closed
abelbraaksma opened this issue Aug 28, 2020 · 4 comments

Comments

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Aug 28, 2020

This may have been discussed somewhere in the years leading up to the implementation of string interpolation, but I can't readily find a decision, and there's no mention in the RFC.

Tests like String interpolation using .NET Formats and String interpolation of basic types are failing on my system (nl-NL) and seem to be written with the assumption that string interpolation is culture invariant.

I think it'd be a good idea we discuss this and there're basically two options:

  • We leave things as-is, this means that interpolation is in line with C# interpolation, in line with %O parsing, but out of line with most string functions in F#.
  • We fix it to be invariant culture, which speeds up runtime exec of such expressions significantly, brings it in line with the string function, but diverges a little bit from %O.

Most string-related stuff in F# is written to be culture invariant. It came as a surprise to me that string interpolation isn't. Once this reaches a larger audience, this may lead to more people surprised about this.

Of course, the opposite position can also be defended with similar arguments.

I just think we need an informed decision here. The number of ways an object can now be stringified in F# gets larger and larger, and each method has its own little quirks. For string interpolation, we already have these odd differences, just saying it wouldn't be a total surprise to diverge a bit from the %O semantics:

printfn $"{None}"  // empty string
printfn $"{null}"    // empty string
printfn $"%O{None}"  // string "<null>"
printfn $"%O{null}"    // string "<null>"

Whatever the decision, we should make sure that there are tests covering cultures with string interpolation, and make the ones currently dealing with floats, negative integers and decimals culture-independent (for instance by setting the thread culture before the test runs, and resetting after).

An alternative idea might be to allow an inline culture string to be present in the interp expr somehow.

@abelbraaksma abelbraaksma changed the title String interpolation and culture sensitivity String interpolation and culture sensitivity lead to tests failing depending on culture Aug 30, 2020
@dsyme
Copy link
Contributor

dsyme commented Aug 31, 2020

Tests like String interpolation using .NET Formats and String interpolation of basic types are failing on my system (nl-NL) and seem to be written with the assumption that string interpolation is culture invariant.

Hmmm could you add which specific lines in the tests are failing, and how to repro in CI (e.g. by setting culture)?

@dsyme
Copy link
Contributor

dsyme commented Aug 31, 2020

We do have to investigate this. My expectation is that these are known culture sensitivities documented here: https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/plaintext-formatting#net-values

The tests should always be passing though, so it's either a test issue or a bug. I'd expect the former but we need to check.

The issue of culture settings and %A is also discussed here: fsharp/fslang-suggestions#897

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 31, 2020

@dsyme, the tests that are failing are any tests that use either a float or a decimal with a fractional part in the stringified output.

  • String interpolation of basic types

       System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
      ----> System.Exception : test case 'vcewweh221q1r' failed, expected "1.01", got "1,01"
    
  • String interpolation using NET Formats

    System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
      ----> System.Exception : test case 'vcewweh222w' failed, expected "abc 1.000", got "abc 1,000"
    

The issue of culture settings and %A is also discussed here: fsharp/fslang-suggestions#897

I'll have to read into that, but here it's about interpolated strings which are rendered by %O. Either way, if that's the direction we're going, I'm fine with that, though we should probably document it in the RFC for interpolated strings.

At least it's consistent with how it works now:

> sprintf "%O" 12.34;;
val it : string = "12,34"    // comma, not dot

and how to repro in CI (e.g. by setting culture)?

Most European cultures would work: de-DE, nl-NL, be-NL, be-FR, it-IT etc.

If this is indeed desired behavior, we should add tests like the ones in the C# interpolated strings manual:

double speedOfLight = 299792.458;
FormattableString message = $"The speed of light is {speedOfLight:N3} km/s.";

System.Globalization.CultureInfo.CurrentCulture = System.Globalization.CultureInfo.GetCultureInfo("nl-NL");
string messageInCurrentCulture = message.ToString();

var specificCulture = System.Globalization.CultureInfo.GetCultureInfo("en-IN");
string messageInSpecificCulture = message.ToString(specificCulture);

string messageInInvariantCulture = FormattableString.Invariant(message);

Console.WriteLine($"{System.Globalization.CultureInfo.CurrentCulture,-10} {messageInCurrentCulture}");
Console.WriteLine($"{specificCulture,-10} {messageInSpecificCulture}");
Console.WriteLine($"{"Invariant",-10} {messageInInvariantCulture}");
// Expected output is:
// nl-NL      The speed of light is 299.792,458 km/s.
// en-IN      The speed of light is 2,99,792.458 km/s.
// Invariant  The speed of light is 299,792.458 km/s.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 31, 2020

I recently used the following pattern to ensure we enforce InvariantCulture for string (which is how it has always been), the same approach can be used by other string-manipulation functions to ensure we follow the thread culture, perhaps we put this stuff in a using block or something:

// Following tests ensure that InvariantCulture is used if type implements IFormattable
        
// safe current culture, then switch culture
let currentCI = Thread.CurrentThread.CurrentCulture
Thread.CurrentThread.CurrentCulture <- CultureInfo.GetCultureInfo("de-DE")

// make sure the culture switch happened, and verify
let wrongResult = 123.456M.ToString()
Assert.AreEqual("123,456", wrongResult)

// test that culture has no influence on decimals with `string`
let correctResult = Operators.string 123.456M
Assert.AreEqual("123.456", correctResult)

// make sure that the German culture is indeed selected for DateTime
let dttm = DateTime(2020, 6, 23)
let wrongResult = dttm.ToString()
Assert.AreEqual("23.06.2020 00:00:00", wrongResult)

// test that culture has no influence on DateTime types when used with `string`
let correctResult = Operators.string dttm
Assert.AreEqual("06/23/2020 00:00:00", correctResult)

// reset the culture
Thread.CurrentThread.CurrentCulture <- currentCI

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

No branches or pull requests

4 participants