diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs index 23a6db9b907..31ca5a5b1bb 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs @@ -27,6 +27,12 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class MessageTemplateAnalyzer : SonarDiagnosticAnalyzer { + private static readonly ImmutableHashSet ValidTemplateKinds = ImmutableHashSet.Create( + SyntaxKind.StringLiteralExpression, + SyntaxKind.AddExpression, + SyntaxKind.InterpolatedStringExpression, + SyntaxKind.InterpolatedStringText); + private static readonly ImmutableHashSet Checks = ImmutableHashSet.Create( new NamedPlaceholdersShouldBeUnique(), new UsePascalCaseForNamedPlaceHolders()); @@ -41,7 +47,7 @@ protected override void Initialize(SonarAnalysisContext context) => var enabledChecks = Checks.Where(x => x.Rule.IsEnabled(c)).ToArray(); if (enabledChecks.Length > 0 && TemplateArgument(invocation, c.SemanticModel) is { } argument - && argument.Expression.IsKind(SyntaxKind.StringLiteralExpression) + && HasValidExpression(argument) && Helpers.MessageTemplates.Parse(argument.Expression.ToString()) is { Success: true } result) { foreach (var check in enabledChecks) @@ -74,4 +80,7 @@ private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invoca && argumentsFound.Length == 1 ? (ArgumentSyntax)argumentsFound[0].Parent : null; + + private static bool HasValidExpression(ArgumentSyntax argument) => + argument.Expression.DescendantNodes().All(x => x.IsAnyKind(ValidTemplateKinds)); } diff --git a/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs b/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs index 140e7a1e27a..7c15ec8bff5 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs @@ -58,11 +58,12 @@ public void Parse_NoPlaceholder(string template) [DataRow("{$199}", "199", 2, 3)] [DataRow("{@0}", "0", 2, 1)] [DataRow("{$world_42}", "world_42", 2, 8)] - public void Parse_Placeholder(string template, string placeholder, int start, int end) + [DataRow(""" "hello" + "{world}" + "!" """, "world", 13, 5)] + public void Parse_Placeholder(string template, string placeholder, int start, int length) { var result = MessageTemplates.Parse(template); ShouldBeSuccess(result, 1); - ShouldBe(result.Placeholders[0], placeholder, start, end); + ShouldBe(result.Placeholders[0], placeholder, start, length); } [DataTestMethod] @@ -115,16 +116,17 @@ public void Parse_Placeholder_Multiple() } [DataTestMethod] - [DataRow("{")] // Left bracket is not allowed - [DataRow("{{{")] // Third left bracket is not allowed (first two are valid) - [DataRow("{}")] // Empty placeholder is not allowed - [DataRow("{{{}}}")] // Empty placeholder is not allowed - [DataRow("Login failed for {User")] // Missing closing bracket - [DataRow("Login failed for {&User}")] // Only '@' and '$' are allowed as prefix - [DataRow("Login failed for {User_%Name}")] // Only alphanumerics and '_' are allowed for placeholders - [DataRow("Retry attempt {Cnt,r}")] // The alignment specifier must be numeric - [DataRow("Retry attempt {Cnt,}")] // Empty alignment specifier is not allowed - [DataRow("Retry attempt {Cnt:}")] // Empty format specifier is not allowed + [DataRow("{")] // Left bracket is not allowed + [DataRow("{{{")] // Third left bracket is not allowed (first two are valid) + [DataRow("{}")] // Empty placeholder is not allowed + [DataRow("{{{}}}")] // Empty placeholder is not allowed + [DataRow("Login failed for {User")] // Missing closing bracket + [DataRow("Login failed for {&User}")] // Only '@' and '$' are allowed as prefix + [DataRow("Login failed for {User_%Name}")] // Only alphanumerics and '_' are allowed for placeholders + [DataRow("Retry attempt {Cnt,r}")] // The alignment specifier must be numeric + [DataRow("Retry attempt {Cnt,}")] // Empty alignment specifier is not allowed + [DataRow("Retry attempt {Cnt:}")] // Empty format specifier is not allowed + [DataRow(""" "hello {" + "world" + "}" """)] // '+' and '"' is not allowed in placeholders public void Parse_Placeholder_Failure(string template) { var result = MessageTemplates.Parse(template); diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/NamedPlaceholdersShouldBeUnique.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/NamedPlaceholdersShouldBeUnique.cs index d77ea1135b9..b4438552716 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/NamedPlaceholdersShouldBeUnique.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/NamedPlaceholdersShouldBeUnique.cs @@ -45,10 +45,17 @@ void Compliant(ILogger logger, string foo, string bar) logger.LogInformation("Hey {{foo}} and {{foo}}", foo, foo); // Compliant logger.LogInformation("Hey {foo} and {{foo}}", foo, foo); // Compliant - // Not string literal + // Contains Interpolation logger.LogWarning($"Hey {foo} and {foo}", foo, foo); // Compliant + logger.LogWarning($"Hey {foo}" + "and {foo}", foo, foo); // Compliant + logger.LogInformation("Hey {foo}" + $"and {foo}" + "{foo}", foo, foo); // Compliant FN - logger.LogInformation($"Hey {{foo}} and {{foo}}", foo, foo); // FN, we do not parse interpolated strings + // Contains Identifier name + logger.LogWarning("Hey {" + foo + "} and {" + foo + "}"); // Compliant + + // False Negatives + logger.LogInformation($"Hey {{foo}} and {{foo}}", foo, foo); // Compliant, we get the template value at syntax level, so we don't escape {{ + logger.LogInformation("Hey" + "{" + "foo} and {foo" +"}"); // Compliant, same as above (I hope this does not exist out there) } void Noncompliant(ILogger logger, string foo, string bar, string baz, Exception ex, EventId eventId) @@ -139,6 +146,26 @@ and again {foo} // ^^^ @-3 {{Message template placeholder 'foo' is not unique.}} // ^^^ Secondary @-2 foo, bar, foo); + + + // SyntaxKinds + logger.LogInformation("Hey {foo}" + "and {foo}", foo, foo); + // ^^^ {{Message template placeholder 'foo' is not unique.}} + // ^^^ Secondary @-1 + logger.LogInformation("Hey {foo}" + $"and of course" + "{foo}", foo, foo); + // ^^^ {{Message template placeholder 'foo' is not unique.}} + // ^^^ Secondary @-1 + logger.LogInformation("Hey {foo}" + $"{{foo}}" + "{foo}", foo, foo); // We miss the second placeholder + // ^^^ {{Message template placeholder 'foo' is not unique.}} + // ^^^ Secondary @-1 + + logger.LogInformation( + "Hey {foo}" + // ^^^ {{Message template placeholder 'foo' is not unique.}} + + + "and {foo}", foo, foo); + // ^^^ Secondary + } // Fake