search

dotnet / roslyn-analyzers

MIT License
1.58k stars 463 forks source link

RS1035 is in conflict with CA1305 (and there are no docs about how to resolve the conflict) #7086

Open bradwilson opened 9 months ago

bradwilson commented 9 months ago

Analyzer

Diagnostic ID: RS1035: The symbol 'CultureInfo.CurrentCulture' is banned for use by analyzers: Analyzers should use the locale given by the compiler command line arguments, not the CurrentCulture

Analyzer source

NuGet Package: Microsoft.CodeAnalysis

Version: 4.8.0

Describe the bug

Analyzer rule CA1305 requires using some culture in calls to string.Format, but rule RS1035 (new in 4.8, I think?) prohibits them. These rules are in conflict.

Further, the documentation link for RS1035 is broken in the Visual Studio UI. Googling for RS1035 yields a few issues in this repo, but does not yield a link to an active documentation page, so the call to action is not implementable.

Steps To Reproduce

Use string.Format(CultureInfo.CurrentCulture, "{0}", 42); in an analyzer linked against Microsoft.CodeAnalysis 4.8.0.

Expected behavior

Instructions on what to do to resolve the conflict.

Actual behavior

No help.

bradwilson commented 9 months ago

The only place I could find this surfaced was here: https://github.com/dotnet/roslyn/blob/fd3194f093806a6b2577a45aa89b0fd60658eee2/src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs#L1727

I was unable to discover any way to get access to this from either AnalysisContext (for analyzers) or CodeFixContext (for fixes).

I have chosen to prefer CA1305 over RS1035 until this conflict can be resolved: https://github.com/xunit/xunit.analyzers/blob/7b8a772f5c4df014c5a3b1f34282e20be68bad08/.editorconfig#L234

mavasani commented 9 months ago

@RikkiGibson

bradwilson commented 8 months ago

Ping @RikkiGibson 😄

RikkiGibson commented 8 months ago

The compiler takes /preferreduilang as a command line argument, and we access it internally through CommonCompiler.Culture. It appears we have neglected to add a public API for it.

Resolution should be via a new public API, and adjusting the diagnostic message to recommend use of the API:

 namespace Microsoft.CodeAnalysis;

 public abstract class CompilationOptions
 {
+    /// <summary>The culture to be used for compiler output.</summary>
+    public CultureInfo Culture { get; }
 }

I'll work on getting an API proposal into a reviewable state by Roslyn API review. It should be very straightforward to implement.

RikkiGibson commented 2 months ago

Revisiting. I think the actual resolution per https://github.com/dotnet/roslyn-analyzers/pull/7167#issuecomment-1939844853 is:

  1. Analyzers should use LocalizableResourceString
  2. Code fixers should be permitted to use CurrentCulture, while analyzers continue to warn

It would be nice to take care of this in the quality milestone, since it seems fairly common to hit it.