-
Notifications
You must be signed in to change notification settings - Fork 14
feat: Implement CodeFixProvider for PH2145 with comprehensive test coverage for 80%+ coverage #879
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
00af2aa
9d34177
7a4b1e7
870299a
5fcc4ee
a491352
1b99fd3
b87c552
847c6dc
a78bf5b
b901914
515fe99
b8bb09d
8a8bfa5
7116e4e
a56d231
01d5ef3
99a935c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,4 +58,4 @@ private bool ContainsStringFormatCall(ExpressionSyntax expression) | |
| return false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,316 @@ | ||
| // © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Composition; | ||
| using System.Linq; | ||
| using System.Text.RegularExpressions; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
| using Microsoft.CodeAnalysis.CSharp; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using Philips.CodeAnalysis.Common; | ||
|
|
||
| namespace Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability | ||
| { | ||
| [ExportCodeFixProvider(LanguageNames.CSharp, | ||
| Name = nameof(AvoidStringFormatInInterpolatedStringCodeFixProvider)), Shared] | ||
| public class AvoidStringFormatInInterpolatedStringCodeFixProvider : | ||
| SingleDiagnosticCodeFixProvider<InterpolationSyntax> | ||
| { | ||
| protected override string Title => "Remove redundant string.Format call"; | ||
|
|
||
| protected override DiagnosticId DiagnosticId => | ||
| DiagnosticId.AvoidStringFormatInInterpolatedString; | ||
|
|
||
| protected override async Task<Document> ApplyFix(Document document, InterpolationSyntax node, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor this method to reduce its cognitive complexity. Can it be reduced by introducing a helper method or 2?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored the ApplyFix method to reduce cognitive complexity by extracting three helper methods:
This makes the code more maintainable and easier to understand. Commit: 515fe99 |
||
| ImmutableDictionary<string, string> properties, CancellationToken cancellationToken) | ||
| { | ||
| SyntaxNode root = await document.GetSyntaxRootAsync(cancellationToken); | ||
|
|
||
| // Get the string.Format invocation from the interpolation | ||
| if (node.Expression is not InvocationExpressionSyntax invocation) | ||
| { | ||
| return document; | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
|
Check failure on line 37 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
||
| Before: | ||
|
Check failure on line 38 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| } | ||
|
Check warning on line 39 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
|
||
| // Verify this is actually a string.Format call | ||
| After: | ||
| } | ||
|
|
||
| // Verify this is actually a string.Format call | ||
| */ | ||
|
|
||
| } | ||
|
|
||
| // Verify this is actually a string.Format call | ||
| SemanticModel semanticModel = await document.GetSemanticModelAsync(cancellationToken); | ||
| SymbolInfo symbolInfo = semanticModel.GetSymbolInfo(invocation, cancellationToken); | ||
| if (symbolInfo.Symbol is not IMethodSymbol methodSymbol || | ||
| methodSymbol.Name != "Format" || | ||
| methodSymbol.ContainingType?.SpecialType != SpecialType.System_String) | ||
| { | ||
| return document; | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
|
Check failure on line 58 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
||
| Before: | ||
|
Check failure on line 59 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| } | ||
|
Check warning on line 60 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
|
||
| // Extract format string and arguments | ||
| After: | ||
| } | ||
|
|
||
| // Extract format string and arguments | ||
| */ | ||
|
|
||
| } | ||
|
|
||
| // Extract format string and arguments | ||
| if (invocation.ArgumentList.Arguments.Count < 1) | ||
| { | ||
| return document; | ||
| } | ||
|
|
||
| ArgumentSyntax formatArgument = invocation.ArgumentList.Arguments[0]; | ||
| if (formatArgument.Expression is not LiteralExpressionSyntax formatLiteral || | ||
| !formatLiteral.Token.IsKind(SyntaxKind.StringLiteralToken)) | ||
| { | ||
| return document; | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
|
Check failure on line 82 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
||
| Before: | ||
|
Check failure on line 83 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| string formatString = formatLiteral.Token.ValueText; | ||
|
Check warning on line 84 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| After: | ||
| var formatString = formatLiteral.Token.ValueText; | ||
| */ | ||
|
|
||
| } | ||
|
|
||
| var formatString = formatLiteral.Token.ValueText; | ||
| ArgumentSyntax[] arguments = invocation.ArgumentList.Arguments.Skip(1).ToArray(); | ||
|
|
||
| // Find the parent interpolated string | ||
| InterpolatedStringExpressionSyntax interpolatedString = node.FirstAncestorOrSelf<InterpolatedStringExpressionSyntax>(); | ||
| if (interpolatedString == null) | ||
| { | ||
| return document; | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
|
Check failure on line 99 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
||
| Before: | ||
|
Check failure on line 100 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| } | ||
|
Check warning on line 101 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
|
||
| // Convert the string.Format to interpolated content | ||
| After: | ||
| } | ||
|
|
||
| // Convert the string.Format to interpolated content | ||
| */ | ||
|
|
||
| } | ||
|
|
||
| // Convert the string.Format to interpolated content | ||
| List<InterpolatedStringContentSyntax> newContents = ConvertStringFormatToInterpolatedContents(formatString, arguments); | ||
| if (newContents == null) | ||
| { | ||
| return document; | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
|
Check failure on line 117 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
||
| Before: | ||
|
Check failure on line 118 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| } | ||
|
Check warning on line 119 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
|
||
| // Replace the interpolation node with the new contents in the parent interpolated string | ||
| After: | ||
| } | ||
|
|
||
| // Replace the interpolation node with the new contents in the parent interpolated string | ||
| */ | ||
|
|
||
| } | ||
|
|
||
| // Replace the interpolation node with the new contents in the parent interpolated string | ||
| var currentContents = interpolatedString.Contents.ToList(); | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| int index = currentContents.IndexOf(node); | ||
|
Check warning on line 134 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| After: | ||
| var index = currentContents.IndexOf(node); | ||
| */ | ||
|
|
||
| var index = currentContents.IndexOf(node); | ||
| if (index == -1) | ||
| { | ||
| return document; | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| } | ||
|
Check warning on line 145 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
|
|
||
| // Remove the current interpolation and insert the new contents | ||
| After: | ||
| } | ||
|
|
||
| // Remove the current interpolation and insert the new contents | ||
| */ | ||
|
|
||
| } | ||
|
|
||
| // Remove the current interpolation and insert the new contents | ||
| currentContents.RemoveAt(index); | ||
| currentContents.InsertRange(index, newContents); | ||
|
|
||
| // Create new interpolated string with the updated contents | ||
| InterpolatedStringExpressionSyntax newInterpolatedString = interpolatedString.WithContents(SyntaxFactory.List(currentContents)); | ||
|
|
||
| // Replace in the syntax tree | ||
| root = root.ReplaceNode(interpolatedString, newInterpolatedString); | ||
|
|
||
| return document.WithSyntaxRoot(root); | ||
| } | ||
|
|
||
| private List<InterpolatedStringContentSyntax> ConvertStringFormatToInterpolatedContents(string formatString, ArgumentSyntax[] arguments) | ||
| { | ||
| var result = new List<InterpolatedStringContentSyntax>(); | ||
| var placeholderPattern = new Regex(@"\{(\d+)(?::([^}]*))?\}"); | ||
|
||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| int lastIndex = 0; | ||
|
Check warning on line 175 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| After: | ||
| var lastIndex = 0; | ||
| */ | ||
|
|
||
| var lastIndex = 0; | ||
|
|
||
| foreach (Match match in placeholderPattern.Matches(formatString)) | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| { | ||
|
Check warning on line 185 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| // Add text before the placeholder | ||
| After: | ||
| { | ||
| // Add text before the placeholder | ||
| */ | ||
|
|
||
| { | ||
| // Add text before the placeholder | ||
| if (match.Index > lastIndex) | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| string textContent = formatString.Substring(lastIndex, match.Index - lastIndex); | ||
|
Check warning on line 197 in Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs
|
||
| After: | ||
| var textContent = formatString.Substring(lastIndex, match.Index - lastIndex); | ||
| */ | ||
|
|
||
| { | ||
| var textContent = formatString.Substring(lastIndex, match.Index - lastIndex); | ||
| if (!string.IsNullOrEmpty(textContent)) | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| var textToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, | ||
| After: | ||
| SyntaxToken textToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, | ||
| */ | ||
|
|
||
| { | ||
| SyntaxToken textToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, | ||
| textContent, textContent, SyntaxTriviaList.Empty); | ||
| result.Add(SyntaxFactory.InterpolatedStringText(textToken)); | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| } | ||
|
|
||
| // Parse the placeholder | ||
| After: | ||
| } | ||
|
|
||
| // Parse the placeholder | ||
| */ | ||
|
|
||
| } | ||
| } | ||
|
|
||
| // Parse the placeholder | ||
| if (int.TryParse(match.Groups[1].Value, out var argIndex) && argIndex < arguments.Length) | ||
| { | ||
| ExpressionSyntax expression = arguments[argIndex].Expression; | ||
| InterpolationSyntax interpolation; | ||
|
|
||
| // Add format specifier if present | ||
| if (match.Groups[2].Success && !string.IsNullOrEmpty(match.Groups[2].Value)) | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| var formatClause = SyntaxFactory.InterpolationFormatClause( | ||
| After: | ||
| InterpolationFormatClauseSyntax formatClause = SyntaxFactory.InterpolationFormatClause( | ||
| */ | ||
|
|
||
| { | ||
| InterpolationFormatClauseSyntax formatClause = SyntaxFactory.InterpolationFormatClause( | ||
| SyntaxFactory.Token(SyntaxKind.ColonToken), | ||
| SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, | ||
| match.Groups[2].Value, match.Groups[2].Value, SyntaxTriviaList.Empty)); | ||
| interpolation = SyntaxFactory.Interpolation(expression, null, formatClause); | ||
| } | ||
| else | ||
| { | ||
| interpolation = SyntaxFactory.Interpolation(expression); | ||
| } | ||
|
|
||
| result.Add(interpolation); | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| { | ||
| // Invalid placeholder - shouldn't happen with valid string.Format, but handle gracefully | ||
| After: | ||
| { | ||
| // Invalid placeholder - shouldn't happen with valid string.Format, but handle gracefully | ||
| */ | ||
|
|
||
| } | ||
| else | ||
| { | ||
| // Invalid placeholder - shouldn't happen with valid string.Format, but handle gracefully | ||
| return null; | ||
|
||
| } | ||
|
|
||
| lastIndex = match.Index + match.Length; | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| } | ||
|
|
||
| // Add remaining text | ||
| After: | ||
| } | ||
|
|
||
| // Add remaining text | ||
| */ | ||
|
|
||
| } | ||
|
|
||
| // Add remaining text | ||
| if (lastIndex < formatString.Length) | ||
| /* Unmerged change from project 'Philips.CodeAnalysis.MaintainabilityAnalyzers(netstandard2.0)' | ||
| Before: | ||
| string remainingText = formatString.Substring(lastIndex); | ||
| if (!string.IsNullOrEmpty(remainingText)) | ||
| { | ||
| var textToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, | ||
| After: | ||
| var remainingText = formatString.Substring(lastIndex); | ||
| if (!string.IsNullOrEmpty(remainingText)) | ||
| { | ||
| SyntaxToken textToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, | ||
| */ | ||
|
|
||
| { | ||
| var remainingText = formatString.Substring(lastIndex); | ||
| if (!string.IsNullOrEmpty(remainingText)) | ||
| { | ||
| SyntaxToken textToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, | ||
| remainingText, remainingText, SyntaxTriviaList.Empty); | ||
| result.Add(SyntaxFactory.InterpolatedStringText(textToken)); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,4 +56,4 @@ public override void Analyze() | |
| } | ||
| } | ||
| } | ||
| } | ||
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,4 +39,4 @@ protected override async Task<Document> ApplyFix(Document document, InvocationEx | |
| return document.WithSyntaxRoot(rootNode); | ||
| } | ||
| } | ||
| } | ||
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,4 +111,4 @@ private void AnalyzeArgument(SyntaxNodeAnalysisContext context) | |
| } | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove edits to this file
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted unwanted changes to AvoidVariableNamedUnderscoreAnalyzer.cs. Commit: 5fcc4ee |
||
| } | ||
| } | ||
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.
remove edits to this file
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.
Reverted unwanted changes to AvoidStringFormatInInterpolatedStringAnalyzer.cs. Commit: 5fcc4ee
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.
Remove the commit of this file. Only whitespace changed.
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.
Reverted the whitespace-only changes to AvoidStringFormatInInterpolatedStringAnalyzer.cs using
git checkout 870299a~1. The file now matches its state before the unwanted whitespace modifications. Commit: 515fe99There 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.
Hmm... it would not show up as a changed file if you reverted successfully.
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.
Confirmed - the files mentioned in the review comments are not present in the current PR changes. Only the CodeFixProvider and updated documentation remain, which is the correct scope for this feature.