diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 9e40ff9f3..19624a777 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -126,8 +126,9 @@ When creating new analyzers: 2. **Set `isEnabledByDefault: false`** initially for testing 3. **Add to appropriate project** (Maintainability, Security, etc.) 4. **Create comprehensive tests** in Philips.CodeAnalysis.Test -5. **Update documentation** in Documentation/ folder -6. **Use current year in copyright** headers: `// © 2025 Koninklijke Philips N.V.` +5. **Ensure 80% code coverage** minimum for all modified code +6. **Update documentation** in Documentation/ folder +7. **Use current year in copyright** headers: `// © 2025 Koninklijke Philips N.V.` ### Performance Considerations Analyzers run during compilation and must be performant: @@ -139,6 +140,7 @@ Analyzers run during compilation and must be performant: ### Pull Request Requirements - **Title format**: Must follow Conventional Commits (feat:, fix:, docs:, etc.) - **All tests pass**: 1903 tests must pass +- **Code coverage**: Modified code must have at least 80% coverage - **Formatting perfect**: Zero formatting violations - **No suppressions**: Fix underlying issues, don't suppress warnings - **Documentation**: Update relevant docs in Documentation/ folder diff --git a/Documentation/Diagnostics/PH2145.md b/Documentation/Diagnostics/PH2145.md index 8b37cdf75..b25701926 100644 --- a/Documentation/Diagnostics/PH2145.md +++ b/Documentation/Diagnostics/PH2145.md @@ -6,7 +6,7 @@ | Diagnostic ID | PH2145 | | Category | [Maintainability](../Maintainability.md) | | Analyzer | [AvoidStringFormatInInterpolatedStringAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringAnalyzer.cs) -| CodeFix | No | +| CodeFix | Yes | | Severity | Info | | Enabled By Default | No | diff --git a/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs b/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs new file mode 100644 index 000000000..695000132 --- /dev/null +++ b/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProvider.cs @@ -0,0 +1,241 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Composition; +using System.Globalization; +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 + { + protected override string Title => "Remove redundant string.Format call"; + + protected override DiagnosticId DiagnosticId => + DiagnosticId.AvoidStringFormatInInterpolatedString; + + protected override async Task ApplyFix(Document document, InterpolationSyntax node, + ImmutableDictionary properties, CancellationToken cancellationToken) + { + SyntaxNode root = await document.GetSyntaxRootAsync(cancellationToken); + + // Validate and extract string.Format information + if (!TryExtractStringFormatInfo(node, out InvocationExpressionSyntax invocation, out var formatString, out ArgumentSyntax[] arguments)) + { + return document; + } + + // Verify this is actually a string.Format call + if (!await IsStringFormatInvocation(document, invocation, cancellationToken)) + { + return document; + } + + // Find the parent interpolated string and apply the transformation + return await ApplyInterpolatedStringTransformation(document, root, node, formatString, arguments); + } + + private bool TryExtractStringFormatInfo(InterpolationSyntax node, out InvocationExpressionSyntax invocation, out string formatString, out ArgumentSyntax[] arguments) + { + invocation = null; + formatString = null; + arguments = null; + + // Get the string.Format invocation from the interpolation + if (node.Expression is not InvocationExpressionSyntax inv) + { + return false; + } + + invocation = inv; + + // Quick syntactic check for "Format" method name before expensive semantic analysis + if (!IsFormatMethodSyntactically(invocation)) + { + return false; + } + + // Extract format string and arguments + if (invocation.ArgumentList.Arguments.Count < 1) + { + return false; + } + + ArgumentSyntax formatArgument = invocation.ArgumentList.Arguments[0]; + if (formatArgument.Expression is not LiteralExpressionSyntax formatLiteral || + !formatLiteral.Token.IsKind(SyntaxKind.StringLiteralToken)) + { + return false; + } + + formatString = formatLiteral.Token.ValueText; + arguments = invocation.ArgumentList.Arguments.Skip(1).ToArray(); + return true; + } + + private static bool IsFormatMethodSyntactically(InvocationExpressionSyntax invocation) + { + return invocation.Expression switch + { + MemberAccessExpressionSyntax memberAccess => memberAccess.Name.Identifier.ValueText == "Format", + IdentifierNameSyntax identifier => identifier.Identifier.ValueText == "Format", + _ => false + }; + } + + private async Task IsStringFormatInvocation(Document document, InvocationExpressionSyntax invocation, CancellationToken cancellationToken) + { + SemanticModel semanticModel = await document.GetSemanticModelAsync(cancellationToken); + SymbolInfo symbolInfo = semanticModel.GetSymbolInfo(invocation, cancellationToken); + return symbolInfo.Symbol is IMethodSymbol methodSymbol && + methodSymbol.ContainingType?.SpecialType == SpecialType.System_String; + } + + private Task ApplyInterpolatedStringTransformation(Document document, SyntaxNode root, InterpolationSyntax node, string formatString, ArgumentSyntax[] arguments) + { + // Find the parent interpolated string + InterpolatedStringExpressionSyntax interpolatedString = node.FirstAncestorOrSelf(); + if (interpolatedString == null) + { + return Task.FromResult(document); + } + + // Convert the string.Format to interpolated content + List newContents = ConvertStringFormatToInterpolatedContents(formatString, arguments); + + // Get current contents and find the position of the node to replace + var currentContents = interpolatedString.Contents.ToList(); + var index = currentContents.IndexOf(node); + if (index == -1) + { + return Task.FromResult(document); + } + + // Remove the current interpolation and insert the new contents + currentContents.RemoveAt(index); + if (newContents.Count > 0) + { + currentContents.InsertRange(index, newContents); + } + // If newContents is empty (empty format string case), we just remove the interpolation + + // 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 Task.FromResult(document.WithSyntaxRoot(root)); + } + + private List ConvertStringFormatToInterpolatedContents(string formatString, ArgumentSyntax[] arguments) + { + var result = new List(); + var placeholderPattern = new Regex(@"\{(\d+)(?::([^}]*))?\}", + RegexOptions.None, TimeSpan.FromSeconds(1)); + var lastIndex = 0; + + MatchCollection matches = placeholderPattern.Matches(formatString); + + foreach (Match match in matches) + { + // Add text before the placeholder + AddTextContent(result, formatString, lastIndex, match.Index); + + // Process the placeholder + if (!TryProcessPlaceholder(result, match, arguments)) + { + // Invalid placeholder - return empty collection for graceful handling + return []; + } + + lastIndex = match.Index + match.Length; + } + + // Add remaining text and handle special cases + HandleRemainingContent(result, formatString, lastIndex, matches.Count); + + return result; + } + + private static void AddTextContent(List result, string formatString, int startIndex, int endIndex) + { + if (endIndex > startIndex) + { + var textContent = formatString.Substring(startIndex, endIndex - startIndex); + if (!string.IsNullOrEmpty(textContent)) + { + result.Add(CreateInterpolatedStringText(textContent)); + } + } + } + + private static bool TryProcessPlaceholder(List result, Match match, ArgumentSyntax[] arguments) + { + if (!int.TryParse(match.Groups[1].Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var argIndex) || + argIndex >= arguments.Length) + { + return false; + } + + ExpressionSyntax expression = arguments[argIndex].Expression; + InterpolationSyntax interpolation = CreateInterpolationWithFormat(expression, match.Groups[2]); + result.Add(interpolation); + return true; + } + + private static InterpolationSyntax CreateInterpolationWithFormat(ExpressionSyntax expression, Group formatGroup) + { + if (formatGroup.Success && !string.IsNullOrEmpty(formatGroup.Value)) + { + InterpolationFormatClauseSyntax formatClause = SyntaxFactory.InterpolationFormatClause( + SyntaxFactory.Token(SyntaxKind.ColonToken), + SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.InterpolatedStringTextToken, + formatGroup.Value, formatGroup.Value, SyntaxTriviaList.Empty)); + return SyntaxFactory.Interpolation(expression, null, formatClause); + } + + return SyntaxFactory.Interpolation(expression); + } + + private static void HandleRemainingContent(List result, string formatString, int lastIndex, int matchCount) + { + // Add remaining text after all placeholders + if (lastIndex < formatString.Length) + { + var remainingText = formatString.Substring(lastIndex); + if (!string.IsNullOrEmpty(remainingText)) + { + result.Add(CreateInterpolatedStringText(remainingText)); + } + } + + // Handle special case: text-only format string with no placeholders + if (result.Count == 0 && matchCount == 0 && !string.IsNullOrEmpty(formatString)) + { + result.Add(CreateInterpolatedStringText(formatString)); + } + } + + private static InterpolatedStringTextSyntax CreateInterpolatedStringText(string text) + { + SyntaxToken textToken = SyntaxFactory.Token(SyntaxTriviaList.Empty, + SyntaxKind.InterpolatedStringTextToken, + text, text, SyntaxTriviaList.Empty); + return SyntaxFactory.InterpolatedStringText(textToken); + } + } +} \ No newline at end of file diff --git a/Philips.CodeAnalysis.Test/Maintainability/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProviderTest.cs b/Philips.CodeAnalysis.Test/Maintainability/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProviderTest.cs new file mode 100644 index 000000000..62f523586 --- /dev/null +++ b/Philips.CodeAnalysis.Test/Maintainability/Maintainability/AvoidStringFormatInInterpolatedStringCodeFixProviderTest.cs @@ -0,0 +1,341 @@ +// © 2025 Koninklijke Philips N.V. See License.md in the project root for license information. + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Philips.CodeAnalysis.Common; +using Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability; +using Philips.CodeAnalysis.Test.Helpers; +using Philips.CodeAnalysis.Test.Verifiers; + +namespace Philips.CodeAnalysis.Test.Maintainability.Maintainability +{ + [TestClass] + public class AvoidStringFormatInInterpolatedStringCodeFixProviderTest : CodeFixVerifier + { + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixSimpleStringFormatTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var firstName = ""John""; + var lastName = ""Doe""; + var result = $""Hello {string.Format(""{0} {1}"", firstName, lastName)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var firstName = ""John""; + var lastName = ""Doe""; + var result = $""Hello {firstName} {lastName}""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixStringFormatWithFormatSpecifierTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var value = 42; + var result = $""Value: {string.Format(""{0:D2}"", value)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var value = 42; + var result = $""Value: {value:D2}""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixMultipleArgumentsWithTextTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var name = ""John""; + var age = 25; + var result = $""User: {string.Format(""Name is {0}, Age is {1:D2}"", name, age)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var name = ""John""; + var age = 25; + var result = $""User: Name is {name}, Age is {age:D2}""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixEmptyFormatStringTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var result = $""Value: {string.Format("""")}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var result = $""Value: ""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixNoPlaceholdersTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var value = 42; + var result = $""Value: {string.Format(""static text"", value)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var value = 42; + var result = $""Value: static text""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText, shouldAllowNewCompilerDiagnostics: true).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixWithMixedTextAndPlaceholdersTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var name = ""John""; + var age = 25; + var status = ""active""; + var result = $""Info: {string.Format(""User {0} is {1:D2} years old and {2}"", name, age, status)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var name = ""John""; + var age = 25; + var status = ""active""; + var result = $""Info: User {name} is {age:D2} years old and {status}""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixWithComplexFormatSpecifiersTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var price = 123.45m; + var count = 7; + var result = $""Order: {string.Format(""{0:C} x {1:D3}"", price, count)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var price = 123.45m; + var count = 7; + var result = $""Order: {price:C} x {count:D3}""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixWithOnlyTextTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var value = 42; + var result = $""Value: {string.Format(""constant text"", value)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var value = 42; + var result = $""Value: constant text""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText, shouldAllowNewCompilerDiagnostics: true).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task CodeFixWithTrailingTextTest() + { + var givenText = @" +using System; + +class Test +{ + public void Method() + { + var name = ""John""; + var result = $""Hello {string.Format(""{0} Doe"", name)}""; + } +} +"; + + var expectedText = @" +using System; + +class Test +{ + public void Method() + { + var name = ""John""; + var result = $""Hello {name} Doe""; + } +} +"; + + await VerifyDiagnostic(givenText, DiagnosticId.AvoidStringFormatInInterpolatedString).ConfigureAwait(false); + await VerifyFix(givenText, expectedText).ConfigureAwait(false); + } + + protected override CodeFixProvider GetCodeFixProvider() + { + return new AvoidStringFormatInInterpolatedStringCodeFixProvider(); + } + + protected override DiagnosticAnalyzer GetDiagnosticAnalyzer() + { + return new AvoidStringFormatInInterpolatedStringAnalyzer(); + } + } +}