diff --git a/src/Analyzers/AnalyzerReleases.Unshipped.md b/src/Analyzers/AnalyzerReleases.Unshipped.md index 6b8e18d4..cac23ffb 100644 --- a/src/Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Analyzers/AnalyzerReleases.Unshipped.md @@ -6,4 +6,5 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- DURABLE0001 | Orchestration | Warning | DateTimeOrchestrationAnalyzer -DURABLE0002 | Orchestration | Warning | GuidOrchestrationAnalyzer \ No newline at end of file +DURABLE0002 | Orchestration | Warning | GuidOrchestrationAnalyzer +DURABLE0003 | Orchestration | Warning | DelayOrchestrationAnalyzer diff --git a/src/Analyzers/KnownTypeSymbols.cs b/src/Analyzers/KnownTypeSymbols.cs index fca0ba4d..d138ca95 100644 --- a/src/Analyzers/KnownTypeSymbols.cs +++ b/src/Analyzers/KnownTypeSymbols.cs @@ -21,6 +21,9 @@ sealed class KnownTypeSymbols(Compilation compilation) INamedTypeSymbol? taskOrchestratorBaseClass; INamedTypeSymbol? durableTaskRegistry; INamedTypeSymbol? guid; + INamedTypeSymbol? thread; + INamedTypeSymbol? task; + INamedTypeSymbol? taskT; /// /// Gets an OrchestrationTriggerAttribute type symbol. @@ -52,6 +55,21 @@ sealed class KnownTypeSymbols(Compilation compilation) /// public INamedTypeSymbol? GuidType => this.GetOrResolveFullyQualifiedType(typeof(Guid).FullName, ref this.guid); + /// + /// Gets a Thread type symbol. + /// + public INamedTypeSymbol? Thread => this.GetOrResolveFullyQualifiedType(typeof(Thread).FullName, ref this.thread); + + /// + /// Gets a Task type symbol. + /// + public INamedTypeSymbol? Task => this.GetOrResolveFullyQualifiedType(typeof(Task).FullName, ref this.task); + + /// + /// Gets a Task<T> type symbol. + /// + public INamedTypeSymbol? TaskT => this.GetOrResolveFullyQualifiedType(typeof(Task<>).FullName, ref this.taskT); + INamedTypeSymbol? GetOrResolveFullyQualifiedType(string fullyQualifiedName, ref INamedTypeSymbol? field) { if (field != null) diff --git a/src/Analyzers/Orchestration/DelayOrchestrationAnalyzer.cs b/src/Analyzers/Orchestration/DelayOrchestrationAnalyzer.cs new file mode 100644 index 00000000..dfaa4142 --- /dev/null +++ b/src/Analyzers/Orchestration/DelayOrchestrationAnalyzer.cs @@ -0,0 +1,109 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Collections.Concurrent; +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.DurableTask.Analyzers.Orchestration; + +/// +/// Analyzer that reports a warning when Task.Delay or Thread.Sleep is used in an orchestration method. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class DelayOrchestrationAnalyzer : OrchestrationAnalyzer +{ + /// + /// Diagnostic ID supported for the analyzer. + /// + public const string DiagnosticId = "DURABLE0003"; + + static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.DelayOrchestrationAnalyzerTitle), Resources.ResourceManager, typeof(Resources)); + static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.DelayOrchestrationAnalyzerMessageFormat), Resources.ResourceManager, typeof(Resources)); + + static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + Title, + MessageFormat, + AnalyzersCategories.Orchestration, + DiagnosticSeverity.Warning, + isEnabledByDefault: true); + + /// + public override ImmutableArray SupportedDiagnostics => [Rule]; + + /// + protected override void RegisterAdditionalCompilationStartAction(CompilationStartAnalysisContext context, OrchestrationAnalysisResult orchestrationAnalysisResult) + { + var knownSymbols = new KnownTypeSymbols(context.Compilation); + + if (knownSymbols.Thread == null || knownSymbols.Task == null || knownSymbols.TaskT == null) + { + return; + } + + ConcurrentBag<(ISymbol Symbol, IInvocationOperation Invocation)> delayUsage = []; + + context.RegisterOperationAction( + ctx => + { + ctx.CancellationToken.ThrowIfCancellationRequested(); + + IInvocationOperation invocation = (IInvocationOperation)ctx.Operation; + + if (!invocation.TargetMethod.ContainingType.Equals(knownSymbols.Thread, SymbolEqualityComparer.Default)) + { + return; + } + + if (invocation.TargetMethod.Name is nameof(Thread.Sleep)) + { + ISymbol method = ctx.ContainingSymbol; + delayUsage.Add((method, invocation)); + } + }, + OperationKind.Invocation); + + context.RegisterOperationAction( + ctx => + { + ctx.CancellationToken.ThrowIfCancellationRequested(); + + IInvocationOperation invocation = (IInvocationOperation)ctx.Operation; + + if (!invocation.TargetMethod.ContainingType.Equals(knownSymbols.Task, SymbolEqualityComparer.Default) && + !invocation.TargetMethod.ContainingType.Equals(knownSymbols.TaskT, SymbolEqualityComparer.Default)) + { + return; + } + + if (invocation.TargetMethod.Name is nameof(Task.Delay)) + { + ISymbol method = ctx.ContainingSymbol; + delayUsage.Add((method, invocation)); + } + }, + OperationKind.Invocation); + + context.RegisterCompilationEndAction(ctx => + { + foreach ((ISymbol symbol, IInvocationOperation operation) in delayUsage) + { + if (symbol is IMethodSymbol method) + { + if (orchestrationAnalysisResult.OrchestrationsByMethod.TryGetValue(method, out ConcurrentBag orchestrations)) + { + string methodName = symbol.Name; + string invocationName = operation.TargetMethod.ToDisplayString(SymbolDisplayFormat.CSharpShortErrorMessageFormat); + string orchestrationNames = string.Join(", ", orchestrations.Select(o => o.Name).OrderBy(n => n)); + + // e.g.: "The method 'Method1' uses 'Thread.Sleep(int)' that may cause non-deterministic behavior when invoked from orchestration 'MyOrchestrator'" + ctx.ReportDiagnostic(Rule, operation, methodName, invocationName, orchestrationNames); + } + } + } + }); + } +} diff --git a/src/Analyzers/Resources.resx b/src/Analyzers/Resources.resx index 259f0111..96e7ca8b 100644 --- a/src/Analyzers/Resources.resx +++ b/src/Analyzers/Resources.resx @@ -129,4 +129,10 @@ System.Guid calls must be deterministic inside an orchestration + + The method '{0}' uses '{1}' that may cause non-deterministic behavior when invoked from orchestration '{2}' + + + Thread.Sleep and Task.Delay calls are not allowed inside an orchestrator + \ No newline at end of file diff --git a/test/Analyzers.Tests/Orchestration/DelayOrchestrationAnalyzerTests.cs b/test/Analyzers.Tests/Orchestration/DelayOrchestrationAnalyzerTests.cs new file mode 100644 index 00000000..39053224 --- /dev/null +++ b/test/Analyzers.Tests/Orchestration/DelayOrchestrationAnalyzerTests.cs @@ -0,0 +1,108 @@ +// 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 DelayOrchestrationAnalyzerTests +{ + [Fact] + public async Task EmptyCodeWithSymbolsAvailableHasNoDiag() + { + string code = @""; + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code); + } + + [Fact] + public async Task DurableFunctionOrchestrationUsingThreadSleepHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +void Method([OrchestrationTrigger] TaskOrchestrationContext context) +{ + {|#0:Thread.Sleep(1000)|}; +} +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Method", "Thread.Sleep(int)", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task DurableFunctionOrchestrationUsingTaskDelayHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +async Task Method([OrchestrationTrigger] TaskOrchestrationContext context) +{ + await {|#0:Task.Delay(1000)|}; +} +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Method", "Task.Delay(int)", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task DurableFunctionOrchestrationUsingTaskTDelayHasDiag() + { + string code = Wrapper.WrapDurableFunctionOrchestration(@" +[Function(""Run"")] +async Task Method([OrchestrationTrigger] TaskOrchestrationContext context) +{ + await {|#0:Task.Delay(1000)|}; +} +"); + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Method", "Task.Delay(int)", "Run"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task TaskOrchestratorUsingThreadSleepHasDiag() + { + string code = Wrapper.WrapTaskOrchestrator(@" +public class MyOrchestrator : TaskOrchestrator +{ + public override Task RunAsync(TaskOrchestrationContext context, object input) + { + Method(); + return Task.FromResult(input); + } + + private void Method() { + {|#0:Thread.Sleep(1000)|}; + } +} +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Method", "Thread.Sleep(int)", "MyOrchestrator"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + [Fact] + public async Task FuncOrchestratorUsingThreadSleepHasDiag() + { + string code = Wrapper.WrapFuncOrchestrator(@" +tasks.AddOrchestratorFunc(""HelloSequence"", context => +{ + {|#0:Thread.Sleep(1000)|}; +}); +"); + + DiagnosticResult expected = BuildDiagnostic().WithLocation(0).WithArguments("Main", "Thread.Sleep(int)", "HelloSequence"); + + await VerifyCS.VerifyDurableTaskAnalyzerAsync(code, expected); + } + + static DiagnosticResult BuildDiagnostic() + { + return VerifyCS.Diagnostic(DelayOrchestrationAnalyzer.DiagnosticId); + } +}