Skip to content

Commit 54503d2

Browse files
CopilotYunchuWangCopilotgithub-code-quality[bot]
authored
Add Roslyn analyzer for non-contextual logger usage in orchestrations (DURABLE0010) (#553)
* Initial plan * Add LoggerOrchestrationAnalyzer implementation and tests Co-authored-by: YunchuWang <[email protected]> * Fix LoggerOrchestrationAnalyzer to handle ILogger<T> and avoid duplicate diagnostics Co-authored-by: YunchuWang <[email protected]> * Simplify IsILoggerType method per code review feedback Co-authored-by: YunchuWang <[email protected]> * Update src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs Co-authored-by: Copilot <[email protected]> * Combine nested if statements in LoggerOrchestrationAnalyzer Co-authored-by: YunchuWang <[email protected]> * Potential fix for pull request finding 'Missed opportunity to use Where' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * Potential fix for pull request finding 'Missed opportunity to use Where' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: YunchuWang <[email protected]> Co-authored-by: wangbill <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
1 parent 34e8e9b commit 54503d2

File tree

6 files changed

+412
-0
lines changed

6 files changed

+412
-0
lines changed

‎src/Analyzers/AnalyzerReleases.Unshipped.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@
66
Rule ID | Category | Severity | Notes
77
--------|----------|----------|-------
88
DURABLE0009 | Orchestration | Info | **GetInputOrchestrationAnalyzer**: Suggests using input parameter binding instead of ctx.GetInput<T>() in orchestration methods.
9+
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.

‎src/Analyzers/KnownTypeSymbols.Net.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ public sealed partial class KnownTypeSymbols
2323
INamedTypeSymbol? cancellationToken;
2424
INamedTypeSymbol? environment;
2525
INamedTypeSymbol? httpClient;
26+
INamedTypeSymbol? iLogger;
2627

2728
/// <summary>
2829
/// Gets a Guid type symbol.
@@ -75,4 +76,9 @@ public sealed partial class KnownTypeSymbols
7576
/// Gets an HttpClient type symbol.
7677
/// </summary>
7778
public INamedTypeSymbol? HttpClient => this.GetOrResolveFullyQualifiedType(typeof(HttpClient).FullName, ref this.httpClient);
79+
80+
/// <summary>
81+
/// Gets an ILogger type symbol.
82+
/// </summary>
83+
public INamedTypeSymbol? ILogger => this.GetOrResolveFullyQualifiedType("Microsoft.Extensions.Logging.ILogger", ref this.iLogger);
7884
}

‎src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
using System.Collections.Immutable;
5+
using System.Linq;
6+
using Microsoft.CodeAnalysis;
7+
using Microsoft.CodeAnalysis.Diagnostics;
8+
using Microsoft.CodeAnalysis.Operations;
9+
using static Microsoft.DurableTask.Analyzers.Orchestration.LoggerOrchestrationAnalyzer;
10+
11+
namespace Microsoft.DurableTask.Analyzers.Orchestration;
12+
13+
/// <summary>
14+
/// Analyzer that reports a warning when a non-contextual ILogger is used in an orchestration method.
15+
/// </summary>
16+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
17+
public sealed class LoggerOrchestrationAnalyzer : OrchestrationAnalyzer<LoggerOrchestrationVisitor>
18+
{
19+
/// <summary>
20+
/// Diagnostic ID supported for the analyzer.
21+
/// </summary>
22+
public const string DiagnosticId = "DURABLE0010";
23+
24+
static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.LoggerOrchestrationAnalyzerTitle), Resources.ResourceManager, typeof(Resources));
25+
static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.LoggerOrchestrationAnalyzerMessageFormat), Resources.ResourceManager, typeof(Resources));
26+
27+
static readonly DiagnosticDescriptor Rule = new(
28+
DiagnosticId,
29+
Title,
30+
MessageFormat,
31+
AnalyzersCategories.Orchestration,
32+
DiagnosticSeverity.Warning,
33+
isEnabledByDefault: true);
34+
35+
/// <inheritdoc/>
36+
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [Rule];
37+
38+
/// <summary>
39+
/// Visitor that inspects the method body for ILogger usage.
40+
/// </summary>
41+
public sealed class LoggerOrchestrationVisitor : MethodProbeOrchestrationVisitor
42+
{
43+
INamedTypeSymbol? iLoggerSymbol;
44+
45+
/// <inheritdoc/>
46+
public override bool Initialize()
47+
{
48+
this.iLoggerSymbol = this.KnownTypeSymbols.ILogger;
49+
if (this.iLoggerSymbol == null)
50+
{
51+
return false;
52+
}
53+
54+
return true;
55+
}
56+
57+
/// <inheritdoc/>
58+
protected override void VisitMethod(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action<Diagnostic> reportDiagnostic)
59+
{
60+
IOperation? methodOperation = semanticModel.GetOperation(methodSyntax);
61+
if (methodOperation is null)
62+
{
63+
return;
64+
}
65+
66+
// Track which parameters we've already reported on to avoid duplicates
67+
HashSet<IParameterSymbol> reportedParameters = new(SymbolEqualityComparer.Default);
68+
69+
// Check for ILogger parameters in the method signature
70+
foreach (IParameterSymbol parameter in methodSymbol.Parameters.Where(
71+
parameter => this.IsILoggerType(parameter.Type) &&
72+
parameter.DeclaringSyntaxReferences.Length > 0))
73+
{
74+
// Found an ILogger parameter - report diagnostic at the parameter location
75+
SyntaxNode parameterSyntax = parameter.DeclaringSyntaxReferences[0].GetSyntax();
76+
reportDiagnostic(RoslynExtensions.BuildDiagnostic(Rule, parameterSyntax, methodSymbol.Name, orchestrationName));
77+
reportedParameters.Add(parameter);
78+
}
79+
80+
// Check for ILogger field or property references (but not parameter references, as those were already reported)
81+
foreach (IOperation descendant in methodOperation.Descendants())
82+
{
83+
ITypeSymbol? typeToCheck = null;
84+
SyntaxNode? syntaxNode = null;
85+
86+
switch (descendant)
87+
{
88+
case IFieldReferenceOperation fieldRef:
89+
typeToCheck = fieldRef.Field.Type;
90+
syntaxNode = fieldRef.Syntax;
91+
break;
92+
case IPropertyReferenceOperation propRef:
93+
typeToCheck = propRef.Property.Type;
94+
syntaxNode = propRef.Syntax;
95+
break;
96+
case IParameterReferenceOperation paramRef:
97+
// Skip parameter references that we already reported on in the parameter list
98+
if (reportedParameters.Contains(paramRef.Parameter))
99+
{
100+
continue;
101+
}
102+
103+
typeToCheck = paramRef.Parameter.Type;
104+
syntaxNode = paramRef.Syntax;
105+
break;
106+
}
107+
108+
if (typeToCheck != null && syntaxNode != null && this.IsILoggerType(typeToCheck))
109+
{
110+
reportDiagnostic(RoslynExtensions.BuildDiagnostic(Rule, syntaxNode, methodSymbol.Name, orchestrationName));
111+
}
112+
}
113+
}
114+
115+
bool IsILoggerType(ITypeSymbol type)
116+
{
117+
if (this.iLoggerSymbol == null)
118+
{
119+
return false;
120+
}
121+
122+
// First check for exact match with ILogger
123+
if (SymbolEqualityComparer.Default.Equals(type, this.iLoggerSymbol))
124+
{
125+
return true;
126+
}
127+
128+
// Check if the type implements ILogger interface (covers ILogger<T> case)
129+
if (type is INamedTypeSymbol namedType)
130+
{
131+
return namedType.AllInterfaces.Any(interfaceType =>
132+
SymbolEqualityComparer.Default.Equals(interfaceType, this.iLoggerSymbol));
133+
}
134+
135+
return false;
136+
}
137+
}
138+
}

‎src/Analyzers/Resources.resx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,12 @@
183183
<data name="ThreadTaskOrchestrationAnalyzerTitle" xml:space="preserve">
184184
<value>Thread and Task calls must be deterministic inside an orchestrator function</value>
185185
</data>
186+
<data name="LoggerOrchestrationAnalyzerMessageFormat" xml:space="preserve">
187+
<value>The method '{0}' uses a non-contextual logger that may cause unexpected behavior when invoked from orchestration '{1}'. Use 'context.CreateReplaySafeLogger()' instead.</value>
188+
</data>
189+
<data name="LoggerOrchestrationAnalyzerTitle" xml:space="preserve">
190+
<value>Orchestrations should use replay-safe loggers created from the orchestration context</value>
191+
</data>
186192
<data name="InputArgumentTypeMismatchAnalyzerMessageFormat" xml:space="preserve">
187193
<value>CallActivityAsync is passing the incorrect type '{0}' instead of '{1}' to the activity function '{2}'</value>
188194
</data>

0 commit comments

Comments
 (0)