DotNetAnalyzers / StyleCopAnalyzers

An implementation of StyleCop rules using the .NET Compiler Platform
MIT License
2.61k stars 506 forks source link

Supressing Rules Using .editorconfig Files #3092

Open RehanSaeed opened 4 years ago

RehanSaeed commented 4 years ago

I've been trying to use an .editorconfig file to supress rules like so:

# XML comment analysis is disabled due to project configuration
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA0001.md
dotnet_diagnostic.SA0001.severity = silent

# Single-line comment should be preceded by blank line
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1515.md
dotnet_diagnostic.SA1515.severity = silent

# A C# code element is missing a documentation header.
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1600.md
dotnet_diagnostic.SA1600.severity = silent

# A C# partial element is missing a documentation header.
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1601.md
dotnet_diagnostic.SA1601.severity = silent

# An item within a C# enumeration is missing an Xml documentation header.
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1602.md
dotnet_diagnostic.SA1602.severity = silent

# Documentation text should not be empty
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1627.md
dotnet_diagnostic.SA1627.severity = silent

# Documentation text should end with a period
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1629.md
dotnet_diagnostic.SA1629.severity = silent

# A C# code file is missing a standard file header.
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1633.md
dotnet_diagnostic.SA1633.severity = silent

# XML comment analysis is disabled due to project configuration
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA0001.md
dotnet_diagnostic.SA0001.severity = silent

This actually works very well for the most part. However, this does not work when you want to supress SA0001. Are there any downsides or limitations to using .editorconfig to supress rules?

sharwell commented 4 years ago

📝 When SA0001 is disabled, there are no guarantees about the behavior of any SA16xx rules (they could work fine, silently be disabled, or silently have behavior that differs from the documentation, and this behavior is allowed to change at will between IDEs and versions of the compiler).

The .editorconfig file can only be used to disable diagnostics that are reported at a location within a file. SA0001 is not one of these rules, so it can only be disabled via a .ruleset file or the <NoWarn> element of the project.

⚠️ In case it was not clear, SA0001 should not be disabled. This diagnostic means there is a problem with the project configuration that needs to be corrected.

julealgon commented 4 years ago

The .editorconfig file can only be used to disable diagnostics that are reported at a location within a file.

This is not true @sharwell ; This works 100% for me:

[/*Tests/**]
# SA0001: XML comment analysis is disabled due to project configuration
dotnet_diagnostic.SA0001.severity = none

Also:

SA0001 should not be disabled. This diagnostic means there is a problem with the project configuration that needs to be corrected.

I have zero intention of exposing XML documentation for a UnitTest project, so I don't see how I should be forced to add it. That's not a "problem with the project configuration" at all.

I still want to have StyleCop enforcing all non-XML-doc rules though on those unit test projects.

sharwell commented 4 years ago

This is not true @sharwell ; This works 100% for me:

This is the Roslyn request to add this support to the analyzer driver (first item in the list): https://github.com/dotnet/roslyn/issues/38042

I'm not sure how this is working for you but from the compiler side it's a documented limitation of .editorconfig files.

I have zero intention of exposing XML documentation for a UnitTest project, so I don't see how I should be forced to add it. That's not a "problem with the project configuration" at all.

I was simply stating the view of this project with respect to the rule. It's caused us a lot of trouble over time with various bug reports so if someone asks to disable it I have to set the proper expectations: StyleCop Analyzers has known incompatibilities with such a configuration and as such strongly encourage that it not be done that way.

julealgon commented 4 years ago

I understand where you are coming from @sharwell , but I think it's ill advised to recommend people to always generate the XML documentation file saying that this is some sort of problem in their projects, when in reality it is a bug in StyleCop analyzers itself. Looks like there is some unwanted coupling in StyleCop's logic there: why even require XML docs to be emitted to be able to validate the summary comments? Those are separate concerns.

As for the editorconfig restriction working here, I tested it multiple times and it is working as intended. If I remove that section, I start getting errors related to lack of documentation as well as SA0001.

sharwell commented 4 years ago

... when in reality it is a bug in StyleCop analyzers itself ...

This is not correct. When the XML output is disabled, the compiler (sometimes) interprets /// as a normal line comment and explicitly tells the analyzers that there are no documentation comments in the source files. Analyzers can only process documentation comments correctly if the compiler parses them correctly.

As for the editorconfig restriction working here, I tested it multiple times and it is working as intended. If I remove that section, I start getting errors related to lack of documentation as well as SA0001.

@mavasani ?

julealgon commented 4 years ago

This is not correct.

You said it yourself StyleCop would potentially not work correctly with the XML generation disabled, not me. That sounded like a bug description if I've ever seen one.

When the XML output is disabled, the compiler (sometimes) interprets /// as a normal line comment and explicitly tells the analyzers that there are no documentation comments in the source files.

That behavior from the compiler sounds like intended behavior to me. Is it not? In any case, if summaries are reported as normal comments in that situation to the analyzer, I don't see why there would be any problems? Can't you then just handle them as normal comments on StyleCop too?

Again, StyleCop has many more features besides comment analysis, so I see zero reason to create this hard coupling with XML generation. I want to have all other bonuses of StyleCop in my test projects for consistency, but class documentation makes zero sense in unit test projects for us (they do on test helper projects though, as those are reusable libraries), so I don't want those being generated.

Adding forced XML generation in these projects is a clear violation of the principle of least astonishment: it will create confusion among our teams, or require "hack"-style comments to be added justifying the flag's presence due to weird behavior from StyleCop.

jrmoreno1 commented 11 months ago

I don't know if this should be a separate issue, but I would prefer that no XML style rules be applied in projects that ends in "Tests" (having found this issue while looking for exactly that).

mvonballmo-uster commented 2 months ago

For anyone who fought their way through the comments to get here, this is my takeaway:

Why can't I have a car without a steering wheel? I'm just going to drive in a straight line anyway. You're making me use a steering wheel I'll never need.