From 6760a696664bdf95bb7dccc7dd252bcd4cb9fb20 Mon Sep 17 00:00:00 2001 From: Gregorius Soedharmo Date: Wed, 13 Nov 2024 22:43:51 +0700 Subject: [PATCH 01/13] Add AK1008 - Check for multiple Stash.Stash() invocation --- ...nvokeStashMoreThanOnceInsideABlockSpecs.cs | 205 ++++++++++++++++++ .../MustNotInvokeStashMoreThanOnceAnalyzer.cs | 95 ++++++++ .../Context/Core/Actor/ActorSymbolFactory.cs | 4 + .../Core/Actor/AkkaCoreActorContext.cs | 7 + .../Core/Actor/IAkkaCoreActorContext.cs | 2 + .../Context/Core/Actor/IStashContext.cs | 44 ++++ .../Utility/CodeAnalysisExtensions.cs | 20 ++ src/Akka.Analyzers/Utility/RuleDescriptors.cs | 9 + 8 files changed, 386 insertions(+) create mode 100644 src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs create mode 100644 src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs create mode 100644 src/Akka.Analyzers/Context/Core/Actor/IStashContext.cs diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs new file mode 100644 index 0000000..3a32c0a --- /dev/null +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs @@ -0,0 +1,205 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2013-2024 .NET Foundation +// +// ----------------------------------------------------------------------- + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Xunit.Abstractions; +using Verify = Akka.Analyzers.Tests.Utility.AkkaVerifier; + +namespace Akka.Analyzers.Tests.Analyzers.AK1000; + +public class MustNotInvokeStashMoreThanOnceInsideABlockSpecs +{ + public static readonly TheoryData SuccessCases = new() + { + // ReceiveActor with single Stash() invocation +""" +// 01 +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor, IWithStash +{ + public MyActor() + { + Receive(str => { + Sender.Tell(str); + Stash.Stash(); // should not flag this + }); + } + + public void Handler() + { + Stash.Stash(); + } + + public IStash Stash { get; set; } +} +""", + + // Non-Actor class that has Stash() methods, we're not responsible for this. +""" +// 02 +public interface INonAkkaStash +{ + public void Stash(); +} + +public class NonAkkaStash : INonAkkaStash +{ + public void Stash() { } +} + +public sealed class MyActor +{ + public MyActor() + { + Stash = new NonAkkaStash(); + } + + public void Test() + { + Stash.Stash(); + Stash.Stash(); // should not flag this + } + + public INonAkkaStash Stash { get; } +} +""", + + // Non-Actor class that uses Stash(), + // we're only responsible for checking usage inside ActorBase class and its descendants. +""" +// 03 +using System; +using Akka.Actor; + +public class MyActor: IWithStash +{ + public MyActor(IStash stash) + { + Stash = stash; + } + + public void Test() + { + Stash.Stash(); + Stash.Stash(); // should not flag this + } + + public IStash Stash { get; set; } +} +""", + // Stash calls inside 2 different code branch +""" +// 04 +using Akka.Actor; + +public sealed class MyActor : ReceiveActor, IWithStash +{ + public MyActor(int n) + { + Receive(str => + { + if(n < 0) + { + Stash!.Stash(); + } + else + { + Stash!.Stash(); // should not flag this + } + }); + } + + public IStash Stash { get; set; } = null!; +} +""", + }; + + public static readonly + TheoryData<(string testData, (int startLine, int startColumn, int endLine, int endColumn) spanData)> + FailureCases = new() + { + // Receive actor invoking Stash() + ( +""" +// 01 +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor, IWithStash +{ + public MyActor() + { + Receive(str => + { + Stash.Stash(); + Stash.Stash(); // Error + }); + } + + public IStash Stash { get; set; } = null!; +} +""", (13, 13, 13, 26)), + + // Receive actor invoking Stash() inside and outside of a code branch + ( +""" +// 02 +using System; +using Akka.Actor; +using System.Threading.Tasks; + +public sealed class MyActor : ReceiveActor, IWithStash +{ + public MyActor(int n) + { + Receive(str => + { + if(n < 0) + { + Stash!.Stash(); + } + + Stash.Stash(); // Error + }); + } + + public IStash Stash { get; set; } = null!; +} +""", (12, 13, 12, 105)), + }; + + private readonly ITestOutputHelper _output; + + public MustNotInvokeStashMoreThanOnceInsideABlockSpecs(ITestOutputHelper output) + { + _output = output; + } + + [Theory] + [MemberData(nameof(SuccessCases))] + public Task SuccessCase(string testCode) + { + return Verify.VerifyAnalyzer(testCode); + } + + [Theory] + [MemberData(nameof(FailureCases))] + public Task FailureCase( + (string testCode, (int startLine, int startColumn, int endLine, int endColumn) spanData) d) + { + var expected = Verify.Diagnostic() + .WithSpan(d.spanData.startLine, d.spanData.startColumn, d.spanData.endLine, d.spanData.endColumn) + .WithSeverity(DiagnosticSeverity.Error); + + return Verify.VerifyAnalyzer(d.testCode, expected); + } + +} + diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs new file mode 100644 index 0000000..12313e8 --- /dev/null +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -0,0 +1,95 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2013-2024 .NET Foundation +// +// ----------------------------------------------------------------------- + +using Akka.Analyzers.Context; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.FlowAnalysis; +using Microsoft.CodeAnalysis.Operations; + +namespace Akka.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class MustNotInvokeStashMoreThanOnceAnalyzer() + : AkkaDiagnosticAnalyzer(RuleDescriptors.Ak1008MustNotInvokeStashMoreThanOnce) +{ + public override void AnalyzeCompilation(CompilationStartAnalysisContext context, AkkaContext akkaContext) + { + Guard.AssertIsNotNull(context); + Guard.AssertIsNotNull(akkaContext); + + context.RegisterSyntaxNodeAction(ctx => AnalyzeMethod(ctx, akkaContext), SyntaxKind.MethodDeclaration, SyntaxKind.ConstructorDeclaration); + } + + private static void AnalyzeMethod(SyntaxNodeAnalysisContext context, AkkaContext akkaContext) + { + var semanticModel = context.SemanticModel; + + // TODO: ControlFlowGraph does not recurse into local functions and lambda anonymous functions, how to grab those? + var controlFlowGraph = ControlFlowGraph.Create(context.Node, semanticModel); + + if (controlFlowGraph == null) + return; + + var stashMethod = akkaContext.AkkaCore.Actor.IStash.Stash!; + var stashInvocations = new Dictionary(); + + // Track Stash.Stash() calls inside each blocks + foreach (var block in controlFlowGraph.Blocks) + { + AnalyzeBlock(block, stashMethod, stashInvocations); + } + + var entryBlock = controlFlowGraph.Blocks.First(b => b.Kind == BasicBlockKind.Entry); + RecurseBlocks(entryBlock, stashInvocations, 0); + } + + private static void AnalyzeBlock(BasicBlock block, IMethodSymbol stashMethod, Dictionary stashInvocations) + { + var stashInvocationCount = 0; + + foreach (var operation in block.Descendants()) + { + switch (operation) + { + case IInvocationOperation invocation: + if(SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, stashMethod)) + stashInvocationCount++; + break; + + case IFlowAnonymousFunctionOperation flow: + // TODO: check for flow anonymous lambda function invocation + break; + + // TODO: check for local function invocation + } + } + + if(stashInvocationCount > 0) + stashInvocations.Add(block, stashInvocationCount); + } + + private static void RecurseBlocks(BasicBlock block, Dictionary stashInvocations, int totalInvocations) + { + if (stashInvocations.TryGetValue(block, out var blockInvocation)) + { + totalInvocations += blockInvocation; + } + + if (totalInvocations > 1) + { + // TODO: report diagnostic + } + + if(block.ConditionalSuccessor is { Destination: not null }) + RecurseBlocks(block.ConditionalSuccessor.Destination, stashInvocations, totalInvocations); + + if(block.FallThroughSuccessor is { Destination: not null }) + RecurseBlocks(block.FallThroughSuccessor.Destination, stashInvocations, totalInvocations); + } +} + diff --git a/src/Akka.Analyzers/Context/Core/Actor/ActorSymbolFactory.cs b/src/Akka.Analyzers/Context/Core/Actor/ActorSymbolFactory.cs index fddd99c..61bde79 100644 --- a/src/Akka.Analyzers/Context/Core/Actor/ActorSymbolFactory.cs +++ b/src/Akka.Analyzers/Context/Core/Actor/ActorSymbolFactory.cs @@ -51,4 +51,8 @@ public static class ActorSymbolFactory public static INamedTypeSymbol? TimerSchedulerInterface(Compilation compilation) => Guard.AssertIsNotNull(compilation) .GetTypeByMetadataName($"{AkkaActorNamespace}.ITimerScheduler"); + + public static INamedTypeSymbol? StashInterface(Compilation compilation) + => Guard.AssertIsNotNull(compilation) + .GetTypeByMetadataName($"{AkkaActorNamespace}.IStash"); } \ No newline at end of file diff --git a/src/Akka.Analyzers/Context/Core/Actor/AkkaCoreActorContext.cs b/src/Akka.Analyzers/Context/Core/Actor/AkkaCoreActorContext.cs index b3f25bb..a3a8719 100644 --- a/src/Akka.Analyzers/Context/Core/Actor/AkkaCoreActorContext.cs +++ b/src/Akka.Analyzers/Context/Core/Actor/AkkaCoreActorContext.cs @@ -23,6 +23,7 @@ private EmptyAkkaCoreActorContext() { } public INamedTypeSymbol? ITellSchedulerType => null; public INamedTypeSymbol? ActorRefsType => null; public INamedTypeSymbol? ITimerSchedulerType => null; + public INamedTypeSymbol? IStashType => null; public IGracefulStopSupportContext GracefulStopSupportSupport => EmptyGracefulStopSupportContext.Instance; public IIndirectActorProducerContext IIndirectActorProducer => EmptyIndirectActorProducerContext.Instance; @@ -33,6 +34,7 @@ private EmptyAkkaCoreActorContext() { } public ITellSchedulerInterfaceContext ITellScheduler => EmptyTellSchedulerInterfaceContext.Instance; public IActorRefsContext ActorRefs => EmptyActorRefsContext.Empty; public ITimerSchedulerContext ITimerScheduler => EmptyTimerSchedulerContext.Instance; + public IStashContext IStash => EmptyStashContext.Instance; } public sealed class AkkaCoreActorContext : IAkkaCoreActorContext @@ -47,6 +49,7 @@ public sealed class AkkaCoreActorContext : IAkkaCoreActorContext private readonly Lazy _lazyTellSchedulerInterface; private readonly Lazy _lazyActorRefsType; private readonly Lazy _lazyITimerSchedulerType; + private readonly Lazy _lazyIStashType; private readonly Lazy _lazyGracefulStopSupport; private readonly Lazy _lazyIIndirectActorProducer; @@ -67,6 +70,7 @@ private AkkaCoreActorContext(Compilation compilation) _lazyTellSchedulerInterface = new Lazy(() => ActorSymbolFactory.TellSchedulerInterface(compilation)); _lazyActorRefsType = new Lazy(() => ActorSymbolFactory.ActorRefs(compilation)); _lazyITimerSchedulerType = new Lazy(() => ActorSymbolFactory.TimerSchedulerInterface(compilation)); + _lazyIStashType = new Lazy(() => ActorSymbolFactory.StashInterface(compilation)); _lazyGracefulStopSupport = new Lazy(() => GracefulStopSupportContext.Get(this)); _lazyIIndirectActorProducer = new Lazy(() => IndirectActorProducerContext.Get(this)); @@ -77,6 +81,7 @@ private AkkaCoreActorContext(Compilation compilation) ITellScheduler = TellSchedulerInterfaceContext.Get(compilation); ActorRefs = ActorRefsContext.Get(this); ITimerScheduler = TimerSchedulerContext.Get(this); + IStash = StashContext.Get(this); } public INamedTypeSymbol? ActorBaseType => _lazyActorBaseType.Value; @@ -89,6 +94,7 @@ private AkkaCoreActorContext(Compilation compilation) public INamedTypeSymbol? ActorRefsType => _lazyActorRefsType.Value; public INamedTypeSymbol? GracefulStopSupportType => _lazyGracefulStopSupportType.Value; public INamedTypeSymbol? ITimerSchedulerType => _lazyITimerSchedulerType.Value; + public INamedTypeSymbol? IStashType => _lazyIStashType.Value; public IGracefulStopSupportContext GracefulStopSupportSupport => _lazyGracefulStopSupport.Value; public IIndirectActorProducerContext IIndirectActorProducer => _lazyIIndirectActorProducer.Value; @@ -99,6 +105,7 @@ private AkkaCoreActorContext(Compilation compilation) public ITellSchedulerInterfaceContext ITellScheduler { get; } public IActorRefsContext ActorRefs { get; } public ITimerSchedulerContext ITimerScheduler { get; } + public IStashContext IStash { get; } public static IAkkaCoreActorContext Get(Compilation compilation) => new AkkaCoreActorContext(compilation); diff --git a/src/Akka.Analyzers/Context/Core/Actor/IAkkaCoreActorContext.cs b/src/Akka.Analyzers/Context/Core/Actor/IAkkaCoreActorContext.cs index 0ab403e..48e7902 100644 --- a/src/Akka.Analyzers/Context/Core/Actor/IAkkaCoreActorContext.cs +++ b/src/Akka.Analyzers/Context/Core/Actor/IAkkaCoreActorContext.cs @@ -22,6 +22,7 @@ public interface IAkkaCoreActorContext public INamedTypeSymbol? ITellSchedulerType { get; } public INamedTypeSymbol? ActorRefsType { get; } public INamedTypeSymbol? ITimerSchedulerType { get; } + public INamedTypeSymbol? IStashType { get; } public IGracefulStopSupportContext GracefulStopSupportSupport { get; } public IIndirectActorProducerContext IIndirectActorProducer { get; } @@ -32,4 +33,5 @@ public interface IAkkaCoreActorContext public ITellSchedulerInterfaceContext ITellScheduler { get; } public IActorRefsContext ActorRefs { get; } public ITimerSchedulerContext ITimerScheduler { get; } + public IStashContext IStash { get; } } diff --git a/src/Akka.Analyzers/Context/Core/Actor/IStashContext.cs b/src/Akka.Analyzers/Context/Core/Actor/IStashContext.cs new file mode 100644 index 0000000..7ae4da6 --- /dev/null +++ b/src/Akka.Analyzers/Context/Core/Actor/IStashContext.cs @@ -0,0 +1,44 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2013-2024 .NET Foundation +// +// ----------------------------------------------------------------------- + +using System.Collections.Immutable; +using Akka.Analyzers.Context.Core.Actor; +using Microsoft.CodeAnalysis; + +namespace Akka.Analyzers.Core.Actor; + +// ReSharper disable once InconsistentNaming +public interface IStashContext +{ + public IMethodSymbol? Stash { get; } +} + +public sealed class EmptyStashContext : IStashContext +{ + public static readonly EmptyStashContext Instance = new(); + private EmptyStashContext() { } + public IMethodSymbol? Stash => null; +} + +public sealed class StashContext : IStashContext +{ + private readonly Lazy _lazyStash; + + public IMethodSymbol Stash => _lazyStash.Value; + + private StashContext(AkkaCoreActorContext context) + { + Guard.AssertIsNotNull(context); + _lazyStash = new Lazy(() => (IMethodSymbol) context.IStashType! + .GetMembers(nameof(Stash)).First()); + } + + public static StashContext Get(AkkaCoreActorContext context) + { + Guard.AssertIsNotNull(context); + return new StashContext(context); + } +} \ No newline at end of file diff --git a/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs b/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs index cfbe9f7..d694761 100644 --- a/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs +++ b/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs @@ -10,6 +10,7 @@ using Akka.Analyzers.Context.Core.Actor; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.FlowAnalysis; namespace Akka.Analyzers; @@ -216,4 +217,23 @@ public static bool MatchesAny(this IMethodSymbol methodSymbol, IReadOnlyCollecti return refMethods.Any(m => ReferenceEquals(m, methodSymbol)); } + public static List Descendants(this BasicBlock block) + { + var descendants = new List(); + foreach (var operation in block.Operations) + { + RecurseOperation(operation, descendants); + } + return descendants; + + static void RecurseOperation(IOperation operation, List descendants) + { + descendants.Add(operation); + foreach (var childOperation in operation.ChildOperations) + { + RecurseOperation(childOperation, descendants); + } + } + } + } \ No newline at end of file diff --git a/src/Akka.Analyzers/Utility/RuleDescriptors.cs b/src/Akka.Analyzers/Utility/RuleDescriptors.cs index 0d2ae1d..4ac47ce 100644 --- a/src/Akka.Analyzers/Utility/RuleDescriptors.cs +++ b/src/Akka.Analyzers/Utility/RuleDescriptors.cs @@ -87,6 +87,15 @@ private static DiagnosticDescriptor Rule( defaultSeverity: DiagnosticSeverity.Error, messageFormat: "Creating timer registration using `{0}()` in `{1}()` will not be honored because they will be " + "cleared immediately. Move timer creation to `PostRestart()` instead."); + + public static DiagnosticDescriptor Ak1008MustNotInvokeStashMoreThanOnce { get; } = Rule( + id: "AK1008", + title: "Stash.Stash() must not be called more than once", + category: AnalysisCategory.ActorDesign, + defaultSeverity: DiagnosticSeverity.Error, + messageFormat: "Stash.Stash() must not be called more than once because it will create duplicate " + + "messages during unstash which violates message ordering immutability."); + #endregion #region AK2000 Rules From 892dce491a79b419456498cdedd66e7ed9a600d8 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 06:18:51 -0600 Subject: [PATCH 02/13] Try fixing `AK1008` using simpler constructs --- .../MustNotInvokeStashMoreThanOnceAnalyzer.cs | 96 ++++++++++--------- .../Utility/CodeAnalysisExtensions.cs | 20 ---- src/Akka.Analyzers/Utility/RuleDescriptors.cs | 4 +- 3 files changed, 51 insertions(+), 69 deletions(-) diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index 12313e8..0b0c3f9 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -7,6 +7,7 @@ using Akka.Analyzers.Context; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.FlowAnalysis; using Microsoft.CodeAnalysis.Operations; @@ -28,68 +29,69 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, private static void AnalyzeMethod(SyntaxNodeAnalysisContext context, AkkaContext akkaContext) { var semanticModel = context.SemanticModel; + var methodDeclaration = (MethodDeclarationSyntax)context.Node; - // TODO: ControlFlowGraph does not recurse into local functions and lambda anonymous functions, how to grab those? - var controlFlowGraph = ControlFlowGraph.Create(context.Node, semanticModel); - - if (controlFlowGraph == null) - return; + // SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, stashMethod) var stashMethod = akkaContext.AkkaCore.Actor.IStash.Stash!; var stashInvocations = new Dictionary(); - // Track Stash.Stash() calls inside each blocks - foreach (var block in controlFlowGraph.Blocks) - { - AnalyzeBlock(block, stashMethod, stashInvocations); - } - - var entryBlock = controlFlowGraph.Blocks.First(b => b.Kind == BasicBlockKind.Entry); - RecurseBlocks(entryBlock, stashInvocations, 0); - } - - private static void AnalyzeBlock(BasicBlock block, IMethodSymbol stashMethod, Dictionary stashInvocations) - { - var stashInvocationCount = 0; + // Find all "Stash.Stash()" calls in the method + var stashCalls = methodDeclaration.DescendantNodes() + .OfType() + .Where(invocation => IsStashInvocation(semanticModel, invocation, stashMethod)); + + // Group calls by their parent block + var callsGroupedByBlock = stashCalls + .GroupBy(GetContainingBlock) + .Where(group => group.Key != null); - foreach (var operation in block.Descendants()) + foreach (var group in callsGroupedByBlock) { - switch (operation) + var callsInBlock = group.ToList(); + + // can't be null - we check for that on line 47 + if (callsInBlock.Count > 1 && !AreCallsSeparatedByConditionals(group.Key!, callsInBlock)) { - case IInvocationOperation invocation: - if(SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, stashMethod)) - stashInvocationCount++; - break; - - case IFlowAnonymousFunctionOperation flow: - // TODO: check for flow anonymous lambda function invocation - break; - - // TODO: check for local function invocation + // we could skip the first stash call here, but since we don't know _which_ call is the offending + // duplicate, we'll just report all of them + foreach (var stashCall in callsInBlock) + { + var diagnostic = Diagnostic.Create( + descriptor:RuleDescriptors.Ak1008MustNotInvokeStashMoreThanOnce, + location:stashCall.GetLocation()); + context.ReportDiagnostic(diagnostic); + } } } - - if(stashInvocationCount > 0) - stashInvocations.Add(block, stashInvocationCount); } - - private static void RecurseBlocks(BasicBlock block, Dictionary stashInvocations, int totalInvocations) + + private static bool IsStashInvocation(SemanticModel model, InvocationExpressionSyntax invocation, IMethodSymbol stashMethod) { - if (stashInvocations.TryGetValue(block, out var blockInvocation)) - { - totalInvocations += blockInvocation; - } + var symbol = model.GetSymbolInfo(invocation).Symbol; + return SymbolEqualityComparer.Default.Equals(symbol, stashMethod); + } + + private static BlockSyntax? GetContainingBlock(SyntaxNode node) + { + return node.AncestorsAndSelf().OfType().FirstOrDefault(); + } + + private static bool AreCallsSeparatedByConditionals(BlockSyntax block, List calls) + { + var conditionals = block.DescendantNodes().OfType().ToList(); - if (totalInvocations > 1) + foreach (var call in calls) { - // TODO: report diagnostic + bool isInConditional = conditionals.Any(ifStatement => ifStatement.Contains(call)); + + if (!isInConditional) + { + return false; + } } - - if(block.ConditionalSuccessor is { Destination: not null }) - RecurseBlocks(block.ConditionalSuccessor.Destination, stashInvocations, totalInvocations); - - if(block.FallThroughSuccessor is { Destination: not null }) - RecurseBlocks(block.FallThroughSuccessor.Destination, stashInvocations, totalInvocations); + + return true; } } diff --git a/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs b/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs index d694761..4c353d3 100644 --- a/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs +++ b/src/Akka.Analyzers/Utility/CodeAnalysisExtensions.cs @@ -216,24 +216,4 @@ public static bool MatchesAny(this IMethodSymbol methodSymbol, IReadOnlyCollecti return refMethods.Any(m => ReferenceEquals(m, methodSymbol)); } - - public static List Descendants(this BasicBlock block) - { - var descendants = new List(); - foreach (var operation in block.Operations) - { - RecurseOperation(operation, descendants); - } - return descendants; - - static void RecurseOperation(IOperation operation, List descendants) - { - descendants.Add(operation); - foreach (var childOperation in operation.ChildOperations) - { - RecurseOperation(childOperation, descendants); - } - } - } - } \ No newline at end of file diff --git a/src/Akka.Analyzers/Utility/RuleDescriptors.cs b/src/Akka.Analyzers/Utility/RuleDescriptors.cs index 4ac47ce..341b2a3 100644 --- a/src/Akka.Analyzers/Utility/RuleDescriptors.cs +++ b/src/Akka.Analyzers/Utility/RuleDescriptors.cs @@ -93,8 +93,8 @@ private static DiagnosticDescriptor Rule( title: "Stash.Stash() must not be called more than once", category: AnalysisCategory.ActorDesign, defaultSeverity: DiagnosticSeverity.Error, - messageFormat: "Stash.Stash() must not be called more than once because it will create duplicate " + - "messages during unstash which violates message ordering immutability."); + messageFormat: "Stash.Stash() must not be called more than once because it will cause runtime errors to be thrown, " + + "due to the Stash actively preventing multiple calls to Stash() creating duplicate messages."); #endregion From b88f35cfcf13bea0fdc933da16c1fc0cbc9778de Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 06:20:15 -0600 Subject: [PATCH 03/13] have some tests passing --- .../AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index 0b0c3f9..211b8bd 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -23,7 +23,7 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, Guard.AssertIsNotNull(context); Guard.AssertIsNotNull(akkaContext); - context.RegisterSyntaxNodeAction(ctx => AnalyzeMethod(ctx, akkaContext), SyntaxKind.MethodDeclaration, SyntaxKind.ConstructorDeclaration); + context.RegisterSyntaxNodeAction(ctx => AnalyzeMethod(ctx, akkaContext), SyntaxKind.MethodDeclaration); } private static void AnalyzeMethod(SyntaxNodeAnalysisContext context, AkkaContext akkaContext) From 0e272a2e4ea3f4b36c3abc8b725afc621f5e3051 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 06:27:44 -0600 Subject: [PATCH 04/13] added check to skip comparisons on non-actor classes --- .../MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs | 2 +- .../AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs index 3a32c0a..a4dac30 100644 --- a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs @@ -77,7 +77,7 @@ public void Test() using System; using Akka.Actor; -public class MyActor: IWithStash +public class MyActor { public MyActor(IStash stash) { diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index 211b8bd..e8b3130 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -32,9 +32,13 @@ private static void AnalyzeMethod(SyntaxNodeAnalysisContext context, AkkaContext var methodDeclaration = (MethodDeclarationSyntax)context.Node; // SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, stashMethod) + + // First: need to check if this method is declared in an ActorBase subclass + var methodSymbol = semanticModel.GetDeclaredSymbol(methodDeclaration); + if (methodSymbol is null || !methodSymbol.ContainingType.IsActorBaseSubclass(akkaContext.AkkaCore)) + return; var stashMethod = akkaContext.AkkaCore.Actor.IStash.Stash!; - var stashInvocations = new Dictionary(); // Find all "Stash.Stash()" calls in the method var stashCalls = methodDeclaration.DescendantNodes() From dd6c43e720935df6fb1768741ca444dabe01c329 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 06:57:58 -0600 Subject: [PATCH 05/13] fixed `UntypedActor` failure cases --- ...nvokeStashMoreThanOnceInsideABlockSpecs.cs | 336 ++++++++++-------- 1 file changed, 193 insertions(+), 143 deletions(-) diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs index a4dac30..2befc53 100644 --- a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs @@ -6,6 +6,7 @@ using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Testing; using Xunit.Abstractions; using Verify = Akka.Analyzers.Tests.Utility.AkkaVerifier; @@ -16,172 +17,219 @@ public class MustNotInvokeStashMoreThanOnceInsideABlockSpecs public static readonly TheoryData SuccessCases = new() { // ReceiveActor with single Stash() invocation -""" -// 01 -using Akka.Actor; -using System.Threading.Tasks; + """ + // 01 + using Akka.Actor; + using System.Threading.Tasks; -public sealed class MyActor : ReceiveActor, IWithStash -{ - public MyActor() - { - Receive(str => { - Sender.Tell(str); - Stash.Stash(); // should not flag this - }); - } - - public void Handler() - { - Stash.Stash(); - } - - public IStash Stash { get; set; } -} -""", + public sealed class MyActor : ReceiveActor, IWithStash + { + public MyActor() + { + Receive(str => { + Sender.Tell(str); + Stash.Stash(); // should not flag this + }); + } + + public void Handler() + { + Stash.Stash(); + } + + public IStash Stash { get; set; } + } + """, // Non-Actor class that has Stash() methods, we're not responsible for this. -""" -// 02 -public interface INonAkkaStash -{ - public void Stash(); -} - -public class NonAkkaStash : INonAkkaStash -{ - public void Stash() { } -} + """ + // 02 + public interface INonAkkaStash + { + public void Stash(); + } -public sealed class MyActor -{ - public MyActor() - { - Stash = new NonAkkaStash(); - } + public class NonAkkaStash : INonAkkaStash + { + public void Stash() { } + } - public void Test() - { - Stash.Stash(); - Stash.Stash(); // should not flag this - } - - public INonAkkaStash Stash { get; } -} -""", + public sealed class MyActor + { + public MyActor() + { + Stash = new NonAkkaStash(); + } + + public void Test() + { + Stash.Stash(); + Stash.Stash(); // should not flag this + } + + public INonAkkaStash Stash { get; } + } + """, // Non-Actor class that uses Stash(), // we're only responsible for checking usage inside ActorBase class and its descendants. -""" -// 03 -using System; -using Akka.Actor; - -public class MyActor -{ - public MyActor(IStash stash) - { - Stash = stash; - } + """ + // 03 + using System; + using Akka.Actor; - public void Test() - { - Stash.Stash(); - Stash.Stash(); // should not flag this - } - - public IStash Stash { get; set; } -} -""", - // Stash calls inside 2 different code branch -""" -// 04 -using Akka.Actor; - -public sealed class MyActor : ReceiveActor, IWithStash -{ - public MyActor(int n) - { - Receive(str => + public class MyActor { - if(n < 0) + public MyActor(IStash stash) { - Stash!.Stash(); + Stash = stash; } - else + + public void Test() { - Stash!.Stash(); // should not flag this + Stash.Stash(); + Stash.Stash(); // should not flag this } - }); - } + + public IStash Stash { get; set; } + } + """, + // Stash calls inside 2 different code branch + """ + // 04 + using Akka.Actor; - public IStash Stash { get; set; } = null!; -} -""", + public sealed class MyActor : ReceiveActor, IWithStash + { + public MyActor(int n) + { + Receive(str => + { + if(n < 0) + { + Stash!.Stash(); + } + else + { + Stash!.Stash(); // should not flag this + } + }); + } + + public IStash Stash { get; set; } = null!; + } + """, }; public static readonly - TheoryData<(string testData, (int startLine, int startColumn, int endLine, int endColumn) spanData)> + TheoryData<(string testData, (int startLine, int startColumn, int endLine, int endColumn)[] spanData)> FailureCases = new() { // Receive actor invoking Stash() ( -""" -// 01 -using System; -using Akka.Actor; -using System.Threading.Tasks; - -public sealed class MyActor : ReceiveActor, IWithStash -{ - public MyActor() - { - Receive(str => - { - Stash.Stash(); - Stash.Stash(); // Error - }); - } + """ + // 01 + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor, IWithStash + { + public MyActor() + { + Receive(str => + { + Stash.Stash(); + Stash.Stash(); // Error + }); + } + + public IStash Stash { get; set; } = null!; + } + """, [ + (13, 13, 13, 26), + (14, 13, 14, 26) + ]), - public IStash Stash { get; set; } = null!; -} -""", (13, 13, 13, 26)), - // Receive actor invoking Stash() inside and outside of a code branch ( -""" -// 02 -using System; -using Akka.Actor; -using System.Threading.Tasks; - -public sealed class MyActor : ReceiveActor, IWithStash -{ - public MyActor(int n) - { - Receive(str => - { - if(n < 0) - { - Stash!.Stash(); - } - - Stash.Stash(); // Error - }); - } + """ + // 02 + using System; + using Akka.Actor; + using System.Threading.Tasks; + + public sealed class MyActor : ReceiveActor, IWithStash + { + public MyActor(int n) + { + Receive(str => + { + if(n < 0) + { + Stash!.Stash(); + } + + Stash.Stash(); // Error + }); + } + + public IStash Stash { get; set; } = null!; + } + """, [(12, 13, 12, 105), + (15, 13, 15, 26)]), + + // UntypedActor invoking Stash() twice without branching + ( + """ + // 03 + using Akka.Actor; + + public class MyUntypedActor : UntypedActor, IWithStash + { + protected override void OnReceive(object message) + { + Stash.Stash(); + Stash.Stash(); // Error + } + + public IStash Stash { get; set; } = null!; + } + """, [(8, 9, 8, 22), + (9, 9, 9, 22)]), + // UntypedActor invoking Stash() twice with a switch, but one is outside + ( + """ + // 04 + using Akka.Actor; + + public class MyUntypedActor : UntypedActor, IWithStash + { + protected override void OnReceive(object message) + { + Stash.Stash(); + + switch(message) + { + case string s: + Stash.Stash(); // Error + break; + } + } + + public IStash Stash { get; set; } = null!; + } + """, [(8, 9, 8, 22), + (13, 17, 13, 30)]), + }; - public IStash Stash { get; set; } = null!; -} -""", (12, 13, 12, 105)), - }; - private readonly ITestOutputHelper _output; - + public MustNotInvokeStashMoreThanOnceInsideABlockSpecs(ITestOutputHelper output) { _output = output; } - + [Theory] [MemberData(nameof(SuccessCases))] public Task SuccessCase(string testCode) @@ -192,14 +240,16 @@ public Task SuccessCase(string testCode) [Theory] [MemberData(nameof(FailureCases))] public Task FailureCase( - (string testCode, (int startLine, int startColumn, int endLine, int endColumn) spanData) d) + (string testCode, (int startLine, int startColumn, int endLine, int endColumn)[] spanData) d) { - var expected = Verify.Diagnostic() - .WithSpan(d.spanData.startLine, d.spanData.startColumn, d.spanData.endLine, d.spanData.endColumn) - .WithSeverity(DiagnosticSeverity.Error); + List expectedResults = new(); + + foreach(var (startLine, startColumn, endLine, endColumn) in d.spanData) + { + var expected = Verify.Diagnostic().WithSpan(startLine, startColumn, endLine, endColumn).WithSeverity(DiagnosticSeverity.Error); + expectedResults.Add(expected); + } - return Verify.VerifyAnalyzer(d.testCode, expected); + return Verify.VerifyAnalyzer(d.testCode, expectedResults.ToArray()); } - -} - +} \ No newline at end of file From fd8351f4ed9895e0ed13f646f720e50a3258112e Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 07:28:32 -0600 Subject: [PATCH 06/13] have `ReceiveActor` cases working --- ...nvokeStashMoreThanOnceInsideABlockSpecs.cs | 4 +- .../MustNotInvokeStashMoreThanOnceAnalyzer.cs | 68 ++++++++++++++----- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs index 2befc53..3b4f1ad 100644 --- a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs @@ -147,8 +147,8 @@ public MyActor() public IStash Stash { get; set; } = null!; } """, [ - (13, 13, 13, 26), - (14, 13, 14, 26) + (12, 13, 12, 26), + (13, 13, 13, 26) ]), // Receive actor invoking Stash() inside and outside of a code branch diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index e8b3130..6aa2655 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -23,25 +23,39 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, Guard.AssertIsNotNull(context); Guard.AssertIsNotNull(akkaContext); - context.RegisterSyntaxNodeAction(ctx => AnalyzeMethod(ctx, akkaContext), SyntaxKind.MethodDeclaration); + // For methods inside the actor + context.RegisterSyntaxNodeAction(ctx => AnalyzeMethodDeclaration(ctx, akkaContext), SyntaxKind.MethodDeclaration); + + // For lambdas, namely Receive and ReceiveAny + context.RegisterSyntaxNodeAction(ctx => + { + var invocationExpr = (InvocationExpressionSyntax)ctx.Node; + var semanticModel = ctx.SemanticModel; + var akkaCore = akkaContext.AkkaCore; + var stashMethod = akkaCore.Actor.IStash.Stash!; + + var invocationSymbol = semanticModel.GetSymbolInfo(invocationExpr.Expression).Symbol; + + // if this invocation expression is not invoking a method OR it's not part of an actor base type, skip + if (invocationSymbol is not null && invocationSymbol.ContainingType.IsActorBaseSubclass(akkaCore)) + { + // if we've made it here, we are inside a context where at least one Stash call has been found + // scope out and see if there are any other stash calls inside the same branch + var invocationParent = invocationExpr.DescendantNodes(); + + DiagnoseSyntaxNodes(invocationParent, semanticModel, stashMethod, ctx); + } + + + + }, SyntaxKind.InvocationExpression); } - private static void AnalyzeMethod(SyntaxNodeAnalysisContext context, AkkaContext akkaContext) + private static void DiagnoseSyntaxNodes(IEnumerable invocationParent, SemanticModel semanticModel, + IMethodSymbol stashMethod, SyntaxNodeAnalysisContext ctx) { - var semanticModel = context.SemanticModel; - var methodDeclaration = (MethodDeclarationSyntax)context.Node; - - // SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, stashMethod) - - // First: need to check if this method is declared in an ActorBase subclass - var methodSymbol = semanticModel.GetDeclaredSymbol(methodDeclaration); - if (methodSymbol is null || !methodSymbol.ContainingType.IsActorBaseSubclass(akkaContext.AkkaCore)) - return; - - var stashMethod = akkaContext.AkkaCore.Actor.IStash.Stash!; - - // Find all "Stash.Stash()" calls in the method - var stashCalls = methodDeclaration.DescendantNodes() + // Find all "Stash.Stash()" calls in the method + var stashCalls = invocationParent .OfType() .Where(invocation => IsStashInvocation(semanticModel, invocation, stashMethod)); @@ -64,11 +78,29 @@ private static void AnalyzeMethod(SyntaxNodeAnalysisContext context, AkkaContext var diagnostic = Diagnostic.Create( descriptor:RuleDescriptors.Ak1008MustNotInvokeStashMoreThanOnce, location:stashCall.GetLocation()); - context.ReportDiagnostic(diagnostic); + ctx.ReportDiagnostic(diagnostic); } } } } + + private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context, AkkaContext akkaContext) + { + var semanticModel = context.SemanticModel; + var methodDeclaration = (MethodDeclarationSyntax)context.Node; + + // SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, stashMethod) + + // First: need to check if this method is declared in an ActorBase subclass + var methodSymbol = semanticModel.GetDeclaredSymbol(methodDeclaration); + if (methodSymbol is null || !methodSymbol.ContainingType.IsActorBaseSubclass(akkaContext.AkkaCore)) + return; + + var stashMethod = akkaContext.AkkaCore.Actor.IStash.Stash!; + + var invocationParent = methodDeclaration.DescendantNodes(); + DiagnoseSyntaxNodes(invocationParent, semanticModel, stashMethod, context); + } private static bool IsStashInvocation(SemanticModel model, InvocationExpressionSyntax invocation, IMethodSymbol stashMethod) { @@ -87,7 +119,7 @@ private static bool AreCallsSeparatedByConditionals(BlockSyntax block, List ifStatement.Contains(call)); + var isInConditional = conditionals.Any(ifStatement => ifStatement.Contains(call)); if (!isInConditional) { From d9ef886af69d2c680e96ce1bc2cf8931a55ef9ff Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 08:02:38 -0600 Subject: [PATCH 07/13] short-circuit calls --- .../MustNotInvokeStashMoreThanOnceAnalyzer.cs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index 6aa2655..877a660 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -43,7 +43,7 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, // scope out and see if there are any other stash calls inside the same branch var invocationParent = invocationExpr.DescendantNodes(); - DiagnoseSyntaxNodes(invocationParent, semanticModel, stashMethod, ctx); + DiagnoseSyntaxNodes(invocationParent.ToArray(), semanticModel, stashMethod, ctx); } @@ -51,18 +51,22 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, }, SyntaxKind.InvocationExpression); } - private static void DiagnoseSyntaxNodes(IEnumerable invocationParent, SemanticModel semanticModel, + private static void DiagnoseSyntaxNodes(IReadOnlyList invocationParent, SemanticModel semanticModel, IMethodSymbol stashMethod, SyntaxNodeAnalysisContext ctx) { // Find all "Stash.Stash()" calls in the method var stashCalls = invocationParent .OfType() - .Where(invocation => IsStashInvocation(semanticModel, invocation, stashMethod)); + .Where(invocation => IsStashInvocation(semanticModel, invocation, stashMethod)).ToArray(); + + // aren't enough calls to merit further analysis + if (stashCalls.Length < 2) + return; // Group calls by their parent block var callsGroupedByBlock = stashCalls .GroupBy(GetContainingBlock) - .Where(group => group.Key != null); + .Where(group => group.Key != null).ToArray(); foreach (var group in callsGroupedByBlock) { @@ -99,7 +103,7 @@ private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context, var stashMethod = akkaContext.AkkaCore.Actor.IStash.Stash!; var invocationParent = methodDeclaration.DescendantNodes(); - DiagnoseSyntaxNodes(invocationParent, semanticModel, stashMethod, context); + DiagnoseSyntaxNodes(invocationParent.ToArray(), semanticModel, stashMethod, context); } private static bool IsStashInvocation(SemanticModel model, InvocationExpressionSyntax invocation, IMethodSymbol stashMethod) From e0354ec6ac02b271133485f97e662560bfb3fa67 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 08:08:06 -0600 Subject: [PATCH 08/13] fix --- ...nvokeStashMoreThanOnceInsideABlockSpecs.cs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs index 3b4f1ad..b3fd6f5 100644 --- a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs @@ -221,6 +221,29 @@ protected override void OnReceive(object message) } """, [(8, 9, 8, 22), (13, 17, 13, 30)]), + + // UntypedActor invoking Stash() twice with a switch, but one is outside + ( + """ + // 05 + using Akka.Actor; + + public class MyUntypedActor : UntypedActor, IWithStash + { + protected override void OnReceive(object message) + { + Stash.Stash(); + + if(message is string s) + { + Stash.Stash(); // Error + } + } + + public IStash Stash { get; set; } = null!; + } + """, [(8, 9, 8, 22), + (13, 17, 13, 30)]), }; private readonly ITestOutputHelper _output; From b331e28eb94832b1c04bcbaafbe04094cda98112 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 09:44:21 -0600 Subject: [PATCH 09/13] fixed specs --- .../AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs index b3fd6f5..cc5f40f 100644 --- a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs @@ -177,7 +177,7 @@ public MyActor(int n) public IStash Stash { get; set; } = null!; } """, [(12, 13, 12, 105), - (15, 13, 15, 26)]), + (14, 13, 14, 26)]), // UntypedActor invoking Stash() twice without branching ( @@ -243,7 +243,7 @@ protected override void OnReceive(object message) public IStash Stash { get; set; } = null!; } """, [(8, 9, 8, 22), - (13, 17, 13, 30)]), + (12, 17, 12, 30)]), }; private readonly ITestOutputHelper _output; From 182f7aaecfc67c34435d9e8517424a2a2cea2408 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 09:46:09 -0600 Subject: [PATCH 10/13] added a positive failure case for `switch` statements --- ...nvokeStashMoreThanOnceInsideABlockSpecs.cs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs index cc5f40f..c827da5 100644 --- a/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs +++ b/src/Akka.Analyzers.Tests/Analyzers/AK1000/MustNotInvokeStashMoreThanOnceInsideABlockSpecs.cs @@ -119,6 +119,32 @@ public MyActor(int n) public IStash Stash { get; set; } = null!; } """, + + // Stash calls inside 2 different code branch (switch statements) + """ + // 05 + using Akka.Actor; + + public sealed class MyActor : ReceiveActor, IWithStash + { + public MyActor(int n) + { + Receive(str => + { + switch(str){ + case null: + Stash!.Stash(); + break; + default: + Stash!.Stash(); // should not flag this + break; + } + }); + } + + public IStash Stash { get; set; } = null!; + } + """, }; public static readonly From 5f17a3669924824fbfbee00fd8a2bd08d100f716 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 09:47:46 -0600 Subject: [PATCH 11/13] some fixes --- .../MustNotInvokeStashMoreThanOnceAnalyzer.cs | 61 ++++++++++++++++--- 1 file changed, 51 insertions(+), 10 deletions(-) diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index 877a660..8f76011 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -63,38 +63,79 @@ private static void DiagnoseSyntaxNodes(IReadOnlyList invocationPare if (stashCalls.Length < 2) return; + foreach(var stashCall in stashCalls) + { + stashCall.GetLocation(); + } + // Group calls by their parent block var callsGroupedByBlock = stashCalls .GroupBy(GetContainingBlock) .Where(group => group.Key != null).ToArray(); + // so we don't log multiple warnings for the same call, in the event someone has done something truly stupid + var alreadyTouched = new Dictionary(); + foreach (var group in callsGroupedByBlock) { var callsInBlock = group.ToList(); - // can't be null - we check for that on line 47 - if (callsInBlock.Count > 1 && !AreCallsSeparatedByConditionals(group.Key!, callsInBlock)) + // simple / dumb check - are there multiple calls to Stash in the same block? + if (callsInBlock.Count > 1) { - // we could skip the first stash call here, but since we don't know _which_ call is the offending - // duplicate, we'll just report all of them foreach (var stashCall in callsInBlock) { - var diagnostic = Diagnostic.Create( - descriptor:RuleDescriptors.Ak1008MustNotInvokeStashMoreThanOnce, - location:stashCall.GetLocation()); - ctx.ReportDiagnostic(diagnostic); + ReportIfNotDoneAlready(alreadyTouched, stashCall); } } + + // depth check + var block = group.Key!; + var descendantCalls = block.DescendantNodes().OfType() + .Where(invocation => IsStashInvocation(semanticModel, invocation, stashMethod)) + .ToList(); + + // if there are no other Stash calls in the block, we can skip this block + if (descendantCalls.Count <= 1) + continue; + + // if there are other Stash calls in the block, they're bad - flag em + foreach (var descendantCall in descendantCalls) + { + ReportIfNotDoneAlready(alreadyTouched, descendantCall); + } + + // also need to flag the original call + ReportIfNotDoneAlready(alreadyTouched, callsInBlock[0]); + + } + + return; + + void ReportIfNotDoneAlready(Dictionary dictionary, InvocationExpressionSyntax descendantCall) + { + // we've already flagged this call as a duplicate, skip it + if (dictionary.TryGetValue(descendantCall, out var value) && value) + return; + + dictionary[descendantCall] = true; + var diagnostic = Diagnostic.Create( + descriptor:RuleDescriptors.Ak1008MustNotInvokeStashMoreThanOnce, + location:descendantCall.GetLocation()); + ctx.ReportDiagnostic(diagnostic); } } + + /* + // we could skip the first stash call here, but since we don't know _which_ call is the offending + // duplicate, we'll just report all of them + */ private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context, AkkaContext akkaContext) { var semanticModel = context.SemanticModel; var methodDeclaration = (MethodDeclarationSyntax)context.Node; - // SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, stashMethod) - // First: need to check if this method is declared in an ActorBase subclass var methodSymbol = semanticModel.GetDeclaredSymbol(methodDeclaration); if (methodSymbol is null || !methodSymbol.ContainingType.IsActorBaseSubclass(akkaContext.AkkaCore)) From c419efb6cb4e700694aded66a8fe272c7a840ed6 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 12:57:05 -0600 Subject: [PATCH 12/13] trying --- .../MustNotInvokeStashMoreThanOnceAnalyzer.cs | 181 ++++++------------ 1 file changed, 62 insertions(+), 119 deletions(-) diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index 8f76011..7c8893f 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -23,156 +23,99 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, Guard.AssertIsNotNull(context); Guard.AssertIsNotNull(akkaContext); - // For methods inside the actor - context.RegisterSyntaxNodeAction(ctx => AnalyzeMethodDeclaration(ctx, akkaContext), SyntaxKind.MethodDeclaration); - // For lambdas, namely Receive and ReceiveAny context.RegisterSyntaxNodeAction(ctx => { - var invocationExpr = (InvocationExpressionSyntax)ctx.Node; + var node = ctx.Node; var semanticModel = ctx.SemanticModel; var akkaCore = akkaContext.AkkaCore; var stashMethod = akkaCore.Actor.IStash.Stash!; + var foundDuplicates = false; - var invocationSymbol = semanticModel.GetSymbolInfo(invocationExpr.Expression).Symbol; - - // if this invocation expression is not invoking a method OR it's not part of an actor base type, skip - if (invocationSymbol is not null && invocationSymbol.ContainingType.IsActorBaseSubclass(akkaCore)) - { - // if we've made it here, we are inside a context where at least one Stash call has been found - // scope out and see if there are any other stash calls inside the same branch - var invocationParent = invocationExpr.DescendantNodes(); + // First: need to check if this method / lambda is declared in an ActorBase subclass + var symbol = semanticModel.GetSymbolInfo(node).Symbol; + if (symbol is null || !symbol.ContainingType.IsActorBaseSubclass(akkaContext.AkkaCore)) + return; - DiagnoseSyntaxNodes(invocationParent.ToArray(), semanticModel, stashMethod, ctx); - } - - - - }, SyntaxKind.InvocationExpression); - } + // Find all stash calls within the method or lambda + var stashCalls = node.DescendantNodes() + .OfType() + .Where(c => IsStashInvocation(semanticModel, c, stashMethod)) + .ToList(); - private static void DiagnoseSyntaxNodes(IReadOnlyList invocationParent, SemanticModel semanticModel, - IMethodSymbol stashMethod, SyntaxNodeAnalysisContext ctx) - { - // Find all "Stash.Stash()" calls in the method - var stashCalls = invocationParent - .OfType() - .Where(invocation => IsStashInvocation(semanticModel, invocation, stashMethod)).ToArray(); - - // aren't enough calls to merit further analysis - if (stashCalls.Length < 2) - return; - - foreach(var stashCall in stashCalls) - { - stashCall.GetLocation(); - } - - // Group calls by their parent block - var callsGroupedByBlock = stashCalls - .GroupBy(GetContainingBlock) - .Where(group => group.Key != null).ToArray(); - - // so we don't log multiple warnings for the same call, in the event someone has done something truly stupid - var alreadyTouched = new Dictionary(); - - foreach (var group in callsGroupedByBlock) - { - var callsInBlock = group.ToList(); + // If there are less than 2 stash calls, we can skip the rest of the analysis + if (stashCalls.Count < 2) + return; - // simple / dumb check - are there multiple calls to Stash in the same block? - if (callsInBlock.Count > 1) + // Flag all stash calls if they are in the same block without branching + var sameScopeCalls = stashCalls + .GroupBy(GetContainingBlock) + .Where(group => group.Key != null).ToArray(); + + foreach (var group in sameScopeCalls) { - foreach (var stashCall in callsInBlock) + var callsInBlock = group.ToList(); + // simple / dumb check - are there multiple calls to Stash in the same block? + if (callsInBlock.Count > 1) { - ReportIfNotDoneAlready(alreadyTouched, stashCall); + foundDuplicates = true; + goto FoundProblems; } } + + // if sameScopeCalls == all the duplicates, skip the depth analysis + if (sameScopeCalls.Sum(c => c.Count()) == stashCalls.Count) + return; - // depth check - var block = group.Key!; - var descendantCalls = block.DescendantNodes().OfType() - .Where(invocation => IsStashInvocation(semanticModel, invocation, stashMethod)) - .ToList(); - - // if there are no other Stash calls in the block, we can skip this block - if (descendantCalls.Count <= 1) - continue; - - // if there are other Stash calls in the block, they're bad - flag em - foreach (var descendantCall in descendantCalls) + + var blockSyntax = node as BlockSyntax ?? node.ChildNodes().OfType().FirstOrDefault(); + if (blockSyntax == null) + return; + + var controlFlowAnalysis = semanticModel.AnalyzeControlFlow(blockSyntax); + if (controlFlowAnalysis is not { Succeeded: true }) + return; + + // Now analyze control flow for mutually exclusive paths + var reachableCalls = stashCalls + .Where(call => controlFlowAnalysis.EntryPoints + .Contains(call.AncestorsAndSelf().OfType().FirstOrDefault()!)).ToList(); + + + if (reachableCalls.Count > 1) { - ReportIfNotDoneAlready(alreadyTouched, descendantCall); + foundDuplicates = true; } - - // also need to flag the original call - ReportIfNotDoneAlready(alreadyTouched, callsInBlock[0]); - - } + FoundProblems: + if (!foundDuplicates) return; + { + foreach (var call in stashCalls) + { + ReportDiagnostic(call, ctx); + } + } + }, SyntaxKind.MethodDeclaration, SyntaxKind.InvocationExpression); return; - void ReportIfNotDoneAlready(Dictionary dictionary, InvocationExpressionSyntax descendantCall) + static void ReportDiagnostic(InvocationExpressionSyntax call, SyntaxNodeAnalysisContext ctx) { - // we've already flagged this call as a duplicate, skip it - if (dictionary.TryGetValue(descendantCall, out var value) && value) - return; - - dictionary[descendantCall] = true; var diagnostic = Diagnostic.Create( - descriptor:RuleDescriptors.Ak1008MustNotInvokeStashMoreThanOnce, - location:descendantCall.GetLocation()); + descriptor: RuleDescriptors.Ak1008MustNotInvokeStashMoreThanOnce, + location: call.GetLocation()); ctx.ReportDiagnostic(diagnostic); } } - - /* - // we could skip the first stash call here, but since we don't know _which_ call is the offending - // duplicate, we'll just report all of them - */ - private static void AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context, AkkaContext akkaContext) - { - var semanticModel = context.SemanticModel; - var methodDeclaration = (MethodDeclarationSyntax)context.Node; - - // First: need to check if this method is declared in an ActorBase subclass - var methodSymbol = semanticModel.GetDeclaredSymbol(methodDeclaration); - if (methodSymbol is null || !methodSymbol.ContainingType.IsActorBaseSubclass(akkaContext.AkkaCore)) - return; - - var stashMethod = akkaContext.AkkaCore.Actor.IStash.Stash!; - - var invocationParent = methodDeclaration.DescendantNodes(); - DiagnoseSyntaxNodes(invocationParent.ToArray(), semanticModel, stashMethod, context); - } - - private static bool IsStashInvocation(SemanticModel model, InvocationExpressionSyntax invocation, IMethodSymbol stashMethod) + private static bool IsStashInvocation(SemanticModel model, InvocationExpressionSyntax invocation, + IMethodSymbol stashMethod) { var symbol = model.GetSymbolInfo(invocation).Symbol; return SymbolEqualityComparer.Default.Equals(symbol, stashMethod); } - + private static BlockSyntax? GetContainingBlock(SyntaxNode node) { return node.AncestorsAndSelf().OfType().FirstOrDefault(); } - - private static bool AreCallsSeparatedByConditionals(BlockSyntax block, List calls) - { - var conditionals = block.DescendantNodes().OfType().ToList(); - - foreach (var call in calls) - { - var isInConditional = conditionals.Any(ifStatement => ifStatement.Contains(call)); - - if (!isInConditional) - { - return false; - } - } - - return true; - } -} - +} \ No newline at end of file From d04d2ae35b018a3556f913003c9495566be02bb7 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 23 Dec 2024 13:16:44 -0600 Subject: [PATCH 13/13] sdfsdfs --- .../MustNotInvokeStashMoreThanOnceAnalyzer.cs | 59 ++++--------------- 1 file changed, 11 insertions(+), 48 deletions(-) diff --git a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs index 7c8893f..f0bf2fe 100644 --- a/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs +++ b/src/Akka.Analyzers/AK1000/MustNotInvokeStashMoreThanOnceAnalyzer.cs @@ -30,7 +30,6 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, var semanticModel = ctx.SemanticModel; var akkaCore = akkaContext.AkkaCore; var stashMethod = akkaCore.Actor.IStash.Stash!; - var foundDuplicates = false; // First: need to check if this method / lambda is declared in an ActorBase subclass var symbol = semanticModel.GetSymbolInfo(node).Symbol; @@ -43,58 +42,21 @@ public override void AnalyzeCompilation(CompilationStartAnalysisContext context, .Where(c => IsStashInvocation(semanticModel, c, stashMethod)) .ToList(); - // If there are less than 2 stash calls, we can skip the rest of the analysis if (stashCalls.Count < 2) return; // Flag all stash calls if they are in the same block without branching - var sameScopeCalls = stashCalls + var groupedByBlock = stashCalls .GroupBy(GetContainingBlock) - .Where(group => group.Key != null).ToArray(); - - foreach (var group in sameScopeCalls) - { - var callsInBlock = group.ToList(); - // simple / dumb check - are there multiple calls to Stash in the same block? - if (callsInBlock.Count > 1) - { - foundDuplicates = true; - goto FoundProblems; - } - } - - // if sameScopeCalls == all the duplicates, skip the depth analysis - if (sameScopeCalls.Sum(c => c.Count()) == stashCalls.Count) - return; - - - var blockSyntax = node as BlockSyntax ?? node.ChildNodes().OfType().FirstOrDefault(); - if (blockSyntax == null) - return; - - var controlFlowAnalysis = semanticModel.AnalyzeControlFlow(blockSyntax); - if (controlFlowAnalysis is not { Succeeded: true }) - return; - - // Now analyze control flow for mutually exclusive paths - var reachableCalls = stashCalls - .Where(call => controlFlowAnalysis.EntryPoints - .Contains(call.AncestorsAndSelf().OfType().FirstOrDefault()!)).ToList(); - + .Where(group => group.Key != null && group.Count() > 1) + .SelectMany(group => group) + .ToList(); - if (reachableCalls.Count > 1) + // Report all calls in the same block + foreach (var call in groupedByBlock) { - foundDuplicates = true; + ReportDiagnostic(call, ctx); } - - FoundProblems: - if (!foundDuplicates) return; - { - foreach (var call in stashCalls) - { - ReportDiagnostic(call, ctx); - } - } }, SyntaxKind.MethodDeclaration, SyntaxKind.InvocationExpression); return; @@ -114,8 +76,9 @@ private static bool IsStashInvocation(SemanticModel model, InvocationExpressionS return SymbolEqualityComparer.Default.Equals(symbol, stashMethod); } - private static BlockSyntax? GetContainingBlock(SyntaxNode node) + private static SyntaxNode? GetContainingBlock(SyntaxNode node) { - return node.AncestorsAndSelf().OfType().FirstOrDefault(); + return node.AncestorsAndSelf() + .FirstOrDefault(n => n is BlockSyntax || n is SwitchSectionSyntax || n is MethodDeclarationSyntax); } -} \ No newline at end of file +}