New rule S6673: Log message template placeholders should be in the right order

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S103-IntentionalFindings-net8.0.json

@@ -0,0 +1,10 @@
+{
+  "Issues": [
+    {
+      "Id": "S103",
+      "Message": "Split this 219 characters long line (which is greater than 200 authorized).",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L9",
+      "Location": "Line 9 Position 1-220"
+    }
+  ]
+}

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1128-IntentionalFindings-net8.0.json

@@ -11,6 +11,12 @@
       "Message": "Remove this unnecessary \u0027using\u0027.",
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6670.cs#L1",
       "Location": "Line 1 Position 1-14"
+    },
+    {
+      "Id": "S1128",
+      "Message": "Remove this unnecessary \u0027using\u0027.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L1",
+      "Location": "Line 1 Position 1-14"
     }
   ]
 }

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1144-IntentionalFindings-net8.0.json

@@ -35,6 +35,12 @@
       "Message": "Remove the unused private field \u0027_log2\u0027.",
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6669.cs#L9",
       "Location": "Line 9 Position 9-36"
+    },
+    {
+      "Id": "S1144",
+      "Message": "Remove the unused private method \u0027Log\u0027.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L8",
+      "Location": "Line 8 Position 22-25"
     }
   ]
 }

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json

@@ -35,6 +35,12 @@
       "Message": "Add or update the header of this file.",
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6672.cs#L1",
       "Location": "Line 1 Position 1-1"
+    },
+    {
+      "Id": "S1451",
+      "Message": "Add or update the header of this file.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L1",
+      "Location": "Line 1 Position 1-1"
     }
   ]
 }

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json

@@ -17,6 +17,12 @@
       "Message": "Make \u0027Method\u0027 a static method.",
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6670.cs#L8",
       "Location": "Line 8 Position 21-27"
+    },
+    {
+      "Id": "S2325",
+      "Message": "Make \u0027Log\u0027 a static method.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L8",
+      "Location": "Line 8 Position 22-25"
     }
   ]
 }

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S4055-IntentionalFindings-net8.0.json

@@ -11,6 +11,12 @@
       "Message": "Replace this string literal with a string retrieved through an instance of the \u0027ResourceManager\u0027 class.",
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6670.cs#L10",
       "Location": "Line 10 Position 25-34"
+    },
+    {
+      "Id": "S4055",
+      "Message": "Replace this string literal with a string retrieved through an instance of the \u0027ResourceManager\u0027 class.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L9",
+      "Location": "Line 9 Position 35-60"
     }
   ]
 }

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6673-IntentionalFindings-net8.0.json

@@ -0,0 +1,10 @@
+{
+  "Issues": [
+    {
+      "Id": "S6673",
+      "Message": "Template placeholders should be in the right order: placeholder \u0027Arg\u0027 does not match with argument \u0027anotherArg\u0027.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L9",
+      "Location": "Line 9 Position 42-45"
+    }
+  ]
+}

analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs

@@ -0,0 +1,11 @@
+using System;
+using Microsoft.Extensions.Logging;
+
+namespace IntentionalFindings
+{
+    public class S6673
+    {
+        private void Log(ILogger logger, int arg, int anotherArg) =>
+            logger.LogInformation("Arg: {Arg} {AnotherArg}", anotherArg, arg); // Noncompliant (S6673) {{Template placeholders should be in the right order: placeholder 'Arg' does not match with argument 'anotherArg'.}}
+    }
+}

analyzers/rspec/cs/S6673.html

@@ -0,0 +1,68 @@
+<p>The positions of arguments in a logging call should match the positions of their <a href="https://messagetemplates.org">message template</a>
+placeholders.</p>
+<h2>Why is this an issue?</h2>
+<p>The placeholders of a <a href="https://messagetemplates.org">message template</a> are defined by their name and their position. Log methods specify
+the values for the placeholder at runtime by passing them in a <a
+href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/params">params array</a>:</p>
+<pre data-diff-id="1" data-diff-type="compliant">
+logger.LogError("{First} placeholder and {Second} one.", first, second);
+</pre>
+<p>This rule raises an issue if the position of an argument does not match the position of the corresponding placeholder:</p>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+// 'first' and 'second' are swapped
+logger.LogError("{First} placeholder and {Second} one.", second, first);
+//                                                       ^^^^^^  ^^^^^
+</pre>
+<h3>What is the potential impact?</h3>
+<p>Logging providers use placeholder names to create key/value pairs in the log entry. The key corresponds to the placeholder and the value is the
+argument passed in the log call.</p>
+<p>If the positions of the placeholder and the argument do not match, the value is associated with the wrong key. This corrupts the logs entry and
+makes log analytics unreliable.</p>
+<h2>How to fix it</h2>
+<p>Make sure that the placeholder positions and the argument positions match. Use local variables, fields, or properties for the arguments and name
+the placeholders accordingly.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<p>'path' and 'fileName' are swapped and therefore assigned to the wrong placeholders.</p>
+<pre data-diff-id="2" data-diff-type="noncompliant">
+logger.LogError("File {FileName} not found in folder {Path}", path, fileName);
+//                                                            ^^^^  ^^^^^^^^
+</pre>
+<h4>Compliant solution</h4>
+<p>Swap the arguments.</p>
+<pre data-diff-id="2" data-diff-type="compliant">
+logger.LogError("File {FileName} not found in folder {Path}", fileName, path);
+</pre>
+<h4>Noncompliant code example</h4>
+<p>'Name' is detected but 'Folder' is not. The placeholder’s name should correspond to the name from the argument.</p>
+<pre data-diff-id="3" data-diff-type="noncompliant">
+logger.LogError("File {Name} not found in folder {Folder}", file.DirectoryName, file.Name);
+//                                                                                   ^^^^
+</pre>
+<h4>Compliant solution</h4>
+<p>Swap the arguments and rename the placeholder to 'DirectoryName'.</p>
+<pre data-diff-id="3" data-diff-type="compliant">
+logger.LogError("File {Name} not found in folder {DirectoryName}", file.Name, file.DirectoryName);
+</pre>
+<h4>Noncompliant code example</h4>
+<p>Not detected: A name for the arguments can not be inferred. Use locals to support detection.</p>
+<pre data-diff-id="4" data-diff-type="noncompliant">
+logger.LogError("Sum is {Sum} and product is {Product}", x * y, x + y); // Not detected
+</pre>
+<h4>Compliant solution</h4>
+<p>Help detection by using arguments with a name.</p>
+<pre data-diff-id="4" data-diff-type="compliant">
+var sum = x + y;
+var product = x * y;
+logger.LogError("Sum is {Sum} and product is {Product}", sum, product);
+</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">Log message
+  template</a> </li>
+  <li> Serilog - <a href="https://github.com/serilog/serilog/wiki/Structured-Data">Structured Data</a> </li>
+  <li> NLog - <a href="https://github.com/NLog/NLog/wiki/How-to-use-structured-logging">How to use structured logging</a> </li>
+</ul>
+

analyzers/rspec/cs/S6673.json

@@ -0,0 +1,17 @@
+{
+  "title": "Log message template placeholders should be in the right order",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "logging"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6673",
+  "sqKey": "S6673",
+  "scope": "Main",
+  "quickfix": "infeasible"
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -300,9 +300,10 @@
     "S6640",
     "S6667",
     "S6668",
+    "S6669",
     "S6670",
     "S6672",
-    "S6669",
+    "S6673",
     "S6674",
     "S6677",
     "S6678",

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

@@ -0,0 +1,102 @@
+/*
+ * 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.
+ */
+
+namespace SonarAnalyzer.Rules.MessageTemplates;
+
+public sealed class LoggingTemplatePlaceHoldersShouldBeInOrder : IMessageTemplateCheck
+{
+    private const string DiagnosticId = "S6673";
+    private const string MessageFormat = "Template placeholders should be in the right order: placeholder '{0}' does not match with argument '{1}'.";
+
+    internal static readonly DiagnosticDescriptor S6673 = DescriptorFactory.Create(DiagnosticId, MessageFormat);
+
+    public DiagnosticDescriptor Rule => S6673;
+
+    public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, MessageTemplatesParser.Placeholder[] placeholders)
+    {
+        var methodSymbol = (IMethodSymbol)context.SemanticModel.GetSymbolInfo(invocation).Symbol;
+        var placeholderValues = PlaceholderValues(invocation, methodSymbol).ToImmutableArray();
+        for (var i = 0; i < placeholders.Length; i++)
+        {
+            var placeholder = placeholders[i];
+            if (placeholder.Name != "_"
+                && !int.TryParse(placeholder.Name, out _)
+                && Array.FindIndex(placeholders, x => x.Name == placeholder.Name) == i // don't raise for duplicate placeholders
+                && OutOfOrderPlaceholderValue(placeholder, i, placeholderValues) is { } outOfOrderArgument)
+            {
+                var templateStart = templateArgument.Expression.GetLocation().SourceSpan.Start;
+                var primaryLocation = Location.Create(context.Tree, new(templateStart + placeholder.Start, placeholder.Length));
+                var secondaryLocation = outOfOrderArgument.GetLocation();
+                context.ReportIssue(Diagnostic.Create(Rule, primaryLocation, [secondaryLocation], placeholder.Name, outOfOrderArgument.ToString()));
+                return; // only raise on the first out-of-order placeholder to make the rule less noisy
+            }
+        }
+    }
+
+    private static IEnumerable<SyntaxNode> PlaceholderValues(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol)
+    {
+        var parameters = methodSymbol.Parameters.Where(x => x.Name == "args"
+            || x.Name.StartsWith("argument")
+            || x.Name.StartsWith("propertyValue"))
+            .ToArray();
+        if (parameters.Length == 0)
+        {
+            yield break;
+        }
+        var parameterLookup = CSharpFacade.Instance.MethodParameterLookup(invocation, methodSymbol);
+        foreach (var parameter in parameters)
+        {
+            if (parameterLookup.TryGetSyntax(parameter, out var expressions))
+            {
+                foreach (var item in expressions)
+                {
+                    yield return item;
+                }
+            }
+        }
+    }
+
+    private static SyntaxNode OutOfOrderPlaceholderValue(MessageTemplatesParser.Placeholder placeholder, int placeholderIndex, ImmutableArray<SyntaxNode> placeholderValues)
+    {
+        if (placeholderIndex < placeholderValues.Length && MatchesName(placeholder.Name, placeholderValues[placeholderIndex]))
+        {
+            return null;
+        }
+        else
+        {
+            for (var i = 0; i < placeholderValues.Length; i++)
+            {
+                if (i != placeholderIndex && MatchesName(placeholder.Name, placeholderValues[i]))
+                {
+                    return placeholderValues[placeholderIndex];
+                }
+            }
+        }
+        return null;
+    }
+
+    private static bool MatchesName(string placeholderName, SyntaxNode placeholderValue) =>
+        placeholderValue switch
+        {
+            MemberAccessExpressionSyntax memberAccess => MatchesName(placeholderName, memberAccess.Name) || MatchesName(placeholderName, memberAccess.Expression),
+            ObjectCreationExpressionSyntax => false,
+            _ => new string(placeholderName.Where(char.IsLetterOrDigit).ToArray()).Equals(placeholderValue.ToString(), StringComparison.OrdinalIgnoreCase)
+        };
+}

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

@@ -32,29 +32,36 @@ public sealed class MessageTemplateAnalyzer : SonarDiagnosticAnalyzer
         SyntaxKind.InterpolatedStringText);
 
     private static readonly ImmutableHashSet<IMessageTemplateCheck> Checks = ImmutableHashSet.Create<IMessageTemplateCheck>(
+               new LoggingTemplatePlaceHoldersShouldBeInOrder(),
                new NamedPlaceholdersShouldBeUnique(),
                new UsePascalCaseForNamedPlaceHolders());
 
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
         Checks.Select(x => x.Rule).ToImmutableArray();
 
     protected override void Initialize(SonarAnalysisContext context) =>
-        context.RegisterNodeAction(c =>
+        context.RegisterCompilationStartAction(cc =>
         {
-            var invocation = (InvocationExpressionSyntax)c.Node;
-            var enabledChecks = Checks.Where(x => x.Rule.IsEnabled(c)).ToArray();
-            if (enabledChecks.Length > 0
-                && MessageTemplateExtractor.TemplateArgument(invocation, c.SemanticModel) is { } argument
-                && HasValidExpression(argument)
-                && MessageTemplatesParser.Parse(argument.Expression.ToString()) is { Success: true } result)
+            if (cc.Compilation.ReferencesAny(KnownAssembly.MicrosoftExtensionsLoggingAbstractions, KnownAssembly.Serilog, KnownAssembly.NLog))
             {
-                foreach (var check in enabledChecks)
+                cc.RegisterNodeAction(c =>
                 {
-                    check.Execute(c, invocation, argument, result.Placeholders);
-                }
+                    var invocation = (InvocationExpressionSyntax)c.Node;
+                    var enabledChecks = Checks.Where(x => x.Rule.IsEnabled(c)).ToArray();
+                    if (enabledChecks.Length > 0
+                        && MessageTemplateExtractor.TemplateArgument(invocation, c.SemanticModel) is { } argument
+                        && HasValidExpression(argument)
+                        && MessageTemplatesParser.Parse(argument.Expression.ToString()) is { Success: true } result)
+                    {
+                        foreach (var check in enabledChecks)
+                        {
+                            check.Execute(c, invocation, argument, result.Placeholders);
+                        }
+                    }
+                },
+                SyntaxKind.InvocationExpression);
             }
-        },
-        SyntaxKind.InvocationExpression);
+        });
 
     private static bool HasValidExpression(ArgumentSyntax argument) =>
         argument.Expression.DescendantNodes().All(x => x.IsAnyKind(ValidTemplateKinds));

analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs

@@ -6597,7 +6597,7 @@ internal static class RuleTypeMappingCS
         ["S6669"] = "CODE_SMELL",
         // ["S6671"],
         ["S6672"] = "CODE_SMELL",
-        // ["S6673"],
+        ["S6673"] = "CODE_SMELL",
         ["S6674"] = "BUG",
         // ["S6675"],
         // ["S6676"],