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