From fa6d7e98ecee321165eccf7be263da8360d92d1f Mon Sep 17 00:00:00 2001 From: Meir Blachman Date: Mon, 4 May 2026 17:55:37 +0300 Subject: [PATCH 1/2] Fix DURABLE0010 false positives when replay-safe logger is passed to helpers The LoggerOrchestrationAnalyzer previously flagged ILogger references inside helper methods called from an orchestrator even when the logger originated from context.CreateReplaySafeLogger(...). This is a common pattern that should not produce a warning. The analyzer now performs a per-orchestration data-flow analysis: - Walks all reachable methods from the orchestration root, recording every ILogger reference and every call-site argument of ILogger type. - Resolves each ILogger reference's safety on demand: locals are resolved through their initializers; helper-method parameters are resolved by requiring every observed call site to pass a safe value. - Recognizes safe sources: TaskOrchestrationContext.CreateReplaySafeLogger invocations (walking OverriddenMethod chains), conditional and coalesce expressions where all operands are safe, and unwraps conversions and parenthesized expressions. - Local function helpers are included in the reachable set. - Write-only references (LHS of simple assignments) are excluded from the candidate list. Fixes microsoft/durabletask-dotnet#717 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 1 + .../LoggerOrchestrationAnalyzer.cs | 399 +++++++++++++++--- .../LoggerOrchestrationAnalyzerTests.cs | 263 ++++++++++++ 3 files changed, 612 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 946b327fb..b6fd43d75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## Unreleased +- Fix DURABLE0010 false positives when a replay-safe logger created via `TaskOrchestrationContext.CreateReplaySafeLogger` is passed to a helper method ([#717](https://github.com/microsoft/durabletask-dotnet/issues/717)) ## v1.24.2 diff --git a/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs b/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs index e1513c5a4..800492e1f 100644 --- a/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs +++ b/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs @@ -4,9 +4,9 @@ using System.Collections.Immutable; using System.Linq; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations; -using static Microsoft.DurableTask.Analyzers.Orchestration.LoggerOrchestrationAnalyzer; namespace Microsoft.DurableTask.Analyzers.Orchestration; @@ -14,7 +14,7 @@ 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 +public sealed class LoggerOrchestrationAnalyzer : OrchestrationAnalyzer { /// /// Diagnostic ID supported for the analyzer. @@ -36,103 +36,400 @@ public sealed class LoggerOrchestrationAnalyzer : OrchestrationAnalyzer SupportedDiagnostics => [Rule]; /// - /// Visitor that inspects the method body for ILogger usage. + /// Visitor that performs a per-orchestration data-flow analysis to identify ILogger usages + /// that do not originate from TaskOrchestrationContext.CreateReplaySafeLogger. /// - public sealed class LoggerOrchestrationVisitor : MethodProbeOrchestrationVisitor + public sealed class LoggerOrchestrationVisitor : OrchestrationVisitor { INamedTypeSymbol? iLoggerSymbol; + INamedTypeSymbol? taskOrchestrationContextSymbol; /// public override bool Initialize() { this.iLoggerSymbol = this.KnownTypeSymbols.ILogger; - if (this.iLoggerSymbol == null) + this.taskOrchestrationContextSymbol = this.KnownTypeSymbols.TaskOrchestrationContext; + return this.iLoggerSymbol != null; + } + + /// + public override void VisitDurableFunction(SemanticModel semanticModel, MethodDeclarationSyntax methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action reportDiagnostic) + { + this.Analyze(semanticModel, methodSyntax, methodSymbol, orchestrationName, reportDiagnostic, reportEntryParamDecls: true); + } + + /// + public override void VisitTaskOrchestrator(SemanticModel semanticModel, MethodDeclarationSyntax methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action reportDiagnostic) + { + this.Analyze(semanticModel, methodSyntax, methodSymbol, orchestrationName, reportDiagnostic, reportEntryParamDecls: true); + } + + /// + public override void VisitFuncOrchestrator(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action reportDiagnostic) + { + // For AddOrchestratorFunc with a method reference, methodSyntax is the referenced method's declaration. + // For AddOrchestratorFunc with a lambda, methodSyntax is the lambda syntax and methodSymbol is the + // containing method (e.g. Main). In the lambda case we do not want to report on the containing method's + // ILogger parameter declarations because those parameters are not part of the orchestrator. + bool reportEntryParamDecls = methodSyntax is MethodDeclarationSyntax; + this.Analyze(semanticModel, methodSyntax, methodSymbol, orchestrationName, reportDiagnostic, reportEntryParamDecls); + } + + void Analyze( + SemanticModel semanticModel, + SyntaxNode rootSyntax, + IMethodSymbol rootSymbol, + string orchestrationName, + Action reportDiagnostic, + bool reportEntryParamDecls) + { + ReplaySafeLoggerFlowAnalysis analysis = new(this.iLoggerSymbol!, this.taskOrchestrationContextSymbol, this.Compilation); + analysis.Run(semanticModel, rootSyntax, rootSymbol); + analysis.Report(Rule, orchestrationName, reportDiagnostic, reportEntryParamDecls); + } + } + + /// + /// Performs a per-orchestration analysis to determine which ILogger references inside + /// the orchestration's reachable code originate from TaskOrchestrationContext.CreateReplaySafeLogger. + /// + sealed class ReplaySafeLoggerFlowAnalysis + { + readonly INamedTypeSymbol iLoggerSymbol; + readonly INamedTypeSymbol? taskOrchestrationContextSymbol; + readonly Compilation compilation; + + readonly HashSet walked = new(SymbolEqualityComparer.Default); + readonly Dictionary> callSites = new(SymbolEqualityComparer.Default); + readonly List candidates = new(); + + IMethodSymbol? entryMethod; + + public ReplaySafeLoggerFlowAnalysis( + INamedTypeSymbol iLoggerSymbol, + INamedTypeSymbol? taskOrchestrationContextSymbol, + Compilation compilation) + { + this.iLoggerSymbol = iLoggerSymbol; + this.taskOrchestrationContextSymbol = taskOrchestrationContextSymbol; + this.compilation = compilation; + } + + public void Run(SemanticModel semanticModel, SyntaxNode rootSyntax, IMethodSymbol rootSymbol) + { + this.entryMethod = rootSymbol; + this.Walk(semanticModel, rootSyntax, rootSymbol); + } + + public void Report(DiagnosticDescriptor rule, string orchestrationName, Action report, bool reportEntryParamDecls) + { + HashSet reportedParamDecls = new(SymbolEqualityComparer.Default); + + if (reportEntryParamDecls && this.entryMethod != null) { - return false; + foreach (IParameterSymbol p in this.entryMethod.Parameters) + { + if (!this.IsILoggerType(p.Type) || p.DeclaringSyntaxReferences.Length == 0) + { + continue; + } + + if (this.IsSymbolSafe(p, NewVisitingSet())) + { + continue; + } + + SyntaxNode parameterSyntax = p.DeclaringSyntaxReferences[0].GetSyntax(); + report(RoslynExtensions.BuildDiagnostic(rule, parameterSyntax, this.entryMethod.Name, orchestrationName)); + reportedParamDecls.Add(p); + } } - return true; + foreach (Candidate c in this.candidates) + { + if (this.IsSymbolSafe(c.Symbol, NewVisitingSet())) + { + continue; + } + + // Avoid duplicate reporting for entry-method parameter references already reported at their declaration. + if (c.Symbol is IParameterSymbol pr && reportedParamDecls.Contains(pr)) + { + continue; + } + + report(RoslynExtensions.BuildDiagnostic(rule, c.Syntax, c.MethodName, orchestrationName)); + } } - /// - protected override void VisitMethod(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol methodSymbol, string orchestrationName, Action reportDiagnostic) + static HashSet NewVisitingSet() => new(SymbolEqualityComparer.Default); + + static IOperation Unwrap(IOperation operation) { - IOperation? methodOperation = semanticModel.GetOperation(methodSyntax); - if (methodOperation is null) + IOperation current = operation; + while (true) { - return; + switch (current) + { + case IConversionOperation conv: + current = conv.Operand; + break; + case IParenthesizedOperation paren: + current = paren.Operand; + break; + default: + return current; + } + } + } + + static IEnumerable GetMethodBodySyntaxes(IMethodSymbol methodSymbol) + { + foreach (SyntaxReference reference in methodSymbol.DeclaringSyntaxReferences) + { + SyntaxNode syntax = reference.GetSyntax(); + if (syntax is MethodDeclarationSyntax or LocalFunctionStatementSyntax) + { + yield return syntax; + } } + } - // Track which parameters we've already reported on to avoid duplicates - HashSet reportedParameters = new(SymbolEqualityComparer.Default); + // Phase 1: walk every reachable method, collecting ILogger candidates and call-site arguments. + // We need this pass (not pure on-demand) to know all call sites of a given parameter, since a + // helper's parameter is safe only if every call site within the orchestration passes a safe value. + void Walk(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol methodSymbol) + { + if (!this.walked.Add(methodSymbol)) + { + return; + } - // Check for ILogger parameters in the method signature - foreach (IParameterSymbol parameter in methodSymbol.Parameters.Where( - parameter => this.IsILoggerType(parameter.Type) && - parameter.DeclaringSyntaxReferences.Length > 0)) + IOperation? methodOperation = semanticModel.GetOperation(methodSyntax); + if (methodOperation is null) { - // 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); + return; } - // Check for ILogger field or property references (but not parameter references, as those were already reported) + string methodName = methodSymbol.Name; foreach (IOperation descendant in methodOperation.Descendants()) { - ITypeSymbol? typeToCheck = null; - SyntaxNode? syntaxNode = null; - switch (descendant) { - case IFieldReferenceOperation fieldRef: - typeToCheck = fieldRef.Field.Type; - syntaxNode = fieldRef.Syntax; + case IFieldReferenceOperation fr when this.IsILoggerType(fr.Field.Type) && !IsWriteOnlyTarget(fr): + this.candidates.Add(new Candidate(methodName, fr.Syntax, fr.Field)); + break; + + case IPropertyReferenceOperation pr when this.IsILoggerType(pr.Property.Type) && !IsWriteOnlyTarget(pr): + this.candidates.Add(new Candidate(methodName, pr.Syntax, pr.Property)); + break; + + case IParameterReferenceOperation pr when this.IsILoggerType(pr.Parameter.Type) && !IsWriteOnlyTarget(pr): + this.candidates.Add(new Candidate(methodName, pr.Syntax, pr.Parameter)); break; - case IPropertyReferenceOperation propRef: - typeToCheck = propRef.Property.Type; - syntaxNode = propRef.Syntax; + + case ILocalReferenceOperation lr when this.IsILoggerType(lr.Local.Type) && !IsWriteOnlyTarget(lr): + this.candidates.Add(new Candidate(methodName, lr.Syntax, lr.Local)); 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; + + case IInvocationOperation invocation: + this.RecordCallSiteArguments(invocation); + this.RecurseIntoCallee(semanticModel, invocation.TargetMethod); break; } + } + } - if (typeToCheck != null && syntaxNode != null && this.IsILoggerType(typeToCheck)) + // True iff the reference is the target of a simple assignment (e.g. LHS of `x = ...`). + // Such references are write-only and shouldn't be analyzed as logger usages. + // Compound assignments (`+=`), increments, ref/out passes, and reads are intentionally + // not treated as write-only because they all observe the current value. + static bool IsWriteOnlyTarget(IOperation reference) + { + return reference.Parent is ISimpleAssignmentOperation assignment + && ReferenceEquals(assignment.Target, reference); + } + + void RecordCallSiteArguments(IInvocationOperation invocation) + { + foreach (IArgumentOperation argument in invocation.Arguments) + { + if (argument.Parameter is IParameterSymbol calleeParam && this.IsILoggerType(calleeParam.Type)) { - reportDiagnostic(RoslynExtensions.BuildDiagnostic(Rule, syntaxNode, methodSymbol.Name, orchestrationName)); + if (!this.callSites.TryGetValue(calleeParam, out List? args)) + { + args = new List(); + this.callSites[calleeParam] = args; + } + + args.Add(argument.Value); } } } - bool IsILoggerType(ITypeSymbol type) + void RecurseIntoCallee(SemanticModel semanticModel, IMethodSymbol callee) + { + if (callee is null) + { + return; + } + + foreach (SyntaxNode calleeSyntax in GetMethodBodySyntaxes(callee)) + { + if (!this.compilation.ContainsSyntaxTree(calleeSyntax.SyntaxTree)) + { + continue; + } + + SemanticModel calleeSemanticModel = semanticModel.SyntaxTree == calleeSyntax.SyntaxTree + ? semanticModel + : this.compilation.GetSemanticModel(calleeSyntax.SyntaxTree); + + this.Walk(calleeSemanticModel, calleeSyntax, callee); + } + } + + // Phase 2: demand-driven safety resolution. + // A symbol is "safe" iff its value is provably derived from CreateReplaySafeLogger. + // The visiting set guards against cycles in the call graph; on revisit we return optimistic + // (true). This matches the original fixed-point analysis: in a pure cycle, only an unsafe + // call site outside the cycle can flip the result to unsafe. + bool IsSymbolSafe(ISymbol symbol, HashSet visiting) + { + if (!visiting.Add(symbol)) + { + return true; + } + + try + { + return symbol switch + { + ILocalSymbol local => this.IsLocalSafe(local, visiting), + IParameterSymbol parameter => this.IsParameterSafe(parameter, visiting), + _ => false, + }; + } + finally + { + visiting.Remove(symbol); + } + } + + bool IsLocalSafe(ILocalSymbol local, HashSet visiting) { - if (this.iLoggerSymbol == null) + // Resolve the local's origin from its declarator's initializer. This is flow-insensitive: + // re-assignments after the initial declaration are not tracked. The common pattern + // `var logger = context.CreateReplaySafeLogger(...);` is fully supported. + if (local.DeclaringSyntaxReferences.Length == 0) + { + return false; + } + + SyntaxNode declSyntax = local.DeclaringSyntaxReferences[0].GetSyntax(); + if (declSyntax is not VariableDeclaratorSyntax declarator || declarator.Initializer is null) { return false; } - // First check for exact match with ILogger + SemanticModel semanticModel = this.compilation.GetSemanticModel(declarator.SyntaxTree); + IOperation? initOperation = semanticModel.GetOperation(declarator.Initializer.Value); + return initOperation != null && this.IsExpressionSafe(initOperation, visiting); + } + + bool IsParameterSafe(IParameterSymbol parameter, HashSet visiting) + { + // Externally-supplied parameters of the orchestrator entry point are never safe. + if (SymbolEqualityComparer.Default.Equals(parameter.ContainingSymbol, this.entryMethod)) + { + return false; + } + + // For helper parameters we require every observed call site to pass a safe value. + // No observed call sites means the helper isn't called from this orchestration's + // reachable graph — be conservative and treat as unsafe. + if (!this.callSites.TryGetValue(parameter, out List? args) || args.Count == 0) + { + return false; + } + + foreach (IOperation arg in args) + { + if (!this.IsExpressionSafe(arg, visiting)) + { + return false; + } + } + + return true; + } + + bool IsExpressionSafe(IOperation expression, HashSet visiting) + { + IOperation expr = Unwrap(expression); + return expr switch + { + IInvocationOperation invocation => this.IsCreateReplaySafeLoggerInvocation(invocation), + ILocalReferenceOperation localRef => this.IsSymbolSafe(localRef.Local, visiting), + IParameterReferenceOperation paramRef => this.IsSymbolSafe(paramRef.Parameter, visiting), + IConditionalOperation cond => cond.WhenFalse != null + && this.IsExpressionSafe(cond.WhenTrue, visiting) + && this.IsExpressionSafe(cond.WhenFalse, visiting), + ICoalesceOperation coalesce => this.IsExpressionSafe(coalesce.Value, visiting) + && this.IsExpressionSafe(coalesce.WhenNull, visiting), + _ => false, + }; + } + + bool IsCreateReplaySafeLoggerInvocation(IInvocationOperation invocation) + { + if (this.taskOrchestrationContextSymbol == null) + { + return false; + } + + IMethodSymbol method = invocation.TargetMethod; + if (method.Name != "CreateReplaySafeLogger") + { + return false; + } + + IMethodSymbol original = method; + while (original.OverriddenMethod != null) + { + original = original.OverriddenMethod; + } + + INamedTypeSymbol? containing = original.ContainingType?.OriginalDefinition; + return containing != null + && SymbolEqualityComparer.Default.Equals(containing, this.taskOrchestrationContextSymbol); + } + + bool IsILoggerType(ITypeSymbol type) + { 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 type is INamedTypeSymbol named + && named.AllInterfaces.Any(i => SymbolEqualityComparer.Default.Equals(i, this.iLoggerSymbol)); + } + + sealed class Candidate + { + public Candidate(string methodName, SyntaxNode syntax, ISymbol symbol) { - return namedType.AllInterfaces.Any(interfaceType => - SymbolEqualityComparer.Default.Equals(interfaceType, this.iLoggerSymbol)); + this.MethodName = methodName; + this.Syntax = syntax; + this.Symbol = symbol; } - return false; + public string MethodName { get; } + + public SyntaxNode Syntax { get; } + + public ISymbol Symbol { get; } } } } diff --git a/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs b/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs index e9aed640a..e64c5c6a7 100644 --- a/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs +++ b/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs @@ -187,6 +187,269 @@ public override Task RunAsync(TaskOrchestrationContext context, string i await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); } + [Fact] + public async Task TaskOrchestratorPassingReplaySafeLoggerToHelperHasNoDiag() + { + // Repro for https://github.com/microsoft/durabletask-dotnet/issues/717: + // a replay-safe logger passed to a helper method should not trigger DURABLE0010. + string code = Wrapper.WrapTaskOrchestrator(@" +public record DemoInput(string Value); +public record DemoResult(string Value); + +public class DemoOrchestrator : TaskOrchestrator +{ + public override async Task RunAsync(TaskOrchestrationContext context, DemoInput input) + { + var logger = context.CreateReplaySafeLogger(nameof(DemoOrchestrator)); + LogData(logger); + return new DemoResult($""Processed: {input.Value}""); + } + + private static void LogData(ILogger logger) + { + logger.LogInformation(""Logging some data for demonstration purposes.""); + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorPassingReplaySafeLoggerThroughLocalAliasHasNoDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger logger = context.CreateReplaySafeLogger(); + ILogger alias = logger; + Helper(alias); + return Task.FromResult(""result""); + } + + private static void Helper(ILogger logger) + { + logger.LogInformation(""Test""); + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorPassingFieldLoggerToHelperHasDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + readonly ILogger logger; + + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + Helper({|#0:this.logger|}); + return Task.FromResult(""result""); + } + + private static void Helper(ILogger logger) + { + {|#1:logger|}.LogInformation(""Test""); + } +} +"); + + DiagnosticResult expectedFieldRef = BuildDiagnostic().WithLocation(0).WithArguments("RunAsync", "MyOrchestrator"); + DiagnosticResult expectedParamRef = BuildDiagnostic().WithLocation(1).WithArguments("Helper", "MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expectedFieldRef, expectedParamRef); + } + + [Fact] + public async Task DurableFunctionOrchestrationPassingReplaySafeLoggerToHelperHasNoDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +void Run([OrchestrationTrigger] TaskOrchestrationContext context) +{ + ILogger logger = context.CreateReplaySafeLogger(nameof(Run)); + Helper(logger); +} + +void Helper(ILogger logger) +{ + logger.LogInformation(""Test""); +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorReplaySafeLoggerIntoLocalFunctionHelperHasNoDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger logger = context.CreateReplaySafeLogger(); + LogData(logger); + return Task.FromResult(""result""); + + static void LogData(ILogger logger) + { + logger.LogInformation(""Test""); + } + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorHelperCalledWithMixedSafeAndUnsafeArgsHasDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + readonly ILogger field; + + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger safe = context.CreateReplaySafeLogger(); + Helper(safe); + Helper({|#0:this.field|}); + return Task.FromResult(""result""); + } + + private static void Helper(ILogger logger) + { + {|#1:logger|}.LogInformation(""Test""); + } +} +"); + + DiagnosticResult expectedFieldRef = BuildDiagnostic().WithLocation(0).WithArguments("RunAsync", "MyOrchestrator"); + DiagnosticResult expectedParamRef = BuildDiagnostic().WithLocation(1).WithArguments("Helper", "MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expectedFieldRef, expectedParamRef); + } + + [Fact] + public async Task TaskOrchestratorTransitiveHelperChainHasNoDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger logger = context.CreateReplaySafeLogger(); + Outer(logger); + return Task.FromResult(""result""); + } + + private static void Outer(ILogger logger) + { + Inner(logger); + } + + private static void Inner(ILogger logger) + { + logger.LogInformation(""Test""); + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorRecursiveHelperWithSafeLoggerHasNoDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger logger = context.CreateReplaySafeLogger(); + Log(logger, 3); + return Task.FromResult(""result""); + } + + private static void Log(ILogger logger, int depth) + { + logger.LogInformation(""Test""); + if (depth > 0) + { + Log(logger, depth - 1); + } + } +} +"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task TaskOrchestratorConditionalAndCoalesceExpressionsForLoggerInitializer() + { + // Conditional with both branches safe → no diag. + // Coalesce with a non-safe LHS → flagged (we cannot prove the LHS is null at runtime). + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + readonly ILogger? maybeNull; + + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger a = context.CreateReplaySafeLogger(); + ILogger b = context.CreateReplaySafeLogger(nameof(MyOrchestrator)); + ILogger conditional = input.Length > 0 ? a : b; + conditional.LogInformation(""ok""); + + ILogger coalesced = {|#0:this.maybeNull|} ?? a; + {|#1:coalesced|}.LogInformation(""flagged""); + + return Task.FromResult(""result""); + } +} +"); + + DiagnosticResult expectedFieldRef = BuildDiagnostic().WithLocation(0).WithArguments("RunAsync", "MyOrchestrator"); + DiagnosticResult expectedLocalRef = BuildDiagnostic().WithLocation(1).WithArguments("RunAsync", "MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expectedFieldRef, expectedLocalRef); + } + + [Fact] + public async Task TaskOrchestratorFieldAssignedFromCreateReplaySafeLoggerIsStillFlagged() + { + // Documents an intentional limitation: even when a field is assigned from CreateReplaySafeLogger, + // we treat field reads as unsafe because subsequent replays would re-execute the assignment + // (potentially with a different context) and the field may also be mutated from elsewhere. + // The assignment LHS itself is not flagged because it is write-only. + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + ILogger? logger; + + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + this.logger = context.CreateReplaySafeLogger(); + {|#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 FuncOrchestratorWithLoggerHasDiag() { From 5682402594807a68f905ad8ef85197450e8681f7 Mon Sep 17 00:00:00 2001 From: Meir Blachman Date: Mon, 4 May 2026 18:24:33 +0300 Subject: [PATCH 2/2] Track local and parameter reassignments in replay-safe logger analysis Address PR review feedback: a local initialized from CreateReplaySafeLogger that is later reassigned to a non-replay-safe value, or a helper parameter reassigned inside the helper, must flip the symbol to unsafe. Records all assignments (initializers + reassignments) and requires every assigned value to be safe (flow-insensitive AND semantics). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../LoggerOrchestrationAnalyzer.cs | 109 +++++++++++++----- .../LoggerOrchestrationAnalyzerTests.cs | 58 ++++++++++ 2 files changed, 138 insertions(+), 29 deletions(-) diff --git a/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs b/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs index 800492e1f..df4a219a2 100644 --- a/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs +++ b/src/Analyzers/Orchestration/LoggerOrchestrationAnalyzer.cs @@ -101,6 +101,7 @@ sealed class ReplaySafeLoggerFlowAnalysis readonly HashSet walked = new(SymbolEqualityComparer.Default); readonly Dictionary> callSites = new(SymbolEqualityComparer.Default); + readonly Dictionary> assignments = new(SymbolEqualityComparer.Default); readonly List candidates = new(); IMethodSymbol? entryMethod; @@ -164,6 +165,16 @@ public void Report(DiagnosticDescriptor rule, string orchestrationName, Action NewVisitingSet() => new(SymbolEqualityComparer.Default); + // True iff the reference is the target of a simple assignment (e.g. LHS of `x = ...`). + // Such references are write-only and shouldn't be analyzed as logger usages. + // Compound assignments (`+=`), increments, ref/out passes, and reads are intentionally + // not treated as write-only because they all observe the current value. + static bool IsWriteOnlyTarget(IOperation reference) + { + return reference.Parent is ISimpleAssignmentOperation assignment + && ReferenceEquals(assignment.Target, reference); + } + static IOperation Unwrap(IOperation operation) { IOperation current = operation; @@ -232,6 +243,15 @@ void Walk(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol me this.candidates.Add(new Candidate(methodName, lr.Syntax, lr.Local)); break; + case IVariableDeclaratorOperation declarator + when this.IsILoggerType(declarator.Symbol.Type) && declarator.Initializer?.Value is IOperation initValue: + this.AddAssignment(declarator.Symbol, initValue); + break; + + case ISimpleAssignmentOperation assignment: + this.RecordAssignmentToLocalOrParameter(assignment); + break; + case IInvocationOperation invocation: this.RecordCallSiteArguments(invocation); this.RecurseIntoCallee(semanticModel, invocation.TargetMethod); @@ -240,16 +260,6 @@ void Walk(SemanticModel semanticModel, SyntaxNode methodSyntax, IMethodSymbol me } } - // True iff the reference is the target of a simple assignment (e.g. LHS of `x = ...`). - // Such references are write-only and shouldn't be analyzed as logger usages. - // Compound assignments (`+=`), increments, ref/out passes, and reads are intentionally - // not treated as write-only because they all observe the current value. - static bool IsWriteOnlyTarget(IOperation reference) - { - return reference.Parent is ISimpleAssignmentOperation assignment - && ReferenceEquals(assignment.Target, reference); - } - void RecordCallSiteArguments(IInvocationOperation invocation) { foreach (IArgumentOperation argument in invocation.Arguments) @@ -289,6 +299,33 @@ void RecurseIntoCallee(SemanticModel semanticModel, IMethodSymbol callee) } } + void RecordAssignmentToLocalOrParameter(ISimpleAssignmentOperation assignment) + { + // Track reassignments to locals and parameters of ILogger type. Field/property targets + // are intentionally ignored — those are handled by the always-unsafe rule for fields/properties. + switch (assignment.Target) + { + case ILocalReferenceOperation lr when this.IsILoggerType(lr.Local.Type): + this.AddAssignment(lr.Local, assignment.Value); + break; + + case IParameterReferenceOperation pr when this.IsILoggerType(pr.Parameter.Type): + this.AddAssignment(pr.Parameter, assignment.Value); + break; + } + } + + void AddAssignment(ISymbol symbol, IOperation value) + { + if (!this.assignments.TryGetValue(symbol, out List? values)) + { + values = new List(); + this.assignments[symbol] = values; + } + + values.Add(value); + } + // Phase 2: demand-driven safety resolution. // A symbol is "safe" iff its value is provably derived from CreateReplaySafeLogger. // The visiting set guards against cycles in the call graph; on revisit we return optimistic @@ -318,23 +355,24 @@ bool IsSymbolSafe(ISymbol symbol, HashSet visiting) bool IsLocalSafe(ILocalSymbol local, HashSet visiting) { - // Resolve the local's origin from its declarator's initializer. This is flow-insensitive: - // re-assignments after the initial declaration are not tracked. The common pattern - // `var logger = context.CreateReplaySafeLogger(...);` is fully supported. - if (local.DeclaringSyntaxReferences.Length == 0) + // A local is safe iff every assignment to it (initializer + reassignments) has a safe value. + // This is flow-insensitive: we don't reason about which assignment is "current" at any + // particular use site. Using OR semantics here would be unsound — a single reassignment + // to an unsafe value reaches every subsequent read. + if (!this.assignments.TryGetValue(local, out List? values) || values.Count == 0) { return false; } - SyntaxNode declSyntax = local.DeclaringSyntaxReferences[0].GetSyntax(); - if (declSyntax is not VariableDeclaratorSyntax declarator || declarator.Initializer is null) + foreach (IOperation value in values) { - return false; + if (!this.IsExpressionSafe(value, visiting)) + { + return false; + } } - SemanticModel semanticModel = this.compilation.GetSemanticModel(declarator.SyntaxTree); - IOperation? initOperation = semanticModel.GetOperation(declarator.Initializer.Value); - return initOperation != null && this.IsExpressionSafe(initOperation, visiting); + return true; } bool IsParameterSafe(IParameterSymbol parameter, HashSet visiting) @@ -345,23 +383,36 @@ bool IsParameterSafe(IParameterSymbol parameter, HashSet visiting) return false; } - // For helper parameters we require every observed call site to pass a safe value. - // No observed call sites means the helper isn't called from this orchestration's - // reachable graph — be conservative and treat as unsafe. - if (!this.callSites.TryGetValue(parameter, out List? args) || args.Count == 0) + // For helper parameters we require every observed call site AND every internal + // reassignment to be safe. No observed sources at all means the helper isn't called + // from this orchestration's reachable graph — be conservative and treat as unsafe. + bool hasObservedSource = false; + + if (this.callSites.TryGetValue(parameter, out List? callArgs)) { - return false; + hasObservedSource = true; + foreach (IOperation arg in callArgs) + { + if (!this.IsExpressionSafe(arg, visiting)) + { + return false; + } + } } - foreach (IOperation arg in args) + if (this.assignments.TryGetValue(parameter, out List? reassignments)) { - if (!this.IsExpressionSafe(arg, visiting)) + hasObservedSource = true; + foreach (IOperation value in reassignments) { - return false; + if (!this.IsExpressionSafe(value, visiting)) + { + return false; + } } } - return true; + return hasObservedSource; } bool IsExpressionSafe(IOperation expression, HashSet visiting) diff --git a/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs b/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs index e64c5c6a7..b6964eaab 100644 --- a/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs +++ b/test/Analyzers.Tests/Orchestration/LoggerOrchestrationAnalyzerTests.cs @@ -450,6 +450,64 @@ public override Task RunAsync(TaskOrchestrationContext context, string i await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); } + [Fact] + public async Task TaskOrchestratorLocalReassignedToUnsafeValueHasDiag() + { + // Even when a local is initialized from CreateReplaySafeLogger, a later reassignment to a + // non-replay-safe value must flip the local to unsafe (flow-insensitive: any reassignment + // reaches every subsequent read). + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + readonly ILogger field; + + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger logger = context.CreateReplaySafeLogger(); + logger = {|#0:this.field|}; + {|#1:logger|}.LogInformation(""Test""); + return Task.FromResult(""result""); + } +} +"); + + DiagnosticResult expected1 = BuildDiagnostic().WithLocation(0).WithArguments("RunAsync", "MyOrchestrator"); + DiagnosticResult expected2 = BuildDiagnostic().WithLocation(1).WithArguments("RunAsync", "MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected1, expected2); + } + + [Fact] + public async Task TaskOrchestratorHelperParameterReassignedToUnsafeValueHasDiag() + { + // Even when every call site passes a safe logger, a reassignment of the parameter inside + // the helper to a non-replay-safe value must flip the parameter to unsafe. + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + readonly ILogger field; + + public override Task RunAsync(TaskOrchestrationContext context, string input) + { + ILogger logger = context.CreateReplaySafeLogger(); + this.Helper(logger); + return Task.FromResult(""result""); + } + + private void Helper(ILogger logger) + { + logger = {|#0:this.field|}; + {|#1:logger|}.LogInformation(""Test""); + } +} +"); + + DiagnosticResult expected1 = BuildDiagnostic().WithLocation(0).WithArguments("Helper", "MyOrchestrator"); + DiagnosticResult expected2 = BuildDiagnostic().WithLocation(1).WithArguments("Helper", "MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected1, expected2); + } + [Fact] public async Task FuncOrchestratorWithLoggerHasDiag() {