CA1305 not working for string interpolation or concatenation without culture

[wizofaus]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.6.2

Diagnostic ID

Repro steps

Write code such as Console.WriteLine("Today's date is " + DateTime.Now + ", and a random number is " + new Random().Next); or Console.WriteLine($"Today's date is {DateTime.Now} and a random number is {new Random().Next()}");

and compile with CA1305 configured as an error

Expected behavior

Both use the user's current culture, and hence could produce unexpected outputs in non-US locales (or even US locale with custom date formats etc)., and should trigger the CA1305 error.

Actual behavior

Code compiles with no warnings etc.

[Evangelink]

@mavasani I am not sure what was/is expected from this ticket.

I don't think that the rule shall be reported on all concatenation nor string interpolation, as this will probably result in a lot of False Positive. There is no way to know whether a string is expected to be localized or not.

I think that the best solution might be to report on string concatenation or string interpolation that are passed as string argument of a method that as an overload with a culture argument. That's probably what was intended here but I wanted to confirm so.

Besides, I tend to think that might be useful to add a list of excluded methods so that we don't have to add a pragma for all let's say logging call that we have if we decided not to localize logs.

WDYT?

[wizofaus]

Adding .ToString() to DateTime.Now or new Random().Next() does produce the warning though, even though it effectively means the same thing. So yes, I would expect it in exactly this case.

FWIW I agree that there are clearly lots of cases where this rule can and does produce "false positives", but ensuring that correct culture is always specified (Invariant or otherwise) is better than having your software unexpectedly output the wrong format when run on a system with a different locale than the one you're developing on.

[AdamSmith-Msft]

I'd love to see an intelligent solution such as what's discussed here: http://blog.frankbakker.net/2015/09/c-string-interpolation-best-practices.html

[Bouke]

@AdamSmith-Msft I came here to reference that blog post. See also the implementation he provided on GH: https://github.com/FrankBakkerNl/StringInterpolationAnalyzer.

[Eli-Black-Work]

Also, this produces a CA1305 warning:

int x = 20;

ShowUserString(string.Format("Number: {0}", x);

but this does not:

int x = 20;

ShowUserString($"Number: {x}");

[FrankBakkerNl]

I just saw my blog post and git repo from five years ago were mentioned here :-) That's cool! This was just me experimenting with this. However It would be very cool to see my ideas beeing picked up here. So please feel free to use my code / thoughts. The rule I implemented is:

If an interpolated string has any interpolations that implement IFormattable, the interpolated string SHOULD be assigned to a FormattableString. This way we are sure the developer has made an explicit choice on how the formattable interpolations should be formatted.

I had not thought about string concatinations (or any implicit ToString() calls for that matter) That would be a nice addition

[mrange]

Adding more cases to this:

return MakePackUri(string.Format(@"{0}.fx.ps", name));

Results in a warning (Great)!

This doesn't:

return MakePackUri($"{name}.fx.ps");

Both have the same issue of depending on the current culture which can produce wanted or as often is the case unwanted difference in behavior.

IMHO if the first one produces as warning the second one should to and a suitable fix should be suggested.

Now I have to search through my code-base manually for unsafe usages of string interpolation.

[Eli-Black-Work]

Would still love to see this implemented. Some people on my team recently "fixed" a bunch of CA1305 warnings by converting code to use interpolated strings 🙂

They converted this:

var time = new DateTime();
string whenToRun = time.ToString();

to this:

var time = new DateTime();
string whenToRun = $"{time}";

And then all of the warnings disappeared.

BTW, it'd be nice if this analyzer (when implemented), guided the user to consider using FormattableString.Invariant() 🙂

e.g.

string example = FormattableString.Invariant($"Today's date is {DateTime.Now} and a random number is {new Random().Next()}");

[Eli-Black-Work]

@Evangelink What would be needed to move forward on this? 🙂

I just had another discussion with a coworker about this while reviewing a PR – it's something that we're running into fairly frequently.

[schibu007]

Any news on this issue? It would really be nice to have this feature.

[davidroth]

We recently had to fix a bug due to not having this analyzer. We had some code which called some API using httpClient and used string interpolation to include some price information.

// netPrice: decimal 
var response = await httpClient.GetAsync($"datasheet?netPrice={netPrice}");

This worked as long as the client machine was in a culture that uses "." as a decimal separator (e.g. English). It broke as soon as the client was run with a culture that used "," as a decimal separator, because the server api apparently expects an invariant culture.

Fix was easy:

 // netPrice: decimal
var response = await httpClient.GetAsync(FormattableString.Invariant($"datasheet/{messageId}?netPrice={netPrice}"));

An analyzer would have saved us here from a bug hitting our customers.

[mrange]

@davidroth I fix one of these in our code base about once a month. Might be more I am not aware that I missed.

Just FYI - AFAIK the FormattableString.Invariant falls back to the slower pattern of using string.Format rather than the quicker pattern using a string builder (kind of). Does the right thing which is the important part but I thought I share this in case you weren't aware of this subtle difference.

PS. A shorter way of doing invariant formatting would be nice too. Devs thinks bit of the point of string interpolation (succinct) is lost when forcing a tax of FormattableString.Invariant on each usage.

[davidroth]

Just FYI - AFAIK the FormattableString.Invariant falls back to the slower pattern of using string.Format rather than the quicker pattern using a string builder (kind of). Does the right thing which is the important part but I thought I share this in case you weren't aware of this subtle difference.

Yeah I know. The following code uses the performance optimized DefaultInterpolatedStringHandler implementation:

string.Create(CultureInfo.InvariantCulture, $"{23m}")

Unfortunately, its ugly.

PS. A shorter way of doing invariant formatting would be nice too. Devs thinks bit of the point of string interpolation (succinct) is lost when forcing a tax of FormattableString.Invariant on each usage.

Agree. The only mitigation is to use using static System.FormattableString which makes it a bit more succinct:

Invariant($"{23m}")

[Alex-Sob]

Also CA1305 documentation is outdated, here it states: "use a C# interpolated string and pass it to the FormattableString.Invariant method."

As Stephen pointed out in his article, new string.Create overlodas should be used for that.

[cremor]

@Alex-Sob Please report documentation issues via the "This page" button at the bottom of the documentation page.

edit: See dotnet/docs#36256