New rule S6668: Logging arguments should be passed to the correct par…

…ameter (#8800)

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

@@ -11,6 +11,12 @@
       "Message": "Remove the unused private field \u0027dateTimeOffset\u0027.",
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6588.cs#L8",
       "Location": "Line 8 Position 9-104"
+    },
+    {
+      "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/S6668.cs#L8",
+      "Location": "Line 8 Position 22-25"
     }
   ]
 }

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

@@ -11,6 +11,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/S6588.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/S6668.cs#L1",
+      "Location": "Line 1 Position 1-1"
     }
   ]
 }

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

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

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

@@ -0,0 +1,10 @@
+{
+  "Issues": [
+    {
+      "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/S6668.cs#L9",
+      "Location": "Line 9 Position 32-55"
+    }
+  ]
+}

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

@@ -0,0 +1,10 @@
+{
+  "Issues": [
+    {
+      "Id": "S6668",
+      "Message": "Logging arguments should be passed to the correct parameter.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6668.cs#L9",
+      "Location": "Line 9 Position 13-31"
+    }
+  ]
+}

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

@@ -0,0 +1,10 @@
+{
+  "Issues": [
+    {
+      "Id": "S6678",
+      "Message": "Use PascalCase for named placeholders.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6668.cs#L9",
+      "Location": "Line 9 Position 43-52"
+    }
+  ]
+}

analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/IntentionalFindings.csproj

@@ -4,6 +4,7 @@
         <TargetFramework>net8.0</TargetFramework>
     </PropertyGroup>
   <ItemGroup>
+    <PackageReference Include="Microsoft.Extensions.Logging" Version="8.0.0" />
     <PackageReference Include="System.Data.SqlClient" Version="4.8.6" />
     <PackageReference Include="Microsoft.EntityFrameworkCore" Version="2.1.0" />
   </ItemGroup>

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

@@ -0,0 +1,11 @@
+using System;
+using Microsoft.Extensions.Logging;
+
+namespace IntentionalFindings
+{
+    public class S6668
+    {
+        private void Log(ILogger logger, Exception exception) =>
+            logger.LogCritical("Expected {exception}.", exception); // Noncompliant (S6668) {{Logging arguments should be passed to the correct parameter.}}
+    }
+}

analyzers/rspec/cs/S6668.html

@@ -0,0 +1,42 @@
+<h2>Why is this an issue?</h2>
+<p>Most logging frameworks have methods that take a log level, an event ID or an exception as a separate input next to the log format and its
+arguments. There is a high chance that if the log level, the event ID or the exception are passed as the arguments to the message format, it was a
+mistake. This rule is going to raise in that scenario.</p>
+<p>The rule covers the following logging frameworks:</p>
+<ul>
+  <li> Nuget package - <a href="https://www.nuget.org/packages/Castle.Core">Castle.Core</a> </li>
+  <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>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+try { }
+catch (Exception ex)
+{
+    logger.LogDebug("An exception occured {Exception} with {EventId}.", ex, eventId); // Noncompliant
+}
+</pre>
+<pre data-diff-id="1" data-diff-type="compliant">
+try { }
+catch (Exception ex)
+{
+    logger.LogDebug(eventId, ex, "An exception occured.");
+}
+</pre>
+<h3>Exceptions</h3>
+<p>This rule will not raise an issue if one of the parameters mentioned above is passed twice, once as a separate argument to the invocation and once
+as an argument to the message format.</p>
+<pre>
+try { }
+catch (Exception ex)
+{
+    logger.LogDebug(ex, "An exception occured {Exception}.", ex); // Compliant
+}
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.loggerextensions">LoggerExtensions
+  Class</a> </li>
+</ul>
+

analyzers/rspec/cs/S6668.json

@@ -0,0 +1,17 @@
+{
+  "title": "Logging arguments should be passed to the correct parameter",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "logging"
+  ],
+  "defaultSeverity": "Minor",
+  "ruleSpecification": "RSPEC-6668",
+  "sqKey": "S6668",
+  "scope": "Main",
+  "quickfix": "targeted"
+}

analyzers/rspec/cs/Sonar_way_profile.json

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

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

@@ -0,0 +1,126 @@
+/*
+ * 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.CSharp;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class LoggingArgumentsShouldBePassedCorrectly : SonarDiagnosticAnalyzer
+{
+    private const string DiagnosticId = "S6668";
+    private const string MessageFormat = "Logging arguments should be passed to the correct parameter.";
+
+    private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
+
+    private static readonly ImmutableArray<KnownType> MicrosoftLoggingExtensionsInvalidTypes =
+        ImmutableArray.Create(KnownType.System_Exception, KnownType.Microsoft_Extensions_Logging_LogLevel, KnownType.Microsoft_Extensions_Logging_EventId);
+    private static readonly ImmutableArray<KnownType> CastleCoreInvalidTypes = ImmutableArray.Create(KnownType.System_Exception);
+    private static readonly ImmutableArray<KnownType> NLogAndSerilogInvalidTypes = ImmutableArray.Create(KnownType.System_Exception, KnownType.Serilog_Events_LogEventLevel, KnownType.NLog_LogLevel);
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterNodeAction(c =>
+            {
+                var invocation = (InvocationExpressionSyntax)c.Node;
+                if (!HasLoggingMethodName(invocation)
+                    || c.SemanticModel.GetSymbolInfo(invocation).Symbol is not IMethodSymbol invocationSymbol)
+                {
+                    return;
+                }
+                if (invocationSymbol.HasContainingType(KnownType.Microsoft_Extensions_Logging_LoggerExtensions, false))
+                {
+                    CheckInvalidParams(invocation, invocationSymbol, c, MicrosoftLoggingExtensionsInvalidTypes);
+                }
+                else if (invocationSymbol.HasContainingType(KnownType.Castle_Core_Logging_ILogger, true))
+                {
+                    CheckInvalidParams(invocation, invocationSymbol, c, CastleCoreInvalidTypes);
+                }
+                else if (invocationSymbol.HasContainingType(KnownType.Serilog_ILogger, true)
+                         || invocationSymbol.HasContainingType(KnownType.Serilog_Log, false)
+                         || invocationSymbol.HasContainingType(KnownType.NLog_ILoggerBase, true)
+                         || invocationSymbol.HasContainingType(KnownType.NLog_ILoggerExtensions, false))
+                {
+                    CheckInvalidParams(invocation, invocationSymbol, c, NLogAndSerilogInvalidTypes);
+                    CheckInvalidTypeParams(invocation, invocationSymbol, c, NLogAndSerilogInvalidTypes);
+                }
+            },
+            SyntaxKind.InvocationExpression);
+
+    private static void CheckInvalidParams(InvocationExpressionSyntax invocation, IMethodSymbol invocationSymbol, SonarSyntaxNodeReportingContext c, ImmutableArray<KnownType> knownTypes)
+    {
+        var paramsParameter = invocationSymbol.Parameters.FirstOrDefault(x => x.IsParams);
+        if (paramsParameter is null)
+        {
+            return;
+        }
+
+        var paramsIndex = invocationSymbol.Parameters.IndexOf(paramsParameter);
+        var invalidArguments = invocation.ArgumentList.Arguments
+            .Where(x => x.GetArgumentIndex() >= paramsIndex
+                        && IsInvalidArgument(x, c.SemanticModel, knownTypes))
+            .Select(x => x.GetLocation())
+            .ToArray();
+
+        if (invalidArguments.Length > 0)
+        {
+            c.ReportIssue(Diagnostic.Create(Rule, invocation.Expression.GetLocation(), (IEnumerable<Location>)invalidArguments));
+        }
+    }
+
+    private static void CheckInvalidTypeParams(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol, SonarSyntaxNodeReportingContext c, ImmutableArray<KnownType> knownTypes)
+    {
+        if (!IsNLogIgnoredOverload(methodSymbol) && methodSymbol.TypeArguments.Any(x => x.DerivesFromAny(knownTypes)))
+        {
+            var typeParameterNames = methodSymbol.TypeParameters.Select(x => x.MetadataName).ToArray();
+            var positions = methodSymbol.ConstructedFrom.Parameters.Where(x => typeParameterNames.Contains(x.Type.MetadataName)).Select(x => methodSymbol.ConstructedFrom.Parameters.IndexOf(x));
+            var invalidArguments = InvalidArguments(invocation, c.SemanticModel, positions, knownTypes).Select(x => x.GetLocation());
+            c.ReportIssue(Diagnostic.Create(Rule, invocation.Expression.GetLocation(), invalidArguments));
+        }
+    }
+
+    private static bool HasLoggingMethodName(InvocationExpressionSyntax invocation) =>
+        MicrosoftExtensionsLogging.Contains(invocation.GetName())
+        || NLogLoggingMethods.Contains(invocation.GetName())
+        || Serilog.Contains(invocation.GetName())
+        || CastleCoreOrCommonCore.Contains(invocation.GetName());
+
+    private static bool IsNLogIgnoredOverload(IMethodSymbol methodSymbol) =>
+        // These overloads are ignored since they will try to convert the T value to an exception.
+        MatchesParams(methodSymbol, KnownType.System_Exception)
+        || MatchesParams(methodSymbol, KnownType.System_IFormatProvider, KnownType.System_Exception)
+        || MatchesParams(methodSymbol, KnownType.NLog_ILogger, KnownType.System_Exception)
+        || MatchesParams(methodSymbol, KnownType.NLog_ILogger, KnownType.System_IFormatProvider, KnownType.System_Exception)
+        || MatchesParams(methodSymbol, KnownType.NLog_LogLevel, KnownType.System_Exception)
+        || MatchesParams(methodSymbol, KnownType.NLog_LogLevel, KnownType.System_IFormatProvider, KnownType.System_Exception);
+
+    private static bool MatchesParams(IMethodSymbol methodSymbol, params KnownType[] knownTypes) =>
+        methodSymbol.Parameters.Length == knownTypes.Length
+        && !methodSymbol.Parameters.Where((x, index) => !x.Type.DerivesFrom(knownTypes[index])).Any();
+
+    private static IEnumerable<ArgumentSyntax> InvalidArguments(InvocationExpressionSyntax invocation, SemanticModel model, IEnumerable<int> positionsToCheck, ImmutableArray<KnownType> knownTypes) =>
+        positionsToCheck
+            .Select(x => invocation.ArgumentList.Arguments[x])
+            .Where(x => IsInvalidArgument(x, model, knownTypes));
+
+    private static bool IsInvalidArgument(ArgumentSyntax argumentSyntax, SemanticModel model, ImmutableArray<KnownType> knownTypes) =>
+        model.GetTypeInfo(argumentSyntax.Expression).Type?.DerivesFromAny(knownTypes) is true;
+}

analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs

@@ -111,9 +111,11 @@ public sealed partial class KnownType
         public static readonly KnownType Microsoft_Extensions_Logging_DebugLoggerFactoryExtensions = new("Microsoft.Extensions.Logging.DebugLoggerFactoryExtensions");
         public static readonly KnownType Microsoft_Extensions_Logging_EventLoggerFactoryExtensions = new("Microsoft.Extensions.Logging.EventLoggerFactoryExtensions");
         public static readonly KnownType Microsoft_Extensions_Logging_EventSourceLoggerFactoryExtensions = new("Microsoft.Extensions.Logging.EventSourceLoggerFactoryExtensions");
+        public static readonly KnownType Microsoft_Extensions_Logging_EventId = new("Microsoft.Extensions.Logging.EventId");
         public static readonly KnownType Microsoft_Extensions_Logging_ILogger = new("Microsoft.Extensions.Logging.ILogger");
         public static readonly KnownType Microsoft_Extensions_Logging_ILoggerFactory = new("Microsoft.Extensions.Logging.ILoggerFactory");
         public static readonly KnownType Microsoft_Extensions_Logging_LoggerExtensions = new("Microsoft.Extensions.Logging.LoggerExtensions");
+        public static readonly KnownType Microsoft_Extensions_Logging_LogLevel = new("Microsoft.Extensions.Logging.LogLevel");
         public static readonly KnownType Microsoft_Extensions_Primitives_StringValues = new("Microsoft.Extensions.Primitives.StringValues");
         public static readonly KnownType Microsoft_Net_Http_Headers_HeaderNames = new("Microsoft.Net.Http.Headers.HeaderNames");
         public static readonly KnownType Microsoft_JSInterop_JSInvokable = new("Microsoft.JSInterop.JSInvokableAttribute");
@@ -152,8 +154,9 @@ public sealed partial class KnownType
         public static readonly KnownType NLog_ILogger = new("NLog.ILogger");
         public static readonly KnownType NLog_ILoggerBase = new("NLog.ILoggerBase");
         public static readonly KnownType NLog_ILoggerExtensions = new("NLog.ILoggerExtensions");
-        public static readonly KnownType NLog_Logger = new("NLog.Logger");
+        public static readonly KnownType NLog_LogLevel = new("NLog.LogLevel");
         public static readonly KnownType NLog_LogManager = new("NLog.LogManager");
+        public static readonly KnownType NLog_Logger = new("NLog.Logger");
         public static readonly KnownType NUnit_Framework_Assert = new("NUnit.Framework.Assert");
         public static readonly KnownType NUnit_Framework_AssertionException = new("NUnit.Framework.AssertionException");
         public static readonly KnownType NUnit_Framework_ExpectedExceptionAttribute = new("NUnit.Framework.ExpectedExceptionAttribute");
@@ -173,6 +176,7 @@ public sealed partial class KnownType
         public static readonly KnownType Org_BouncyCastle_Crypto_Generators_DHParametersGenerator = new("Org.BouncyCastle.Crypto.Generators.DHParametersGenerator");
         public static readonly KnownType Org_BouncyCastle_Crypto_Generators_DsaParametersGenerator = new("Org.BouncyCastle.Crypto.Generators.DsaParametersGenerator");
         public static readonly KnownType Org_BouncyCastle_Crypto_Parameters_RsaKeyGenerationParameters = new("Org.BouncyCastle.Crypto.Parameters.RsaKeyGenerationParameters");
+        public static readonly KnownType Serilog_Events_LogEventLevel = new("Serilog.Events.LogEventLevel");
         public static readonly KnownType Serilog_ILogger = new("Serilog.ILogger");
         public static readonly KnownType Serilog_LoggerConfiguration = new("Serilog.LoggerConfiguration");
         public static readonly KnownType Serilog_Log = new("Serilog.Log");

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

@@ -6592,7 +6592,7 @@ internal static class RuleTypeMappingCS
         // ["S6665"],
         // ["S6666"],
         ["S6667"] = "CODE_SMELL",
-        // ["S6668"],
+        ["S6668"] = "CODE_SMELL",
         // ["S6669"],
         // ["S6670"],
         // ["S6671"],