diff --git a/src/Analyzers/AnalyzerReleases.Unshipped.md b/src/Analyzers/AnalyzerReleases.Unshipped.md index 0317d471..9f0ef3b7 100644 --- a/src/Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Analyzers/AnalyzerReleases.Unshipped.md @@ -6,3 +6,4 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- DURABLE0009 | Orchestration | Info | **GetInputOrchestrationAnalyzer**: Suggests using input parameter binding instead of ctx.GetInput() in orchestration methods. +DURABLE0010 | Orchestration | Warning | **LoggerOrchestrationAnalyzer**: Warns when a non-contextual ILogger is used in an orchestration method. Orchestrations should use `context.CreateReplaySafeLogger()` instead of injecting ILogger directly. diff --git a/src/Analyzers/KnownTypeSymbols.Net.cs b/src/Analyzers/KnownTypeSymbols.Net.cs index 9803273a..c9bde3ed 100644 --- a/src/Analyzers/KnownTypeSymbols.Net.cs +++ b/src/Analyzers/KnownTypeSymbols.Net.cs @@ -23,6 +23,7 @@ public sealed partial class KnownTypeSymbols INamedTypeSymbol? cancellationToken; INamedTypeSymbol? environment; INamedTypeSymbol? httpClient; + INamedTypeSymbol? iLogger; /// /// Gets a Guid type symbol. @@ -75,4 +76,9 @@ public sealed partial class KnownTypeSymbols /// Gets an HttpClient type symbol. /// public INamedTypeSymbol? HttpClient => this.GetOrResolveFullyQualifiedType(typeof(HttpClient).FullName, ref this.httpClient); + + /// + /// Gets an ILogger type symbol. + /// + public INamedTypeSymbol? ILogger => this.GetOrResolveFullyQualifiedType("Microsoft.Extensions.Logging.ILogger", ref this.iLogger); } diff --git a/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs b/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs new file mode 100644 index 00000000..e1513c5a --- /dev/null +++ b/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs @@ -0,0 +1,138 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; +using static Microsoft.DurableTask.Analyzers.Orchestration.LoggerOrchestrationAnalyzer; + +namespace Microsoft.DurableTask.Analyzers.Orchestration; + +/// +/// Analyzer that reports a warning when a non-contextual ILogger is used in an orchestration method. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class LoggerOrchestrationAnalyzer : OrchestrationAnalyzer +{ + /// + /// Diagnostic ID supported for the analyzer. + /// + public const string DiagnosticId = "DURABLE0010"; + + static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.LoggerOrchestrationAnalyzerTitle), Resources.ResourceManager, typeof(Resources)); + static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.LoggerOrchestrationAnalyzerMessageFormat), Resources.ResourceManager, typeof(Resources)); + + static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + Title, + MessageFormat, + AnalyzersCategories.Orchestration, + DiagnosticSeverity.Warning, + isEnabledByDefault: true); + + /// + public override ImmutableArray SupportedDiagnostics => [Rule]; + + /// + /// Visitor that inspects the method body for ILogger usage. + /// + public sealed class LoggerOrchestrationVisitor : MethodProbeOrchestrationVisitor + { + INamedTypeSymbol? iLoggerSymbol; + + /// + public override bool Initialize() + { + this.iLoggerSymbol = this.KnownTypeSymbols.ILogger; + if (this.iLoggerSymbol == null) + { + return false; + } + + return true; + } + + /// + protected override void VisitMethod(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action reportDiagnostic) + { + IOperation? methodOperation = semanticModel.GetOperation(methodSyntax); + if (methodOperation is null) + { + return; + } + + // Track which parameters we've already reported on to avoid duplicates + HashSet reportedParameters = new(SymbolEqualityComparer.Default); + + // Check for ILogger parameters in the method signature + foreach (IParameterSymbol parameter in methodSymbol.Parameters.Where( + parameter => this.IsILoggerType(parameter.Type) && + parameter.DeclaringSyntaxReferences.Length > 0)) + { + // Found an ILogger parameter - report diagnostic at the parameter location + SyntaxNode parameterSyntax = parameter.DeclaringSyntaxReferences[0].GetSyntax(); + reportDiagnostic(RoslynExtensions.BuildDiagnostic(Rule, parameterSyntax, methodSymbol.Name, orchestrationName)); + reportedParameters.Add(parameter); + } + + // Check for ILogger field or property references (but not parameter references, as those were already reported) + foreach (IOperation descendant in methodOperation.Descendants()) + { + ITypeSymbol? typeToCheck = null; + SyntaxNode? syntaxNode = null; + + switch (descendant) + { + case IFieldReferenceOperation fieldRef: + typeToCheck = fieldRef.Field.Type; + syntaxNode = fieldRef.Syntax; + break; + case IPropertyReferenceOperation propRef: + typeToCheck = propRef.Property.Type; + syntaxNode = propRef.Syntax; + break; + case IParameterReferenceOperation paramRef: + // Skip parameter references that we already reported on in the parameter list + if (reportedParameters.Contains(paramRef.Parameter)) + { + continue; + } + + typeToCheck = paramRef.Parameter.Type; + syntaxNode = paramRef.Syntax; + break; + } + + if (typeToCheck != null && syntaxNode != null && this.IsILoggerType(typeToCheck)) + { + reportDiagnostic(RoslynExtensions.BuildDiagnostic(Rule, syntaxNode, methodSymbol.Name, orchestrationName)); + } + } + } + + bool IsILoggerType(ITypeSymbol type) + { + if (this.iLoggerSymbol == null) + { + return false; + } + + // First check for exact match with ILogger + if (SymbolEqualityComparer.Default.Equals(type, this.iLoggerSymbol)) + { + return true; + } + + // Check if the type implements ILogger interface (covers ILogger case) + if (type is INamedTypeSymbol namedType) + { + return namedType.AllInterfaces.Any(interfaceType => + SymbolEqualityComparer.Default.Equals(interfaceType, this.iLoggerSymbol)); + } + + return false; + } + } +} diff --git a/src/Analyzers/Resources.resx b/src/Analyzers/Resources.resx index 6b56987f..fd55d29f 100644 --- a/src/Analyzers/Resources.resx +++ b/src/Analyzers/Resources.resx @@ -183,6 +183,12 @@ Thread and Task calls must be deterministic inside an orchestrator function + + The method '{0}' uses a non-contextual logger that may cause unexpected behavior when invoked from orchestration '{1}'. Use 'context.CreateReplaySafeLogger()' instead. + + + Orchestrations should use replay-safe loggers created from the orchestration context + CallActivityAsync is passing the incorrect type '{0}' instead of '{1}' to the activity function '{2}' diff --git a/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs b/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs new file mode 100644 index 00000000..e9aed640 --- /dev/null +++ b/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs @@ -0,0 +1,260 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.CodeAnalysis.Testing; +using Microsoft.DurableTask.Analyzers.Orchestration; + +using VerifyCS = Microsoft.DurableTask.Analyzers.Tests.Verifiers.CSharpAnalyzerVerifier; + +namespace Microsoft.DurableTask.Analyzers.Tests.Orchestration; + +public class LoggerOrchestrationAnalyzerTests +{ + [Fact] + public async Task EmptyCodeWithNoSymbolsAvailableHasNoDiag() + { + string code = @""; + + // checks that empty code with no assembly references of Durable Functions has no diagnostics. + // this guarantees that if someone adds our analyzer to a project that doesn't use Durable Functions, + // the analyzer won't crash/they won't get any diagnostics + await VerifyCS.VerifyAnalyzerAsync(code); + } + + [Fact] + public async Task EmptyCodeWithSymbolsAvailableHasNoDiag() + { + string code = @""; + + // checks that empty code with access to assembly references of Durable Functions has no diagnostics + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task NonOrchestrationHasNoDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +void Method(ILogger logger){ + logger.LogInformation(""Test""); +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task DurableFunctionOrchestrationWithLoggerParameterHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +void Run([OrchestrationTrigger] TaskOrchestrationContext context, {|#0:ILogger logger|}) +{ + logger.LogInformation(""Test""); +} +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task DurableFunctionOrchestrationWithLoggerGenericParameterHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +void Run([OrchestrationTrigger] TaskOrchestrationContext context, {|#0:ILogger logger|}) +{ + logger.LogInformation(""Test""); +} +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task DurableFunctionOrchestrationWithLoggerFieldReferenceHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +private readonly ILogger logger; + +[Function(""Run"")] +void Run([OrchestrationTrigger] TaskOrchestrationContext context) +{ + {|#0:this.logger|}.LogInformation(""Test""); +} +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task DurableFunctionOrchestrationWithLoggerPropertyReferenceHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +private ILogger Logger { get; set; } + +[Function(""Run"")] +void Run([OrchestrationTrigger] TaskOrchestrationContext context) +{ + {|#0:this.Logger|}.LogInformation(""Test""); +} +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Run", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task DurableFunctionOrchestrationInvokingMethodWithLoggerHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +private readonly ILogger logger; + +[Function(""Run"")] +void Run([OrchestrationTrigger] TaskOrchestrationContext context) +{ + Helper(); +} + +void Helper() +{ + {|#0:this.logger|}.LogInformation(""Test""); +} +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Helper", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task DurableFunctionOrchestrationUsingContextLoggerHasNoDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +void Run([OrchestrationTrigger] TaskOrchestrationContext context) +{ + ILogger logger = context.CreateReplaySafeLogger(nameof(Run)); + logger.LogInformation(""Test""); +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorWithLoggerParameterHasDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + readonly ILogger logger; + + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + {|#0:this.logger|}.LogInformation(""Test""); + return Task.FromResult(""result""); + } +} +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("RunAsync", "MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task TaskOrchestratorUsingContextLoggerHasNoDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger logger = context.CreateReplaySafeLogger(); + logger.LogInformation(""Test""); + return Task.FromResult(""result""); + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task FuncOrchestratorWithLoggerHasDiag() + { + string code = @" +using System; +using Microsoft.DurableTask; +using Microsoft.DurableTask.Worker; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; + +public class Program +{ + static ILogger staticLogger; + + public static void Main() + { + new ServiceCollection().AddDurableTaskWorker(builder => + { + builder.AddTasks(tasks => + { + tasks.AddOrchestratorFunc(""MyRun"", context => + { + {|#0:staticLogger|}.LogInformation(""Test""); + return ""result""; + }); + }); + }); + } +} +"; + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Main", "MyRun"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task FuncOrchestratorUsingContextLoggerHasNoDiag() + { + string code = Wrapper.WrapFuncOrchestrator(@" +tasks.AddOrchestratorFunc(""HelloSequence"", context => +{ + ILogger logger = context.CreateReplaySafeLogger(""HelloSequence""); + logger.LogInformation(""Test""); + return ""result""; +}); +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task ActivityFunctionWithLoggerHasNoDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""MyActivity"")] +string MyActivity([ActivityTrigger] string input, ILogger logger) +{ + logger.LogInformation(""Test""); + return ""result""; +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + static DiagnosticResult BuildDiagnostic() + { + return VerifyCS.Diagnostic(LoggerOrchestrationAnalyzer.DiagnosticId); + } +} diff --git a/test/Analyzers.Tests/Wrapper.cs b/test/Analyzers.Tests/Wrapper.cs index a3b36523..91dcbd48 100644 --- a/test/Analyzers.Tests/Wrapper.cs +++ b/test/Analyzers.Tests/Wrapper.cs @@ -99,6 +99,7 @@ static string Usings() using Microsoft.DurableTask.Client; using Microsoft.DurableTask.Worker; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; "; }