Adjust message for banned symbol CultureInfo.CurrentCulture

[RikkiGibson]

Related to #7086

I looked into this scenario further and found the doc Localizing Analyzers. Basically, it appears that the pattern we want analyzer authors to use is to put culture-dependent formattable strings in a .resx file, and refer to those strings using LocalizableResourceString.

It's possible that we will still want to introduce a public API to get the CultureInfo which is being used by the compiler, mentioned in #7086 (comment). But I thought I would get this change in first thing, as it should help some people hitting the issue to move forward without needing to wait for a possible public API change and for a new compiler version to ship.

[bradwilson]

Interesting. This completely changes the meaning of the message, and now makes me wonder whether this has anything to do with analyzers at all.

Consider the xUnit.net analyzer for ensuring that async assertions are awaited. We did not write this analyzer to ensure that all async methods are awaited, but rather only our own. (Yes, of course we always want you to pay attention to async methods. 😂) We did this because we wanted to highlight a pattern of false positives that come from not doing what you think you're doing.

Here, the old message said "CurrentCulture isn't right, because the user can specify the culture via the command line". That is an entirely reasonable thing to say, and a very good reason to have this message. Now, though, you're saying "you should be using localized strings", which while it may be good general purpose advice, doesn't feel like it's the responsibility of an analyzer for Roslyn analyzers.

If this rule is really about using localizable strings—as opposed to ensuring proper formatting for values going into formatted strings—then I definitely feel safe in ignoring the rule entirely, since I don't localize strings. But I suspect that there was value in the previous message, which is now lost in the new message.

Another way to look at this: LocalizableResourceString is not a replacement for String.Format, which is where I ran across this message originally. So now the advice feels both inappropriately placed ("please localize things" is how it reads to me now) as well as incorrect (because the suggestion isn't a replacement for using CultureInfo.CurrentCulture in String.Format).

Does that make sense? Am I misinterpreting the intention of this change? Am I misinterpreting the original intention of the rule?

[RikkiGibson]

LocalizableResourceString is not a replacement for String.Format, which is where I ran across this message originally

I think we are trying to say: yes, LocalizableResourceString is the replacement for string.Format.

When you have a format string within an analyzer, we are saying: even when you are not intending to actually localize your analyzer, we want you to adopt the same indirection which we support for localized analyzers. Basically:

• Put your format string in a resx file
• Refer to it using a LocalizableResourceString
• Pass the LocalizableResourceString to the DiagnosticDescriptor constructor and any other relevant Roslyn APIs
• The analyzer host will take your LocalizableResourceString, lookup the best string for the compiler's culture in your resource file, and format the diagnostic arguments using the compiler's culture.

It's quite possible that this suggested flow is failing to support some use cases for string.Format and other culture-dependent APIs in analyzers. In which case, that would be further motivation to expose the culture in compilation options, as suggested in #7086 (comment).

[RikkiGibson]

I think the scenario/flow I outlined above may not be relevant for the problem the xunit analyzers are experiencing. It looks like many of the xunit analyzers' use of string.Format is actually in the title/message for CodeAction.Create.

https://github.com/search?q=repo%3Axunit%2Fxunit.analyzers+string.Format+path%3A%2F%5Esrc%5C%2Fxunit.analyzers.fixes%5C%2F%2F&type=code&p=1

@mavasani @dotnet/roslyn-ide How are code fixers supposed to produce localized title/message strings? I don't see any CodeAction constructor which takes LocalizableString/LocalizableResourceString, for example. In the compiler we don't want people using CurrentCulture because the /preferreduilang compiler argument is supposed to win there. That appears to be why the LocalizableString type is used in DiagnosticDescriptor and so on. But what should people be doing in the IDE? It seems Roslyn IDE itself is accessing strings right off the resources type? Does this mean that in analyzers we don't want people using CurrentCulture, but in code fixers we are fine with people using it implicitly? If so, how should the banned API rules be adjusted to reflect that?

[bradwilson]

It looks like many of the xunit analyzers' use of string.Format is actually in the title/message for CodeAction.Create

This analysis is correct. There are zero instances of string.Format in the analyzers, and we use the descriptor's formatting capabilities (though not with localized strings, but that shouldn't matter; we let you do the formatting and choose the culture).

Does this mean that in analyzers we don't want people using CurrentCulture, but in code fixers we are fine with people using it implicitly?

Understanding the situation now, yes, I'm 100% convinced that my decision to ignore RS1035 is correct and shouldn't cause me any concern. If/when you have the equivalent of formatted messages for fixers, I'll happy switch to those APIs and re-enable the rule, but the end result should be effectively identical, since fixers don't have a notion of the "command line culture" at all.

[bradwilson]

It might be worth considering where other Microsoft.CodeAnalysis might run afoul of this rule as well. I don't create any refactorings or code generators (yet), but it's worth understanding whether this rule would also incorrectly flag for any of those scenarios.

[bradwilson]

Ping @mavasani @jaredpar, thoughts about how to reconcile RS1035's ban against the fact that there are no APIs available in CodeAction with formatting support (either via Create or the CodeAction constructor)? Or perhaps update RS1035 to only be proscriptive when you see string.Format being used with DiagnosticDescriptor?

[RikkiGibson]

though not with localized strings, but that shouldn't matter; we let you do the formatting and choose the culture

I think you are right about this, there isn't actually a need for analyzers to adopt LocalizableResourceString if they are not localized.

Let's confirm what code fixes are expected to be doing. FWIW, since code fixes are not supposed to be in the same assembly as analyzers (since analyzers must not depend on the workspaces layer), we might be able to solve this by introducing distinct sets of banned APIs for analyzer projects and code fixer projects, and simply excluding CurrentCulture/CurrentUICulture from the banned set for code fixer projects.

[bradwilson]

since code fixes are not supposed to be in the same assembly as analyzers

We follow this rule. 😄

excluding CurrentCulture/CurrentUICulture from the banned set for code fixer projects

👍🏼 This would be great so we can re-enable RS1035 so that it might be able to find other things we shouldn't be using.