From 1274bc0c42bd639a3dfb32d35ef0095d7d0a61a0 Mon Sep 17 00:00:00 2001 From: Gregory Paidis Date: Mon, 26 Feb 2024 17:52:04 +0100 Subject: [PATCH 1/4] New rule S6674: Log message template should be syntactically correct --- analyzers/rspec/cs/S6674.html | 46 +++++ analyzers/rspec/cs/S6674.json | 15 ++ analyzers/rspec/cs/Sonar_way_profile.json | 1 + .../MessageTemplateAnalyzer.cs | 2 +- .../Rules/MessageTemplatesShouldBeCorrect.cs | 160 ++++++++++++++++++ .../Helpers/MessageTemplates.cs | 7 +- .../PackagingTests/RuleTypeMappingCS.cs | 2 +- .../MessageTemplatesShouldBeCorrectTest.cs | 139 +++++++++++++++ .../MessageTemplatesShouldBeCorrect.cs | 115 +++++++++++++ 9 files changed, 483 insertions(+), 4 deletions(-) create mode 100644 analyzers/rspec/cs/S6674.html create mode 100644 analyzers/rspec/cs/S6674.json create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs diff --git a/analyzers/rspec/cs/S6674.html b/analyzers/rspec/cs/S6674.html new file mode 100644 index 00000000000..7cdc825118a --- /dev/null +++ b/analyzers/rspec/cs/S6674.html @@ -0,0 +1,46 @@ +

A message template must conform to the specification. The rule raises an issue if the template string +violates the template string grammar.

+

Why is this an issue?

+

A message template needs to comply with a set of rules. Logging provider parse the template and enrich log entries with +the information found in the template. An unparsable message template leads to corrupted log entries and might result in a loss of information in the +logs.

+

The rule covers the following logging frameworks:

+ +

Code examples

+

Noncompliant code example

+
+logger.LogError("Login failed for {User", user);       // Noncompliant: Missing closing bracket
+logger.LogError("Login failed for {{User}", user);     // Noncompliant: Opening bracket is escaped
+logger.LogError("Login failed for {&User}", user);     // Noncompliant: Only '@' and '$' are allowed as prefix
+logger.LogError("Login failed for {User-Name}", user); // Noncompliant: Only chars, numbers, and underscore are allowed for placeholders
+logger.LogDebug("Retry attempt {Cnt,r}", cnt);         // Noncompliant: The alignment specifier must be numeric
+logger.LogDebug("Retry attempt {Cnt:}", cnt);          // Noncompliant: Empty format specifier is not allowed
+
+

Compliant solution

+
+logger.LogError("Login failed for {User}", user);
+logger.LogError("Login failed for {{{User}}}", user);
+logger.LogError("Login failed for {User}", user);
+logger.LogError("Login failed for {User_Name}", user);
+logger.LogDebug("Retry attempt {Cnt,-5}", cnt);
+logger.LogDebug("Retry attempt {Cnt:000}", cnt);
+
+

Resources

+

Documentation

+ + diff --git a/analyzers/rspec/cs/S6674.json b/analyzers/rspec/cs/S6674.json new file mode 100644 index 00000000000..fd91d32b09c --- /dev/null +++ b/analyzers/rspec/cs/S6674.json @@ -0,0 +1,15 @@ +{ + "title": "Log message template should be syntactically correct", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": ["logging"], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-6674", + "sqKey": "S6674", + "scope": "Main", + "quickfix": "infeasible" +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index ca5cc1fd588..bcc049da95a 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -293,6 +293,7 @@ "S6640", "S6667", "S6668", + "S6674", "S6677", "S6678", "S6797", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs index 31ca5a5b1bb..45193518f76 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs @@ -58,7 +58,7 @@ protected override void Initialize(SonarAnalysisContext context) => }, SyntaxKind.InvocationExpression); - private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) => + public static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) => TemplateArgument(invocation, model, KnownType.Microsoft_Extensions_Logging_LoggerExtensions, MicrosoftExtensionsLogging, "message") ?? TemplateArgument(invocation, model, KnownType.Serilog_Log, Serilog, "messageTemplate") ?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate", checkDerivedTypes: true) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs new file mode 100644 index 00000000000..88e0e58eb64 --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs @@ -0,0 +1,160 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using System.Text.RegularExpressions; + +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class MessageTemplatesShouldBeCorrect : SonarDiagnosticAnalyzer +{ + private const string DiagnosticId = "S6674"; + private const string MessageFormat = "Log message template {0}."; + + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(c => + { + var invocation = (InvocationExpressionSyntax)c.Node; + if (MessageTemplateAnalyzer.TemplateArgument(invocation, c.SemanticModel) is { } argument + && argument.Expression.IsKind(SyntaxKind.StringLiteralExpression) + && TemplateValidator.ContainsErrors(argument.Expression.ToString(), out var errors)) + { + var templateStart = argument.Expression.GetLocation().SourceSpan.Start; + foreach (var error in errors) + { + var location = Location.Create(c.Tree, new(templateStart + error.Start, error.Length)); + c.ReportIssue(Diagnostic.Create(Rule, location, error.Message)); + } + } + }, + SyntaxKind.InvocationExpression); + + private static class TemplateValidator + { + private const string TextPattern = @"([^\{]|\{\{|\}\})+"; + private const string HolePattern = @"{(?[^\}]*)}"; + private const string TemplatePattern = $"^({TextPattern}|{HolePattern})*$"; + // This is similar to the regex used for MessageTemplatesAnalyzer, but it is far more permissive. + // The goal is to manually parse the placeholders, so that we can report more specific issues than just "malformed template". + private static readonly Regex TemplateRegex = new(TemplatePattern, RegexOptions.Compiled, TimeSpan.FromMilliseconds(300)); + + private static readonly Regex PlaceholderNameRegex = new("^[0-9a-zA-Z_]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout); + private static readonly Regex PlaceholderAlignmentRegex = new("^-?[0-9]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout); + + public static bool ContainsErrors(string template, out List errors) + { + var result = Helpers.MessageTemplates.Parse(template, TemplateRegex); + errors = result.Success + ? result.Placeholders.Select(ParsePlaceholder).Where(x => x is not null).ToList() + : [new("should be syntactically correct", 0, template.Length)]; + return errors.Count > 0; + } + + private static ParsingError ParsePlaceholder(Helpers.MessageTemplates.Placeholder placeholder) + { + if (placeholder.Length == 0) + { + return new($"should not contain empty placeholder", placeholder.Start - 1, 2); // highlight both brackets '{}', since there is no content + } + + var parts = Split(placeholder.Name); + if (!PlaceholderNameRegex.SafeIsMatch(parts.Name)) + { + return new($"placeholder '{parts.Name}' should only contain chars, numbers, and underscore", placeholder); + } + else if (parts.Alignment is not null && !PlaceholderAlignmentRegex.SafeIsMatch(parts.Alignment)) + { + return new($"placeholder '{parts.Name}' should have numeric alignment instead of '{parts.Alignment}'", placeholder); + } + else if (parts.Format == string.Empty) + { + return new($"placeholder '{parts.Name}' should not have empty format", placeholder); + } + else + { + return null; + } + } + + // pattern is: name[,alignment][:format] + private static Parts Split(string placeholder) + { + var start = placeholder[0] is '@' or '$' ? 1 : 0; // skip prefix + + string name; + string alignment = null; + string format = null; + + var formatIndex = placeholder.IndexOf(':'); + var alignmentIndex = placeholder.IndexOf(','); + if (formatIndex >= 0 && alignmentIndex > formatIndex) + { + // example {name:format,alignment} + // ^^^^^^^^^^^^^^^^ all of this is format, need to reset alignment + alignmentIndex = -1; + } + + if (NotFound(alignmentIndex)) + { + if (NotFound(formatIndex)) + { + name = placeholder.Substring(start); + } + else + { + name = placeholder.Substring(start, formatIndex - start); + format = IsEmpty(formatIndex) ? string.Empty : placeholder.Substring(formatIndex + 1); + } + } + else + { + if (NotFound(formatIndex)) + { + name = placeholder.Substring(start, alignmentIndex - start); + alignment = IsEmpty(alignmentIndex) ? string.Empty : placeholder.Substring(alignmentIndex + 1); + } + else + { + name = placeholder.Substring(start, alignmentIndex - start); + alignment = placeholder.Substring(alignmentIndex + 1, formatIndex - alignmentIndex - 1); + format = placeholder.Substring(formatIndex + 1); + } + } + + return new(name, alignment, format); + + bool NotFound(int index) => index == -1; + bool IsEmpty(int index) => index == placeholder.Length - 1; + } + + private sealed record Parts(string Name, string Alignment, string Format); + + public sealed record ParsingError(string Message, int Start, int Length) + { + public ParsingError(string message, Helpers.MessageTemplates.Placeholder placeholder) + : this(message, placeholder.Start, placeholder.Length) + { } + } + } +} diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs index 2c6b2f41e51..d6bc8420ee1 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs @@ -42,9 +42,12 @@ public static class MessageTemplates /// Matches and extracts placeholders from a template string. /// For more info, see Message Templates. /// - public static ParseResult Parse(string template) + public static ParseResult Parse(string template) => + Parse(template, TemplateRegex); + + public static ParseResult Parse(string template, Regex regex) { - if (TemplateRegex.SafeMatch(template) is { Success: true } match) + if (regex.SafeMatch(template) is { Success: true } match) { var placeholders = match.Groups["Placeholder"].Captures .OfType() diff --git a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs index 8278d29005e..9f10915ef21 100644 --- a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs @@ -6598,7 +6598,7 @@ internal static class RuleTypeMappingCS // ["S6671"], // ["S6672"], // ["S6673"], - // ["S6674"], + ["S6674"] = "BUG", // ["S6675"], // ["S6676"], ["S6677"] = "BUG", diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs new file mode 100644 index 00000000000..f4b0eb78c06 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs @@ -0,0 +1,139 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using SonarAnalyzer.Rules.CSharp; + +namespace SonarAnalyzer.Test.Rules; + +[TestClass] +public class MessageTemplatesShouldBeCorrectTest +{ + private static readonly IEnumerable LoggingReferences = + NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions() + .Concat(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion)) + .Concat(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion)); + + private static readonly VerifierBuilder Builder = new VerifierBuilder() + .AddReferences(LoggingReferences); + + [TestMethod] + public void MessageTemplatesShouldBeCorrect_CS() => + Builder.AddPaths("MessageTemplatesShouldBeCorrect.cs").Verify(); + + [DataTestMethod] + [DataRow("LogCritical")] + [DataRow("LogDebug")] + [DataRow("LogError")] + [DataRow("LogInformation")] + [DataRow("LogTrace")] + [DataRow("LogWarning")] + public void MessageTemplatesShouldBeCorrect_MicrosoftExtensionsLogging_CS(string methodName) => + Builder.AddSnippet($$$""" + using System; + using Microsoft.Extensions.Logging; + + public class Program + { + public void Method(ILogger logger, string user, int cnt) + { + Console.WriteLine("Login failed for {User", user); // Compliant + logger.{{{methodName}}}("Login failed for {User}", user); // Compliant + + logger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} + logger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + logger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} + logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + } + } + """).Verify(); + + [DataTestMethod] + [DataRow("Debug")] + [DataRow("Error")] + [DataRow("Information")] + [DataRow("Fatal")] + [DataRow("Warning")] + [DataRow("Verbose")] + public void MessageTemplatesShouldBeCorrect_Serilog_CS(string methodName) => + Builder.AddSnippet($$$""" + using System; + using Serilog; + using Serilog.Events; + + public class Program + { + public void Method(ILogger logger, string user, int cnt) + { + Console.WriteLine("Login failed for {User", user); // Compliant + logger.{{{methodName}}}("Login failed for {User}", user); // Compliant + + logger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} + logger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + logger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} + logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + } + } + """).Verify(); + + [DataTestMethod] + [DataRow("Debug")] + [DataRow("ConditionalDebug")] + [DataRow("Error")] + [DataRow("Fatal")] + [DataRow("Info")] + [DataRow("Trace")] + [DataRow("ConditionalTrace")] + [DataRow("Warn")] + public void MessageTemplatesShouldBeCorrect_NLog_CS(string methodName) => + Builder.AddSnippet($$$""" + using System; + using NLog; + + public class Program + { + public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, string user, int cnt) + { + Console.WriteLine("Login failed for {User", user); // Compliant + logger.{{{methodName}}}("Login failed for {User}", user); // Compliant + + iLogger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} + iLogger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + iLogger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + iLogger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} + iLogger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + + logger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} + logger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + logger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} + logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + + myLogger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} + myLogger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + myLogger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + myLogger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} + myLogger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + } + } + public class MyLogger : Logger { } + """).Verify(); +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs new file mode 100644 index 00000000000..5abbeda9c4e --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs @@ -0,0 +1,115 @@ +using Microsoft.Extensions.Logging; +using System; + +public class Program +{ + public void Compliant(ILogger logger, string user) + { + Console.WriteLine("Login failed for {User", user); + + logger.LogError(""); + logger.LogError("Login failed for User"); + logger.LogError("{User}", user); + logger.LogError("{User}{User}{User}", user); + logger.LogError("User{User}User", user); + + logger.LogError($"Login failed for {{User", user); + logger.LogError("Login failed for {User}", user); + logger.LogError("Login failed for {{User}}", user); + logger.LogError("Login failed for {{{User}}}", user); + + logger.LogError("{{ }} {{ {{{{ {{🔥}}"); // On fire + + logger.LogError("Login failed for {$User,42}", user); + logger.LogError("Login failed for {@User:format}", user); + logger.LogError("Login failed for {User42,42:format}", user); + logger.LogError("Login failed for {User_Name:format,42}", user); + + logger.LogError("Login failed for {User: }", user); + logger.LogError("Login failed for {User: -f0rmat c@n contain _any!@#$thing_ {you{want{ }", user); + logger.LogError("Login failed for {User,-42}", user); + + logger.LogError("Login failed for {{User}", user); // Compliant FN. Syntactically valid, in reality it's probably a typo. + } + + public void InvalidSyntax(ILogger logger, string user) + { + logger.LogError("Login failed for {User", user); // Noncompliant {{Log message template should be syntactically correct.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^ + logger.LogError("Login failed for {{{User", user); // Noncompliant {{Log message template should be syntactically correct.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^^^ + logger.LogError("{", user); // Noncompliant {{Log message template should be syntactically correct.}} + // ^^^ + logger.LogError("{{{", user); // Noncompliant {{Log message template should be syntactically correct.}} + // ^^^^^ + } + + public void EmptyPlaceholder(ILogger logger, string user) + { + logger.LogError("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + logger.LogError("{}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + logger.LogError("{{{}}}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + logger.LogError("{{Login for {} user }}failed", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + } + + public void InvalidName(ILogger logger, string user) + { + logger.LogError("Login failed for {@}", user); // Noncompliant {{Log message template placeholder '' should only contain chars, numbers, and underscore.}} + // ^ + logger.LogError("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + // ^^^^^ + logger.LogError("Login failed for { {User} }", user); // Noncompliant {{Log message template placeholder ' {User' should only contain chars, numbers, and underscore.}} + // ^^^^^^ + logger.LogError("Login failed for {User-Name}", user); // Noncompliant {{Log message template placeholder 'User-Name' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^^ + logger.LogError("Login failed for {User Name}", user); // Noncompliant {{Log message template placeholder 'User Name' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^^ + logger.LogError("Login failed for {user}, server is on {🔥}", user, "fire"); // Noncompliant {{Log message template placeholder '🔥' should only contain chars, numbers, and underscore.}} + // ^^ + logger.LogError("Retry attempt {@,:}", user); // Noncompliant {{Log message template placeholder '' should only contain chars, numbers, and underscore.}} + // ^^^ + } + + public void InvalidAlignmentAndFormat(ILogger logger, int cnt) + { + logger.LogError("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} + // ^^^^^ + logger.LogError("Retry attempt {Cnt,}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of ''.}} + // ^^^^ + logger.LogError("Retry attempt {Cnt,-foo}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of '-foo'.}} + // ^^^^^^^^ + logger.LogError("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + // ^^^^ + logger.LogError("Retry attempt {Cnt,:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of ''.}} + // ^^^^^ + } + + public void Complex(ILogger logger, string user, int cnt, int total) + { + logger.LogError("[User {user name,42}] Retry attempt {Cnt,:}", user, cnt); + // ^^^^^^^^^^^^ {{Log message template placeholder 'user name' should only contain chars, numbers, and underscore.}} + // ^^^^^ @-1 {{Log message template placeholder 'Cnt' should have numeric alignment instead of ''.}} + + + logger.LogError("[User {user%,42}] Retry {{attempt}} {@Cnt:foo,} {$Total,-5000:}", user, cnt, total); + // ^^^^^^^^ {{Log message template placeholder 'user%' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^^^^^^ @-1 {{Log message template placeholder 'Total' should not have empty format.}} + + logger.LogError(@"[User {user,42}] Retry attempt {@Cnt + :foo,}", user, cnt); // Noncompliant@-1 + + logger.LogError(@" + [User {user name}] + Retry attempt {@Cnt:} + Out of total {$Total,-abc:format}", + user, cnt, total); + // ^^^^^^^^^ @-3 {{Log message template placeholder 'user name' should only contain chars, numbers, and underscore.}} + // ^^^^^ @-3 {{Log message template placeholder 'Cnt' should not have empty format.}} + // ^^^^^^^^^^^^^^^^^^ @-3 {{Log message template placeholder 'Total' should have numeric alignment instead of '-abc'.}} + } + +} From 35c2bade2c4bcc9dea0d00c3b8d4e6ac7a161ea2 Mon Sep 17 00:00:00 2001 From: Gregory Paidis Date: Wed, 28 Feb 2024 16:06:26 +0100 Subject: [PATCH 2/4] Review 1 --- .../Helpers/MessageTemplateExtractor.cs | 49 +++++++++++++ .../MessageTemplateAnalyzer.cs | 27 +------- .../Rules/MessageTemplatesShouldBeCorrect.cs | 4 +- .../MessageTemplatesShouldBeCorrectTest.cs | 68 ++++++++----------- .../MessageTemplatesShouldBeCorrect.cs | 20 +++--- 5 files changed, 89 insertions(+), 79 deletions(-) create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs b/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs new file mode 100644 index 00000000000..25efe1a6142 --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs @@ -0,0 +1,49 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using static Roslyn.Utilities.SonarAnalyzer.Shared.LoggingFrameworkMethods; + +namespace SonarAnalyzer.Helpers; + +internal static class MessageTemplateExtractor +{ + public static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) => + TemplateArgument(invocation, model, KnownType.Microsoft_Extensions_Logging_LoggerExtensions, MicrosoftExtensionsLogging, "message") + ?? TemplateArgument(invocation, model, KnownType.Serilog_Log, Serilog, "messageTemplate") + ?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate") + ?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerExtensions, NLogLoggingMethods, "message") + ?? TemplateArgument(invocation, model, KnownType.NLog_ILogger, NLogLoggingMethods, "message", checkDerivedTypes: true) + ?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerBase, NLogILoggerBase, "message", checkDerivedTypes: true); + + private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, + SemanticModel model, + KnownType type, + ICollection methods, + string template, + bool checkDerivedTypes = false) => + methods.Contains(invocation.GetIdentifier().ToString()) + && model.GetSymbolInfo(invocation).Symbol is IMethodSymbol method + && method.HasContainingType(type, checkDerivedTypes) + && CSharpFacade.Instance.MethodParameterLookup(invocation, method) is { } lookup + && lookup.TryGetSyntax(template, out var argumentsFound) // Fetch Argument.Expression with IParameterSymbol.Name == templateName + && argumentsFound.Length == 1 + ? (ArgumentSyntax)argumentsFound[0].Parent + : null; +} diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs index 45193518f76..9163baf7974 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs @@ -20,8 +20,6 @@ using SonarAnalyzer.Rules.MessageTemplates; -using static Roslyn.Utilities.SonarAnalyzer.Shared.LoggingFrameworkMethods; - namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] @@ -46,7 +44,7 @@ protected override void Initialize(SonarAnalysisContext context) => var invocation = (InvocationExpressionSyntax)c.Node; var enabledChecks = Checks.Where(x => x.Rule.IsEnabled(c)).ToArray(); if (enabledChecks.Length > 0 - && TemplateArgument(invocation, c.SemanticModel) is { } argument + && MessageTemplateExtractor.TemplateArgument(invocation, c.SemanticModel) is { } argument && HasValidExpression(argument) && Helpers.MessageTemplates.Parse(argument.Expression.ToString()) is { Success: true } result) { @@ -58,29 +56,6 @@ protected override void Initialize(SonarAnalysisContext context) => }, SyntaxKind.InvocationExpression); - public static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) => - TemplateArgument(invocation, model, KnownType.Microsoft_Extensions_Logging_LoggerExtensions, MicrosoftExtensionsLogging, "message") - ?? TemplateArgument(invocation, model, KnownType.Serilog_Log, Serilog, "messageTemplate") - ?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate", checkDerivedTypes: true) - ?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerExtensions, NLogLoggingMethods, "message") - ?? TemplateArgument(invocation, model, KnownType.NLog_ILogger, NLogLoggingMethods, "message", checkDerivedTypes: true) - ?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerBase, NLogILoggerBase, "message", checkDerivedTypes: true); - - private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, - SemanticModel model, - KnownType type, - ICollection methods, - string template, - bool checkDerivedTypes = false) => - methods.Contains(invocation.GetIdentifier().ToString()) - && model.GetSymbolInfo(invocation).Symbol is IMethodSymbol method - && method.HasContainingType(type, checkDerivedTypes) - && CSharpFacade.Instance.MethodParameterLookup(invocation, method) is { } lookup - && lookup.TryGetSyntax(template, out var argumentsFound) // Fetch Argument.Expression with IParameterSymbol.Name == templateName - && 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/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs index 88e0e58eb64..77d3808a508 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs @@ -36,7 +36,7 @@ protected override void Initialize(SonarAnalysisContext context) => context.RegisterNodeAction(c => { var invocation = (InvocationExpressionSyntax)c.Node; - if (MessageTemplateAnalyzer.TemplateArgument(invocation, c.SemanticModel) is { } argument + if (MessageTemplateExtractor.TemplateArgument(invocation, c.SemanticModel) is { } argument && argument.Expression.IsKind(SyntaxKind.StringLiteralExpression) && TemplateValidator.ContainsErrors(argument.Expression.ToString(), out var errors)) { @@ -81,7 +81,7 @@ private static ParsingError ParsePlaceholder(Helpers.MessageTemplates.Placeholde var parts = Split(placeholder.Name); if (!PlaceholderNameRegex.SafeIsMatch(parts.Name)) { - return new($"placeholder '{parts.Name}' should only contain chars, numbers, and underscore", placeholder); + return new($"placeholder '{parts.Name}' should only contain letters, numbers, and underscore", placeholder); } else if (parts.Alignment is not null && !PlaceholderAlignmentRegex.SafeIsMatch(parts.Alignment)) { diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs index f4b0eb78c06..b3709d465d9 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs @@ -25,17 +25,14 @@ namespace SonarAnalyzer.Test.Rules; [TestClass] public class MessageTemplatesShouldBeCorrectTest { - private static readonly IEnumerable LoggingReferences = - NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions() - .Concat(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion)) - .Concat(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion)); - - private static readonly VerifierBuilder Builder = new VerifierBuilder() - .AddReferences(LoggingReferences); + private static readonly VerifierBuilder Builder = new VerifierBuilder(); [TestMethod] public void MessageTemplatesShouldBeCorrect_CS() => - Builder.AddPaths("MessageTemplatesShouldBeCorrect.cs").Verify(); + Builder + .AddReferences(NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions()) + .AddPaths("MessageTemplatesShouldBeCorrect.cs") + .Verify(); [DataTestMethod] [DataRow("LogCritical")] @@ -45,22 +42,22 @@ public void MessageTemplatesShouldBeCorrect_CS() => [DataRow("LogTrace")] [DataRow("LogWarning")] public void MessageTemplatesShouldBeCorrect_MicrosoftExtensionsLogging_CS(string methodName) => - Builder.AddSnippet($$$""" + Builder + .AddReferences(NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions()) + .AddSnippet($$$""" using System; using Microsoft.Extensions.Logging; public class Program { - public void Method(ILogger logger, string user, int cnt) + public void Method(ILogger logger, string user, int count) { Console.WriteLine("Login failed for {User", user); // Compliant logger.{{{methodName}}}("Login failed for {User}", user); // Compliant - logger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} - logger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} - logger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} - logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} - logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + logger.{{{methodName}}}("{", user); // Noncompliant + logger.Log(LogLevel.Information, "{", user); // Noncompliant + LoggerExtensions.{{{methodName}}}(logger, "{", user); // Noncompliant } } """).Verify(); @@ -73,23 +70,23 @@ public void Method(ILogger logger, string user, int cnt) [DataRow("Warning")] [DataRow("Verbose")] public void MessageTemplatesShouldBeCorrect_Serilog_CS(string methodName) => - Builder.AddSnippet($$$""" + Builder + .AddReferences(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion)) + .AddSnippet($$$""" using System; using Serilog; using Serilog.Events; public class Program { - public void Method(ILogger logger, string user, int cnt) + public void Method(ILogger logger, string user, int count) { Console.WriteLine("Login failed for {User", user); // Compliant logger.{{{methodName}}}("Login failed for {User}", user); // Compliant - logger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} - logger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} - logger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} - logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} - logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + logger.{{{methodName}}}("{", user); // Noncompliant + Log.{{{methodName}}}("{", user); // Noncompliant + Log.Write(LogEventLevel.Verbose, "{", user); // Noncompliant } } """).Verify(); @@ -104,34 +101,23 @@ public void Method(ILogger logger, string user, int cnt) [DataRow("ConditionalTrace")] [DataRow("Warn")] public void MessageTemplatesShouldBeCorrect_NLog_CS(string methodName) => - Builder.AddSnippet($$$""" + Builder + .AddReferences(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion)) + .AddSnippet($$$""" using System; using NLog; public class Program { - public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, string user, int cnt) + public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, string user, int count) { Console.WriteLine("Login failed for {User", user); // Compliant logger.{{{methodName}}}("Login failed for {User}", user); // Compliant - iLogger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} - iLogger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} - iLogger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} - iLogger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} - iLogger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} - - logger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} - logger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} - logger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} - logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} - logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} - - myLogger.{{{methodName}}}("{", user); // Noncompliant {{Log message template should be syntactically correct.}} - myLogger.{{{methodName}}}("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} - myLogger.{{{methodName}}}("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} - myLogger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} - myLogger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + iLogger.{{{methodName}}}("{", user); // Noncompliant + logger.{{{methodName}}}("{", user); // Noncompliant + myLogger.{{{methodName}}}("{", user); // Noncompliant + ILoggerExtensions.ConditionalDebug(iLogger, "{", user); // Noncompliant } } public class MyLogger : Logger { } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs index 5abbeda9c4e..aba940e400e 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs @@ -58,19 +58,19 @@ public void EmptyPlaceholder(ILogger logger, string user) public void InvalidName(ILogger logger, string user) { - logger.LogError("Login failed for {@}", user); // Noncompliant {{Log message template placeholder '' should only contain chars, numbers, and underscore.}} + logger.LogError("Login failed for {@}", user); // Noncompliant {{Log message template placeholder '' should only contain letters, numbers, and underscore.}} // ^ - logger.LogError("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + logger.LogError("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain letters, numbers, and underscore.}} // ^^^^^ - logger.LogError("Login failed for { {User} }", user); // Noncompliant {{Log message template placeholder ' {User' should only contain chars, numbers, and underscore.}} + logger.LogError("Login failed for { {User} }", user); // Noncompliant {{Log message template placeholder ' {User' should only contain letters, numbers, and underscore.}} // ^^^^^^ - logger.LogError("Login failed for {User-Name}", user); // Noncompliant {{Log message template placeholder 'User-Name' should only contain chars, numbers, and underscore.}} + logger.LogError("Login failed for {User-Name}", user); // Noncompliant {{Log message template placeholder 'User-Name' should only contain letters, numbers, and underscore.}} // ^^^^^^^^^ - logger.LogError("Login failed for {User Name}", user); // Noncompliant {{Log message template placeholder 'User Name' should only contain chars, numbers, and underscore.}} + logger.LogError("Login failed for {User Name}", user); // Noncompliant {{Log message template placeholder 'User Name' should only contain letters, numbers, and underscore.}} // ^^^^^^^^^ - logger.LogError("Login failed for {user}, server is on {🔥}", user, "fire"); // Noncompliant {{Log message template placeholder '🔥' should only contain chars, numbers, and underscore.}} + logger.LogError("Login failed for {user}, server is on {🔥}", user, "fire"); // Noncompliant {{Log message template placeholder '🔥' should only contain letters, numbers, and underscore.}} // ^^ - logger.LogError("Retry attempt {@,:}", user); // Noncompliant {{Log message template placeholder '' should only contain chars, numbers, and underscore.}} + logger.LogError("Retry attempt {@,:}", user); // Noncompliant {{Log message template placeholder '' should only contain letters, numbers, and underscore.}} // ^^^ } @@ -91,12 +91,12 @@ public void InvalidAlignmentAndFormat(ILogger logger, int cnt) public void Complex(ILogger logger, string user, int cnt, int total) { logger.LogError("[User {user name,42}] Retry attempt {Cnt,:}", user, cnt); - // ^^^^^^^^^^^^ {{Log message template placeholder 'user name' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^^^^^ {{Log message template placeholder 'user name' should only contain letters, numbers, and underscore.}} // ^^^^^ @-1 {{Log message template placeholder 'Cnt' should have numeric alignment instead of ''.}} logger.LogError("[User {user%,42}] Retry {{attempt}} {@Cnt:foo,} {$Total,-5000:}", user, cnt, total); - // ^^^^^^^^ {{Log message template placeholder 'user%' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^ {{Log message template placeholder 'user%' should only contain letters, numbers, and underscore.}} // ^^^^^^^^^^^^^ @-1 {{Log message template placeholder 'Total' should not have empty format.}} logger.LogError(@"[User {user,42}] Retry attempt {@Cnt @@ -107,7 +107,7 @@ public void Complex(ILogger logger, string user, int cnt, int total) Retry attempt {@Cnt:} Out of total {$Total,-abc:format}", user, cnt, total); - // ^^^^^^^^^ @-3 {{Log message template placeholder 'user name' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^^ @-3 {{Log message template placeholder 'user name' should only contain letters, numbers, and underscore.}} // ^^^^^ @-3 {{Log message template placeholder 'Cnt' should not have empty format.}} // ^^^^^^^^^^^^^^^^^^ @-3 {{Log message template placeholder 'Total' should have numeric alignment instead of '-abc'.}} } From 311e331bbb9f247ec6a68a0fb249da72a9a55e8a Mon Sep 17 00:00:00 2001 From: Gregory Paidis Date: Wed, 28 Feb 2024 17:04:37 +0100 Subject: [PATCH 3/4] Review 2 --- .../Helpers/MessageTemplateExtractor.cs | 2 +- .../MessageTemplates/IMessageTemplateCheck.cs | 2 +- .../MessageTemplateAnalyzer.cs | 2 +- .../NamedPlaceholdersShouldBeUnique.cs | 2 +- .../UsePascalCaseForNamedPlaceHolders.cs | 4 +- .../Rules/MessageTemplatesShouldBeCorrect.cs | 79 ++++++++----------- ...Templates.cs => MessageTemplatesParser.cs} | 2 +- .../Helpers/MessageTemplatesTest.cs | 14 ++-- 8 files changed, 45 insertions(+), 62 deletions(-) rename analyzers/src/SonarAnalyzer.Common/Helpers/{MessageTemplates.cs => MessageTemplatesParser.cs} (98%) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs b/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs index 25efe1a6142..9eaa4ce288b 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs @@ -27,7 +27,7 @@ internal static class MessageTemplateExtractor public static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) => TemplateArgument(invocation, model, KnownType.Microsoft_Extensions_Logging_LoggerExtensions, MicrosoftExtensionsLogging, "message") ?? TemplateArgument(invocation, model, KnownType.Serilog_Log, Serilog, "messageTemplate") - ?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate") + ?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate", checkDerivedTypes: true) ?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerExtensions, NLogLoggingMethods, "message") ?? TemplateArgument(invocation, model, KnownType.NLog_ILogger, NLogLoggingMethods, "message", checkDerivedTypes: true) ?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerBase, NLogILoggerBase, "message", checkDerivedTypes: true); diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/IMessageTemplateCheck.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/IMessageTemplateCheck.cs index 9a1d33ab29d..90a767604f6 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/IMessageTemplateCheck.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/IMessageTemplateCheck.cs @@ -24,5 +24,5 @@ public interface IMessageTemplateCheck { DiagnosticDescriptor Rule { get; } - void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Helpers.MessageTemplates.Placeholder[] placeholders); + void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, MessageTemplatesParser.Placeholder[] placeholders); } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs index 9163baf7974..f16f8f7c67a 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs @@ -46,7 +46,7 @@ protected override void Initialize(SonarAnalysisContext context) => if (enabledChecks.Length > 0 && MessageTemplateExtractor.TemplateArgument(invocation, c.SemanticModel) is { } argument && HasValidExpression(argument) - && Helpers.MessageTemplates.Parse(argument.Expression.ToString()) is { Success: true } result) + && MessageTemplatesParser.Parse(argument.Expression.ToString()) is { Success: true } result) { foreach (var check in enabledChecks) { diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/NamedPlaceholdersShouldBeUnique.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/NamedPlaceholdersShouldBeUnique.cs index e9038b4d891..ff958b21d42 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/NamedPlaceholdersShouldBeUnique.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/NamedPlaceholdersShouldBeUnique.cs @@ -29,7 +29,7 @@ public sealed class NamedPlaceholdersShouldBeUnique : IMessageTemplateCheck public DiagnosticDescriptor Rule => S6677; - public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Helpers.MessageTemplates.Placeholder[] placeholders) + public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, MessageTemplatesParser.Placeholder[] placeholders) { var duplicatedGroups = placeholders .Where(x => x.Name != "_" && !int.TryParse(x.Name, out _)) // exclude wildcard "_" and index placeholders like {42} diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/UsePascalCaseForNamedPlaceHolders.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/UsePascalCaseForNamedPlaceHolders.cs index df988b8a2c2..844af0844d5 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/UsePascalCaseForNamedPlaceHolders.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/UsePascalCaseForNamedPlaceHolders.cs @@ -18,7 +18,7 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -using static SonarAnalyzer.Helpers.MessageTemplates; +using static SonarAnalyzer.Helpers.MessageTemplatesParser; namespace SonarAnalyzer.Rules.MessageTemplates; @@ -31,7 +31,7 @@ public sealed class UsePascalCaseForNamedPlaceHolders : IMessageTemplateCheck public DiagnosticDescriptor Rule => S6678; - public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Helpers.MessageTemplates.Placeholder[] placeholders) + public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Placeholder[] placeholders) { var nonPascalCasePlaceholders = placeholders.Where(x => char.IsLower(x.Name[0])).ToArray(); if (nonPascalCasePlaceholders.Length > 0) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs index 77d3808a508..e5b15fe0442 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs @@ -52,57 +52,51 @@ protected override void Initialize(SonarAnalysisContext context) => private static class TemplateValidator { + private const int EmptyPlaceholderSize = 2; // "{}" + private const string TextPattern = @"([^\{]|\{\{|\}\})+"; private const string HolePattern = @"{(?[^\}]*)}"; private const string TemplatePattern = $"^({TextPattern}|{HolePattern})*$"; // This is similar to the regex used for MessageTemplatesAnalyzer, but it is far more permissive. // The goal is to manually parse the placeholders, so that we can report more specific issues than just "malformed template". private static readonly Regex TemplateRegex = new(TemplatePattern, RegexOptions.Compiled, TimeSpan.FromMilliseconds(300)); - private static readonly Regex PlaceholderNameRegex = new("^[0-9a-zA-Z_]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout); private static readonly Regex PlaceholderAlignmentRegex = new("^-?[0-9]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout); public static bool ContainsErrors(string template, out List errors) { - var result = Helpers.MessageTemplates.Parse(template, TemplateRegex); + var result = MessageTemplatesParser.Parse(template, TemplateRegex); errors = result.Success ? result.Placeholders.Select(ParsePlaceholder).Where(x => x is not null).ToList() : [new("should be syntactically correct", 0, template.Length)]; return errors.Count > 0; } - private static ParsingError ParsePlaceholder(Helpers.MessageTemplates.Placeholder placeholder) + private static ParsingError ParsePlaceholder(MessageTemplatesParser.Placeholder placeholder) { if (placeholder.Length == 0) { - return new($"should not contain empty placeholder", placeholder.Start - 1, 2); // highlight both brackets '{}', since there is no content + return new("should not contain empty placeholder", placeholder.Start - 1, EmptyPlaceholderSize); } - var parts = Split(placeholder.Name); - if (!PlaceholderNameRegex.SafeIsMatch(parts.Name)) - { - return new($"placeholder '{parts.Name}' should only contain letters, numbers, and underscore", placeholder); - } - else if (parts.Alignment is not null && !PlaceholderAlignmentRegex.SafeIsMatch(parts.Alignment)) - { - return new($"placeholder '{parts.Name}' should have numeric alignment instead of '{parts.Alignment}'", placeholder); - } - else if (parts.Format == string.Empty) - { - return new($"placeholder '{parts.Name}' should not have empty format", placeholder); - } - else + return "🔥" switch { - return null; - } + _ when !PlaceholderNameRegex.SafeIsMatch(parts.Name) => + new($"placeholder '{parts.Name}' should only contain letters, numbers, and underscore", placeholder), + + _ when parts.Alignment is not null && !PlaceholderAlignmentRegex.SafeIsMatch(parts.Alignment) => + new($"placeholder '{parts.Name}' should have numeric alignment instead of '{parts.Alignment}'", placeholder), + + _ when parts.Format == string.Empty => + new($"placeholder '{parts.Name}' should not have empty format", placeholder), + + _ => null, + }; } // pattern is: name[,alignment][:format] private static Parts Split(string placeholder) { - var start = placeholder[0] is '@' or '$' ? 1 : 0; // skip prefix - - string name; string alignment = null; string format = null; @@ -115,44 +109,33 @@ private static Parts Split(string placeholder) alignmentIndex = -1; } - if (NotFound(alignmentIndex)) + if (formatIndex == -1) { - if (NotFound(formatIndex)) - { - name = placeholder.Substring(start); - } - else - { - name = placeholder.Substring(start, formatIndex - start); - format = IsEmpty(formatIndex) ? string.Empty : placeholder.Substring(formatIndex + 1); - } + formatIndex = placeholder.Length; } else { - if (NotFound(formatIndex)) - { - name = placeholder.Substring(start, alignmentIndex - start); - alignment = IsEmpty(alignmentIndex) ? string.Empty : placeholder.Substring(alignmentIndex + 1); - } - else - { - name = placeholder.Substring(start, alignmentIndex - start); - alignment = placeholder.Substring(alignmentIndex + 1, formatIndex - alignmentIndex - 1); - format = placeholder.Substring(formatIndex + 1); - } + format = placeholder.Substring(formatIndex + 1); + } + if (alignmentIndex == -1) + { + alignmentIndex = formatIndex; + } + else + { + alignment = placeholder.Substring(alignmentIndex + 1, formatIndex - alignmentIndex - 1); } + var start = placeholder[0] is '@' or '$' ? 1 : 0; // skip prefix + var name = placeholder.Substring(start, alignmentIndex - start); return new(name, alignment, format); - - bool NotFound(int index) => index == -1; - bool IsEmpty(int index) => index == placeholder.Length - 1; } private sealed record Parts(string Name, string Alignment, string Format); public sealed record ParsingError(string Message, int Start, int Length) { - public ParsingError(string message, Helpers.MessageTemplates.Placeholder placeholder) + public ParsingError(string message, MessageTemplatesParser.Placeholder placeholder) : this(message, placeholder.Start, placeholder.Length) { } } diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplatesParser.cs similarity index 98% rename from analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs rename to analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplatesParser.cs index d6bc8420ee1..ddf329b6904 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplatesParser.cs @@ -25,7 +25,7 @@ namespace SonarAnalyzer.Helpers; /// /// Grammar can be found here. /// -public static class MessageTemplates +public static class MessageTemplatesParser { private const string NamePattern = "[0-9a-zA-Z_]+"; private const string PlaceholderPattern = $"(?{NamePattern})"; diff --git a/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs b/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs index 7c15ec8bff5..3e4a2ccbad9 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Helpers/MessageTemplatesTest.cs @@ -35,7 +35,7 @@ public class MessageTemplatesTest [DataRow("{{hello {{world}}}}")] public void Parse_NoPlaceholder(string template) { - var result = MessageTemplates.Parse(template); + var result = MessageTemplatesParser.Parse(template); ShouldBeSuccess(result); } @@ -61,7 +61,7 @@ public void Parse_NoPlaceholder(string template) [DataRow(""" "hello" + "{world}" + "!" """, "world", 13, 5)] public void Parse_Placeholder(string template, string placeholder, int start, int length) { - var result = MessageTemplates.Parse(template); + var result = MessageTemplatesParser.Parse(template); ShouldBeSuccess(result, 1); ShouldBe(result.Placeholders[0], placeholder, start, length); } @@ -87,7 +87,7 @@ public void Parse_Placeholder(string template, string placeholder, int start, in [DataRow("hello {world:format,42}")] // semantically looks like a typo, format and alignment are reversed, but it's syntactically valid. public void Parse_Placeholder_Named_Alignment_Format(string template) { - var result = MessageTemplates.Parse(template); + var result = MessageTemplatesParser.Parse(template); ShouldBeSuccess(result, 1); ShouldBe(result.Placeholders[0], "world", 7, 5); } @@ -104,7 +104,7 @@ public void Parse_Placeholder_Multiple() Logic {@_orchestrates42} the mind, Programmer's {ballet:_}. """; - var result = MessageTemplates.Parse(template); + var result = MessageTemplatesParser.Parse(template); ShouldBeSuccess(result, 6); ShouldBe(result.Placeholders[0], "silent", 12, 6); ShouldBe(result.Placeholders[1], "0", 56, 1); @@ -129,20 +129,20 @@ public void Parse_Placeholder_Multiple() [DataRow(""" "hello {" + "world" + "}" """)] // '+' and '"' is not allowed in placeholders public void Parse_Placeholder_Failure(string template) { - var result = MessageTemplates.Parse(template); + var result = MessageTemplatesParser.Parse(template); result.Should().NotBeNull(); result.Success.Should().BeFalse(); result.Placeholders.Should().BeNull(); } - private static void ShouldBeSuccess(MessageTemplates.ParseResult actual, int placeholderCount = 0) + private static void ShouldBeSuccess(MessageTemplatesParser.ParseResult actual, int placeholderCount = 0) { actual.Should().NotBeNull(); actual.Success.Should().BeTrue(); actual.Placeholders.Should().NotBeNull().And.HaveCount(placeholderCount); } - private static void ShouldBe(MessageTemplates.Placeholder actual, string name, int start, int length) + private static void ShouldBe(MessageTemplatesParser.Placeholder actual, string name, int start, int length) { actual.Should().NotBeNull(); actual.Name.Should().Be(name); From 0ad78f0aed2c4adaf4c4b19412640b06ebf8fea4 Mon Sep 17 00:00:00 2001 From: Gregory Paidis Date: Wed, 28 Feb 2024 17:40:16 +0100 Subject: [PATCH 4/4] Review 3 --- .../MessageTemplateExtractor.cs | 2 +- .../MessageTemplatesShouldBeCorrect.cs | 1 + .../MessageTemplatesShouldBeCorrectTest.cs | 87 ++++++++++++------- 3 files changed, 58 insertions(+), 32 deletions(-) rename analyzers/src/SonarAnalyzer.CSharp/{Helpers => Rules/MessageTemplates}/MessageTemplateExtractor.cs (98%) rename analyzers/src/SonarAnalyzer.CSharp/Rules/{ => MessageTemplates}/MessageTemplatesShouldBeCorrect.cs (99%) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateExtractor.cs similarity index 98% rename from analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs rename to analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateExtractor.cs index 9eaa4ce288b..fbe92a17e50 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateExtractor.cs @@ -20,7 +20,7 @@ using static Roslyn.Utilities.SonarAnalyzer.Shared.LoggingFrameworkMethods; -namespace SonarAnalyzer.Helpers; +namespace SonarAnalyzer.Rules.MessageTemplates; internal static class MessageTemplateExtractor { diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplatesShouldBeCorrect.cs similarity index 99% rename from analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs rename to analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplatesShouldBeCorrect.cs index e5b15fe0442..fcd34fc9402 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplatesShouldBeCorrect.cs @@ -19,6 +19,7 @@ */ using System.Text.RegularExpressions; +using SonarAnalyzer.Rules.MessageTemplates; namespace SonarAnalyzer.Rules.CSharp; diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs index b3709d465d9..868d89455e6 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs @@ -35,13 +35,14 @@ public void MessageTemplatesShouldBeCorrect_CS() => .Verify(); [DataTestMethod] - [DataRow("LogCritical")] - [DataRow("LogDebug")] - [DataRow("LogError")] - [DataRow("LogInformation")] - [DataRow("LogTrace")] - [DataRow("LogWarning")] - public void MessageTemplatesShouldBeCorrect_MicrosoftExtensionsLogging_CS(string methodName) => + [DataRow("LogCritical", "")] + [DataRow("LogDebug", "")] + [DataRow("LogError", "")] + [DataRow("LogInformation", "")] + [DataRow("LogTrace", "")] + [DataRow("LogWarning", "")] + [DataRow("Log", "LogLevel.Information,")] + public void MessageTemplatesShouldBeCorrect_MicrosoftExtensionsLogging_CS(string methodName, string logLevel) => Builder .AddReferences(NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions()) .AddSnippet($$$""" @@ -50,26 +51,27 @@ public void MessageTemplatesShouldBeCorrect_MicrosoftExtensionsLogging_CS(string public class Program { - public void Method(ILogger logger, string user, int count) + public void Method(ILogger logger, string user) { - Console.WriteLine("Login failed for {User", user); // Compliant - logger.{{{methodName}}}("Login failed for {User}", user); // Compliant + Console.WriteLine("Login failed for {User", user); // Compliant + logger.{{{methodName}}}({{{logLevel}}} "Login failed for {User}", user); // Compliant - logger.{{{methodName}}}("{", user); // Noncompliant - logger.Log(LogLevel.Information, "{", user); // Noncompliant - LoggerExtensions.{{{methodName}}}(logger, "{", user); // Noncompliant + logger.{{{methodName}}}({{{logLevel}}} "{", user); // Noncompliant + LoggerExtensions.{{{methodName}}}(logger, {{{logLevel}}} "{", user); // Noncompliant } } - """).Verify(); + """) + .Verify(); [DataTestMethod] - [DataRow("Debug")] - [DataRow("Error")] - [DataRow("Information")] - [DataRow("Fatal")] - [DataRow("Warning")] - [DataRow("Verbose")] - public void MessageTemplatesShouldBeCorrect_Serilog_CS(string methodName) => + [DataRow("Debug", "")] + [DataRow("Error", "")] + [DataRow("Information", "")] + [DataRow("Fatal", "")] + [DataRow("Warning", "")] + [DataRow("Verbose", "")] + [DataRow("Write", "LogEventLevel.Verbose,")] + public void MessageTemplatesShouldBeCorrect_Serilog_CS(string methodName, string logEventLevel) => Builder .AddReferences(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion)) .AddSnippet($$$""" @@ -79,17 +81,17 @@ public void MessageTemplatesShouldBeCorrect_Serilog_CS(string methodName) => public class Program { - public void Method(ILogger logger, string user, int count) + public void Method(ILogger logger, string user) { - Console.WriteLine("Login failed for {User", user); // Compliant - logger.{{{methodName}}}("Login failed for {User}", user); // Compliant + Console.WriteLine("Login failed for {User", user); // Compliant + logger.{{{methodName}}}({{{logEventLevel}}} "Login failed for {User}", user); // Compliant - logger.{{{methodName}}}("{", user); // Noncompliant - Log.{{{methodName}}}("{", user); // Noncompliant - Log.Write(LogEventLevel.Verbose, "{", user); // Noncompliant + logger.{{{methodName}}}({{{logEventLevel}}} "{", user); // Noncompliant + Log.{{{methodName}}}({{{logEventLevel}}} "{", user); // Noncompliant } } - """).Verify(); + """) + .Verify(); [DataTestMethod] [DataRow("Debug")] @@ -109,7 +111,7 @@ public void MessageTemplatesShouldBeCorrect_NLog_CS(string methodName) => public class Program { - public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, string user, int count) + public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, string user) { Console.WriteLine("Login failed for {User", user); // Compliant logger.{{{methodName}}}("Login failed for {User}", user); // Compliant @@ -117,9 +119,32 @@ public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, string use iLogger.{{{methodName}}}("{", user); // Noncompliant logger.{{{methodName}}}("{", user); // Noncompliant myLogger.{{{methodName}}}("{", user); // Noncompliant - ILoggerExtensions.ConditionalDebug(iLogger, "{", user); // Noncompliant } } public class MyLogger : Logger { } - """).Verify(); + """) + .Verify(); + + [DataTestMethod] + [DataRow("ConditionalDebug")] + [DataRow("ConditionalTrace")] + public void MessageTemplatesShouldBeCorrect_NLog_ConditionalExtensions_CS(string methodName) => + Builder + .AddReferences(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion)) + .AddSnippet($$$""" + using System; + using NLog; + + public class Program + { + public void Method(ILogger iLogger, string user) + { + Console.WriteLine("Login failed for {User", user); // Compliant + + ILoggerExtensions.{{{methodName}}}(iLogger, "{", user); // Noncompliant + } + } + public class MyLogger : Logger { } + """) + .Verify(); }