From 2fb060132c8a8941432370e4244fd0d2e52f5f2b Mon Sep 17 00:00:00 2001 From: Gregorius Soedharmo Date: Tue, 27 Feb 2024 20:21:23 +0700 Subject: [PATCH] [AK1002] Fix false positive (#72) * [AK1002] Fix false positive * Move expensive type checking to the end --- ...cefulStopInsideReceiveAsyncAnalyzerSpec.cs | 240 +++++++++++++++++- ...racefulStopInsideReceiveAsyncFixerSpecs.cs | 170 ++++++++++++- ...tGracefulStopInsideReceiveAsyncAnalyzer.cs | 36 ++- .../Utility/CodeAnalysisExtensions.cs | 84 ++++++ 4 files changed, 513 insertions(+), 17 deletions(-) diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzerSpec.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzerSpec.cs index 0dab3c3..32abc2f 100644 --- a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzerSpec.cs +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzerSpec.cs @@ -145,13 +145,40 @@ public Task GracefulStop(TimeSpan timeout) } } """, + + // GracefulStop being called on actor other than Self +""" +using System; +using Akka.Actor; + +public class MyActor: ReceiveActor +{ + public MyActor() + { + ReceiveAsync(async p => + { + var actorSelection = Context.System.ActorSelection(p.Path); + var actorRef = await actorSelection.ResolveOne(TimeSpan.FromSeconds(3)); + await actorRef.GracefulStop(TimeSpan.FromSeconds(3)); // should not flag this + }); + + ReceiveAnyAsync(async _ => + { + var actorSelection = Context.System.ActorSelection(ActorPath.Parse("")); + var actorRef = await actorSelection.ResolveOne(TimeSpan.FromSeconds(3)); + await actorRef.GracefulStop(TimeSpan.FromSeconds(3)); // should not flag this + }); + } +} +""", + }; public static readonly TheoryData<(string testData, (int startLine, int startColumn, int endLine, int endColumn) spanData)> FailureCases = new() { - // Receive actor invoking await GracefulStop() inside a ReceiveAsync block + // Receive actor invoking await Context.Self.GracefulStop() inside a ReceiveAsync block ( """ using System; @@ -170,7 +197,142 @@ public MyActor() } """, (11, 13, 11, 69)), - // Receive actor invoking await GracefulStop() inside a ReceiveAnyAsync block + // Receive actor invoking await GracefulStop() on ActorContext stored in a variable + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + public MyActor() + { + ReceiveAsync(async str => + { + var ctx = Context; + await ctx.Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } +} +""", (12, 13, 12, 65)), + + // Receive actor invoking await GracefulStop() on ActorContext stored inside a field + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + private readonly IActorContext _context; + + public MyActor() + { + _context = Context; + + ReceiveAsync(async str => + { + await _context.Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } +} +""", (15, 13, 15, 70)), + + // Receive actor invoking await GracefulStop() on ActorContext stored inside a property + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + private IActorContext MyContext { get; } + + public MyActor() + { + MyContext = Context; + + ReceiveAsync(async str => + { + await MyContext.Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } +} +""", (15, 13, 15, 71)), + + // Receive actor invoking await GracefulStop() on ActorContext returned by a function + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + private IActorContext MyContext() => Context; + + public MyActor() + { + ReceiveAsync(async str => + { + await MyContext().Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } +} +""", (13, 13, 13, 73)), + + // Receive actor invoking await Context.Self.GracefulStop() inside a lambda function inside ReceiveAsync() + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + public MyActor() + { + ReceiveAsync(async str => + { + async Task InnerLambda() + { + await Context.Self.GracefulStop(TimeSpan.FromSeconds(3)); + } + + await InnerLambda(); + }); + } +} +""", (13, 17, 13, 73)), + + // Receive actor invoking await GracefulStop() on ActorContext passed as function parameter of a lambda function inside ReceiveAsync() + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + public MyActor() + { + ReceiveAsync(async str => + { + async Task InnerLambda(IActorContext ctx) + { + await ctx.Self.GracefulStop(TimeSpan.FromSeconds(3)); + } + + await InnerLambda(Context); + }); + } +} +""", (13, 17, 13, 69)), + + // Receive actor invoking await Context.Self.GracefulStop() inside a ReceiveAnyAsync block ( """ using System; @@ -189,7 +351,7 @@ public MyActor() } """, (11, 13, 11, 69)), - // Receive actor invoking await GracefulStop() inside a ReceiveAsync with no code block + // Receive actor invoking await Context.Self.GracefulStop() inside a ReceiveAsync with no code block ( """ using System; @@ -205,7 +367,7 @@ public MyActor() } """, (9, 43, 9, 99)), - // Receive actor invoking await GracefulStop() inside a ReceiveAnyAsync with no code block + // Receive actor invoking await Context.Self.GracefulStop() inside a ReceiveAnyAsync with no code block ( """ using System; @@ -220,6 +382,76 @@ public MyActor() } } """, (9, 38, 9, 94)), + + // Receive actor invoking await Self.GracefulStop() inside a ReceiveAsync block + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + public MyActor() + { + ReceiveAsync(async str => + { + await Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } +} +""", (11, 13, 11, 61)), + + // Receive actor invoking await Self.GracefulStop() inside a ReceiveAnyAsync block + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + public MyActor() + { + ReceiveAnyAsync(async obj => + { + await Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } +} +""", (11, 13, 11, 61)), + + // Receive actor invoking await Self.GracefulStop() inside a ReceiveAsync with no code block + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + public MyActor() + { + ReceiveAsync(async str => await Self.GracefulStop(TimeSpan.FromSeconds(3))); + } +} +""", (9, 43, 9, 91)), + + // Receive actor invoking await Self.GracefulStop() inside a ReceiveAnyAsync with no code block + ( +""" +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor +{ + public MyActor() + { + ReceiveAnyAsync(async obj => await Self.GracefulStop(TimeSpan.FromSeconds(3))); + } +} +""", (9, 38, 9, 86)), }; [Theory] diff --git a/src/Akka.Analyzers.Tests/Fixes/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncFixerSpecs.cs b/src/Akka.Analyzers.Tests/Fixes/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncFixerSpecs.cs index 6262f4b..ed1e8b2 100644 --- a/src/Akka.Analyzers.Tests/Fixes/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncFixerSpecs.cs +++ b/src/Akka.Analyzers.Tests/Fixes/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncFixerSpecs.cs @@ -12,7 +12,7 @@ namespace Akka.Analyzers.Tests.Fixes.AK1000; public class MustNotAwaitGracefulStopInsideReceiveAsyncFixerSpecs { [Fact] - public Task RemoveAwaitInsideReceiveAsyncMethod() + public Task RemoveAwaitInsideReceiveAsyncMethod_ContextSelf() { var before = """ @@ -57,7 +57,50 @@ public MyActor() } [Fact] - public Task RemoveAwaitInsideReceiveAnyAsyncMethod() + public Task RemoveAwaitInsideReceiveAsyncMethod_Self() + { + const string before = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAsync(async str => { + await Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } + } + """; + + const string after = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAsync(async str => { + _ = Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } + } + """; + + var expectedDiagnostic = Verify.Diagnostic() + .WithSpan(10, 13, 10, 61) + .WithArguments(); + + return Verify.VerifyCodeFix(before, after, + MustNotAwaitGracefulStopInsideReceiveAsyncFixer.Key_FixAwaitGracefulStop, expectedDiagnostic); + } + + [Fact] + public Task RemoveAwaitInsideReceiveAnyAsyncMethod_ContextSelf() { var before = """ @@ -102,7 +145,50 @@ public MyActor() } [Fact] - public Task RemoveAwaitInsideOneLinerReceiveAsyncMethod() + public Task RemoveAwaitInsideReceiveAnyAsyncMethod_Self() + { + const string before = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAnyAsync(async obj => { + await Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } + } + """; + + const string after = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAnyAsync(async obj => { + _ = Self.GracefulStop(TimeSpan.FromSeconds(3)); + }); + } + } + """; + + var expectedDiagnostic = Verify.Diagnostic() + .WithSpan(10, 13, 10, 61) + .WithArguments(); + + return Verify.VerifyCodeFix(before, after, + MustNotAwaitGracefulStopInsideReceiveAsyncFixer.Key_FixAwaitGracefulStop, expectedDiagnostic); + } + + [Fact] + public Task RemoveAwaitInsideOneLinerReceiveAsyncMethod_ContextSelf() { var before = """ @@ -142,7 +228,45 @@ public MyActor() } [Fact] - public Task RemoveAwaitInsideOneLinerReceiveAnyAsyncMethod() + public Task RemoveAwaitInsideOneLinerReceiveAsyncMethod_Self() + { + const string before = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAsync(async str => await Self.GracefulStop(TimeSpan.FromSeconds(3))); + } + } + """; + + const string after = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAsync(async str => _ = Self.GracefulStop(TimeSpan.FromSeconds(3))); + } + } + """; + + var expectedDiagnostic = Verify.Diagnostic() + .WithSpan(9, 43, 9, 91); + + return Verify.VerifyCodeFix(before, after, + MustNotAwaitGracefulStopInsideReceiveAsyncFixer.Key_FixAwaitGracefulStop, expectedDiagnostic); + } + + [Fact] + public Task RemoveAwaitInsideOneLinerReceiveAnyAsyncMethod_ContextSelf() { var before = """ @@ -180,4 +304,42 @@ public MyActor() return Verify.VerifyCodeFix(before, after, MustNotAwaitGracefulStopInsideReceiveAsyncFixer.Key_FixAwaitGracefulStop, expectedDiagnostic); } + + [Fact] + public Task RemoveAwaitInsideOneLinerReceiveAnyAsyncMethod_Self() + { + const string before = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAnyAsync(async obj => await Self.GracefulStop(TimeSpan.FromSeconds(3))); + } + } + """; + + const string after = """ + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor + { + public MyActor() + { + ReceiveAnyAsync(async obj => _ = Self.GracefulStop(TimeSpan.FromSeconds(3))); + } + } + """; + + var expectedDiagnostic = Verify.Diagnostic() + .WithSpan(9, 38, 9, 86); + + return Verify.VerifyCodeFix(before, after, + MustNotAwaitGracefulStopInsideReceiveAsyncFixer.Key_FixAwaitGracefulStop, expectedDiagnostic); + } } \ No newline at end of file diff --git a/src/Akka.Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzer.cs index 0303212..0198bf7 100644 --- a/src/Akka.Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotAwaitGracefulStopInsideReceiveAsyncAnalyzer.cs @@ -24,18 +24,23 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, context.RegisterSyntaxNodeAction(ctx => { var invocationExpr = (InvocationExpressionSyntax)ctx.Node; - - if(!IsGracefulStopInvocation(invocationExpr, ctx.SemanticModel, akkaContext)) + var semanticModel = ctx.SemanticModel; + + if(!IsGracefulStopInvocation(invocationExpr, semanticModel, akkaContext)) return; // Check 1: GracefulStop() should not be awaited if (invocationExpr.Parent is not AwaitExpressionSyntax awaitExpression) return; - // Check 2: Ensure called within ReceiveAsync lambda expression - if (!invocationExpr.IsInsideReceiveAsyncLambda(ctx.SemanticModel, akkaContext)) + // Check 2: Ensure called within ReceiveAsync or ReceiveAnyAsync lambda expression + if (!invocationExpr.IsInsideReceiveAsyncLambda(semanticModel, akkaContext)) return; + // Check 3: Ensure method is accessing ActorBase.Self or ActorContext.Self + if(!invocationExpr.IsAccessingActorSelf(semanticModel, akkaContext)) + return; + var diagnostic = Diagnostic.Create(RuleDescriptors.Ak1002DoNotAwaitOnGracefulStop, awaitExpression.GetLocation()); ctx.ReportDiagnostic(diagnostic); }, SyntaxKind.InvocationExpression); @@ -55,13 +60,26 @@ private static bool IsGracefulStopInvocation( SemanticModel semanticModel, AkkaContext akkaContext) { - // Get the method symbol from the invocation expression + // Expression need to be a member access + if (invocationExpression.Expression is not MemberAccessExpressionSyntax memberAccess) + return false; + + // Expression name should be "GracefulStop" + if (memberAccess.Name.Identifier.Text is not "GracefulStop") + return false; + + // Expression need to be a method invocation if (semanticModel.GetSymbolInfo(invocationExpression).Symbol is not IMethodSymbol methodSymbol) return false; - // Check if the method name is 'GracefulStop', that it is an extension method, - // and it is defined inside the GracefulStopSupport static class - return methodSymbol is { Name: "GracefulStop", IsExtensionMethod: true } - && SymbolEqualityComparer.Default.Equals(methodSymbol.ContainingType, akkaContext.AkkaCore.GracefulStopSupportType); + // Check if the method is an extension method + if(methodSymbol is not { IsExtensionMethod: true }) + return false; + + // Check that method is defined inside the GracefulStopSupport static class + if (!SymbolEqualityComparer.Default.Equals(methodSymbol.ContainingType, akkaContext.AkkaCore.GracefulStopSupportType)) + return false; + + return true; } } \ No newline at end of file diff --git a/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs b/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs index 06a7be9..b31d866 100644 --- a/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs +++ b/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs @@ -86,4 +86,88 @@ public static bool IsReceiveAsyncInvocation(this InvocationExpressionSyntax invo return methodSymbol.Name is "ReceiveAsync" or "ReceiveAnyAsync" && SymbolEqualityComparer.Default.Equals(methodSymbol.ContainingType, akkaContext.AkkaCore.ReceiveActorType); } + + public static bool IsAccessingActorSelf( + this InvocationExpressionSyntax invocationExpression, + SemanticModel semanticModel, + AkkaContext akkaContext) + { + // Expression need to be a member access + if (invocationExpression.Expression is not MemberAccessExpressionSyntax memberAccess) + return false; + + return IsAccessingActorBaseSelf(memberAccess, semanticModel, akkaContext) || + IsAccessingActorContextSelf(memberAccess, semanticModel, akkaContext); + } + + private static bool IsAccessingActorBaseSelf( + MemberAccessExpressionSyntax memberAccess, + SemanticModel semanticModel, + AkkaContext akkaContext) + { + // Method accesses an identifier called "Self" + if (memberAccess.Expression is not IdentifierNameSyntax { Identifier.Text: "Self" } identifier) + return false; + + // Make sure that `Self` identifier is a property + if (semanticModel.GetSymbolInfo(identifier).Symbol is not IPropertySymbol propertySymbol) + return false; + + // `Self` is a property declared inside `ActorBase` (ActorBase.Self) + if (SymbolEqualityComparer.Default.Equals(propertySymbol.ContainingType, akkaContext.AkkaCore.ActorBaseType)) + return true; + + return false; + } + + private static bool IsAccessingActorContextSelf( + MemberAccessExpressionSyntax memberAccess, + SemanticModel semanticModel, + AkkaContext akkaContext) + { + // The object being accessed by the invocation needs to be a member access itself + if (memberAccess.Expression is not MemberAccessExpressionSyntax selfMemberAccess) + return false; + + // Member access needs to be called "Self" + if (selfMemberAccess.Name.Identifier.Text is not "Self") + return false; + + // Self member access is accessing something that needs to derive from IActorContext + var symbol = semanticModel.GetSymbolInfo(selfMemberAccess.Expression).Symbol; + return symbol switch + { + IPropertySymbol p => p.Type.IsDerivedOrImplements(akkaContext.AkkaCore.ActorContextType!), + IFieldSymbol f => f.Type.IsDerivedOrImplements(akkaContext.AkkaCore.ActorContextType!), + ILocalSymbol l => l.Type.IsDerivedOrImplements(akkaContext.AkkaCore.ActorContextType!), + IParameterSymbol p => p.Type.IsDerivedOrImplements(akkaContext.AkkaCore.ActorContextType!), + IMethodSymbol m => m.ReturnType.IsDerivedOrImplements(akkaContext.AkkaCore.ActorContextType!), + _ => false + }; + } + + public static bool IsDerivedOrImplements(this ITypeSymbol typeSymbol, ITypeSymbol baseSymbol) + { + if (SymbolEqualityComparer.Default.Equals(typeSymbol, baseSymbol)) + return true; + + // Check interfaces directly implemented by the type + foreach (var interfaceType in typeSymbol.AllInterfaces) + { + if (IsDerivedOrImplements(interfaceType, baseSymbol)) + return true; + } + + // Recursively check base types + var baseType = typeSymbol.BaseType; + while (baseType != null) + { + if(IsDerivedOrImplements(baseType, baseSymbol)) + return true; + baseType = baseType.BaseType; + } + + return false; + } + } \ No newline at end of file