Skip to content
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

Non-deterministic System.DateTime usage Roslyn Analyzer #284

Merged
merged 21 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7ef6afd
datetime analyzer
allantargino Apr 5, 2024
b107350
adding xunit package
allantargino Apr 8, 2024
74b6b3d
renaming Analyzers.Test to Analyzers.Tests
allantargino Apr 8, 2024
abe7f76
supporting class based orchestrators
allantargino Apr 9, 2024
4c029fb
commenting implementation details
allantargino Apr 10, 2024
88ff4a0
flatten helpers folder structure
allantargino Apr 10, 2024
9365704
removing cache wrapper from KnownTypeSymbols
allantargino Apr 10, 2024
838eef4
moving strings to resources file
allantargino Apr 10, 2024
e80a6c8
reusing test case
allantargino Apr 10, 2024
a3d3799
fix namespaces + small rewrite for better readability
allantargino Apr 10, 2024
77a9be7
renaming roslyn method for better clarity
allantargino Apr 11, 2024
ddfa241
using explicit type + new convention
allantargino Apr 11, 2024
90e9def
guards in case RunAsync methods are not available in analyzer
allantargino Apr 11, 2024
4c97c96
Resource.Designer.cs created by code generation
allantargino Apr 11, 2024
da05012
enabling StyleCop analyzers and fixing style issues
allantargino Apr 12, 2024
f0120f3
updating stylecop analyzers versions
allantargino Apr 12, 2024
1dfb481
documenting members
allantargino Apr 12, 2024
47f0fb9
removing resx embedded resource from analyzers
allantargino Apr 12, 2024
d7274bd
Update src/Analyzers/Orchestration/OrchestrationAnalyzer.cs
allantargino Apr 17, 2024
6a2dd99
fixing comments grammar/additional comments
allantargino Apr 17, 2024
9175423
Merge branch 'datetime-analyzer' of github.com:allantargino/durableta…
allantargino Apr 17, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Microsoft.DurableTask.sln
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Benchmarks", "test\Benchmar
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Analyzers", "src\Analyzers\Analyzers.csproj", "{998E9D97-BD36-4A9D-81FC-5DAC1CE40083}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Analyzers.Test", "test\Analyzers.Test\Analyzers.Test.csproj", "{541FCCCE-1059-4691-B027-F761CD80DE92}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Analyzers.Tests", "test\Analyzers.Tests\Analyzers.Tests.csproj", "{541FCCCE-1059-4691-B027-F761CD80DE92}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Expand Down
3 changes: 3 additions & 0 deletions src/Analyzers/AnalyzerReleases.Shipped.md
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

8 changes: 8 additions & 0 deletions src/Analyzers/AnalyzerReleases.Unshipped.md
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
9 changes: 9 additions & 0 deletions src/Analyzers/Helpers/AnalyzersCategories.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

namespace Microsoft.DurableTask.Analyzers.Helpers;

static class AnalyzersCategories
{
public const string Orchestration = "Orchestration";
}
44 changes: 44 additions & 0 deletions src/Analyzers/Helpers/KnownTypeSymbols.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using Microsoft.CodeAnalysis;

namespace Microsoft.DurableTask.Analyzers.Helpers;

/// <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;

Cached<INamedTypeSymbol?> orchestrationTriggerAttribute;
allantargino marked this conversation as resolved.
Show resolved Hide resolved
public INamedTypeSymbol? OrchestrationTriggerAttribute => this.GetOrResolveFullyQualifiedType("Microsoft.Azure.Functions.Worker.OrchestrationTriggerAttribute", ref this.orchestrationTriggerAttribute);

Cached<INamedTypeSymbol?> functionAttribute;
public INamedTypeSymbol? FunctionAttribute => this.GetOrResolveFullyQualifiedType("Microsoft.Azure.Functions.Worker.FunctionAttribute", ref this.functionAttribute);

INamedTypeSymbol? GetOrResolveFullyQualifiedType(string fullyQualifiedName, ref Cached<INamedTypeSymbol?> field)
{
if (field.HasValue)
{
return field.Value;
}

INamedTypeSymbol? type = this.compilation.GetTypeByMetadataName(fullyQualifiedName);
field = new(type);
return type;
}

// We could use Lazy<T> here, but because we need to use the `compilation` variable instance,
// that would require us to initiate the Lazy<T> lambdas in the constructor.
// Because not all analyzers use all symbols, we would be allocating unnecessary lambdas.
readonly struct Cached<T>(T value)
{
public readonly bool HasValue = true;
public readonly T Value = value;
}
}
61 changes: 61 additions & 0 deletions src/Analyzers/Helpers/RoslynExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections.Immutable;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis;

namespace Microsoft.DurableTask.Analyzers.Helpers;
allantargino marked this conversation as resolved.
Show resolved Hide resolved

static class RoslynExtensions
{
public static bool TryGetSingleValueFromAttribute<T>(this ISymbol? symbol, INamedTypeSymbol attributeSymbol, out T value)
{
if (symbol.TryGetConstructorArgumentsFromAttribute(attributeSymbol, out ImmutableArray<TypedConstant> constructorArguments))
{
object? valueObj = constructorArguments.FirstOrDefault().Value;
if (valueObj != null)
{
value = (T)valueObj;
return true;
}
}

value = default!;
return false;
}

public static bool TryGetConstructorArgumentsFromAttribute(this ISymbol? symbol, INamedTypeSymbol attributeSymbol, out ImmutableArray<TypedConstant> constructorArguments)
{
if (symbol != null)
{
foreach (AttributeData attribute in symbol.GetAttributes())
{
if (attributeSymbol.Equals(attribute.AttributeClass, SymbolEqualityComparer.Default))
{
constructorArguments = attribute.ConstructorArguments;
return true;
}
}
}

return false;
}

public static bool ContainsAttributeInAnyMethodArguments(this IMethodSymbol methodSymbol, INamedTypeSymbol attributeSymbol)
{
return methodSymbol.Parameters
.SelectMany(p => p.GetAttributes())
.Any(a => attributeSymbol.Equals(a.AttributeClass, SymbolEqualityComparer.Default));
}

public static void ReportDiagnostic(this CompilationAnalysisContext ctx, DiagnosticDescriptor descriptor, IOperation operation, params string[] messageArgs)
{
ctx.ReportDiagnostic(BuildDiagnostic(descriptor, operation.Syntax, messageArgs));
}

public static Diagnostic BuildDiagnostic(DiagnosticDescriptor descriptor, SyntaxNode syntaxNode, params string[] messageArgs)
{
return Diagnostic.Create(descriptor, syntaxNode.GetLocation(), messageArgs);
}
}
70 changes: 70 additions & 0 deletions src/Analyzers/Orchestration/DateTimeOrchestrationAnalyzer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections.Concurrent;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeAnalysis;
using Microsoft.DurableTask.Analyzers.Helpers;

namespace Microsoft.DurableTask.Analyzers.Orchestration;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class DateTimeOrchestrationAnalyzer : OrchestrationAnalyzer
allantargino marked this conversation as resolved.
Show resolved Hide resolved
{
public const string DiagnosticId = "DURABLE0001";

static readonly DiagnosticDescriptor Rule = new(
DiagnosticId,
"DateTime calls must be deterministic inside an orchestration function",
allantargino marked this conversation as resolved.
Show resolved Hide resolved
"The method '{0}' uses '{1}' that may cause non-deterministic behavior when invoked from Orchestration Function '{2}'",
AnalyzersCategories.Orchestration,
DiagnosticSeverity.Warning,
isEnabledByDefault: true);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [Rule];

protected override void RegisterAdditionalCompilationStartAction(CompilationStartAnalysisContext context, OrchestrationAnalysisResult orchestrationAnalysisResult)
{
INamedTypeSymbol systemDateTimeSymbol = context.Compilation.GetSpecialType(SpecialType.System_DateTime);

ConcurrentBag<(ISymbol symbol, IPropertyReferenceOperation operation)> dateTimeUsage = [];
allantargino marked this conversation as resolved.
Show resolved Hide resolved

// 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) &&
property.Name is nameof(DateTime.Now) or nameof(DateTime.UtcNow) or nameof(DateTime.Today))
{
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<OrchestrationMethod> orchestrations))
{
string methodName = symbol.Name;
string dateTimePropertyName = operation.Property.ToString();
string functionsNames = string.Join(", ", orchestrations.Select(o => o.FunctionName).OrderBy(n => n));

// e.g.: "The method 'Method1' uses 'System.Date.Now' that may cause non-deterministic behavior when invoked from Orchestration Function 'Run1'"
ctx.ReportDiagnostic(Rule, operation, methodName, dateTimePropertyName, functionsNames);
}
}
}
});
}
}
131 changes: 131 additions & 0 deletions src/Analyzers/Orchestration/OrchestrationAnalyzer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections.Concurrent;
using System.Diagnostics;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.DurableTask.Analyzers.Helpers;

namespace Microsoft.DurableTask.Analyzers.Orchestration;

public abstract class OrchestrationAnalyzer : DiagnosticAnalyzer
{
public override void Initialize(AnalysisContext context)
bachuv marked this conversation as resolved.
Show resolved Hide resolved
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.EnableConcurrentExecution();

context.RegisterCompilationStartAction(context =>
{
var knownSymbols = new KnownTypeSymbols(context.Compilation);

if (knownSymbols.OrchestrationTriggerAttribute == null || knownSymbols.FunctionAttribute == null)
{
// symbols not available in this compilation, skip analysis
return;
}

OrchestrationAnalysisResult result = new();

context.RegisterSyntaxNodeAction(ctx =>
{
ctx.CancellationToken.ThrowIfCancellationRequested();

// Checks whether the declared method is an orchestration, if not, returns
if (ctx.ContainingSymbol is not IMethodSymbol methodSymbol ||
!methodSymbol.ContainsAttributeInAnyMethodArguments(knownSymbols.OrchestrationTriggerAttribute) ||
!methodSymbol.TryGetSingleValueFromAttribute(knownSymbols.FunctionAttribute, out string functionName))
{
return;
}

var orchestration = new OrchestrationMethod(functionName, methodSymbol);
var methodSyntax = (MethodDeclarationSyntax)ctx.Node;

FindInvokedMethods(ctx.SemanticModel, methodSyntax, methodSymbol, orchestration, result);
}, SyntaxKind.MethodDeclaration);

// allows concrete implementations to register specific actions/analysis and then check if they happen in any of the orchestration methods
this.RegisterAdditionalCompilationStartAction(context, result);
});
}

// Recursively find all methods invoked by the orchestration method
static void FindInvokedMethods(
SemanticModel semanticModel, MethodDeclarationSyntax callerSyntax, IMethodSymbol callerSymbol,
OrchestrationMethod rootOrchestration, OrchestrationAnalysisResult result)
{
if (!TryTrackMethod(semanticModel, callerSyntax, callerSymbol, rootOrchestration, result))
{
// previously tracked method, leave to avoid infinite recursion
return;
}

foreach (InvocationExpressionSyntax invocationSyntax in callerSyntax.DescendantNodes().OfType<InvocationExpressionSyntax>())
{
IOperation? operation = semanticModel.GetOperation(invocationSyntax);
if (operation == null || operation is not IInvocationOperation invocation)
{
continue;
}

IMethodSymbol calleeMethodSymbol = invocation.TargetMethod;
if (calleeMethodSymbol == null)
{
continue;
}

// iterating over multiple syntax references is needed because the same method can be declared in multiple places (e.g. partial classes)
IEnumerable<MethodDeclarationSyntax> calleeSyntaxes = calleeMethodSymbol.DeclaringSyntaxReferences.Select(r => r.GetSyntax()).OfType<MethodDeclarationSyntax>();
foreach (MethodDeclarationSyntax calleeSyntax in calleeSyntaxes)
{
FindInvokedMethods(semanticModel, calleeSyntax, calleeMethodSymbol, rootOrchestration, result);
}
}
}

static bool TryTrackMethod(SemanticModel semanticModel, MethodDeclarationSyntax callerSyntax, IMethodSymbol callerSymbol,
allantargino marked this conversation as resolved.
Show resolved Hide resolved
OrchestrationMethod rootOrchestration, OrchestrationAnalysisResult result)
{
ConcurrentBag<OrchestrationMethod> orchestrations = result.OrchestrationsByMethod.GetOrAdd(callerSymbol, []);
if (orchestrations.Contains(rootOrchestration))
{
return false;
}

orchestrations.Add(rootOrchestration);

return true;
}

/// <summary>
/// Register additional actions to be executed after the compilation has started.
/// It is expected from a concrete implementation of <see cref="OrchestrationAnalyzer"/> to register a
/// <see cref="CompilationStartAnalysisContext.RegisterCompilationEndAction"/>
/// and then compare that any discovered violations happened in any of the symbols in <paramref name="orchestrationAnalysisResult"/>.
/// </summary>
/// <param name="context">Context originally provided by <see cref="AnalysisContext.RegisterCompilationAction"/></param>
/// <param name="orchestrationAnalysisResult">Collection of symbols referenced by orchestrations</param>
protected abstract void RegisterAdditionalCompilationStartAction(CompilationStartAnalysisContext context, OrchestrationAnalysisResult orchestrationAnalysisResult);

protected readonly struct OrchestrationAnalysisResult
{
public ConcurrentDictionary<IMethodSymbol, ConcurrentBag<OrchestrationMethod>> OrchestrationsByMethod { get; }

public OrchestrationAnalysisResult()
{
this.OrchestrationsByMethod = new(SymbolEqualityComparer.Default);
}
}

[DebuggerDisplay("[{FunctionName}] {OrchestrationMethodSymbol.Name}")]
protected readonly struct OrchestrationMethod(string functionName, IMethodSymbol orchestrationMethodSymbol)
{
public string FunctionName { get; } = functionName;
public IMethodSymbol OrchestrationMethodSymbol { get; } = orchestrationMethodSymbol;
}
}
Loading
Loading