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(