Add SER305/SER306: waiting for a queued transaction result

Directory.Packages.props

@@ -33,6 +33,9 @@
     <!-- analyzer test harness; deliberately newer Roslyn than the shipped floor above (the harness needs it,
          and we cannot test at 4.3 without pinning the whole world) -->
     <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing" Version="1.1.2" />
+    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing" Version="1.1.2" />
+    <!-- StackExchange.Redis.CodeFixes overrides this down to the shipped floor (4.3.0); the version here is
+         the one the test harness needs, exactly as for the analyzer reference above -->
     <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.14.0" />
     <PackageVersion Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="5.6.0" />
     <PackageVersion Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.3" />

StackExchange.Redis.slnx

@@ -1,6 +1,7 @@
 <Solution>
   <Folder Name="/eng/">
     <Project Path="eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj" />
+    <Project Path="eng/StackExchange.Redis.CodeFixes/StackExchange.Redis.CodeFixes.csproj" />
   </Folder>
   <Folder Name="/Solution Items/">
     <File Path=".editorconfig" />

docs/Transactions.md

@@ -117,6 +117,38 @@ var wasSet = (bool) db.ScriptEvaluate(@"if redis.call('hexists', KEYS[1], 'Uniqu
 
 (note that the response from `ScriptEvaluate` and `ScriptEvaluateAsync` is variable depending on your exact script; the response can be interpreted by casting - in this case as a `bool`)
 
+Don't await inside the transaction
+---
+
+This is the most common mistake with transactions, and it does not fail loudly - it hangs.
+
+Commands queued on an `ITransaction` (or an `IBatch`) are *buffered*, not sent. They go to the server only when
+you call `Execute`/`ExecuteAsync`, so the `Task` handed back at the point of queueing cannot complete until
+after that call. Awaiting it there waits for something that can only happen further down the method:
+
+```csharp
+var tran = db.CreateTransaction();
+var value = await tran.StringGetAsync(key);   // never completes - nothing has been sent yet
+await tran.ExecuteAsync();                    // never reached
+```
+
+Everything else in the async API rewards awaiting promptly, and `ITransaction` offers the same method names, so
+this reads as perfectly ordinary code. Instead, capture the tasks and await them *after* `Execute`:
+
+```csharp
+var tran = db.CreateTransaction();
+var pending = tran.StringGetAsync(key);
+_ = tran.StringSetAsync(other, "value");      // discard the ones you do not need
+if (await tran.ExecuteAsync())
+{
+    var value = await pending;
+}
+```
+
+The analyzer reports this as [SER305](rules/SER305) - an **error**, not a suggestion - and offers both fixes.
+The one case that does not hang is `CommandFlags.FireAndForget`, which completes immediately carrying the
+default value; awaiting that is legal but tells you nothing, which is [SER306](rules/SER306).
+
 Do you need a transaction at all?
 ---
 

docs/rules/SER305.md

@@ -0,0 +1,84 @@
+# SER305: waiting for a queued command before it is executed will never complete
+
+Commands queued on an `ITransaction` or `IBatch` are **not sent** when you call them - they are buffered, and
+go to the server only when you call `Execute`/`ExecuteAsync`. The `Task` you get back therefore cannot complete
+until then, so awaiting it at the point of queueing waits for something that can only happen after the line
+that is waiting. Your code stops there, permanently.
+
+```c#
+// flagged - this hangs, for good
+var tran = db.CreateTransaction();
+var value = await tran.StringGetAsync(key);   // SER305: nothing has been sent yet
+await tran.ExecuteAsync();                    // never reached
+
+// suggested - capture the task, await it after Execute
+var tran = db.CreateTransaction();
+var pending = tran.StringGetAsync(key);
+await tran.ExecuteAsync();
+var value = await pending;
+
+// or, if you do not want the result at all
+_ = tran.StringSetAsync(key, value);
+await tran.ExecuteAsync();
+```
+
+This is the one rule here reported as an **error**, because unlike the rest of the family it does not describe
+code that could be better - it describes code that cannot work. There is no arrangement of the surrounding
+lines that rescues it: the wait sits at the queueing site, so even a previous `Execute` does not help, because
+*this* command was queued after it and will never be sent.
+
+It applies equally to a batch (`db.CreateBatch()`), which buffers in exactly the same way, and to every way of
+waiting - `await`, `.Result`, `.Wait()`, `.GetAwaiter().GetResult()`.
+
+## Why this happens so easily
+
+It is the one place where the advice everyone has absorbed - *await your tasks, and await them promptly* - is
+exactly wrong. Every other `IDatabaseAsync` method returns a task you should await straight away, and
+`ITransaction` offers the same method names on what looks like the same interface. Nothing about the call site
+suggests that this one is a buffer rather than a dispatch.
+
+## Fixes
+
+Two are offered in the IDE:
+
+- **Discard the queued result** - `_ = tran.StringSetAsync(...)`. Right when you only wanted the write.
+- **Capture it and await after Execute** - introduces a local for the task and moves the `await` below the
+  `Execute[Async]` call.
+
+The second is only offered where it is unambiguously safe: the `await` form, with an `Execute[Async]` on the
+same transaction later in the same block, and - if the wait declares a variable - nothing between the two
+already using that variable, since the declaration is what moves.
+
+## Cases that are deliberately not flagged
+
+- **A task captured to a local, then awaited.** `var pending = tran.StringGetAsync(key); await pending;` is
+  broken if it is above the `Execute` and *correct* if it is below - it is the recommended fix. Telling those
+  apart means reasoning about statement order, and this rule is an error: it must not guess. The same goes for
+  `await Task.WhenAll(...)` over queued tasks.
+- **Fire-and-forget.** `CommandFlags.FireAndForget` returns an already-completed task, so awaiting it does not
+  hang. It is still not giving you an answer, which is [SER306](SER306) - a warning, not an error.
+- **Flags this rule cannot read.** `tran.StringGetAsync(key, flags)`, where `flags` is a variable, might carry
+  `FireAndForget` at runtime, so nothing is reported at all.
+- **A transaction reached through `IDatabaseAsync`.** A helper taking `IDatabaseAsync` cannot tell a
+  transaction from a plain database, and neither can this rule.
+
+## Suppressing
+
+Reported as an **error**, so it fails the build. If you are suppressing it, please check first that the code
+really does complete - this rule fires only on shapes that cannot.
+
+```xml
+<NoWarn>$(NoWarn);SER305</NoWarn>
+```
+
+or locally:
+
+```c#
+#pragma warning disable SER305
+```
+
+If you believe it has flagged something that works, that is a bug in the rule and one that reaches every
+consumer of the package - please
+[report it](https://github.com/StackExchange/StackExchange.Redis/issues/new) with the code as written.
+
+See also [Transactions](../Transactions) and [SER306](SER306).

docs/rules/SER306.md

@@ -0,0 +1,65 @@
+# SER306: waiting for a fire-and-forget result yields nothing
+
+`CommandFlags.FireAndForget` means "send this, and do not report back". The command is queued and sent like any
+other, but the task you get is **already completed, carrying the default value** - it is not, and never will
+be, the server's answer. Reading it is not an error, it is just always `null`/`0`/`false`.
+
+```c#
+// flagged - value is always RedisValue.Null, whenever you read it
+var tran = db.CreateTransaction();
+var value = await tran.StringGetAsync(key, CommandFlags.FireAndForget);
+await tran.ExecuteAsync();
+
+// suggested - if you want the result, do not ask for fire-and-forget
+var pending = tran.StringGetAsync(key);
+await tran.ExecuteAsync();
+var value = await pending;
+
+// or, if fire-and-forget is what you meant, discard it
+_ = tran.StringSetAsync(key, value, flags: CommandFlags.FireAndForget);
+await tran.ExecuteAsync();
+```
+
+Unlike [SER305](SER305) this does not hang, which is why it is a **warning** rather than an error, and why it
+has its own ID: deliberate fire-and-forget code needs to be able to silence this without silencing the rule
+that says a transaction cannot work.
+
+The only fix offered is **discard the queued result**. Capturing the task and awaiting it after `Execute` -
+which is the fix for SER305 - gains nothing here, because the value does not improve with waiting.
+
+## Where it applies
+
+Anywhere the flag is visible at compile time, on a transaction or a batch:
+
+```c#
+tran.StringGetAsync(key, CommandFlags.FireAndForget)                             // flagged
+tran.StringGetAsync(key, CommandFlags.FireAndForget | CommandFlags.DemandMaster) // flagged
+tran.StringGetAsync(key, flags | CommandFlags.FireAndForget)                     // flagged - `|` only sets bits
+tran.StringGetAsync(key, flags)                                                  // not flagged - unknowable
+```
+
+The third case works because `|` can only ever *add* the flag, whatever `flags` holds at runtime. There is no
+matching treatment of `&`/`~` to prove the flag *absent*: that would feed [SER305](SER305), which is an error,
+and an error must not rest on a partly-understood expression.
+
+## Cases that are deliberately not flagged
+
+- **Fire-and-forget straight to the database.** `db.StringSetAsync(key, value, flags: FireAndForget)` is
+  ordinary code; this rule is only about waiting for a result you have declined.
+- **A discarded result.** `_ = tran.StringSetAsync(...)` is exactly what the rule asks for.
+
+## Suppressing
+
+Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down.
+
+```xml
+<NoWarn>$(NoWarn);SER306</NoWarn>
+```
+
+or locally:
+
+```c#
+#pragma warning disable SER306
+```
+
+See also [Transactions](../Transactions) and [SER305](SER305).

docs/rules/index.md

@@ -8,7 +8,7 @@ separately:
 
 | Range | Meaning |
 |---|---|
-| `SER300`-`SER349` | usage guidance about your code |
+| `SER300`-`SER349` | rules about your code - mostly guidance, but see the correctness rules below |
 | `SER350`-`SER399` | build-level problems from the source generators |
 
 Note that `SER0xx` is a different thing entirely: those are the [`[Experimental]` API gates](../exp/SER004),
@@ -28,6 +28,13 @@ want `StringSet`. Reach for the async form in new code.
 Argument names in the suggestion (`key`, `value`, `entries`) are a sketch of the shape, not literal text -
 substitute your own expressions.
 
+## Correctness
+
+Unlike everything under [Usage](#usage), these describe code that does not do what it looks like it does.
+
+- [SER305](SER305) - **error**: waiting for a queued command before `Execute[Async]` never completes
+- [SER306](SER306) - waiting for a fire-and-forget result, which is always the default value
+
 ## Usage
 
 - [SER300](SER300) - transaction may be replaceable by a conditional argument (any server version)
@@ -70,7 +77,11 @@ These are heuristics, and the list above is where the effort has gone - but it i
 rare, not impossible. If one of these rules flags something it should not have, that is a bug in the rule rather
 than something to work around: please
 [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new) with the transaction as written.
-`SER350` is not in this family - it reports a build problem rather than offering guidance.
+
+Three rules are not in this family, because they are not suggesting an improvement to working code: `SER350`
+reports a build problem, and [SER305](SER305)/[SER306](SER306) report code that does not work. Those two have
+their own, much narrower quiet-lists on their own pages - they are flow-insensitive by design, so most of the
+caveats above (a loop, an `if`, a transaction passed elsewhere) simply do not arise.
 
 ## Declaring your server version
 
@@ -94,10 +105,13 @@ rule's message names the version it needs, so you can tell at a glance whether i
 
 ## Severity, and turning it down
 
-These are **warnings** by default. The code they flag is correct - it works, and it will keep working - so a
-warning is arguably strong; they are warnings anyway because information-level diagnostics are not printed by
-`dotnet build`, which means outside an IDE they are invisible, and a suggestion nobody ever sees is not worth
-shipping.
+These are **warnings** by default, with one exception: [SER305](SER305) is an **error**, because the code it
+flags cannot work rather than merely being improvable. It is the only one here that is not a matter of taste,
+and it is worth reading before suppressing.
+
+For the rest, the code they flag is correct - it works, and it will keep working - so a warning is arguably
+strong; they are warnings anyway because information-level diagnostics are not printed by `dotnet build`, which
+means outside an IDE they are invisible, and a suggestion nobody ever sees is not worth shipping.
 
 The consequence worth knowing before you upgrade: if you build with `TreatWarningsAsErrors`, these **will fail
 your build** on code that previously compiled. Nothing is broken - you have a choice of acting on them or

eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md

@@ -4,5 +4,12 @@
 ; released, because consumers put them in NoWarn and .editorconfig. See Diagnostics.cs for the SER3xx map.
 ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md
 ;
-; Empty: the initial set is recorded directly in AnalyzerReleases.Shipped.md under 3.1. New rules added after
-; that release go here first, under a "### New Rules" table, and move across when they ship.
+; The initial set is recorded directly in AnalyzerReleases.Shipped.md under 3.1. New rules added after that
+; release go here first, and move across when they ship.
+
+### New Rules
+
+Rule ID | Category | Severity | Notes
+--------|----------|----------|-------
+SER305  | Usage    | Error    | QueuedResultAnalyzer: waiting for a command queued on a transaction or batch, before Execute[Async]() sends it, never completes
+SER306  | Usage    | Warning  | QueuedResultAnalyzer: waiting for a fire-and-forget result reads the default value rather than the server's answer

eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs

@@ -434,7 +434,7 @@ int GetTupleIndex(BasicArray<ParamInfo> parameters, bool needsMap)
                 return index;
             }
 
-            // const string CommandFlagsType = "StackExchange.Redis.CommandFlags";
+            const string CommandFlagsType = "StackExchange.Redis.CommandFlags";
             const string RedisKeyType = "StackExchange.Redis.RedisKey";
             const string RedisChannelType = "StackExchange.Redis.RedisChannel";
             // types that carry key(s) internally without "RedisKey" in their name, so the
@@ -475,11 +475,32 @@ void AppendTupleTypes()
                     }
 
                     // captured args are a plain struct unless the owning database rewrites keys, in which
-                    // case the mapping members - and the interface that carries them - are emitted below
+                    // case the mapping members - and the interface that carries them - are emitted below.
+                    // A captured CommandFlags is surfaced through IFlaggedRedisArgs regardless, so that a
+                    // funnel holding an opaque TState can still see fire-and-forget; the two compose.
                     writer.Append(")");
+                    int flagsArg = -1;
+                    for (int p = 0; p < parameters.Length; p++)
+                    {
+                        // suffix rather than substring: this must not match a nullable, an array, or a
+                        // container *of* flags, none of which the funnel could read as a plain value
+                        if (parameters[p].Type.EndsWith(CommandFlagsType, StringComparison.Ordinal))
+                        {
+                            flagsArg = p;
+                            break;
+                        }
+                    }
+
+                    bool firstBase = true;
                     if (cls.IsMutator)
                     {
                         writer.Append(" : global::StackExchange.Redis.IMappableRedisArgs");
+                        firstBase = false;
+                    }
+
+                    if (flagsArg >= 0)
+                    {
+                        writer.Append(firstBase ? " : " : ", ").Append("global::StackExchange.Redis.IFlaggedRedisArgs");
                     }
                     writer.NewLine().Append("{").Indent();
                     for (int p = 0; p < parameters.Length; p++)
@@ -488,6 +509,15 @@ void AppendTupleTypes()
                             .Append(" Arg").Append(p).Append(" = arg").Append(p).Append(";");
                     }
 
+                    if (flagsArg >= 0)
+                    {
+                        // explicit implementation, so it cannot collide with a captured argument's own name;
+                        // readonly only on a mutator's struct, since a readonly struct's members already are
+                        writer.NewLine().Append(cls.IsMutator ? "readonly " : "")
+                            .Append("global::StackExchange.Redis.CommandFlags global::StackExchange.Redis.IFlaggedRedisArgs.Flags => Arg")
+                            .Append(flagsArg).Append(";");
+                    }
+
                     if (!cls.IsMutator)
                     {
                         // nothing rewrites keys here, so there is no Map/UnMapper to emit at all