From 4fe2a8b557ec1c220db869151d156369ba4a2edd Mon Sep 17 00:00:00 2001 From: Costin Zaharia <56015273+costin-zaharia-sonarsource@users.noreply.github.com> Date: Thu, 29 Feb 2024 11:11:41 +0100 Subject: [PATCH] New rule S6668: Logging arguments should be passed to the correct parameter (#8800) --- .../S1144-IntentionalFindings-net8.0.json | 6 + .../S1451-IntentionalFindings-net8.0.json | 6 + .../S2325-IntentionalFindings-net8.0.json | 6 + .../S4055-IntentionalFindings-net8.0.json | 10 + .../S6668-IntentionalFindings-net8.0.json | 10 + .../S6678-IntentionalFindings-net8.0.json | 10 + .../IntentionalFindings.csproj | 1 + .../IntentionalFindings/S6668.cs | 11 + analyzers/rspec/cs/S6668.html | 42 +++ analyzers/rspec/cs/S6668.json | 17 + analyzers/rspec/cs/Sonar_way_profile.json | 1 + ...LoggingArgumentsShouldBePassedCorrectly.cs | 126 +++++++ .../SonarAnalyzer.Common/Helpers/KnownType.cs | 6 +- .../PackagingTests/RuleTypeMappingCS.cs | 2 +- ...ingArgumentsShouldBePassedCorrectlyTest.cs | 308 ++++++++++++++++++ ...LoggingArgumentsShouldBePassedCorrectly.cs | 39 +++ 16 files changed, 599 insertions(+), 2 deletions(-) create mode 100644 analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S4055-IntentionalFindings-net8.0.json create mode 100644 analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6668-IntentionalFindings-net8.0.json create mode 100644 analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6678-IntentionalFindings-net8.0.json create mode 100644 analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6668.cs create mode 100644 analyzers/rspec/cs/S6668.html create mode 100644 analyzers/rspec/cs/S6668.json create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Rules/LoggingArgumentsShouldBePassedCorrectly.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/Rules/LoggingArgumentsShouldBePassedCorrectlyTest.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/TestCases/LoggingArgumentsShouldBePassedCorrectly.cs diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1144-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1144-IntentionalFindings-net8.0.json index 65debb65010..ddbbe21554e 100644 --- a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1144-IntentionalFindings-net8.0.json +++ b/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" } ] } \ No newline at end of file diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json index 7d5ca9702f3..1158b9d53f1 100644 --- a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json +++ b/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" } ] } \ No newline at end of file diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json index a523a08be2a..7c078caf048 100644 --- a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S2325-IntentionalFindings-net8.0.json +++ b/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" } ] } \ No newline at end of file diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S4055-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S4055-IntentionalFindings-net8.0.json new file mode 100644 index 00000000000..ad98030de46 --- /dev/null +++ b/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" + } + ] +} \ No newline at end of file diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6668-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6668-IntentionalFindings-net8.0.json new file mode 100644 index 00000000000..f0715a16c8b --- /dev/null +++ b/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" + } + ] +} \ No newline at end of file diff --git a/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6678-IntentionalFindings-net8.0.json b/analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6678-IntentionalFindings-net8.0.json new file mode 100644 index 00000000000..274792062ea --- /dev/null +++ b/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" + } + ] +} \ No newline at end of file diff --git a/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/IntentionalFindings.csproj b/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/IntentionalFindings.csproj index eee7ea6aff5..cc3e806fd5a 100644 --- a/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/IntentionalFindings.csproj +++ b/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/IntentionalFindings.csproj @@ -4,6 +4,7 @@ net8.0 + diff --git a/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6668.cs b/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6668.cs new file mode 100644 index 00000000000..e64e4b02535 --- /dev/null +++ b/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.}} + } +} diff --git a/analyzers/rspec/cs/S6668.html b/analyzers/rspec/cs/S6668.html new file mode 100644 index 00000000000..4fa33d7d17e --- /dev/null +++ b/analyzers/rspec/cs/S6668.html @@ -0,0 +1,42 @@ +

Why is this an issue?

+

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.

+

The rule covers the following logging frameworks:

+ +
+try { }
+catch (Exception ex)
+{
+    logger.LogDebug("An exception occured {Exception} with {EventId}.", ex, eventId); // Noncompliant
+}
+
+
+try { }
+catch (Exception ex)
+{
+    logger.LogDebug(eventId, ex, "An exception occured.");
+}
+
+

Exceptions

+

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.

+
+try { }
+catch (Exception ex)
+{
+    logger.LogDebug(ex, "An exception occured {Exception}.", ex); // Compliant
+}
+
+

Resources

+

Documentation

+ + diff --git a/analyzers/rspec/cs/S6668.json b/analyzers/rspec/cs/S6668.json new file mode 100644 index 00000000000..b0abe537c91 --- /dev/null +++ b/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" +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index 9efd2194c57..ca5cc1fd588 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -292,6 +292,7 @@ "S6618", "S6640", "S6667", + "S6668", "S6677", "S6678", "S6797", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LoggingArgumentsShouldBePassedCorrectly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LoggingArgumentsShouldBePassedCorrectly.cs new file mode 100644 index 00000000000..3fc2eca4cd7 --- /dev/null +++ b/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 MicrosoftLoggingExtensionsInvalidTypes = + ImmutableArray.Create(KnownType.System_Exception, KnownType.Microsoft_Extensions_Logging_LogLevel, KnownType.Microsoft_Extensions_Logging_EventId); + private static readonly ImmutableArray CastleCoreInvalidTypes = ImmutableArray.Create(KnownType.System_Exception); + private static readonly ImmutableArray NLogAndSerilogInvalidTypes = ImmutableArray.Create(KnownType.System_Exception, KnownType.Serilog_Events_LogEventLevel, KnownType.NLog_LogLevel); + + public override ImmutableArray 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 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)invalidArguments)); + } + } + + private static void CheckInvalidTypeParams(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol, SonarSyntaxNodeReportingContext c, ImmutableArray 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 InvalidArguments(InvocationExpressionSyntax invocation, SemanticModel model, IEnumerable positionsToCheck, ImmutableArray knownTypes) => + positionsToCheck + .Select(x => invocation.ArgumentList.Arguments[x]) + .Where(x => IsInvalidArgument(x, model, knownTypes)); + + private static bool IsInvalidArgument(ArgumentSyntax argumentSyntax, SemanticModel model, ImmutableArray knownTypes) => + model.GetTypeInfo(argumentSyntax.Expression).Type?.DerivesFromAny(knownTypes) is true; +} diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs index 086a39ad841..c543706dd90 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs +++ b/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"); diff --git a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs index 6ffc1e2bda8..8278d29005e 100644 --- a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs +++ b/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"], diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/LoggingArgumentsShouldBePassedCorrectlyTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/LoggingArgumentsShouldBePassedCorrectlyTest.cs new file mode 100644 index 00000000000..dd363f58cb9 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/LoggingArgumentsShouldBePassedCorrectlyTest.cs @@ -0,0 +1,308 @@ +/* + * 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 LoggingArgumentsShouldBePassedCorrectlyTest +{ + private readonly VerifierBuilder builder = new VerifierBuilder(); + + [TestMethod] + public void LoggingArgumentsShouldBePassedCorrectly_CS() => + builder.AddPaths("LoggingArgumentsShouldBePassedCorrectly.cs").AddReferences(NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions()).Verify(); + + [DataTestMethod] + [DataRow("Log", "LogLevel.Warning,")] + [DataRow("LogCritical")] + [DataRow("LogDebug")] + [DataRow("LogError")] + [DataRow("LogInformation")] + [DataRow("LogTrace")] + [DataRow("LogWarning")] + public void LoggingArgumentsShouldBePassedCorrectly_MicrosoftExtensionsLogging_NonCompliant_CS(string methodName, string logLevel = "") => + builder.AddSnippet($$""" + using System; + using Microsoft.Extensions.Logging; + using Microsoft.Extensions.Logging.Abstractions; + + public class Program + { + public void Method(ILogger logger, Exception e) + { + logger.{{methodName}}({{logLevel}} "Expected exception."); + logger.{{methodName}}({{logLevel}} e, "Expected exception."); + logger.{{methodName}}({{logLevel}} new EventId(), "Expected exception."); + logger.{{methodName}}({{logLevel}} new EventId(), e, "Expected exception."); + LoggerExtensions.{{methodName}}(logger, {{logLevel}} "Expected exception."); + LoggerExtensions.{{methodName}}(logger, {{logLevel}} new EventId(), e, "Expected exception."); + + logger.{{methodName}}({{logLevel}} new EventId(), e, "Expected exception.", e, new EventId(), LogLevel.Critical); // Noncompliant (exception, event id, log level) + // Secondary @-1 + // Secondary @-2 + // Secondary @-3 + logger.{{methodName}}({{logLevel}} new EventId(), "Expected exception.", e, new EventId(), LogLevel.Critical); // Noncompliant (exception, event id, log level) + // Secondary @-1 + // Secondary @-2 + // Secondary @-3 + logger.{{methodName}}({{logLevel}} e, "Expected exception.", e, new EventId(), LogLevel.Critical); // Noncompliant (event id, log level) + // Secondary @-1 + // Secondary @-2 + // Secondary @-3 + logger.{{methodName}}({{logLevel}} "Expected exception.", e, new EventId(), LogLevel.Critical); // Noncompliant (exception, event id, log level) + // Secondary @-1 + // Secondary @-2 + // Secondary @-3 + LoggerExtensions.{{methodName}}(logger, {{logLevel}} "Expected exception.", e, new EventId(), LogLevel.Critical); // Noncompliant (exception, event id, log level) + // Secondary @-1 + // Secondary @-2 + // Secondary @-3 + LoggerExtensions.{{methodName}}(logger, {{logLevel}} new EventId(), e, "Expected exception.", e, new EventId(), LogLevel.Critical); // Noncompliant (event id, log level) + // Secondary @-1 + // Secondary @-2 + // Secondary @-3 + } + } + """) + .AddReferences(NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions()) + .Verify(); + + [DataTestMethod] + [DataRow("DebugFormat")] + [DataRow("ErrorFormat")] + [DataRow("FatalFormat")] + [DataRow("InfoFormat")] + [DataRow("TraceFormat")] + [DataRow("WarnFormat")] + public void LoggingArgumentsShouldBePassedCorrectly_CastleCore_CS(string methodName) => + builder.AddSnippet($$""" + using System; + using System.Globalization; + using Castle.Core.Logging; + + public class Program + { + public void Method(ILogger logger, string message, Exception e) + { + logger.{{methodName}}(message); // Compliant + logger.{{methodName}}(message, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}(CultureInfo.CurrentCulture, message); // Compliant + logger.{{methodName}}(CultureInfo.CurrentCulture, message, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}(e, message); // Compliant + logger.{{methodName}}(e, message, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}(e, CultureInfo.CurrentCulture, message); // Compliant + logger.{{methodName}}(e, CultureInfo.CurrentCulture, message, e); // Noncompliant + // Secondary @-1 + } + } + """) + .AddReferences(NuGetMetadataReference.CastleCore()) + .Verify(); + + [DataTestMethod] + [DataRow("Log", "LogLevel.Debug,")] + [DataRow("Debug")] + [DataRow("ConditionalDebug")] + [DataRow("Error")] + [DataRow("Fatal")] + [DataRow("Info")] + [DataRow("Trace")] + [DataRow("ConditionalTrace")] + [DataRow("Warn")] + public void LoggingArgumentsShouldBePassedCorrectly_NLog_CS(string methodName, string logLevel = "") => + builder.AddSnippet($$""" + using System; + using System.Globalization; + using NLog; + + public class Program + { + public void Method(Logger logger, Exception e) + { + logger.{{methodName}}({{logLevel}} e); // Compliant + logger.{{methodName}}({{logLevel}} CultureInfo.CurrentCulture, e); // Compliant + logger.{{methodName}}({{logLevel}} e, "Message!"); // Compliant + logger.{{methodName}}({{logLevel}} e, "Message!", e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}}e, CultureInfo.CurrentCulture, "Message!", e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}}CultureInfo.CurrentCulture, "Message!", 1, 2, 3, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}} "Message!"); // Compliant + logger.{{methodName}}({{logLevel}} "Message!", 1, 2, 3, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}} "Message!", e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}} CultureInfo.CurrentCulture, "Message!", e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}} "Message!", 1); // Compliant + logger.{{methodName}}({{logLevel}} "Message!", e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}}CultureInfo.CurrentCulture, "Message!", 1, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}} "Message!", 1, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}} CultureInfo.CurrentCulture, "Message!", 1, 2, e); // Noncompliant + // Secondary @-1 + logger.{{methodName}}({{logLevel}} "Message!", 1, LogLevel.Debug, e); // Noncompliant + // Secondary @-1 + // Secondary @-2 + ILoggerExtensions.{{methodName}}(logger, {{logLevel}} e, null); // Compliant + } + } + """) + .AddReferences(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion)) + .Verify(); + + [DataTestMethod] + [DataRow("ConditionalDebug")] + [DataRow("ConditionalTrace")] + public void LoggingArgumentsShouldBePassedCorrectly_NLog_ConditionalExtensions_CS(string methodName) => + builder.AddSnippet($$""" + using System; + using System.Globalization; + using NLog; + + public class Program + { + public void Method(Logger logger, Exception e) + { + ILoggerExtensions.{{methodName}}(logger, e); // Compliant + ILoggerExtensions.{{methodName}}(logger, CultureInfo.CurrentCulture, e); // Compliant + ILoggerExtensions.{{methodName}}(logger, e, "Message!"); // Compliant + ILoggerExtensions.{{methodName}}(logger, e, "Message!", e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, e, CultureInfo.CurrentCulture, "Message!", e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, CultureInfo.CurrentCulture, "Message!", 1, 2, 3, e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, "Message!"); // Compliant + ILoggerExtensions.{{methodName}}(logger, "Message!", 1, 2, 3, e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, "Message!", e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, CultureInfo.CurrentCulture, "Message!", e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, "Message!", 1); // Compliant + ILoggerExtensions.{{methodName}}(logger, "Message!", e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, CultureInfo.CurrentCulture, "Message!", 1, e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, "Message!", 1, e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, CultureInfo.CurrentCulture, "Message!", 1, 2, e); // Noncompliant + // Secondary @-1 + ILoggerExtensions.{{methodName}}(logger, "Message!", 1, 2, e); // Noncompliant + // Secondary @-1 + } + } + """) + .AddReferences(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion)) + .Verify(); + + [DataTestMethod] + [DataRow("Debug")] + [DataRow("Error")] + [DataRow("Fatal")] + [DataRow("Information")] + [DataRow("Verbose")] + [DataRow("Warning")] + public void LoggingArgumentsShouldBePassedCorrectly_Serilog_CS(string methodName) => + builder.AddSnippet($$""" + using System; + using Serilog; + + public class Program + { + public void Method(ILogger logger, string message, Exception e) + { + Log.{{methodName}}("Message!"); + Log.{{methodName}}("Message!", e); // Noncompliant + // Secondary @-1 + Log.{{methodName}}("Message!", 1); + Log.{{methodName}}("Message!", e); // Noncompliant + // Secondary @-1 + Log.{{methodName}}("Message!", 1, e); // Noncompliant + // Secondary @-1 + Log.{{methodName}}("Message!", 1, e, true); // Noncompliant + // Secondary @-1 + Log.{{methodName}}("Message!", 1, 2, 3, e, true); // Noncompliant + // Secondary @-1 + Log.{{methodName}}(e, "Message"); + Log.{{methodName}}(e, "Message", e); // Noncompliant + // Secondary @-1 + Log.{{methodName}}(e, "Message", 1, e); // Noncompliant + // Secondary @-1 + Log.{{methodName}}(e, "Message", 1, e, true); // Noncompliant + // Secondary @-1 + Log.{{methodName}}(e, "Message!", 1, 2, 3, e, true); // Noncompliant + // Secondary @-1 + } + } + """) + .AddReferences(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion)) + .Verify(); + + [TestMethod] + public void LoggingArgumentsShouldBePassedCorrectly_Serilog_Write_CS() => + builder.AddSnippet(""" + using System; + using Serilog; + using Serilog.Events; + + public class Program + { + public void Method(LogEvent logEvent, LogEventLevel level, Exception exception) + { + Log.Write(logEvent); + Log.Write(level, "Message"); + Log.Write(level, "Message", level); // Noncompliant + // Secondary @-1 + Log.Write(level, "Message", 1); + Log.Write(level, "Message", level, 1); // Noncompliant + // Secondary @-1 + Log.Write(level, "Message", 1, exception, 2); // Noncompliant + // Secondary @-1 + Log.Write(level, "Message", level, exception, 1, 2); // Noncompliant + // Secondary @-1 + // Secondary @-2 + Log.Write(level, exception, "Message"); + Log.Write(level, exception, "Message", level); // Noncompliant + // Secondary @-1 + Log.Write(level, exception, "Message", level, exception); // Noncompliant + // Secondary @-1 + // Secondary @-2 + Log.Write(level, exception, "Message", level, exception, 1); // Noncompliant + // Secondary @-1 + // Secondary @-2 + Log.Write(level, exception, "Message", level, exception, 1, 2); // Noncompliant + // Secondary @-1 + // Secondary @-2 + } + } + """) + .AddReferences(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion)) + .Verify(); +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/LoggingArgumentsShouldBePassedCorrectly.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/LoggingArgumentsShouldBePassedCorrectly.cs new file mode 100644 index 00000000000..1ae2177c8b5 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/LoggingArgumentsShouldBePassedCorrectly.cs @@ -0,0 +1,39 @@ +using System; +using Microsoft.Extensions.Logging; + +public class Program +{ + private void Log(ILogger logger, Exception e) + { + LogWarning(null, null); // Error [CS0103] The name 'LogWarning' does not exist in the current context + + logger.LogCritical("Expected exception.", e); +// ^^^^^^^^^^^^^^^^^^ {{Logging arguments should be passed to the correct parameter.}} +// ^ Secondary @-1 + logger.LogWarning(new EventId(1), e.InnerException, "Message!"); + logger.LogWarning(new EventId(1), e?.InnerException, "Message!"); + logger.LogWarning(new EventId(1), e.InnerException.InnerException, "Message!"); + logger.LogWarning(new EventId(1), (Exception)e.InnerException, "Message!"); + logger.LogCritical(args: new object[] { e, LogLevel.Critical }, message: "Expected exception."); // FN + logger.LogCritical(message: "Expected exception.", eventId: new EventId(42), exception: new Exception(), args: e); +// ^^^^^^^^^^^^^^^^^^ +// ^^^^^^^ Secondary@-1 + logger.LogCritical("Expected exception.", new NullReferenceException()); // Noncompliant + // Secondary@-1 + } + + private void Log(CustomLogger logger, Exception e) + { + logger.LogCritical("Expected exception.", e, LogLevel.Critical); +// ^^^^^^^^^^^^^^^^^^ +// ^ Secondary @-1 +// ^^^^^^^^^^^^^^^^^ Secondary @-2 + } + + private class CustomLogger : ILogger + { + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) { } + public bool IsEnabled(LogLevel logLevel) => false; + public IDisposable BeginScope(TState state) => null; + } +}