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

[Breaking] Warn unadorned string interpolation when the interpoland implements IFormattable / is generic #1306

Open
5 of 6 tasks
Happypig375 opened this issue Aug 17, 2023 · 4 comments

Comments

@Happypig375
Copy link
Contributor

Happypig375 commented Aug 17, 2023

A quick test. Is this function pure?

let sum x y = $"{x} + {y} = {x + y}"

Looks like the output purely depends on its inputs. Right?
sum 1 2 gives 1 + 2 = 3.
sum 1.1 1.1 gives 1.1 + 1.1 = 2.2. Right?

let sum x y = $"{x} + {y} = {x + y}"
System.Globalization.CultureInfo.CurrentCulture <- System.Globalization.CultureInfo.CreateSpecificCulture "en-US"
printfn $"{sum 1.1 1.1}"
System.Globalization.CultureInfo.CurrentCulture <- System.Globalization.CultureInfo.CreateSpecificCulture "tk-TK"
printfn $"{sum 1.1 1.1}"
1.1 + 1.1 = 2.2
1,1 + 1,1 = 2,2

What a pitfall - there is action at a distance! This function is actually impure. But if we used the format specifiers:

let sum x y = $"%g{x} + %g{y} = %g{x + y}"
System.Globalization.CultureInfo.CurrentCulture <- System.Globalization.CultureInfo.CreateSpecificCulture "en-US"
printfn $"{sum 1.1 1.1}"
System.Globalization.CultureInfo.CurrentCulture <- System.Globalization.CultureInfo.CreateSpecificCulture "tk-TK"
printfn $"{sum 1.1 1.1}"
1.1 + 1.1 = 2.2
1.1 + 1.1 = 2.2

We shouldn't create these pitfalls for newcomers to F#.

I propose we turn the warning against unadorned string interpolation on if the interpoland implements IFormattable or is generic.

The warning message would instruct the user to take action using one of the code fixes:

assume culture invariant (change all)
assume culture invariant (change once)
assume culture aware (change all)
assume culture aware (change once)

Invariants would come first since that is what F# code usually assume.

With #897 implemented (say = for equal everywhere aka "culture invariance" and # for not equal everywhere aka "culture aware"), the new code after codefix would either look like

let sum x y = $"%={x} + %={y} = %={x + y}"

or

let sum x y = $"%#{x} + %#{y} = %#{x + y}"

which are equivalent to

let sum x y = $"%=O{x} + %=O{y} = %=O{x + y}"

or

let sum x y = $"%#O{x} + %#O{y} = %#O{x + y}"

respectively, with one difference: the version without O is subject to FS3579 while the version with O is not.

The implementation can just change .ToString() to F# string function which does this properly.

This would only apply to types implementing IFormattable or generic types. Known non-IFormattable types like strings, booleans, DUs and records would be unaffected.

We can also warn for %O though it's not as important as the unadorned string interpolation.

After 3 years (same timeframe as .NET LTS support), we may change the default to culture invariant and remove this warning. (This is what makes this suggestion not purely a tooling suggestion)

The existing way of approaching this problem in F# is either to look out for such mistakes manually or enforce usage of % formatting everywhere.

Pros and Cons

The advantages of making this adjustment to F# are

  1. Easier to understand for beginners
  2. Avoid a class of potential bugs
  3. A better default for F#

The disadvantage of making this adjustment to F# is that this is a breaking change for those who turn WarnAsError on.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): M

Related suggestions:

Affidavit (please submit!)

Please tick these items by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on StackOverflow) and I have searched StackOverflow for discussions of this issue
  • This is a language change and not purely a tooling change (e.g. compiler bug, editor support, warning/error messages, new warning, non-breaking optimisation) belonging to the compiler and tooling repository
  • This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@BentTranberg
Copy link

BentTranberg commented Aug 17, 2023

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

@Happypig375
Copy link
Contributor Author

Happypig375 commented Aug 18, 2023

Actually we can exempt integers since they seem to format the same by default whether current culture or invariant culture. If they use .NET formatting specifiers then they would be looking for culture specific formatting - we can ignore them.

@charlesroddie
Copy link

charlesroddie commented Aug 18, 2023

The detection doesn't seem pristine here. It is an attempt to warn on culture-dependence. Something being IFormattable has the ability, in addition to ToString() to apply a format string as an additional input. There is nothing requiring IFormattable types to have culture-independent behaviour on formatting with a format string, or culture-dependent behaviour with ToString(). So an unformatted IFormatable object may not be a problem, and a formatted IFormattable object may be a problem. Also there is nothing preventing a non-IFormattable type from having culture-dependent behaviour.

F# string interpolation is not responsible for the culture-dependent behaviour here - objects implementing ToString() in a culture-dependent way are -, and nor does F# have the information required to fix this.

There is already a way to switch off the pitfall described: culture-invariant mode.

@Happypig375
Copy link
Contributor Author

@charlesroddie F#'s string function guarantees culture-invariance by testing for IFormattable and passing in InvariantCulture. If we had used it, this wouldn't have been an issue. We can't change C# types implementing .ToString() in a culture dependent way but we can avoid calling .ToString() directly and instead call our own string function.

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

No branches or pull requests

3 participants