-
Notifications
You must be signed in to change notification settings - Fork 55
Non-deterministic System.DateTime usage Roslyn Analyzer #284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
7ef6afd
datetime analyzer
allantargino b107350
adding xunit package
allantargino 74b6b3d
renaming Analyzers.Test to Analyzers.Tests
allantargino abe7f76
supporting class based orchestrators
allantargino 4c029fb
commenting implementation details
allantargino 88ff4a0
flatten helpers folder structure
allantargino 9365704
removing cache wrapper from KnownTypeSymbols
allantargino 838eef4
moving strings to resources file
allantargino e80a6c8
reusing test case
allantargino a3d3799
fix namespaces + small rewrite for better readability
allantargino 77a9be7
renaming roslyn method for better clarity
allantargino ddfa241
using explicit type + new convention
allantargino 90e9def
guards in case RunAsync methods are not available in analyzer
allantargino 4c97c96
Resource.Designer.cs created by code generation
allantargino da05012
enabling StyleCop analyzers and fixing style issues
allantargino f0120f3
updating stylecop analyzers versions
allantargino 1dfb481
documenting members
allantargino 47f0fb9
removing resx embedded resource from analyzers
allantargino d7274bd
Update src/Analyzers/Orchestration/OrchestrationAnalyzer.cs
allantargino 6a2dd99
fixing comments grammar/additional comments
allantargino 9175423
Merge branch 'datetime-analyzer' of github.com:allantargino/durableta…
allantargino File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ; Shipped analyzer releases | ||
| ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| ; Unshipped analyzer release | ||
| ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md | ||
|
|
||
| ### New Rules | ||
|
|
||
| Rule ID | Category | Severity | Notes | ||
| --------|----------|----------|------- | ||
| DURABLE0001 | Orchestration | Warning | DateTimeOrchestrationAnalyzer |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| namespace Microsoft.DurableTask.Analyzers; | ||
|
|
||
| /// <summary> | ||
| /// Provides a set of well-known categories that are used by the analyzers diagnostics. | ||
| /// </summary> | ||
| static class AnalyzersCategories | ||
| { | ||
| /// <summary> | ||
| /// The category for the orchestration related analyzers. | ||
| /// </summary> | ||
| public const string Orchestration = "Orchestration"; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using Microsoft.CodeAnalysis; | ||
|
|
||
| namespace Microsoft.DurableTask.Analyzers; | ||
|
|
||
| /// <summary> | ||
| /// Provides a set of well-known types that are used by the analyzers. | ||
| /// Inspired by KnownTypeSymbols class in | ||
| /// <see href="https://github.com/dotnet/runtime/blob/2a846acb1a92e811427babe3ff3f047f98c5df02/src/libraries/System.Text.Json/gen/Helpers/KnownTypeSymbols.cs">System.Text.Json.SourceGeneration</see> source code. | ||
| /// Lazy initialization is used to avoid the the initialization of all types during class construction, since not all symbols are used by all analyzers. | ||
| /// </summary> | ||
| sealed class KnownTypeSymbols(Compilation compilation) | ||
| { | ||
| readonly Compilation compilation = compilation; | ||
|
|
||
| INamedTypeSymbol? functionOrchestrationAttribute; | ||
| INamedTypeSymbol? functionNameAttribute; | ||
| INamedTypeSymbol? taskOrchestratorInterface; | ||
| INamedTypeSymbol? taskOrchestratorBaseClass; | ||
| INamedTypeSymbol? durableTaskRegistry; | ||
|
|
||
| /// <summary> | ||
| /// Gets an OrchestrationTriggerAttribute type symbol. | ||
| /// </summary> | ||
| public INamedTypeSymbol? FunctionOrchestrationAttribute => this.GetOrResolveFullyQualifiedType("Microsoft.Azure.Functions.Worker.OrchestrationTriggerAttribute", ref this.functionOrchestrationAttribute); | ||
|
|
||
| /// <summary> | ||
| /// Gets a FunctionNameAttribute type symbol. | ||
| /// </summary> | ||
| public INamedTypeSymbol? FunctionNameAttribute => this.GetOrResolveFullyQualifiedType("Microsoft.Azure.Functions.Worker.FunctionAttribute", ref this.functionNameAttribute); | ||
|
|
||
| /// <summary> | ||
| /// Gets an ITaskOrchestrator type symbol. | ||
| /// </summary> | ||
| public INamedTypeSymbol? TaskOrchestratorInterface => this.GetOrResolveFullyQualifiedType("Microsoft.DurableTask.ITaskOrchestrator", ref this.taskOrchestratorInterface); | ||
|
|
||
| /// <summary> | ||
| /// Gets a TaskOrchestrator type symbol. | ||
| /// </summary> | ||
| public INamedTypeSymbol? TaskOrchestratorBaseClass => this.GetOrResolveFullyQualifiedType("Microsoft.DurableTask.TaskOrchestrator`2", ref this.taskOrchestratorBaseClass); | ||
allantargino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary> | ||
| /// Gets a DurableTaskRegistry type symbol. | ||
| /// </summary> | ||
| public INamedTypeSymbol? DurableTaskRegistry => this.GetOrResolveFullyQualifiedType("Microsoft.DurableTask.DurableTaskRegistry", ref this.durableTaskRegistry); | ||
allantargino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| INamedTypeSymbol? GetOrResolveFullyQualifiedType(string fullyQualifiedName, ref INamedTypeSymbol? field) | ||
| { | ||
| if (field != null) | ||
| { | ||
| return field; | ||
| } | ||
|
|
||
| return field = this.compilation.GetTypeByMetadataName(fullyQualifiedName); | ||
| } | ||
| } | ||
87 changes: 87 additions & 0 deletions
87
src/Analyzers/Orchestration/DateTimeOrchestrationAnalyzer.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| // 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; | ||
|
|
||
| /// <summary> | ||
| /// Analyzer that reports a warning when a non-deterministic DateTime property is used in an orchestration method. | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public sealed class DateTimeOrchestrationAnalyzer : OrchestrationAnalyzer | ||
allantargino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| /// <summary> | ||
| /// Diagnostic ID supported for the analyzer. | ||
| /// </summary> | ||
| public const string DiagnosticId = "DURABLE0001"; | ||
|
|
||
| static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.DateTimeOrchestrationAnalyzerTitle), Resources.ResourceManager, typeof(Resources)); | ||
| static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.DateTimeOrchestrationAnalyzerMessageFormat), Resources.ResourceManager, typeof(Resources)); | ||
|
|
||
| static readonly DiagnosticDescriptor Rule = new( | ||
| DiagnosticId, | ||
| Title, | ||
| MessageFormat, | ||
| AnalyzersCategories.Orchestration, | ||
| DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [Rule]; | ||
|
|
||
| /// <inheritdoc/> | ||
| protected override void RegisterAdditionalCompilationStartAction(CompilationStartAnalysisContext context, OrchestrationAnalysisResult orchestrationAnalysisResult) | ||
| { | ||
| INamedTypeSymbol systemDateTimeSymbol = context.Compilation.GetSpecialType(SpecialType.System_DateTime); | ||
|
|
||
| // stores the symbols (such as methods) and the DateTime references used in them | ||
| ConcurrentBag<(ISymbol Symbol, IPropertyReferenceOperation Operation)> dateTimeUsage = []; | ||
|
|
||
| // search for usages of DateTime.Now, DateTime.UtcNow, DateTime.Today and store them | ||
| context.RegisterOperationAction( | ||
| ctx => | ||
| { | ||
| ctx.CancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| var operation = (IPropertyReferenceOperation)ctx.Operation; | ||
| IPropertySymbol property = operation.Property; | ||
|
|
||
| if (!property.ContainingSymbol.Equals(systemDateTimeSymbol, SymbolEqualityComparer.Default)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (property.Name is nameof(DateTime.Now) or nameof(DateTime.UtcNow) or nameof(DateTime.Today)) | ||
allantargino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| ISymbol method = ctx.ContainingSymbol; | ||
| dateTimeUsage.Add((method, operation)); | ||
| } | ||
| }, | ||
| OperationKind.PropertyReference); | ||
|
|
||
| // compare whether the found DateTime usages occur in methods invoked by orchestrations | ||
| context.RegisterCompilationEndAction(ctx => | ||
| { | ||
| foreach ((ISymbol symbol, IPropertyReferenceOperation operation) in dateTimeUsage) | ||
| { | ||
| if (symbol is IMethodSymbol method) | ||
| { | ||
| if (orchestrationAnalysisResult.OrchestrationsByMethod.TryGetValue(method, out ConcurrentBag<AnalyzedOrchestration> orchestrations)) | ||
| { | ||
| string methodName = symbol.Name; | ||
| string dateTimePropertyName = operation.Property.ToString(); | ||
| string orchestrationNames = string.Join(", ", orchestrations.Select(o => o.Name).OrderBy(n => n)); | ||
|
|
||
| // e.g.: "The method 'Method1' uses 'System.Date.Now' that may cause non-deterministic behavior when invoked from orchestration 'MyOrchestrator'" | ||
| ctx.ReportDiagnostic(Rule, operation, methodName, dateTimePropertyName, orchestrationNames); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.