Skip to content

Commit

Permalink
Rule S3416: include constructors and reduce FPs (#8941)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gregory-paidis-sonarsource
gregory-paidis-sonarsource authored Mar 19, 2024
1 parent 6c6eb0b commit 0418ca4
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 24 deletions.
30 changes: 23 additions & 7 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/LoggersShouldBeNamedForEnclosingType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ public sealed class LoggersShouldBeNamedForEnclosingType : SonarDiagnosticAnalyz
KnownAssembly.Log4Net,
];

private static readonly ImmutableArray<KnownType> Loggers = ImmutableArray.Create(
KnownType.Microsoft_Extensions_Logging_ILogger,
KnownType.Microsoft_Extensions_Logging_ILogger_TCategoryName,
KnownType.NLog_Logger,
KnownType.NLog_ILogger,
KnownType.NLog_ILoggerBase,
KnownType.log4net_ILog);

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterCompilationStartAction(cc =>
{
Expand All @@ -51,8 +59,7 @@ private static void Process(SonarSyntaxNodeReportingContext context)
var invocation = (InvocationExpressionSyntax)context.Node;

if (invocation.GetName() is "GetLogger" or "CreateLogger"
&& invocation.Ancestors().OfType<TypeDeclarationSyntax>().FirstOrDefault() is { } enclosingType // filter out top-level statements
&& !IsArgumentInObjectCreation(invocation)
&& EnclosingTypeNode(invocation) is { } enclosingType // filter out top-level statements, choose new() first and then enclosing type
&& ExtractArgument(invocation) is { } argument
&& context.SemanticModel.GetSymbolInfo(invocation).Symbol is IMethodSymbol method
&& IsValidMethod(method)
Expand All @@ -62,8 +69,12 @@ private static void Process(SonarSyntaxNodeReportingContext context)
}
}

private static bool IsArgumentInObjectCreation(InvocationExpressionSyntax invocation) =>
invocation.Ancestors().OfType<ObjectCreationExpressionSyntax>().Any();
private static SyntaxNode EnclosingTypeNode(InvocationExpressionSyntax invocation)
{
var ancestors = invocation.Ancestors();
return (SyntaxNode)ancestors.OfType<ObjectCreationExpressionSyntax>().FirstOrDefault() // prioritize new() over enclosing type
?? ancestors.OfType<TypeDeclarationSyntax>().FirstOrDefault();
}

// Extracts T for generic argument, nameof or typeof expressions
private static SyntaxNode ExtractArgument(InvocationExpressionSyntax invocation)
Expand Down Expand Up @@ -108,10 +119,15 @@ bool MatchesGeneric() =>
&& method.TypeParameters.Length == 1;
}

private static bool MatchesEnclosingType(SyntaxNode argument, TypeDeclarationSyntax typeSyntax, SemanticModel model) =>
private static bool MatchesEnclosingType(SyntaxNode argument, SyntaxNode enclosingNode, SemanticModel model) =>
model.GetTypeInfo(argument).Type is { } argumentType
&& model.GetDeclaredSymbol(typeSyntax).GetSymbolType() is { } enclosingType
&& (enclosingType.Equals(argumentType) || argumentType.TypeKind is TypeKind.TypeParameter); // we do not want to raise on CreateLogger<T> if T is not concrete
&& EnclosingTypeSymbol(model, enclosingNode) is { } enclosingType
&& (enclosingType.Equals(argumentType)
|| argumentType.TypeKind is TypeKind.TypeParameter // Do not raise on CreateLogger<T> if T is not concrete
|| enclosingType.DerivesOrImplementsAny(Loggers)); // Do not raise on Decorator pattern

private static ITypeSymbol EnclosingTypeSymbol(SemanticModel model, SyntaxNode enclosingNode) =>
(model.GetDeclaredSymbol(enclosingNode) ?? model.GetSymbolInfo(enclosingNode).Symbol).GetSymbolType();

private static SyntaxNode ExtractGeneric(InvocationExpressionSyntax invocation)
{
Expand Down
61 changes: 44 additions & 17 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/LoggersShouldBeNamedForEnclosingType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,32 +170,51 @@ class LoggerHelper

public class Service
{
public Service(ILogger<Service> logger)
{
}
public Service(ILogger logger) { }
public Service(ILogger logger, string otherParameter) { }
public Service(ILogger<Service> logger) { }
public Service(ILogger<Service> logger, string otherParameter) { }
public Service(Service service) { }
public Service(AnotherService service) { }
}

public Service(ILogger<Service> logger, string otherParameter)
{
}
public class AnotherService
{
public AnotherService(Service service) { }
public AnotherService(ILogger<Service> logger) { }
}


public class Factory
{
private readonly ILogger<Service> logger = LoggerFactory.Create(builder => { }).CreateLogger<Service>(); // Noncompliant
private readonly ILogger<Service> logger = LoggerFactory.Create(builder => { }).CreateLogger<Service>(); // Noncompliant

public Service CreateType(ILoggerFactory loggerFactory)
public void CreateType(ILoggerFactory loggerFactory, string otherParameter)
{
return new Service(loggerFactory.CreateLogger<Service>());
}

public Service CreateType(ILoggerFactory loggerFactory, string otherParameter)
{
return new Service(loggerFactory.CreateLogger<Service>(), otherParameter);
new Service(loggerFactory.CreateLogger<Service>()); // Compliant
new Service(loggerFactory.CreateLogger<AnotherService>()); // Noncompliant

new Service(loggerFactory.CreateLogger<Service>(), otherParameter);
new Service(loggerFactory.CreateLogger(nameof(Service)), otherParameter);
new Service(loggerFactory.CreateLogger(typeof(Service)), otherParameter);
new Service(loggerFactory.CreateLogger(typeof(Service).Name), otherParameter);
new Service(loggerFactory.CreateLogger(typeof(Service).FullName), otherParameter);
new Service(loggerFactory.CreateLogger(typeof(Service).AssemblyQualifiedName), otherParameter);

new Service(loggerFactory.CreateLogger<string>(), otherParameter); // Noncompliant
new Service(loggerFactory.CreateLogger(nameof(AnotherService)), otherParameter); // Noncompliant
new Service(loggerFactory.CreateLogger(typeof(AnotherService)), otherParameter); // Noncompliant
new Service(loggerFactory.CreateLogger(typeof(string).Name), otherParameter); // Noncompliant
new Service(loggerFactory.CreateLogger(typeof(string).FullName), otherParameter); // Noncompliant
new Service(loggerFactory.CreateLogger(typeof(Decorator<Service>).AssemblyQualifiedName), otherParameter); // Noncompliant

new AnotherService(new Service(loggerFactory.CreateLogger<Service>())); // Compliant
new Service(new AnotherService(loggerFactory.CreateLogger<Service>())); // Noncompliant
}

public Service CreateType_LocalVariable(ILoggerFactory loggerFactory)
{
var logger = loggerFactory.CreateLogger<Service>(); // Noncompliant FP
var logger = loggerFactory.CreateLogger<Service>(); // Noncompliant FP
return new Service(logger);
}

Expand All @@ -206,7 +225,16 @@ public Service CreateType_LocalVariableField()

public Service CreateType_Decorator(ILoggerFactory loggerFactory)
{
return new Service(new Decorator<Service>(loggerFactory.CreateLogger<Service>()));
return new Service(new Decorator<Service>(loggerFactory.CreateLogger<Service>())); // Compliant
return new Service(new Decorator(loggerFactory.CreateLogger(nameof(Service)))); // Compliant
}

class Decorator : ILogger
{
public Decorator(ILogger logger) { }
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter) { }
public bool IsEnabled(LogLevel logLevel) => true;
public IDisposable BeginScope<TState>(TState state) => null;
}

private class Decorator<T> : ILogger<T>
Expand All @@ -217,4 +245,3 @@ public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Except
public IDisposable BeginScope<TState>(TState state) => null;
}
}

0 comments on commit 0418ca4

Please sign in to comment.