From 699775bbe1f00ab47ae65ba4b0e085525bc3a191 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 6 Aug 2026 11:18:47 +0100 Subject: [PATCH] Transaction analyzer: a lambda/local-function boundary is not "might queue more than once" TryGetBranch lumped IAnonymousFunctionOperation and ILocalFunctionOperation in with ILoopOperation and returned false, which the caller reads as "this call can queue N times" and uses to disqualify the whole transaction. Roslyn hands out no separate operation block for a lambda or local function - the body arrives as part of the containing method - so every transaction written inside one was silently invisible. That includes top-level statements, where the entire program body is one synthesised method, so the analyzer said nothing at all about the commonest way a small repro gets written. The repeat risk belongs to the *captured* transaction, not to the boundary itself. A transaction that is a local of the function being walked out of is created afresh on each invocation, so one invocation holds one whole transaction and the counts within it are exact; whatever encloses the function governs how many transactions there are, not what goes into each. The two boundary cases now stop the walk and accept when the transaction local belongs to that function, and keep returning false when it was captured from outside. A loop inside such a function is still hit first, as it must be. Tests: the two existing negatives were already the captured shape, so they are unchanged and still pass. Four added - transaction declared in a local function (the reported shape), in a lambda, in a local function called in a loop (N transactions, not one with N commands), and a loop inside such a function (still suppressed). docs/rules/index.md stated the old blanket behaviour as an intentional limitation; corrected. No AnalyzerReleases change: detection coverage only, no new or altered diagnostic IDs. --- docs/rules/index.md | 5 +- .../TransactionAnalyzer.cs | 26 +++- .../DetectionShape.cs | 113 ++++++++++++++++++ 3 files changed, 138 insertions(+), 6 deletions(-) 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(