Review 1

SonarSource · Feb 28, 2024 · d516880 · d516880
1 parent 390d8d1
commit d516880
Show file tree

Hide file tree

Showing 5 changed files with 89 additions and 79 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Helpers/MessageTemplateExtractor.cs 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<string> 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
@@ -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<string> 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
@@ -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
@@ -25,17 +25,14 @@ namespace SonarAnalyzer.Test.Rules;
 [TestClass]
 public class MessageTemplatesShouldBeCorrectTest
 {
-    private static readonly IEnumerable<MetadataReference> LoggingReferences =
-        NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions()
-        .Concat(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion))
-        .Concat(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion));
-
-    private static readonly VerifierBuilder Builder = new VerifierBuilder<MessageTemplatesShouldBeCorrect>()
-        .AddReferences(LoggingReferences);
+    private static readonly VerifierBuilder Builder = new VerifierBuilder<MessageTemplatesShouldBeCorrect>();
 
     [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 { }