New rule S6674: Log message template should be syntactically correct

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": [],
+  "defaultSeverity": "Critical",
+  "ruleSpecification": "RSPEC-6674",
+  "sqKey": "S6674",
+  "scope": "Main",
+  "quickfix": "infeasible"
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -292,6 +292,7 @@
     "S6618",
     "S6640",
     "S6667",
+    "S6674",
     "S6677",
     "S6678",
     "S6797",

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

@@ -51,7 +51,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")

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

@@ -0,0 +1,164 @@
+/*
+ * 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<DiagnosticDescriptor> 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 = @"{(?<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 = 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);
+            }
+
+            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)
+            {
+                // case like {name:format,alignment}
+                // where           ^^^^^^^^^^^^^^^^ 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); // TODO: check boundaries here+next line, for {User,:}
+                    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)
+            { }
+        }
+    }
+}

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 <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>()

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",

analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs

@@ -0,0 +1,114 @@
+/*
+ * 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<MetadataReference> LoggingReferences =
+        NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions()
+        .Concat(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion))
+        .Concat(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion));
+
+    private static readonly VerifierBuilder Builder = new VerifierBuilder<MessageTemplatesShouldBeCorrect>()
+        .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}}}("Login failed for {User", user);            // Noncompliant: Missing closing bracket
+                    logger.{{{methodName}}}("Login failed for {{User}", user);          // Noncompliant: Opening bracket is escaped
+                    logger.{{{methodName}}}("Login failed for {&User}", user);      // Noncompliant: Only '@' and '$' are allowed as prefix
+                    logger.{{{methodName}}}("Login failed for {User-Name}", user);      // Noncompliant: Only chars, numbers, and underscore are allowed for placeholders
+                    logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt);              // Noncompliant: The alignment specifier must be numeric
+                    logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt);               // Noncompliant: Empty format specifier is not allowed
+                }
+            }
+            """).Verify();
+
+    [DataTestMethod]
+    [DataRow("Debug")]
+    [DataRow("Error")]
+    [DataRow("Information")]
+    [DataRow("Fatal")]
+    [DataRow("Warning")]
+    [DataRow("Verbose")]
+    public void MessageTemplatesShouldBeCorrect_Serilog_CS(string methodName) =>
+        Builder.AddSnippet($$"""
+            using Serilog;
+            using Serilog.Events;
+
+            public class Program
+            {
+                public void Method(ILogger logger, int arg)
+                {
+                    logger.{{methodName}}("Arg: {Arg}", arg);       // Compliant
+                    logger.{{methodName}}("Arg: {arg}", arg);       // Noncompliant
+                }
+            }
+            """).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 NLog;
+
+            public class Program
+            {
+                public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, int arg)
+                {
+                    logger.{{methodName}}("Arg: {Arg}", arg);       // Compliant
+                    logger.{{methodName}}("Arg: {arg}", arg);       // Noncompliant
+                }
+            }
+            public class MyLogger : Logger { }
+            """).Verify();
+}