IDE0058 should offer excluded_symbol_names configuration

[shuebner]

Analyzer

Diagnostic ID: IDE0058

Describe the improvement

I would like to enable IDE0058 with warning severity. However, that leads to a lot of useless warnings for any kind of builder, e. g. StringBuilder. I would like to be able to exclude types like StringBuilder from IDE0058.

Describe suggestions on how to achieve the rule

Offer excluded_symbol_names configuration for IDE0058

Additional context

[mavasani]

IDExxxx code style rules are implemented in Roslyn repo, moving..

[sharwell]

I'm a bit concerned that the option presented might not be sufficient to support enabling IDE0058 at warning severity. I'd be very interested in seeing a test performed (e.g. via a diagnostic suppressor) to try and validate how many false positives remain in real-world code bases after implementing this exclusion.

[sharwell]

@shuebner Here's an example of a diagnostic suppressor we created to allow a rule (VSTHRD200) to be used in a context where it wasn't originally intended (https://github.com/microsoft/vs-threading/issues/670):

https://github.com/dotnet/roslyn-analyzers/blob/d5f910ba4c4aa62a3e9017aba3e5e745c905f865/src/Roslyn.Diagnostics.Analyzers/Core/RelaxTestNamingSuppressor.cs

[shuebner]

I'm a bit concerned that the option presented might not be sufficient to support enabling IDE0058 at warning severity. I'd be very interested in seeing a test performed (e.g. via a diagnostic suppressor) to try and validate how many false positives remain in real-world code bases after implementing this exclusion.

I am not following. At the extreme you would list all types on which IDE0058 reports in the current solution. No false positives remain. Then, when you ignore a return value from a new type, that is a new warning, i. e. we err on the safe side. If it is another false positive from a builder or fluent api, you add the type to excluded_symbol_names. I do not see the problem here.

I assume the suppressor was for scenarios where test classes are not in their own file system folder and can thus not have their own .editorconfig? At first, I was thinking about writing a suppressor that would read a special IDE0058 setting from the .editorconfig, allowing me to specify types on which to suppress warnings. But then I saw that excluded_symbol_names existed and already does exactly that. Why introduce another independent mechanism like a suppressor when there is already an established, consistent mechanism for the use-case? excluded_symbol_names is already available for a plethora of rules. With documentation ID format it even uses a well-defined format that enables exclusion of types or even single methods on types. The only thing that may be missing (at least I did not see it documented) is the usage of wildcards in the documentation ID to e. g. exclude whole namespaces. So, why make up anything new?

[ggirard07]

@shuebner Here's an example of a diagnostic suppressor we created

Can you provide a bit more information about how this could be reused in any project to disable this rule (VSTHRD200) in this specific test context? Are we supposed to copy/paste code directly in test assembly? Or are we supposed to find which NuGet package this suppressor and enable it through editorconfig?

[shuebner]

@ggirard07 before going down the diagnostic suppressor road, please be aware that diagnostic suppressors pretty much only work with the dotnet CLI and VS for Windows. They are not supported by e. g. Rider, VS for Mac or VS Code, i. e. you will still get the warnings there.

[glen-84]

Duplicate of #47832.