Add a helper diagnostic for enforcing IDE0005 (remove unnecessary usi… #58835
Conversation
…ngs) on build Due to dotnet#41640, enabling IDE0005 on build requires users to enable generation of XML documentation comments. Even though this is documented with a note on IDE0005's [doc page](https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0005), we have had numerous reports of users not figuring this out and spending lot of cycles fighting with this, especially given other IDE diagnostics work just fine on build. We have had many user reports of the same: dotnet#58103, dotnet#53720, dotnet#57539, OpenRA/OpenRA#19747 and numerous other offline queries. This change enhances the IDE0005 analyzer to now detect the case when IDE0005 is being reported as a warning or an error in the IDE, but `GenerateDocumentationFile` is `false` for the project, which would mean IDE0005 wouldn't be reported on build. The analyzer reports a special helper diagnostic for this case, which recommends the user to set this property to `true` in their project file to enable IDE0005 on build. This should reduce the pain for customers who try to enforce IDE0005 on build and get hit by this issue.
| // NOTE: This is a special helper diagnostic ID which is reported when the remove unnecesssary diagnostic ID (IDE0005) is | ||
| // ecalated to a warning or an error, but 'GenerateDocumentationFile' is false, which leads to IDE0005 not being reported | ||
| // on command line builds. See https://github.com/dotnet/roslyn/issues/41640 for more details. | ||
| internal const string EnableGenerateDocumentationFileId = "EnableGenerateDocumentationFile"; |
There was a problem hiding this comment.
I am in two minds whether this diagnostic should get an IDExxxx ID or a special ID like this one is just fine.
There was a problem hiding this comment.
I'm slightly worried about the messaging here. Users need to be able to quickly identify the following as the recommended approach for low-hassle enabling of this feature:
<PropertyGroup>
<!--
Make sure any documentation comments which are included in code get checked for syntax during the build, but do
not report warnings for missing comments.
CS1573: Parameter 'parameter' has no matching param tag in the XML comment for 'parameter' (but other parameters do)
CS1591: Missing XML comment for publicly visible type or member 'Type_or_Member'
CS1712: Type parameter 'type_parameter' has no matching typeparam tag in the XML comment on 'type_or_member' (but other type parameters do)
-->
<GenerateDocumentationFile>True</GenerateDocumentationFile>
<NoWarn>$(NoWarn),1573,1591,1712</NoWarn>
</PropertyGroup>
If we feel we should encourage the users to off CS1573, CS1591 and CS1712, then we can provide the entire XML snippet you mentioned above in the diagnostic's description, and they can see it on expanding the diagnostic. Are we sure we always want users to disable these 3 CS warnings? |
In my experience with SA0001, these three warnings are the primary reason users complain about turning this feature on. The warnings are not reported when documentation is disabled, so if the user accepted that state prior to IDE0005 we can assume they are still OK with not having these warnings after IDE0005 is enabled. |
|
@sharwell Can you take another look at the PR? |
|
@jmarolf Can you please review? |
...aces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/DiagnosticDescriptorExtensions.cs
Show resolved
Hide resolved
...ore/Analyzers/RemoveUnnecessaryImports/AbstractRemoveUnnecessaryImportsDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
…verity configuration key-values Existing DiagnosticDescriptorExtensions helper methods rely on these key-value pairs being part of the project analyzer config options, and they seem to have been broken by recent code refactoring that added StructuredAnalyzerConfigOptions. I am also adding a new GetEffectiveSeverity extension method, which is needed by dotnet#58835
…verity configuration key-values Existing DiagnosticDescriptorExtensions helper methods rely on these key-value pairs being part of the project analyzer config options, and they seem to have been broken by recent code refactoring that added StructuredAnalyzerConfigOptions. I am also adding a new GetEffectiveSeverity extension method, which is needed by dotnet#58835
…verity configuration key-values Existing DiagnosticDescriptorExtensions helper methods rely on these key-value pairs being part of the project analyzer config options, and they seem to have been broken by recent code refactoring that added StructuredAnalyzerConfigOptions. I am also adding a new GetEffectiveSeverity extension method, which is needed by dotnet#58835
…iveSeverity extension method to look for severity configuration options from SyntaxTreeOptionsProvider
...ore/Analyzers/RemoveUnnecessaryImports/AbstractRemoveUnnecessaryImportsDiagnosticAnalyzer.cs
Show resolved
Hide resolved
|
@sharwell Can you please review? |
...aces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/DiagnosticDescriptorExtensions.cs
Show resolved
Hide resolved
...aces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/DiagnosticDescriptorExtensions.cs
Show resolved
Hide resolved
src/EditorFeatures/Core/EditorConfigSettings/DataProvider/Analyzer/AnalyzerSettingsProvider.cs
Outdated
Show resolved
Hide resolved
|
@sharwell Any more feedback here? |
Merge queue setting changed
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
ghost
modified the milestones:
|
Think we need to adjust this change. As implemented this is a new warning on toolset upgrade which is a breaking change. It's impacting internal partners at the moment and stopping .net 8 adoption. Need to put this into a warning wave. |
…ngs) on build
Due to #41640, enabling IDE0005 on build requires users to enable generation of XML documentation comments. Even though this is documented with a note on IDE0005's doc page, we have had numerous reports of users not figuring this out and spending lot of cycles fighting with this, especially given other IDE diagnostics work just fine on build. We have had many user reports of the same: #58103, #53720, #57539, OpenRA/OpenRA#19747, https://developercommunity.visualstudio.com/t/visual-studio-reports-errors-that-dotnet/10237527 and numerous other offline queries.
This change enhances the IDE0005 analyzer to now detect the case when IDE0005 is being reported as a warning or an error in the IDE, but
GenerateDocumentationFileisfalsefor the project, which would mean IDE0005 wouldn't be reported on build. The analyzer reports a special helper diagnostic for this case, which recommends the user to set this property totruein their project file to enable IDE0005 on build. This should reduce the pain for customers who try to enforce IDE0005 on build and get hit by this issue.