diff --git a/docs/rules/index.md b/docs/rules/index.md
index 5f8115b88..722fbb0fb 100644
--- a/docs/rules/index.md
+++ b/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`
diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs
index d26e34ee0..45757f551 100644
--- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs
+++ b/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: false, and the caller disqualifies the whole transaction.
///
///
+ /// That repeat risk belongs to the captured 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.
+ ///
+ ///
/// A call under an if, switch or try is different: it runs at most once, so it is fine
/// on its own terms, but only if every other call on the same transaction is under the same one.
/// Two commands in the same if 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 if would compare equal.
///
///
- 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);
}
///
diff --git a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs
index 83d302268..84601dc7b 100644
--- a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs
+++ b/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 M(IDatabase db, RedisKey key)
+ {
+ return await SetIfNotExists();
+
+ async Task 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 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(