New rule S6674: Log message template should be syntactically correct (#…

…8818)

analyzers/rspec/cs/S6674.html

@@ -0,0 +1,46 @@
+<p>A <a href="https://messagetemplates.org/">message template</a> must conform to the specification. The rule raises an issue if the template string
+violates the template string grammar.</p>
+<h2>Why is this an issue?</h2>
+<p>A message template needs to comply with a set of rules. <a
+href="https://learn.microsoft.com/en-us/dotnet/core/extensions/logging-providers">Logging provider</a> 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.</p>
+<p>The rule covers the following logging frameworks:</p>
+<ul>
+  <li> Nuget package - <a href="https://www.nuget.org/packages/Serilog">Serilog</a> </li>
+  <li> Nuget package - <a href="https://www.nuget.org/packages/NLog">Nlog</a> </li>
+  <li> Nuget package - <a href="https://www.nuget.org/packages/Microsoft.Extensions.Logging">Microsoft.Extensions.Logging</a> </li>
+</ul>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+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 {&amp;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
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+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);
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Message Templates - <a href="https://messagetemplates.org/">Message template specification</a> </li>
+  <li> Microsoft Learn - <a
+  href="https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#log-message-template-formatting">Log message template
+  formatting</a> </li>
+  <li> NLog - <a href="https://github.com/NLog/NLog/wiki/How-to-use-structured-logging">How to use structured logging</a> </li>
+  <li> Serilog - <a href="https://github.com/serilog/serilog/wiki/Structured-Data">Structured Data</a> </li>
+  <li> Serilog - <a
+  href="https://github.com/Suchiman/SerilogAnalyzer/blob/master/README.md#serilog002-message-template-syntax-verifier"><code>Serilog002</code>:
+  Message template syntax verifier</a> </li>
+</ul>
+

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"
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -293,6 +293,7 @@
     "S6640",
     "S6667",
     "S6668",
+    "S6674",
     "S6677",
     "S6678",
     "S6797",

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);
 }

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,9 +44,9 @@ 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)
+                && MessageTemplatesParser.Parse(argument.Expression.ToString()) is { Success: true } result)
             {
                 foreach (var check in enabledChecks)
                 {
@@ -58,29 +56,6 @@ protected override void Initialize(SonarAnalysisContext context) =>
         },
         SyntaxKind.InvocationExpression);
 
-    private 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));
 }

analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/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.Rules.MessageTemplates;
+
+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", 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;
+}

analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplatesShouldBeCorrect.cs

@@ -0,0 +1,144 @@
+/*
+ * 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;
+using SonarAnalyzer.Rules.MessageTemplates;
+
+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<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterNodeAction(c =>
+        {
+            var invocation = (InvocationExpressionSyntax)c.Node;
+            if (MessageTemplateExtractor.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 int EmptyPlaceholderSize = 2; // "{}"
+
+        private const string TextPattern = @"([^\{]|\{\{|\}\})+";
+        private const string HolePattern = @"{(?<Placeholder>[^\}]*)}";
+        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<ParsingError> errors)
+        {
+            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(MessageTemplatesParser.Placeholder placeholder)
+        {
+            if (placeholder.Length == 0)
+            {
+                return new("should not contain empty placeholder", placeholder.Start - 1, EmptyPlaceholderSize);
+            }
+            var parts = Split(placeholder.Name);
+            return "🔥" switch
+            {
+                _ 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)
+        {
+            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 (formatIndex == -1)
+            {
+                formatIndex = placeholder.Length;
+            }
+            else
+            {
+                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);
+        }
+
+        private sealed record Parts(string Name, string Alignment, string Format);
+
+        public sealed record ParsingError(string Message, int Start, int Length)
+        {
+            public ParsingError(string message, MessageTemplatesParser.Placeholder placeholder)
+                : this(message, placeholder.Start, placeholder.Length)
+            { }
+        }
+    }
+}

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}

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)

analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs → analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplatesParser.cs

@@ -25,7 +25,7 @@ namespace SonarAnalyzer.Helpers;
 /// <summary>
 /// Grammar can be found <a href="https://github.com/messagetemplates/grammar"> here</a>.
 /// </summary>
-public static class MessageTemplates
+public static class MessageTemplatesParser
 {
     private const string NamePattern = "[0-9a-zA-Z_]+";
     private const string PlaceholderPattern = $"(?<Placeholder>{NamePattern})";
@@ -42,9 +42,12 @@ public static class MessageTemplates
     /// Matches and extracts placeholders from a template string.
     /// For more info, see <a href="https://messagetemplates.org/">Message Templates</a>.
     /// </summary>
-    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<Capture>()