-
Notifications
You must be signed in to change notification settings - Fork 14
Implement PH2163: Avoid NoWarn project settings for diagnostic suppression #999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Copilot
wants to merge
4
commits into
main
Choose a base branch
from
copilot/fix-998
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
69f7600
Initial plan
Copilot 4c72bd2
Implement PH2163: Avoid NoWarn project settings for analyzer suppression
Copilot 013c3be
Fix formatting issues in PH2163 analyzer - final implementation complete
Copilot 7453880
Address review feedback: enable by default, remove try-catch, simplif…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # PH2163: Avoid NoWarn for analyzer suppression | ||
|
|
||
| | Property | Value | | ||
| |--|--| | ||
| | Package | [Philips.CodeAnalysis.MaintainabilityAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MaintainabilityAnalyzers) | | ||
| | Diagnostic ID | PH2163 | | ||
| | Category | [Maintainability](../Maintainability.md) | | ||
| | Analyzer | [AvoidNoWarnAnalyzerSuppressionAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidNoWarnAnalyzerSuppressionAnalyzer.cs) | ||
| | CodeFix | No | | ||
| | Severity | Error | | ||
| | Enabled By Default | Yes | | ||
|
|
||
| ## Introduction | ||
|
|
||
| The NoWarn project setting should be avoided for suppressing diagnostic warnings. Instead, use .editorconfig files which provide better maintainability, team consistency, and granular control over diagnostic severity. | ||
|
|
||
| This analyzer detects when `<NoWarn>` elements are used in project files (.csproj, .vbproj) and suggests using .editorconfig configuration instead. | ||
|
|
||
| ## Example | ||
|
|
||
| ### Bad | ||
| ```xml | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <NoWarn>$(NoWarn);CS8002;PH2071</NoWarn> | ||
| </PropertyGroup> | ||
| </Project> | ||
| ``` | ||
|
|
||
| ### Good | ||
| ```ini | ||
| # .editorconfig | ||
| [*.cs] | ||
| dotnet_diagnostic.CS8002.severity = none | ||
| dotnet_diagnostic.PH2071.severity = none | ||
| ``` | ||
|
|
||
| ## Configuration | ||
|
|
||
| This analyzer does not offer any special configuration. The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply. | ||
| Solution-level analyzers are enabled by default. To configure, consider using a [.globalconfig](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-files#global-analyzerconfig) file. | ||
|
|
||
| ## Benefits of .editorconfig over NoWarn | ||
|
|
||
| 1. **Granular control**: Configure different severities (none, suggestion, warning, error) rather than just on/off | ||
| 2. **File-specific configuration**: Apply rules to specific file patterns | ||
| 3. **Team consistency**: Shared configuration that works across different IDEs and tools | ||
| 4. **Version control friendly**: Easily track and review changes to analyzer configuration | ||
| 5. **IDE integration**: Better support in Visual Studio and other editors | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
109 changes: 109 additions & 0 deletions
109
...alysis.MaintainabilityAnalyzers/Maintainability/AvoidNoWarnAnalyzerSuppressionAnalyzer.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| // © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Philips.CodeAnalysis.Common; | ||
|
|
||
| namespace Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability | ||
| { | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public class AvoidNoWarnAnalyzerSuppressionAnalyzer : SolutionAnalyzer | ||
| { | ||
| private const string Title = @"Avoid NoWarn for analyzer suppression"; | ||
| private const string MessageFormat = @"Use .editorconfig instead of NoWarn project setting to suppress diagnostics. Configure with 'dotnet_diagnostic.{id}.severity = none'."; | ||
| private const string Description = @"NoWarn project settings should be avoided for diagnostic suppression. Use .editorconfig files for better maintainability and team consistency."; | ||
|
|
||
| public AvoidNoWarnAnalyzerSuppressionAnalyzer() | ||
| : base(DiagnosticId.AvoidNoWarnAnalyzerSuppression, Title, MessageFormat, Description, Categories.Maintainability, isEnabled: true) | ||
| { | ||
| } | ||
|
|
||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
| context.EnableConcurrentExecution(); | ||
|
|
||
| context.RegisterCompilationAction(AnalyzeCompilation); | ||
| } | ||
|
|
||
| private void AnalyzeCompilation(CompilationAnalysisContext context) | ||
| { | ||
| var projectFilePath = TryFindProjectFileFromSourcePaths(context); | ||
| if (string.IsNullOrEmpty(projectFilePath) || !File.Exists(projectFilePath)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| AnalyzeProjectFile(context, projectFilePath); | ||
| } | ||
|
|
||
| private void AnalyzeProjectFile(CompilationAnalysisContext context, string projectFilePath) | ||
| { | ||
| var content = File.ReadAllText(projectFilePath); | ||
|
|
||
| // Simple check for NoWarn elements - avoid XDocument overhead | ||
| var lowerContent = content.ToLowerInvariant(); | ||
| if (lowerContent.Contains("<nowarn>") || lowerContent.Contains("<nowarn ")) | ||
| { | ||
| var diagnostic = Diagnostic.Create(Rule, Location.None); | ||
| context.ReportDiagnostic(diagnostic); | ||
| } | ||
| } | ||
|
|
||
| private string TryFindProjectFileFromSourcePaths(CompilationAnalysisContext context) | ||
| { | ||
| List<string> sourceDirectories = GetSourceDirectories(context); | ||
|
|
||
| foreach (var sourceDir in sourceDirectories) | ||
| { | ||
| var foundPath = SearchForProjectFile(sourceDir); | ||
| if (foundPath != null) | ||
| { | ||
| return foundPath; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private static List<string> GetSourceDirectories(CompilationAnalysisContext context) | ||
| { | ||
| return context.Compilation.SyntaxTrees | ||
| .Where(tree => !string.IsNullOrEmpty(tree.FilePath)) | ||
| .Select(tree => Path.GetDirectoryName(tree.FilePath)) | ||
| .Where(dir => !string.IsNullOrEmpty(dir)) | ||
| .Distinct(StringComparer.OrdinalIgnoreCase) | ||
| .ToList(); | ||
| } | ||
|
|
||
| private static string SearchForProjectFile(string startDirectory) | ||
| { | ||
| if (string.IsNullOrEmpty(startDirectory) || !Directory.Exists(startDirectory)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var currentDir = new DirectoryInfo(startDirectory); | ||
|
|
||
| // Search up the directory tree for project files | ||
| while (currentDir != null) | ||
| { | ||
| IEnumerable<FileInfo> projectFiles = currentDir.GetFiles("*.csproj").Concat(currentDir.GetFiles("*.vbproj")); | ||
| FileInfo projectFile = projectFiles.FirstOrDefault(); | ||
|
|
||
| if (projectFile != null) | ||
| { | ||
| return projectFile.FullName; | ||
| } | ||
|
|
||
| currentDir = currentDir.Parent; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
| } |
60 changes: 60 additions & 0 deletions
60
...alysis.Test/Maintainability/Maintainability/AvoidNoWarnAnalyzerSuppressionAnalyzerTest.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. | ||
|
|
||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| using Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability; | ||
| using Philips.CodeAnalysis.Test.Helpers; | ||
| using Philips.CodeAnalysis.Test.Verifiers; | ||
|
|
||
| namespace Philips.CodeAnalysis.Test.Maintainability.Maintainability | ||
| { | ||
| [TestClass] | ||
| public class AvoidNoWarnAnalyzerSuppressionAnalyzerTest : DiagnosticVerifier | ||
| { | ||
| protected override DiagnosticAnalyzer GetDiagnosticAnalyzer() | ||
| { | ||
| return new AvoidNoWarnAnalyzerSuppressionAnalyzer(); | ||
| } | ||
|
|
||
| private string GetTestCode() | ||
| { | ||
| return @" | ||
| class Foo | ||
| { | ||
| public void DoSomething() | ||
| { | ||
| var x = 1; | ||
| } | ||
| } | ||
| "; | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [TestCategory(TestDefinitions.UnitTests)] | ||
| public async Task AvoidNoWarnAnalyzerSuppressionAnalyzerSuccessfulCompilationNoErrorAsync() | ||
| { | ||
| await VerifySuccessfulCompilation(GetTestCode()).ConfigureAwait(false); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [TestCategory(TestDefinitions.UnitTests)] | ||
| public void AvoidNoWarnAnalyzerSuppressionAnalyzerHasCorrectDiagnosticId() | ||
| { | ||
| var analyzer = new AvoidNoWarnAnalyzerSuppressionAnalyzer(); | ||
| System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.DiagnosticDescriptor> descriptors = analyzer.SupportedDiagnostics; | ||
| Assert.HasCount(1, descriptors); | ||
| Assert.IsTrue(descriptors.Any(d => d.Id == "PH2163")); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [TestCategory(TestDefinitions.UnitTests)] | ||
| public void ExtractAnalyzerCodesTest() | ||
| { | ||
| // Test the static method indirectly by ensuring analyzer doesn't crash | ||
| var analyzer = new AvoidNoWarnAnalyzerSuppressionAnalyzer(); | ||
| Assert.IsNotNull(analyzer); | ||
| } | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's enable by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabled by default in commit 7453880. Updated both the analyzer constructor (
isEnabled: true) and documentation.