Transaction analyzer: fix local functions / lambdas

docs/rules/index.md

@@ -52,8 +52,9 @@ whatever each rule's own page lists.
 - **Commands that do not always queue together.** A command inside an `if`, `switch` or `try` is only collapsible
   with commands inside the *same* one; opposite arms of an `if`/`else` never queue together at all. A whole
   transaction inside a conditional is ordinary code and is still flagged.
-- **Commands that might queue more than once**: inside a loop, or inside a lambda or local function, where one
-  call site is any number of queued commands.
+- **Commands that might queue more than once**: inside a loop, or inside a lambda or local function that queues
+  onto a transaction from outside itself, where one call site is any number of queued commands. A transaction
+  created *and* completed within the lambda or local function is one per invocation, so it is still flagged.
 - **Arguments the single command cannot express.** The suggestions are sketches, but only ever of a rewrite that
   keeps what you wrote. N x `StringSet(key, value, expiry)` is *not* `MSET` - MSET takes one expiry for the whole
   batch, not one per key - so that stays quiet rather than quietly making your keys permanent. Likewise a `When`

eng/StackExchange.Redis.Build/TransactionAnalyzer.cs

@@ -123,7 +123,7 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols
                     && SymbolEqualityComparer.Default.Equals(instanceLocal, local))
                 {
                     // tran.Something(...) - a queued command, a condition, or the terminator
-                    var repeats = !TryGetBranch(invocation, block, out var branch);
+                    var repeats = !TryGetBranch(invocation, block, local, out var branch);
                     usage.Add(invocation, known, branch, repeats);
                 }
                 else
@@ -248,6 +248,16 @@ private static string VersionClause(ServerVersion version)
     /// not collapsible into anything: <c>false</c>, and the caller disqualifies the whole transaction.
     /// </para>
     /// <para>
+    /// That repeat risk belongs to the <em>captured</em> transaction, not to the function boundary itself. A
+    /// transaction that is a local of the lambda or local function we are walking out of is created afresh on
+    /// each invocation, so one invocation holds one entire transaction and the counts within it are exact -
+    /// stop at that boundary and report what we have. Whatever encloses the function then governs how many
+    /// transactions there are, not what goes into each. Getting this wrong silences the analyzer completely
+    /// for anyone who writes their transaction in a local function, top-level statements included, since there
+    /// the whole program body is one synthesised method and Roslyn hands us no separate block for the nested
+    /// function.
+    /// </para>
+    /// <para>
     /// A call under an <c>if</c>, <c>switch</c> or <c>try</c> is different: it runs at most once, so it is fine
     /// on its own terms, but only if every <em>other</em> call on the same transaction is under the same one.
     /// Two commands in the same <c>if</c> body always queue together and a compound command really does replace
@@ -257,7 +267,7 @@ private static string VersionClause(ServerVersion version)
     /// operation, or the two arms of one <c>if</c> would compare equal.
     /// </para>
     /// </remarks>
-    private static bool TryGetBranch(IOperation operation, IOperation block, out SyntaxNode? branch)
+    private static bool TryGetBranch(IOperation operation, IOperation block, ISymbol transaction, out SyntaxNode? branch)
     {
         branch = null;
         var previous = operation;
@@ -266,10 +276,15 @@ private static bool TryGetBranch(IOperation operation, IOperation block, out Syn
             switch (node)
             {
                 case ILoopOperation:
-                case IAnonymousFunctionOperation:
-                case ILocalFunctionOperation:
                     return false;
 
+                // captured from outside: unbounded, so no. Declared inside: this function body is effectively
+                // the block, and we are done walking
+                case IAnonymousFunctionOperation { Symbol: { } lambda }:
+                    return DeclaredIn(lambda);
+                case ILocalFunctionOperation { Symbol: { } localFunction }:
+                    return DeclaredIn(localFunction);
+
                 // keep walking after finding one: an enclosing loop still trumps it
                 case IConditionalOperation:
                 case ISwitchOperation:
@@ -283,6 +298,9 @@ private static bool TryGetBranch(IOperation operation, IOperation block, out Syn
         }
 
         return true;
+
+        bool DeclaredIn(IMethodSymbol function)
+            => SymbolEqualityComparer.Default.Equals(transaction.ContainingSymbol, function);
     }
 
     /// <summary>

tests/StackExchange.Redis.Build.Tests/DetectionShape.cs

@@ -216,6 +216,119 @@ public async Task M(IDatabase db, RedisKey key, RedisKey other)
         }
         """);
 
+    [Fact]
+    // The control for the two above, and the reason the function boundary is not itself disqualifying: those
+    // capture a transaction that outlives them, which is what makes the invocation count matter. A transaction
+    // *declared* in the local function is one per invocation however often it runs, so the counts inside are
+    // exact. This shape - one transaction, one helper method - is the single commonest way the guarded pattern
+    // gets written, and treating it as unknowable silenced the whole analyzer for it.
+    public Task TransactionDeclaredInLocalFunction_IsStillFlagged() => VerifyAsync(
+        """
+        using StackExchange.Redis;
+        using System.Threading.Tasks;
+        class C
+        {
+            public async Task<bool> M(IDatabase db, RedisKey key)
+            {
+                return await SetIfNotExists();
+
+                async Task<bool> SetIfNotExists()
+                {
+                    var tran = db.CreateTransaction();
+                    _ = tran.StringSetAsync(key, "value");
+                    var cond = {|#0:tran.AddCondition(Condition.KeyNotExists(key))|};
+                    await tran.ExecuteAsync();
+                    return cond.WasSatisfied;
+                }
+            }
+        }
+        """,
+        Diagnostic("SER300").WithLocation(0).WithArguments(
+            "Condition.KeyNotExists",
+            "StringSetAsync",
+            "StringSet[Async](key, value, When.NotExists)"));
+
+    [Fact]
+    // and in a lambda, where the transaction is likewise per-invocation
+    public Task TransactionDeclaredInLambda_IsStillFlagged() => VerifyAsync(
+        """
+        using StackExchange.Redis;
+        using System;
+        using System.Threading.Tasks;
+        class C
+        {
+            public Func<Task> M(IDatabase db, RedisKey key) => async () =>
+            {
+                var tran = db.CreateTransaction();
+                {|#0:tran.AddCondition(Condition.KeyNotExists(key))|};
+                _ = tran.StringSetAsync(key, "value");
+                await tran.ExecuteAsync();
+            };
+        }
+        """,
+        Diagnostic("SER300").WithLocation(0).WithArguments(
+            "Condition.KeyNotExists",
+            "StringSetAsync",
+            "StringSet[Async](key, value, When.NotExists)"));
+
+    [Fact]
+    // Calling it in a loop makes N transactions, not one transaction with N commands, so the per-transaction
+    // counts still hold. The enclosing loop governs how many transactions there are, not what goes into each.
+    public Task TransactionDeclaredInLocalFunctionCalledInLoop_IsStillFlagged() => VerifyAsync(
+        """
+        using StackExchange.Redis;
+        using System.Threading.Tasks;
+        class C
+        {
+            public async Task M(IDatabase db, RedisKey[] keys)
+            {
+                foreach (var key in keys)
+                {
+                    await SetIfNotExists(key);
+                }
+
+                async Task SetIfNotExists(RedisKey key)
+                {
+                    var tran = db.CreateTransaction();
+                    {|#0:tran.AddCondition(Condition.KeyNotExists(key))|};
+                    _ = tran.StringSetAsync(key, "value");
+                    await tran.ExecuteAsync();
+                }
+            }
+        }
+        """,
+        Diagnostic("SER300").WithLocation(0).WithArguments(
+            "Condition.KeyNotExists",
+            "StringSetAsync",
+            "StringSet[Async](key, value, When.NotExists)"));
+
+    [Fact]
+    // A loop *inside* the function still disqualifies: the walk hits it before the boundary, as it must
+    public Task LoopInsideLocalFunctionDeclaringTransaction_IsNotFlagged() => VerifyAsync(
+        """
+        using StackExchange.Redis;
+        using System.Threading.Tasks;
+        class C
+        {
+            public async Task M(IDatabase db, RedisKey key, RedisValue[] values)
+            {
+                await Queue();
+
+                async Task Queue()
+                {
+                    var tran = db.CreateTransaction();
+                    tran.AddCondition(Condition.KeyNotExists(key));
+                    foreach (var value in values)
+                    {
+                        _ = tran.StringSetAsync(key, value);
+                    }
+
+                    await tran.ExecuteAsync();
+                }
+            }
+        }
+        """);
+
     [Fact]
     // the helper may queue anything at all; our counts describe only the part we can see
     public Task TransactionPassedToAnotherMethod_IsNotFlagged() => VerifyAsync(