diff --git a/.claude/skills/implement-resp-command/SKILL.md b/.claude/skills/implement-resp-command/SKILL.md index 04edf7b6e..dff1b62f5 100644 --- a/.claude/skills/implement-resp-command/SKILL.md +++ b/.claude/skills/implement-resp-command/SKILL.md @@ -1,6 +1,6 @@ --- name: implement-resp-command -description: Add a new Redis/RESP command (or overload) to StackExchange.Redis end-to-end — enum, interfaces, RedisDatabase implementation, ResultProcessor, public-API tracking, and the ResultProcessor + RoundTrip unit tests. Use when asked to "add/implement/support a Redis command", wire up a new RESP command, expose a server feature on IDatabase/IDatabaseAsync, or add a result processor. +description: Add a new Redis/RESP command (or overload) to StackExchange.Redis end-to-end — enum, interfaces, RedisDatabase implementation, ResultProcessor, public-API tracking, the ResultProcessor + RoundTrip unit tests, and TransactionAnalyzer coverage where the command replaces a transaction. Use when asked to "add/implement/support a Redis command", wire up a new RESP command, expose a server feature on IDatabase/IDatabaseAsync, or add a result processor. --- # Implement a new RESP command @@ -56,6 +56,38 @@ Before writing anything, get the command's exact argument order and reply shape 8. **Gate pre-release server features** behind `[Experimental(Experiments.Server_8_x)]` when appropriate (see `src/RESPite/Shared/Experiments.cs`). +9. **Ask whether the command is an *atomic composition*** — does it do in one round-trip what callers currently write a `MULTI`/`WATCH` transaction (or several queued commands) to achieve? A surprising number of new commands are exactly that: `GETDEL`, `GETEX`, `HGETDEL`, `SMOVE`, `SET ... NX/GET/IFEQ`, `SMISMEMBER`, every `M*`/variadic form. If yes, teach `TransactionAnalyzer` about it, or the people who would benefit most never find out it exists — see the section below. + +## If the command replaces a transaction + +`eng/StackExchange.Redis.Build/TransactionAnalyzer.cs` ships inside the package and tells consumers when a transaction they wrote is now one command. A new atomic command that isn't added there is invisible: the analyzer keeps quiet about exactly the code your command was written to replace. This is cheap to do at the time and nobody comes back for it later. + +Work out which shape the command replaces, and add a row to the matching table in that file: + +| The transaction it replaces | Table | Rule | +|---|---|---| +| one `AddCondition` + one write, where the command now takes that condition as an argument | `Map`, family A | SER300 | +| one `AddCondition` + one write, where a *newer* command subsumes both | `Map`, family B | SER301 | +| one `AddCondition` + one write, where the write's own return value already answers the condition | `Map`, family C | SER302 | +| two different queued commands | `MapPair` | SER303 | +| the same command queued N times, now a variadic overload | `MapVariadic` | SER304 | + +Beyond the suggestion text, a row states as much of the following as its table has columns for: + +- **The server version the *suggestion* needs** — not the one the flagged code needs. Use the same `RedisFeatures` constant the live integration test gates on, and `ServerVersion.Any` where the form predates anything realistically in service (saying "requires 2.6 or later" is noise). This is what lets a project declaring `` see only what it can act on. +- **A coverage set** — the parameter names the suggestion still carries. Anything the caller wrote that isn't in it makes the rule stay quiet, because a rewrite that silently drops an argument is worse than no suggestion: N x `StringSet(key, value, expiry)` is not `MSET`, and "helpfully" collapsing it makes the keys permanent. State names *kept*, never names dropped, so that a parameter added to an overload later fails safe. `CommandFlags` is exempt globally. Family C passes `null` meaning "everything", because it keeps the command as written and deletes only the condition. +- **Whether the same member or field has to match**, not just the same key (`Map`'s `SameMember`, `MapVariadic`'s `RequiresMember`). A condition about member `"a"` says nothing about a write to member `"b"`, and collapsing the two drops a real guard. +- **Order, where the commands are not commutative** (`MapPair`). `SET ... GET` returns the value from *before* the write; `SET` clears any TTL, so `StringSet` + `KeyExpire` is `SET ... EX` while the reverse is not. Map one direction and pin the other with a negative test. +- **Which way the keys go** (`MapVariadic`'s `ManyKeys`). `SADD` takes one key and many values, so N calls must be on the *same* key; `MSET`/`DEL` take many keys, so those must be on *different* ones. A mapping in the wrong direction suggests a command that does something else entirely. + +**Write down what you decided *not* to map, and why.** The near-misses are the dangerous part and the comments in those tables are load-bearing: `ListRightPop` + `ListLeftPush` is not `LMOVE` (inside a transaction the pop's result is an unresolved `Task`, so the pushed value is a different one), N x `ListLeftPop` is not `LMPOP` (which pops from the first non-empty key, not from each). If you talk yourself out of a mapping, leave the reasoning where the next person will hit it. + +Then: + +- **Tests** in `tests/StackExchange.Redis.Build.Tests/` — a positive in `SER30x.cs`, and the negatives that matter in `DetectionShape.cs`. The negatives are the point: they are correct code a keener analyzer would suggest breaking, in a diagnostic shipped to every consumer. If your mapping needs the same key, the same member, a particular order, or the absence of an argument, there is a test for each, or the constraint isn't real. +- **A row in `docs/rules/SER30x.md`**, since every message links to that page for the caveats it can't carry itself. +- **A new rule ID** (rather than a row in an existing table) additionally needs a descriptor in `Diagnostics.cs` and an entry in `AnalyzerReleases.Unshipped.md`; IDs are a public contract once released, because consumers put them in `NoWarn`. + ## Tests — the two layers that matter ### ResultProcessor unit test (parsing in isolation) @@ -114,4 +146,5 @@ The in-process managed server (`toys/StackExchange.Redis.Server`) may also need - `dotnet build Build.csproj -c Release /p:CI=true` — analyzers + `TreatWarningsAsErrors` must pass (this catches a missing `PublicAPI.Unshipped.txt` entry). - `dotnet test tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj -f net10.0 --filter "FullyQualifiedName~MyCommand"` — runs your new unit tests without any server. +- `dotnet test tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj` — if you touched `TransactionAnalyzer`. Also needs no server, and takes seconds. - Double-check no shipped signature changed (back-compat). diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 09ac62e08..80f33c9c3 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -130,6 +130,28 @@ jobs: - name: .NET Build run: dotnet build Build.csproj -c Release /p:CI=true + # The analyzer and the props that configures it reach consumers only through the package, and a + # packaging regression is silent: no diagnostics, ever, for anybody, with a build that still succeeds. + # The real pack below runs only on pushes to main/v3, so check it here where PRs will see it too. + - name: Verify package contents + run: | + $out = "${env:GITHUB_WORKSPACE}\.packcheck" + dotnet pack src/StackExchange.Redis/StackExchange.Redis.csproj --no-build -c Release /p:PackageOutputPath=$out /p:CI=true + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + $nupkg = Get-ChildItem "$out\StackExchange.Redis.*.nupkg" | Select-Object -First 1 + if (-not $nupkg) { Write-Error "no package was produced"; exit 1 } + Add-Type -AssemblyName System.IO.Compression.FileSystem + $zip = [System.IO.Compression.ZipFile]::OpenRead($nupkg.FullName) + try { $names = @($zip.Entries | ForEach-Object { $_.FullName }) } finally { $zip.Dispose() } + $missing = @() + foreach ($required in @("analyzers/dotnet/cs/StackExchange.Redis.Build.dll", "build/StackExchange.Redis.props")) { + if ($names -contains $required) { Write-Host "ok: $required" } else { $missing += $required } + } + if ($missing.Count) { + Write-Error "$($nupkg.Name) is missing: $($missing -join ', ')" + Write-Host "package contained:"; $names | Sort-Object | ForEach-Object { Write-Host " $_" } + exit 1 + } - name: StackExchange.Redis.Tests run: | $exitCode = 0 diff --git a/Directory.Packages.props b/Directory.Packages.props index c5261d0e1..66df6c65b 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -13,11 +13,27 @@ - - + + + + + + + diff --git a/docs/Transactions.md b/docs/Transactions.md index 4d8deca27..7f78db126 100644 --- a/docs/Transactions.md +++ b/docs/Transactions.md @@ -116,3 +116,34 @@ 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`) + +Do you need a transaction at all? +--- + +A great many transactions in real code exist only to make one command conditional, or to make two commands +atomic - and in most of those cases a single command already does the job. That is worth preferring: one +round-trip instead of two, evaluated atomically on the server, with no `WATCH` and so no possibility of +aborting under contention and needing a retry loop. + +```csharp +// a transaction to set a key only if it is absent... +var tran = db.CreateTransaction(); +tran.AddCondition(Condition.KeyNotExists(key)); +_ = tran.StringSetAsync(key, value); +if (await tran.ExecuteAsync()) { /* ... */ } + +// ...is just this +if (await db.StringSetAsync(key, value, when: When.NotExists)) { /* ... */ } +``` + +Since 3.1 the package ships a Roslyn analyzer that points these out in your own build, as warnings. It covers conditions that duplicate a `when:` argument, +compare-and-set that a newer server does in one command, conditions that ask what the command already reports, +and pairs or repetitions of commands that collapse into one call. + +See [Analyzer rules](rules/) for the full list, what changes when you apply each suggestion - the result can +change meaning, so they are worth reading before rewriting - and how to declare your server version so you only +see suggestions you can act on. + +None of this makes transactions redundant. Cross-key compare-and-set, several genuinely independent conditions, +and multi-command units with no single-command equivalent are exactly what `MULTI`/`EXEC` and `WATCH` are for, +and the analyzer deliberately stays quiet about them. diff --git a/docs/index.md b/docs/index.md index a49d708b2..d8aea9a54 100644 --- a/docs/index.md +++ b/docs/index.md @@ -58,6 +58,7 @@ Documentation - [Timeouts](Timeouts) - guidance on dealing with timeout problems - [Thread Theft](ThreadTheft) - guidance on avoiding TPL threading problems - [RESP Logging](RespLogging) - capturing and validating RESP streams +- [Analyzer rules](rules/) - the `SER3xx` suggestions reported by the analyzer shipped in the package Questions and Contributions --- diff --git a/docs/rules/SER300.md b/docs/rules/SER300.md new file mode 100644 index 000000000..98a64f588 --- /dev/null +++ b/docs/rules/SER300.md @@ -0,0 +1,80 @@ +# SER300: transaction may be replaceable by a conditional argument + +A transaction whose only job is to make one command conditional can usually be replaced by that command's own +`when:` argument - a single round-trip that cannot abort under contention. + +```c# +// flagged +var tran = db.CreateTransaction(); +tran.AddCondition(Condition.KeyNotExists(key)); +_ = tran.StringSetAsync(key, value); +await tran.ExecuteAsync(); + +// suggested +await db.StringSetAsync(key, value, when: When.NotExists); +``` + +The conditional forms have existed as long as the commands have, so unlike [SER301](SER301) this needs no +particular server version. + +## Why this is worth changing + +`AddCondition` is implemented with `WATCH`. The transaction takes two round-trips (watch and check, then +`MULTI`/`EXEC`), and it can abort: if another client touches the key in between, `Execute()` returns `false` and +correct code has to retry. The conditional command is one round-trip and the server evaluates the condition +atomically, so there is no abort to handle. + +## What changes when you apply it + +Read this before rewriting - the collapsed form is not a drop-in for every caller. + +- **The result means something different.** `tran.Execute()` returns "the conditions held and the commands ran". + The single command returns its own result, which for this rule usually coincides (`StringSet` with + `When.NotExists` returns whether it set) but is not the same thing by definition. +- **The queued `Task` goes away.** If you awaited the task from the queued command, await the single command + instead; there is no longer a separate "did the transaction commit" answer to check first. +- **`CommandFlags` must be carried over verbatim.** In particular a transaction containing a + `CommandFlags.FireAndForget` command does not behave like a fire-and-forget single command. + +## Cases that are deliberately not flagged + +The rule only fires on one condition guarding one queued command with the *same key expression*, because those +are the cases with an exact equivalent. It stays quiet for cross-key conditions, several conditions or +commands, and pairings with no atomic equivalent (`HashExists` + `HashSet`, `HashEqual`, `ListIndexEqual`, the +`*Length*` conditions). + +It also stays quiet where the queued command already passes its own `when:` argument. That is not an argument to +move but a statement to overwrite - and `Condition.KeyNotExists` guarding a `When.Exists` write says "only if +absent, and only if present", which is not code to be rewriting on a guess. An `expiry` is fine, on the other +hand, and still flagged: `SET` takes one alongside `NX`, and that lock-acquire shape is much of what this rule +is for. + +Plus everything under [when these rules stay quiet](index#when-these-rules-stay-quiet). + +See also [Transactions](../Transactions). + +## Guidance, not a verdict + +This rule is a heuristic. It reads your source text - it cannot see your keys, your server, or what you know +about the code - so it is deliberately conservative and stays quiet wherever it is unsure. Everything it flags +still works, and will keep working: this is a suggestion, not a defect report. + +That conservatism is meant to make a false positive rare, not impossible. **If you think the rule has flagged +something it should not have, please [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new)**, +including the transaction as written. A rule that fires on correct code is a bug in the rule - and one that +reaches every consumer of the package - so it is worth fixing rather than quietly suppressing. + +## Suppressing + +The flagged code is correct, just not optimal - but this is reported as a **warning**, so if you build with +`TreatWarningsAsErrors` it will fail your build until you act on it or turn it down. To silence it: + +```xml +$(NoWarn);SER300 +``` + +or locally: + +```c# +#pragma warning disable SER300 +``` diff --git a/docs/rules/SER301.md b/docs/rules/SER301.md new file mode 100644 index 000000000..99ec41abc --- /dev/null +++ b/docs/rules/SER301.md @@ -0,0 +1,97 @@ +# SER301: transaction may be replaceable by a single atomic operation + +A transaction implementing compare-and-set can usually be replaced by the equivalent conditional command on a +server that supports it. + +```c# +// flagged +var tran = db.CreateTransaction(); +tran.AddCondition(Condition.StringEqual(key, token)); +_ = tran.KeyDeleteAsync(key); +await tran.ExecuteAsync(); + +// suggested +await db.StringDeleteAsync(key, ValueCondition.Equal(token)); +``` + +That example is the canonical lock-release; `LockRelease` does the same thing for you. + +## Server version + +These commands (`SET IFEQ`/`IFNE`, `DELIFEQ`) arrived in **Redis 8.4** - see +[Compare-And-Swap / Compare-And-Delete](../CompareAndSwap). This is the whole reason the rule has its own ID +rather than sharing [SER300](SER300): an analyzer cannot see which server you will connect to, so if you target +an older server you want to silence this one while keeping SER300, and a shared ID would not let you. + +The library's own compatibility fallbacks suppress this rule rather than being rewritten, for the same reason. + +### Declaring your server version + +Rather than silencing the rule outright, you can tell it what you are running, and it will only suggest things +your server can actually do: + +```xml + + 7.4 + +``` + +or equivalently in `.editorconfig` / `.globalconfig`, which takes precedence: + +```ini +redis.min_server_version = 7.4 +``` + +Major.minor is what is read; a patch component is accepted and ignored. **Unset means show everything** - a +suggestion you cannot use yet is still worth knowing about, and defaulting to silence would hide the rule from +exactly the people who have not thought about server versions. A value that cannot be parsed is treated as +unset, so a typo cannot silently hide suggestions. + +This affects only the version-gated rules. [SER300](SER300) is unaffected however low you set it, because the +conditional argument forms it suggests are as old as the commands themselves. + +## Why this is worth changing + +`AddCondition` is `WATCH`-based: two round-trips, and it can abort under contention, so correct code needs a +retry loop. The conditional command is one round-trip evaluated atomically on the server, with no abort. + +## What changes when you apply it + +- **The result means something different.** `tran.Execute()` returns "the conditions held and the commands + ran"; the single command returns its own result. +- **The queued `Task` goes away**, so rewire anything that awaited it. +- **`CommandFlags` must be carried over verbatim**, including `FireAndForget`. + +## Cases that are deliberately not flagged + +Only one condition guarding one queued command on the *same key expression* is flagged. Cross-key +compare-and-set genuinely needs the transaction (or Lua), and there is no server-side compare-and-set for hash +fields or list indices, so `HashEqual` and `ListIndexEqual` are left alone. + +Plus everything under [when these rules stay quiet](index#when-these-rules-stay-quiet). + +## Guidance, not a verdict + +This rule is a heuristic. It reads your source text - it cannot see your keys, your server, or what you know +about the code - so it is deliberately conservative and stays quiet wherever it is unsure. Everything it flags +still works, and will keep working: this is a suggestion, not a defect report. + +That conservatism is meant to make a false positive rare, not impossible. **If you think the rule has flagged +something it should not have, please [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new)**, +including the transaction as written. A rule that fires on correct code is a bug in the rule - and one that +reaches every consumer of the package - so it is worth fixing rather than quietly suppressing. + +## Suppressing + +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. To +silence: + +```xml +$(NoWarn);SER301 +``` + +or locally: + +```c# +#pragma warning disable SER301 +``` diff --git a/docs/rules/SER302.md b/docs/rules/SER302.md new file mode 100644 index 000000000..271cffc30 --- /dev/null +++ b/docs/rules/SER302.md @@ -0,0 +1,73 @@ +# SER302: transaction condition may be redundant + +The condition asks exactly what the queued command already tells you through its return value, so the +transaction buys nothing but a round-trip and the risk of aborting. + +```c# +// flagged +var tran = db.CreateTransaction(); +tran.AddCondition(Condition.SetContains(key, member)); +_ = tran.SetRemoveAsync(key, member); +await tran.ExecuteAsync(); + +// suggested +bool removed = await db.SetRemoveAsync(key, member); +``` + +`SetRemove` returns `false` when the member was not there - which is what the condition was checking. + +Applies to `SetNotContains` + `SetAdd`, `SetContains` + `SetRemove`, `SortedSetContains` + +`SortedSetRemove`, `HashExists` + `HashDelete`, `KeyExists` + `KeyDelete`, and `KeyExists` + `KeyExpire` +(`EXPIRE` already returns `false` for a missing key). No particular server version is involved: these commands +have always reported this. + +## What changes when you apply it + +This is a bigger change than [SER300](SER300), which is why it has its own ID: the fix deletes the transaction +rather than moving an argument, and **the result changes meaning**. + +- `tran.Execute()` returning `false` means "the guard did not hold, so nothing ran". +- The single command returning `false` means "it ran, and had no effect". + +Those usually amount to the same decision, but not always - code that logs, retries, or reports differently +between "someone beat me to it" and "there was nothing to do" needs a second look. The queued `Task` also +disappears, and `CommandFlags` must be carried over verbatim. + +## Cases that are deliberately not flagged + +- **Different member or field.** A condition about member `"a"` does not guard a write to member `"b"`; that + transaction is doing real work, and the rule stays quiet even though the key matches. +- **`ListIndexExists` + `ListSetByIndex`.** `LSET` reports an out-of-range index by *failing*, not by returning + `false` (`ListSetByIndex` returns `Task`, not `Task`), so dropping the condition would turn an aborted + transaction into an exception. That is a change in behaviour, not a simplification. + +Plus everything under [when these rules stay quiet](index#when-these-rules-stay-quiet) - except the one about +arguments, which does not apply here. This rule keeps the command exactly as you wrote it and deletes only the +condition, so there is nothing it could drop. + +See also [Transactions](../Transactions). + +## Guidance, not a verdict + +This rule is a heuristic. It reads your source text - it cannot see your keys, your server, or what you know +about the code - so it is deliberately conservative and stays quiet wherever it is unsure. Everything it flags +still works, and will keep working: this is a suggestion, not a defect report. + +That conservatism is meant to make a false positive rare, not impossible. **If you think the rule has flagged +something it should not have, please [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new)**, +including the transaction as written. A rule that fires on correct code is a bug in the rule - and one that +reaches every consumer of the package - so it is worth fixing rather than quietly suppressing. + +## Suppressing + +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. + +```xml +$(NoWarn);SER302 +``` + +or locally: + +```c# +#pragma warning disable SER302 +``` diff --git a/docs/rules/SER303.md b/docs/rules/SER303.md new file mode 100644 index 000000000..e4152f8d6 --- /dev/null +++ b/docs/rules/SER303.md @@ -0,0 +1,100 @@ +# SER303: transaction may be replaceable by a single compound command + +There is no condition here at all - the transaction exists only to make two commands atomic, and a single +command already does both. + +```c# +// flagged +var tran = db.CreateTransaction(); +var value = tran.StringGetAsync(key); +_ = tran.KeyDeleteAsync(key); +await tran.ExecuteAsync(); + +// suggested +RedisValue value = await db.StringGetDeleteAsync(key); +``` + +| Queued pair | Single command | Server | +|---|---|---| +| `StringGet` + `KeyDelete` | `StringGetDelete` (GETDEL) | 6.2 | +| `StringGet` + `KeyExpire` | `StringGetSetExpiry` (GETEX) | 6.2 | +| `StringGet` + `KeyPersist` | `StringGetSetExpiry(key, null)` (GETEX PERSIST) | 6.2 | +| `StringGet` + `StringSet` | `StringSetAndGet` (SET ... GET) | 6.2 | +| `HashGet` + `HashDelete` | `HashFieldGetAndDelete` (HGETDEL) | 8.0 | +| `StringSet` + `KeyExpire` | `StringSet(key, value, expiry)` (SET ... EX) | any | +| `SetRemove` + `SetAdd` | `SetMove` (SMOVE) | any | + +The requirement varies across this family, from "any server" for SMOVE up to 8.0 for HGETDEL, so each message +names its own - see [declaring your server version](index#declaring-your-server-version) to be shown only what +your server supports. + +One caveat on `StringSet` + `KeyExpire`: an absolute expiry works too, because `Expiration` converts implicitly +from `DateTime` as well as `TimeSpan` - but the `SET ... EXAT` that produces does want a 6.2 server, where the +relative form has worked since 2.6.12. The version column above is the relative case, which is the common one. + +## Order matters + +Which way round the pair is queued is part of the meaning, for two different reasons. + +The reads return a value: `SET ... GET` hands back the value from *before* the write, so it matches a queued get +followed by a set - and **not** a set followed by a get, which asks for the value afterwards. That pairing is +left alone. + +The writes overwrite each other: `SET` clears any TTL on the key, so `StringSet` + `KeyExpire` is one command +with a lifetime, while `KeyExpire` + `StringSet` ends with no expiry at all. Only the first order is flagged. +For the same reason a `StringSet` that *already* carries an expiry, followed by a `KeyExpire` that overrides it, +is left alone: which of the two lifetimes the single command should carry is a guess. + +`SetRemove` + `SetAdd` is the exception: within a transaction both effects happen regardless of order, so either +spelling is flagged. + +## What changes when you apply it + +- `tran.Execute()` returns whether the transaction ran; the compound command returns its own result - usually + the value you were reading anyway. +- The queued `Task`s collapse into the single command's result. +- `CommandFlags` must be carried over verbatim. + +## Cases that are deliberately not flagged + +- **`ListRightPop` + `ListLeftPush`.** This looks like `LMOVE`, and it is not. `LMOVE` moves *the element it + popped*; inside a transaction the pop's result is an unresolved `Task`, so the caller cannot pass it to the + push - whatever value is being pushed is a different one, and `LMOVE` would not reproduce it. The same + reasoning rules out every read-modify-write pairing. +- **Different keys** (or different members, for `SetMove`) - those are genuinely two operations. +- **Anything with a condition**, which is [SER300](SER300)-[SER302](SER302) territory. +- **A key local reassigned between the two calls** - the keys are compared as source text, so a reassignment + means identical text can be two different keys, and the rule stays quiet. +- **Three or more queued commands**, and anything queued in a loop. Note that the same command repeated - which + can be three or more - is [SER304](SER304) rather than this rule. + +Plus everything under [when these rules stay quiet](index#when-these-rules-stay-quiet), which is where the +family-wide cases live - a third queued command, commands in different branches, and arguments the compound +command cannot carry. + +See also [Transactions](../Transactions). + +## Guidance, not a verdict + +This rule is a heuristic. It reads your source text - it cannot see your keys, your server, or what you know +about the code - so it is deliberately conservative and stays quiet wherever it is unsure. Everything it flags +still works, and will keep working: this is a suggestion, not a defect report. + +That conservatism is meant to make a false positive rare, not impossible. **If you think the rule has flagged +something it should not have, please [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new)**, +including the transaction as written. A rule that fires on correct code is a bug in the rule - and one that +reaches every consumer of the package - so it is worth fixing rather than quietly suppressing. + +## Suppressing + +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. + +```xml +$(NoWarn);SER303 +``` + +or locally: + +```c# +#pragma warning disable SER303 +``` diff --git a/docs/rules/SER304.md b/docs/rules/SER304.md new file mode 100644 index 000000000..fc88f4a39 --- /dev/null +++ b/docs/rules/SER304.md @@ -0,0 +1,100 @@ +# SER304: repeated queued operations may suit the variadic overload + +The same command is queued several times over, and one variadic call does the lot - one round-trip, atomic on +the server, no transaction needed. + +```c# +// flagged +var tran = db.CreateTransaction(); +_ = tran.SetAddAsync(key, "a"); +_ = tran.SetAddAsync(key, "b"); +await tran.ExecuteAsync(); + +// suggested +long added = await db.SetAddAsync(key, new RedisValue[] { "a", "b" }); +``` + +## What it covers + +**One key, many values** - every call must be on the same key: + +| Repeated | Single call | Server | +|---|---|---| +| `SetAdd` / `SetRemove` | `SetAdd(key, values)` / `SetRemove(key, values)` | any | +| `SortedSetAdd` / `SortedSetRemove` | `SortedSetAdd(key, entries)` / `SortedSetRemove(key, members)` | any | +| `HashSet` / `HashDelete` | `HashSet(key, entries)` / `HashDelete(key, fields)` | any | +| `ListLeftPush` / `ListRightPush` | `ListLeftPush(key, values)` / `ListRightPush(key, values)` | any | +| `SetContains` | `SetContains(key, values)` (SMISMEMBER) | 6.2 | + +**Many keys** - the calls must be on *different* keys: + +| Repeated | Single call | Server | +|---|---|---| +| `StringSet` | `StringSet(KeyValuePair[])` (MSET) | any | +| `StringGet` | `StringGet(keys)` (MGET) | any | +| `KeyDelete` | `KeyDelete(keys)` (DEL) | any | +| `KeyExists` | `KeyExists(keys)` (EXISTS) | any | + +Which direction applies is the whole distinction: `SADD` takes one key and many values, so calls across +different keys have no single-command form; `MSET` takes many keys, so calls on one key are not what this is +about. Neither is flagged in the wrong direction. + +Most of these variadic forms arrived in Redis 2.4, which predates anything realistically in service, so no +version is mentioned. SMISMEMBER at 6.2 is recent enough to say so - see +[declaring your server version](index#declaring-your-server-version). + +## What changes when you apply it + +This is why it has its own ID rather than sharing [SER303](SER303): **the result changes shape**, not just +meaning. + +- N calls each returning `bool` become one returning a `long` count. You learn how many were added or removed, + not which ones. +- N calls each returning a value become one returning an array (`StringGet`, or `bool[]` for `SetContains`). +- The individual queued `Task`s disappear, so anything awaiting them individually needs rewiring. +- `CommandFlags` must be carried over verbatim. + +If your code genuinely needs to know *which* of the members was new, the per-call form is the right one and this +suggestion is not for you - suppress it. + +## Cases that are deliberately not flagged + +- **Commands queued in a loop.** This is the most common way the shape arises in practice, and it stays quiet on + purpose: a loop body is one call site, and we cannot show that the key expression is loop-invariant, so we + cannot tell a same-key collapse from a per-key one. Guessing would be worse than silence. +- **A key local reassigned between the calls.** The keys are compared as source text, which is only sound while + the locals hold the same value throughout; a reassignment anywhere in the method means identical text can be + two different keys, so the rule stays quiet. This applies to every rule in this family. +- **N x `ListLeftPop` across keys is not `LMPOP`.** LMPOP pops from the first *non-empty* key of those given, + not from each of them - a different operation, however similar the argument list looks. Same for `ZMPOP`. +- **Anything with a condition**, which is [SER300](SER300)-[SER302](SER302) territory. +- **Calls carrying an argument the variadic form has no room for.** `MSET` takes one expiry for the whole batch + rather than one per key, and the variadic `HashSet` has no `When`, so calls that pass those are left alone - + collapsing them would silently drop the argument, and in the `MSET` case leave your keys with no expiry at all. + +Plus everything under [when these rules stay quiet](index#when-these-rules-stay-quiet). + +## Guidance, not a verdict + +This rule is a heuristic. It reads your source text - it cannot see your keys, your server, or what you know +about the code - so it is deliberately conservative and stays quiet wherever it is unsure. Everything it flags +still works, and will keep working: this is a suggestion, not a defect report. + +That conservatism is meant to make a false positive rare, not impossible. **If you think the rule has flagged +something it should not have, please [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new)**, +including the transaction as written. A rule that fires on correct code is a bug in the rule - and one that +reaches every consumer of the package - so it is worth fixing rather than quietly suppressing. + +## Suppressing + +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. + +```xml +$(NoWarn);SER304 +``` + +or locally: + +```c# +#pragma warning disable SER304 +``` diff --git a/docs/rules/SER350.md b/docs/rules/SER350.md new file mode 100644 index 000000000..b74c4bf0d --- /dev/null +++ b/docs/rules/SER350.md @@ -0,0 +1,29 @@ +# SER350: language version too low for generated code + +The `[AsciiHash]` source generator emits UTF-8 string literals (`"..."u8`), which are **C# 11**. The project +using the attribute is set to an older language version, so rather than emitting code that cannot compile, the +generator emitted nothing and reported this. + +## Fixing it + +Raise the language version: + +```xml +11 +``` + +The language version is not tied to the target framework, so an old `TargetFramework` is not a barrier: any +.NET 7 or later SDK can compile C# 11 for `netstandard2.0` or `net472`. Those TFMs just *default* lower (C# 7.3), +which is how this is usually reached. + +## Why this is a warning + +Unlike the other rules in this space, this one reports a real problem rather than a suggestion, and it cannot +fire spuriously - the language version is known, and it is only checked when `[AsciiHash]` is actually used. The +alternatives are both worse: emit anyway and put errors inside generated code you cannot edit, or emit nothing +silently and surface it as an unexplained "partial method has no implementing declaration". + +## Suppressing + +Suppressing leaves the partial members unimplemented, so the build will fail anyway with a less helpful message. +Raise `` instead, or stop using `[AsciiHash]` in that project. diff --git a/docs/rules/index.md b/docs/rules/index.md new file mode 100644 index 000000000..5f8115b88 --- /dev/null +++ b/docs/rules/index.md @@ -0,0 +1,124 @@ +# Analyzer rules + +StackExchange.Redis ships a Roslyn analyzer inside the package, so these rules are reported in your own build +with no extra reference. Each diagnostic links here from its message. + +The `SER3xx` range belongs to this analyzer, and is split so that the two kinds of report can be configured +separately: + +| Range | Meaning | +|---|---| +| `SER300`-`SER349` | usage guidance about your code | +| `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), +which mean "this API is preview", not "consider changing this code". + +## Reading the suggestions + +Messages name the replacement as `StringSet[Async](...)`, following the convention used elsewhere in these docs: +there is a `StringSet` and a `StringSetAsync`, and you want whichever matches the code around it. The `[Async]` +is not something to type. + +Which one that is depends on how you were finishing the transaction, not on the call being replaced - commands +queued on an `ITransaction` are always the `...Async` ones, because that is the only surface it offers. If you +were writing `await tran.ExecuteAsync()`, you want `StringSetAsync`; if you were writing `tran.Execute()`, you +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. + +## Usage + +- [SER300](SER300) - transaction may be replaceable by a conditional argument (any server version) +- [SER301](SER301) - transaction may be replaceable by a single atomic operation (needs a newer server) +- [SER302](SER302) - condition may be redundant; the command already reports whether it acted (any server version) +- [SER303](SER303) - two queued operations may be a single compound command (varies by pair) +- [SER304](SER304) - the same operation queued repeatedly may suit the variadic overload (mostly any server) + +## Build + +- [SER350](SER350) - language version too low for generated code + +## When these rules stay quiet + +Every rule here is deliberately conservative. It ships to every consumer of the package, and a wrong suggestion +on correct code is worse than no suggestion at all, so the following apply across the whole family - on top of +whatever each rule's own page lists. + +- **Anything else queued on the same transaction.** These rules describe a whole transaction, not a fragment of + one. A third queued command means the transaction is doing more than the rule accounts for - and that includes + a raw `tran.ExecuteAsync("SOMECMD", ...)` for something the library has no wrapper for. +- **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. +- **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` + on a command whose variadic form has none, an `ExpireWhen` where GETEX has no NX/XX, and your own `when:` + argument where the suggestion *is* a `when:` argument. `CommandFlags` is the exception: it is on everything, no + suggestion mentions it, and you carry it over verbatim. +- **A transaction passed to another method, stored in a field, or otherwise captured** - what it queues elsewhere + is not visible from here. +- **A key or member local reassigned anywhere in the method.** Keys are compared as source text, which is only + sound while the locals hold the same value throughout. + +These are heuristics, and the list above is where the effort has gone - but it is meant to make a false positive +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. + +## Declaring your server version + +Some suggestions need a recent server, and an analyzer cannot see the server you will connect to. Declare your +floor and you will only be shown suggestions you can act on: + +```xml + + 7.4 + +``` + +or, taking precedence, in `.editorconfig` / `.globalconfig`: + +```ini +redis.min_server_version = 7.4 +``` + +Unset shows everything, which is the default: a suggestion you cannot use yet is still worth knowing about. Each +rule's message names the version it needs, so you can tell at a glance whether it applies to you. + +## 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. + +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 +turning them down. + +Per rule, in `.editorconfig`: + +```ini +dotnet_diagnostic.SER300.severity = suggestion # or none, silent, warning, error +``` + +Or for the whole family, in your project file: + +```xml +$(NoWarn);SER300;SER301;SER302;SER303;SER304 +``` + +Or at a single site, where the transaction is deliberate: + +```c# +#pragma warning disable SER301 // deliberate fallback for older servers +``` + +If you want the old behaviour everywhere, `suggestion` is the severity that matches what these shipped as +before: visible in the IDE, absent from the build log. diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md new file mode 100644 index 000000000..9ca930621 --- /dev/null +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md @@ -0,0 +1,17 @@ +; Shipped analyzer releases; see AnalyzerReleases.Unshipped.md for the convention. +; Recorded as shipped from the release that first carries the analyzer, rather than being staged in Unshipped +; first: these IDs go out with 3.1 as the initial set, so there is no window in which they are unshipped, and +; nothing is gained by tracking them in two places on the way. Later additions do go through Unshipped. + +## Release 3.1 + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +SER300 | Usage | Warning | TransactionAnalyzer: transaction may be replaceable by a conditional argument (any server) +SER301 | Usage | Warning | TransactionAnalyzer: transaction may be replaceable by a single atomic operation (newer server) +SER302 | Usage | Warning | TransactionAnalyzer: condition may be redundant; the queued command already reports whether it acted +SER303 | Usage | Warning | TransactionAnalyzer: two queued operations may be a single compound command +SER304 | Usage | Warning | TransactionAnalyzer: repeated queued operations may suit the variadic overload +SER350 | Build | Warning | AsciiHashGenerator: generated code requires a newer C# language version, so nothing was generated diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md new file mode 100644 index 000000000..b9d09f18c --- /dev/null +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md @@ -0,0 +1,8 @@ +; Unshipped analyzer release +; Tracks the diagnostics reported by the analyzers/generators shipped inside the StackExchange.Redis package. +; This is the analyzer equivalent of PublicAPI.Unshipped.txt: a diagnostic ID is a public contract once +; 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. diff --git a/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs b/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs index b00675c40..5b97d6af6 100644 --- a/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs +++ b/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs @@ -12,63 +12,97 @@ namespace StackExchange.Redis.Build; [Generator(LanguageNames.CSharp)] public class AsciiHashGenerator : IIncrementalGenerator { + /// + /// The emitted code uses UTF-8 string literals, which are C# 11. + /// + private const LanguageVersion MinimumLanguageVersion = LanguageVersions.CSharp11; + + /// + /// The attribute that drives this generator, by metadata name. + /// + /// + /// Fully qualified, and matched by the host rather than by us: ForAttributeWithMetadataName indexes + /// attributes across the compilation once and only calls us for real matches. The predicates below used to + /// compare attribute *text* on every attribute in every file, which was both slower and looser - it would + /// have matched an unrelated attribute that happened to be called AsciiHash, and missed one reached + /// through an alias. + /// + private const string AsciiHashAttributeName = "RESPite.AsciiHashAttribute"; + public void Initialize(IncrementalGeneratorInitializationContext context) { // looking for [AsciiHash] partial static class Foo { } var types = context.SyntaxProvider - .CreateSyntaxProvider( - static (node, _) => node is ClassDeclarationSyntax decl && IsStaticPartial(decl.Modifiers) && - HasAsciiHash(decl.AttributeLists), + .ForAttributeWithMetadataName( + AsciiHashAttributeName, + static (node, _) => node is ClassDeclarationSyntax decl && IsStaticPartial(decl.Modifiers), TransformTypes) .Where(pair => pair.Name is { Length: > 0 }) .Collect(); // looking for [AsciiHash] partial static bool TryParse(input, out output) { } var methods = context.SyntaxProvider - .CreateSyntaxProvider( - static (node, _) => node is MethodDeclarationSyntax decl && IsStaticPartial(decl.Modifiers) && - HasAsciiHash(decl.AttributeLists), + .ForAttributeWithMetadataName( + AsciiHashAttributeName, + static (node, _) => node is MethodDeclarationSyntax decl && IsStaticPartial(decl.Modifiers), TransformMethods) .Where(pair => pair.Name is { Length: > 0 }) .Collect(); // looking for [AsciiHash] partial static bool TryFormat(enum input, out string/ReadOnlySpan output) { } var formatMethods = context.SyntaxProvider - .CreateSyntaxProvider( - static (node, _) => node is MethodDeclarationSyntax decl && IsStaticPartial(decl.Modifiers) && - HasAsciiHash(decl.AttributeLists), + .ForAttributeWithMetadataName( + AsciiHashAttributeName, + static (node, _) => node is MethodDeclarationSyntax decl && IsStaticPartial(decl.Modifiers), TransformFormatMethods) .Where(pair => pair.Name is { Length: > 0 }) .Collect(); // looking for [AsciiHash("some type")] enum Foo { } var enums = context.SyntaxProvider - .CreateSyntaxProvider( - static (node, _) => node is EnumDeclarationSyntax decl && HasAsciiHash(decl.AttributeLists), + .ForAttributeWithMetadataName( + AsciiHashAttributeName, + static (node, _) => node is EnumDeclarationSyntax, TransformEnums) .Where(pair => pair.Name is { Length: > 0 }) .Collect(); + // The code we emit uses UTF-8 string literals ("..."u8), so it will not compile below C# 11. Old TFMs + // default below that (netstandard2.0 and net472 default to C# 7.3), but the language version is not + // tied to the target framework - any consumer on a .NET 7 or later SDK can opt in with , + // so this should be rare and is trivially fixable. The point is to *say* that: emitting anyway would + // put errors inside generated code the consumer cannot edit, and emitting nothing silently would + // surface as an unexplained "no implementing declaration". See Diagnostics.LanguageVersionTooLow. + var languageVersion = context.ParseOptionsProvider.Select(static (options, _) + => options is CSharpParseOptions cs ? cs.LanguageVersion.MapSpecifiedToEffectiveVersion() : LanguageVersion.Latest); + context.RegisterSourceOutput( - types.Combine(methods).Combine(formatMethods).Combine(enums), + types.Combine(methods).Combine(formatMethods).Combine(enums).Combine(languageVersion), (ctx, content) => - Generate(ctx, content.Left.Left.Left, content.Left.Left.Right, content.Left.Right, content.Right)); - - static bool IsStaticPartial(SyntaxTokenList tokens) - => tokens.Any(SyntaxKind.StaticKeyword) && tokens.Any(SyntaxKind.PartialKeyword); - - static bool HasAsciiHash(SyntaxList attributeLists) - { - foreach (var attribList in attributeLists) { - foreach (var attrib in attribList.Attributes) + if (content.Right < MinimumLanguageVersion) { - if (attrib.Name.ToString() is nameof(AsciiHashAttribute) or nameof(AsciiHash)) return true; + // only complain if there was actually something to generate + var (t, m, f, e) = (content.Left.Left.Left.Left, content.Left.Left.Left.Right, content.Left.Left.Right, content.Left.Right); + if (t.Length + m.Length + f.Length + e.Length != 0) + { + ctx.ReportDiagnostic(Diagnostic.Create( + Diagnostics.LanguageVersionTooLow, + location: null, + nameof(AsciiHashAttribute), + "11", + content.Right.ToDisplayString())); + } + + return; } - } - return false; - } + var left = content.Left; + Generate(ctx, left.Left.Left.Left, left.Left.Left.Right, left.Left.Right, left.Right); + }); + + static bool IsStaticPartial(SyntaxTokenList tokens) + => tokens.Any(SyntaxKind.StaticKeyword) && tokens.Any(SyntaxKind.PartialKeyword); } private static string GetName(INamedTypeSymbol type) @@ -114,11 +148,13 @@ private static string GetName(INamedTypeSymbol type) } private (string Namespace, string ParentType, string Name, int Count, int MaxChars, int MaxBytes) TransformEnums( - GeneratorSyntaxContext ctx, CancellationToken cancellationToken) + GeneratorAttributeSyntaxContext ctx, CancellationToken cancellationToken) { // extract the name and value (defaults to name, but can be overridden via attribute) and the location - if (ctx.SemanticModel.GetDeclaredSymbol(ctx.Node) is not INamedTypeSymbol { TypeKind: TypeKind.Enum } named) return default; - if (TryGetAsciiHashAttribute(named.GetAttributes()) is not { } attrib) return default; + if (ctx.TargetSymbol is not INamedTypeSymbol { TypeKind: TypeKind.Enum } named) return default; + // list patterns would need System.Index, which netstandard2.0 does not have + if (ctx.Attributes.IsDefaultOrEmpty) return default; + var attrib = ctx.Attributes[0]; var innerName = GetRawValue("", attrib); if (string.IsNullOrWhiteSpace(innerName)) return default; @@ -150,12 +186,14 @@ private static string GetName(INamedTypeSymbol type) } private (string Namespace, string ParentType, string Name, string Value) TransformTypes( - GeneratorSyntaxContext ctx, + GeneratorAttributeSyntaxContext ctx, CancellationToken cancellationToken) { // extract the name and value (defaults to name, but can be overridden via attribute) and the location - if (ctx.SemanticModel.GetDeclaredSymbol(ctx.Node) is not INamedTypeSymbol { TypeKind: TypeKind.Class } named) return default; - if (TryGetAsciiHashAttribute(named.GetAttributes()) is not { } attrib) return default; + if (ctx.TargetSymbol is not INamedTypeSymbol { TypeKind: TypeKind.Class } named) return default; + // list patterns would need System.Index, which netstandard2.0 does not have + if (ctx.Attributes.IsDefaultOrEmpty) return default; + var attrib = ctx.Attributes[0]; string ns = "", parentType = ""; if (named.ContainingType is { } containingType) @@ -195,10 +233,10 @@ private static string GetRawValue(string name, AttributeData? asciiHashAttribute (string Type, string Name, bool IsBytes, RefKind RefKind) From, (string Type, string Name, RefKind RefKind) To, (string Name, bool Value, RefKind RefKind) CaseSensitive, BasicArray<(string EnumMember, string ParseText)> Members, int DefaultValue) TransformMethods( - GeneratorSyntaxContext ctx, + GeneratorAttributeSyntaxContext ctx, CancellationToken cancellationToken) { - if (ctx.SemanticModel.GetDeclaredSymbol(ctx.Node) is not IMethodSymbol + if (ctx.TargetSymbol is not IMethodSymbol { IsStatic: true, IsPartialDefinition: true, @@ -212,14 +250,16 @@ private static string GetRawValue(string name, AttributeData? asciiHashAttribute }, } method) return default; - if (TryGetAsciiHashAttribute(method.GetAttributes()) is not { } attrib) return default; + // list patterns would need System.Index, which netstandard2.0 does not have + if (ctx.Attributes.IsDefaultOrEmpty) return default; + var attrib = ctx.Attributes[0]; if (method.ContainingType is not { } containingType) return default; var parentType = GetName(containingType); var ns = containingType.ContainingNamespace.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat); var arg = method.Parameters[0]; - if (arg is not { IsOptional: false, RefKind: RefKind.None or RefKind.In or RefKind.Ref or RefKind.RefReadOnlyParameter }) return default; + if (arg is not { IsOptional: false, RefKind: RefKind.None or RefKind.In or RefKind.Ref or RefKinds.RefReadOnlyParameter }) return default; static bool IsBytes(ITypeSymbol type) { @@ -283,7 +323,7 @@ static bool IsBytes(ITypeSymbol type) arg = method.Parameters[2]; if (arg is not { - RefKind: RefKind.None or RefKind.In or RefKind.Ref or RefKind.RefReadOnlyParameter, + RefKind: RefKind.None or RefKind.In or RefKind.Ref or RefKinds.RefReadOnlyParameter, Type.SpecialType: SpecialType.System_Boolean, }) { @@ -321,10 +361,10 @@ static bool IsBytes(ITypeSymbol type) private (string Namespace, string ParentType, Accessibility Accessibility, string Name, (string Type, string Name, RefKind RefKind) From, (string Type, string Name, RefKind RefKind, bool IsBytes) To, BasicArray<(string EnumMember, string FormatText)> Members) TransformFormatMethods( - GeneratorSyntaxContext ctx, + GeneratorAttributeSyntaxContext ctx, CancellationToken cancellationToken) { - if (ctx.SemanticModel.GetDeclaredSymbol(ctx.Node) is not IMethodSymbol + if (ctx.TargetSymbol is not IMethodSymbol { IsStatic: true, IsPartialDefinition: true, @@ -338,7 +378,7 @@ static bool IsBytes(ITypeSymbol type) }, } method) return default; - if (TryGetAsciiHashAttribute(method.GetAttributes()) is not { }) return default; + if (ctx.Attributes.IsDefaultOrEmpty) return default; if (method.ContainingType is not { } containingType) return default; var parentType = GetName(containingType); @@ -348,7 +388,7 @@ static bool IsBytes(ITypeSymbol type) if (arg is not { IsOptional: false, - RefKind: RefKind.None or RefKind.In or RefKind.Ref or RefKind.RefReadOnlyParameter, + RefKind: RefKind.None or RefKind.In or RefKind.Ref or RefKinds.RefReadOnlyParameter, Type: INamedTypeSymbol { TypeKind: TypeKind.Enum }, }) return default; var from = (arg.Type.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), arg.Name, arg.RefKind); @@ -847,7 +887,7 @@ private static bool HasCaseSensitiveCharacters(BasicArray<(string EnumMember, st RefKind.In => "in ", RefKind.Out => "out ", RefKind.Ref => "ref ", - RefKind.RefReadOnlyParameter or RefKind.RefReadOnly => "ref readonly ", + RefKinds.RefReadOnlyParameter or RefKind.RefReadOnly => "ref readonly ", _ => throw new NotSupportedException($"RefKind {refKind} is not yet supported."), }; private static string Format(Accessibility accessibility) => accessibility switch diff --git a/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs b/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs index a8df6744c..a74e4e3e6 100644 --- a/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs +++ b/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs @@ -33,6 +33,18 @@ public void Initialize(IncrementalGeneratorInitializationContext ctx) ctx.RegisterSourceOutput(interfaces.Combine(classes), static (ctx, content) => Generate(ctx, content.Left, content.Right)); } + /// + /// The only assembly this generator has anything to say about. + /// + /// + /// This is repo-internal machinery, but it now ships as an analyzer inside the StackExchange.Redis package + /// (for AsciiHashGenerator's benefit), so it is loaded by every consumer. It can never generate + /// anything useful for them - the semantic checks below reject anything that isn't our own + /// StackExchange.Redis declaration - so short-circuit on the assembly name first, and skip even the + /// semantic-model query for the consumers who happen to declare a type called IDatabase. + /// + private const string OwningAssembly = "StackExchange.Redis"; + static KnownInterfaces Identify(string type) => type switch { "IDatabase" => KnownInterfaces.IDatabase, @@ -85,6 +97,8 @@ symbol is private static InterfaceInfo ExtractInterfaceMethods(GeneratorSyntaxContext context, CancellationToken cancel) { + if (context.SemanticModel.Compilation.AssemblyName is not OwningAssembly) return default; + // note: we deliberately do NOT interpret anything here - just capture the raw shape of every // method (name, return type, and per-parameter name/type/modifiers/optionality/default) so that // later passes have everything they might need. @@ -130,6 +144,8 @@ private static InterfaceInfo ExtractInterfaceMethods(GeneratorSyntaxContext cont private ClassInfo ExtractClasses(GeneratorSyntaxContext context, CancellationToken cancel) { + if (context.SemanticModel.Compilation.AssemblyName is not OwningAssembly) return default; + // note: we deliberately do NOT interpret anything here - just capture the raw shape of every // method (name, return type, and per-parameter name/type/modifiers/optionality/default) so that // later passes have everything they might need. diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs new file mode 100644 index 000000000..b930b125f --- /dev/null +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -0,0 +1,174 @@ +using Microsoft.CodeAnalysis; + +namespace StackExchange.Redis.Build; + +/// +/// Diagnostics reported by the analyzers and generators shipped inside the StackExchange.Redis package. +/// +/// +/// +/// The SER identifier space is shared with the [Experimental] API gates in +/// RESPite.Experiments, which own SER0xx and mean something quite different ("this API is +/// preview"). Everything reported by this assembly lives in SER3xx, split as: +/// +/// +/// SER300-SER349: usage guidance about consumer code (the analyzers). +/// SER350-SER399: build-level problems (the generators). +/// +/// +/// These are a public contract: once shipped, an ID cannot be reused or re-pointed, because consumers put +/// them in NoWarn and .editorconfig. +/// +/// +/// Everything here defaults to , including the usage rules, whose code +/// is correct rather than broken. That is a deliberate change from an earlier default: information-level diagnostics are not printed by dotnet +/// build at all, so outside an IDE the rules simply did not exist, and a suggestion nobody sees is not +/// worth shipping. The cost is real and should be understood rather than discovered: a consumer building with +/// TreatWarningsAsErrors gets a *failing build* on upgrade, on code that works. They can turn any of +/// these down per-rule in .editorconfig or NoWarn, and the help pages say how - but the first +/// experience is a broken build, and that is the trade being made on purpose. +/// +/// +/// Which is also why the usage rules hedge - "may be replaceable", "looks like", "consider" - rather than +/// asserting. They are heuristics over source text, so a false positive is rare rather than impossible, and +/// arriving as a warning already overstates the case; wording them as findings of fact would overstate it +/// twice. The build-level does not hedge, because it is not guessing. +/// +/// +internal static class Diagnostics +{ + private const string UsageCategory = "Usage", BuildCategory = "Build"; + + /// + /// Where the docs for a rule live; docs/rules/{id}.md on the published site. + /// + /// + /// Separate from the exp/ pages used by the [Experimental] gates, which mean something else + /// entirely ("this API is preview"). Every ID below must have a page, because the message can only carry a + /// sketch of the rewrite - the caveats that actually catch people out (the result changes meaning, the + /// queued task disappears, CommandFlags has to be carried over) only fit in prose. + /// + private const string HelpLinkFormat = "https://stackexchange.github.io/StackExchange.Redis/rules/{0}"; + + /// + /// Family A: the condition duplicates a when: argument that already exists on the queued command. + /// + /// + /// The cheapest and safest family: a purely mechanical rewrite that needs no newer server, because the + /// conditional form has existed as long as the command has. Kept separate from precisely because that one is version-dependent and this is not. + /// + public static readonly DiagnosticDescriptor PreferConditionalArgument = new( + id: "SER300", + title: "Transaction may be replaceable by a conditional argument", + messageFormat: "Consider expressing this transaction ({0} guarding {1}) as {2} - the condition duplicates an argument the command already has", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "A transaction whose only purpose is to make one operation conditional can usually be replaced by the command's own conditional argument, which is a single round-trip and cannot abort under contention.", + helpLinkUri: HelpLink("SER300")); + + /// + /// Family B: a newer single command subsumes both the condition and the write. + /// + /// + /// Separate ID from because the suggestion is only actionable + /// against a new enough server (compare-and-set needs 8.4 - see RedisFeatures.SetWithValueCheck and + /// DeleteWithValueCheck), and an analyzer cannot see the server it will talk to. A consumer stuck on + /// an older server wants to silence this one while keeping SER300, which a shared ID would prevent. This is + /// also why the library's own compatibility fallbacks suppress it rather than being rewritten. + /// + /// The required version is per-mapping data rather than part of the rule (see ServerVersion), so the + /// message can name it and a project that declares its own floor - <RedisMinServerVersion>, or + /// redis.min_server_version in .editorconfig - gets only the suggestions it can act on. + /// + /// + public static readonly DiagnosticDescriptor PreferNewerAtomicOperation = new( + id: "SER301", + title: "Transaction may be replaceable by a single atomic operation", + messageFormat: "Consider expressing this transaction ({0} guarding {1}) as {2}, which is atomic on the server and needs no WATCH (requires server {3} or later)", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "A transaction implementing compare-and-set can usually be replaced by the equivalent conditional command on servers that support it, which is a single round-trip and cannot abort under contention.", + helpLinkUri: HelpLink("SER301")); + + /// + /// Family C: the condition asks what the queued command already answers. + /// + /// + /// Its own ID rather than sharing because the fix is a different + /// shape: it deletes the transaction instead of moving an argument into the command, and what the caller + /// observes changes meaning - Execute() returning false ("the guard failed, nothing ran") + /// becomes the command's own false ("it ran and had no effect"). Those coincide in intent but a + /// caller distinguishing them wants to notice. Version-free: these return values have always been there. + /// + public static readonly DiagnosticDescriptor RedundantCondition = new( + id: "SER302", + title: "Transaction condition may be redundant", + messageFormat: "This transaction ({0} guarding {1}) looks redundant - consider {2}", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "A condition that checks what the queued command already reports through its return value usually buys nothing: the transaction costs an extra round-trip and can abort, and the command alone says whether it acted.", + helpLinkUri: HelpLink("SER302")); + + /// + /// Family D: no condition at all - two queued commands that are one compound command. + /// + /// + /// The message assembles its own version clause (argument 3) rather than baking one into the format, + /// because unlike the requirement genuinely varies across this + /// family - SMOVE is as old as sets, HGETDEL is 8.0 - and "requires server 1.0 or later" would be noise. + /// + public static readonly DiagnosticDescriptor PreferCompoundCommand = new( + id: "SER303", + title: "Transaction may be replaceable by a single compound command", + messageFormat: "These two queued operations ({0} then {1}) look like one command - consider {2}{3}", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "A transaction used only to make two operations atomic can usually be replaced by the single command that does both, which is one round-trip and cannot abort.", + helpLinkUri: HelpLink("SER303")); + + /// + /// Family D, second flavour: the same command queued repeatedly, where one variadic call does the lot. + /// + /// + /// Separate from because the result changes *shape* rather than just + /// meaning: N calls each returning bool become one returning a count, and N returning a value become + /// one returning an array. Somebody happy to adopt GETDEL may well not want to rework how they read results, + /// and a shared ID would not let them separate the two. + /// + public static readonly DiagnosticDescriptor PreferVariadicOverload = new( + id: "SER304", + title: "Repeated queued operations may suit the variadic overload", + messageFormat: "These {1} queued {0} calls look like one command - consider {2}{3}", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "The same command queued several times over can usually be a single variadic call, which is one round-trip and needs no transaction to be atomic.", + helpLinkUri: HelpLink("SER304")); + + /// + /// The generated code cannot be compiled at the language version in effect, so nothing was generated. + /// + /// + /// Expected to be rare, and always fixable by the consumer with <LangVersion> - the language + /// version is not tied to the target framework, so an old TFM is not a barrier on a current SDK. A warning + /// rather than info even so, because it cannot fire spuriously (we know the language version, and only + /// look when [AsciiHash] is actually used) and the alternatives are both worse: errors inside + /// generated code, or an unexplained "partial method has no implementing declaration". + /// + public static readonly DiagnosticDescriptor LanguageVersionTooLow = new( + id: "SER350", + title: "Language version too low for generated code", + messageFormat: "'{0}' requires C# {1} or later, but this project uses C# {2}; no code was generated. Raise to use this feature.", + category: BuildCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: HelpLink("SER350")); + + private static string HelpLink(string id) => string.Format(HelpLinkFormat, id); +} diff --git a/eng/StackExchange.Redis.Build/RoslynShims.cs b/eng/StackExchange.Redis.Build/RoslynShims.cs new file mode 100644 index 000000000..30ad00828 --- /dev/null +++ b/eng/StackExchange.Redis.Build/RoslynShims.cs @@ -0,0 +1,34 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; + +namespace StackExchange.Redis.Build; + +/// +/// Values that exist in newer Roslyn than we compile against. +/// +/// +/// This assembly ships as an analyzer inside the StackExchange.Redis package, so it is deliberately built +/// against an old Roslyn to stay loadable in older hosts (see Directory.Packages.props). That is a +/// *compile-time* floor only: at run-time we are hosted by the consumer's compiler, which may be far newer +/// and can therefore hand us values that did not exist when we were built. Matching on the numeric value +/// keeps us correct in both directions, so prefer a shim here over raising the floor. +/// +internal static class RefKinds +{ + /// ref readonly parameters (C# 12); gained this in Roslyn 4.8. + public const RefKind RefReadOnlyParameter = (RefKind)4; +} + +/// +/// Language versions that post-date the Roslyn we compile against; see for why. +/// +internal static class LanguageVersions +{ + /// C# 11; gained this in Roslyn 4.4. + /// + /// Only ever compared against a version the host reports, so the numeric value is what matters. Note that + /// anything lower than this is a version our Roslyn already knows about, which is what keeps + /// ToDisplayString() on the failure path safe. + /// + public const LanguageVersion CSharp11 = (LanguageVersion)1100; +} diff --git a/eng/StackExchange.Redis.Build/ServerVersion.cs b/eng/StackExchange.Redis.Build/ServerVersion.cs new file mode 100644 index 000000000..1e3d2feab --- /dev/null +++ b/eng/StackExchange.Redis.Build/ServerVersion.cs @@ -0,0 +1,95 @@ +using System.Globalization; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace StackExchange.Redis.Build; + +/// +/// The server version a suggestion needs, and the caller's declared minimum to compare it against. +/// +/// +/// Only major/minor: server features land on minor boundaries, and the extra precision would be false anyway +/// (release candidates report as the *previous* minor with a high patch - 8.4 RC1 is 8.3.224 - so a patch +/// comparison would need the same RC fudging RedisFeatures does, for no benefit to a suggestion). +/// +internal readonly struct ServerVersion +{ + /// The suggestion works on any server this library supports, so there is nothing to say. + public static ServerVersion Any => default; + + public ServerVersion(int major, int minor) + { + Major = major; + Minor = minor; + } + + public int Major { get; } + public int Minor { get; } + + /// Is this an actual requirement, as opposed to ? + public bool IsSpecified => Major != 0; + + /// Would a server at support a feature needing this version? + public bool IsSatisfiedBy(ServerVersion available) + => !IsSpecified + || !available.IsSpecified // nothing declared: assume the newest, which is why the default shows all + || available.Major > Major + || (available.Major == Major && available.Minor >= Minor); + + /// + public override string ToString() => Major + "." + Minor; + + /// + /// The minimum server version the project has declared, if any. + /// + /// + /// Two spellings, matching the two ways a consumer can reasonably configure an analyzer: an + /// .editorconfig/.globalconfig entry, or an MSBuild property surfaced through + /// CompilerVisibleProperty. Unset means show everything - a version-gated suggestion is still useful + /// to someone who has not thought about server versions yet, and silence by default would hide the rule + /// from exactly the people it is for. + /// + public static ServerVersion FromOptions(AnalyzerOptions? options) + { + if (options is not null) + { + var global = options.AnalyzerConfigOptionsProvider.GlobalOptions; + if (global.TryGetValue("redis.min_server_version", out var value) && TryParse(value, out var version)) + { + return version; + } + + // the MSBuild property, surfaced by the CompilerVisibleProperty declared in + // the build/ props we ship; the build_property. prefix is how MSBuild properties arrive here + if (global.TryGetValue("build_property.RedisMinServerVersion", out value) && TryParse(value, out version)) + { + return version; + } + } + + return Any; + } + + /// + /// Parses "8", "8.4", "8.4.1" - anything past the minor is accepted and ignored. + /// + /// + /// Deliberately lenient: a value we cannot read is treated as "unset" and so shows everything, because + /// silently hiding suggestions over a typo in a config value would be very hard to work out. + /// + private static bool TryParse(string? text, out ServerVersion version) + { + version = Any; + if (string.IsNullOrWhiteSpace(text)) return false; + + // invariant throughout: this is a version from a config file, not something a human typed in a locale + var parts = text!.Trim().Split('.'); + if (!int.TryParse(parts[0], NumberStyles.Integer, CultureInfo.InvariantCulture, out var major) || major <= 0) return false; + + var minor = 0; + if (parts.Length > 1 && !int.TryParse(parts[1], NumberStyles.Integer, CultureInfo.InvariantCulture, out minor)) return false; + if (minor < 0) return false; + + version = new ServerVersion(major, minor); + return true; + } +} diff --git a/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj b/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj index 3cde6f5f6..1d4959e24 100644 --- a/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj +++ b/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj @@ -8,7 +8,17 @@ - + + + + + + + + + + diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs new file mode 100644 index 000000000..d26e34ee0 --- /dev/null +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -0,0 +1,927 @@ +using System.Collections.Immutable; +using System.Globalization; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace StackExchange.Redis.Build; + +/// +/// Spots ITransaction/ITransactionAsync usage that a single conditional command does better. +/// +/// +/// +/// Three shapes, all of them unambiguous by construction: one condition guarding one command on a +/// syntactically identical key (SER300-SER302), two commands that one compound command covers (SER303), and the +/// same command queued repeatedly where a variadic overload covers it (SER304). +/// +/// +/// Deliberately conservative, because this ships to every consumer of the package and a false positive on +/// correct code is worse than staying quiet. Anything cleverer - a condition on a different key, a transaction +/// whose result feeds back into control flow, commands queued in a loop, a transaction handed to another method +/// - is left alone on purpose: partial inference that works inconsistently would be more confusing than none. +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class TransactionAnalyzer : DiagnosticAnalyzer +{ + /// + public override ImmutableArray SupportedDiagnostics { get; } + = ImmutableArray.Create( + Diagnostics.PreferConditionalArgument, + Diagnostics.PreferNewerAtomicOperation, + Diagnostics.RedundantCondition, + Diagnostics.PreferCompoundCommand, + Diagnostics.PreferVariadicOverload); + + /// + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + // The cheap short-circuit that matters: this analyzer ships to everyone who references the package, + // but the vast majority of compilations contain no transactions at all. Resolving the types once per + // compilation and bailing means those projects pay a couple of metadata lookups and nothing else. + context.RegisterCompilationStartAction(static ctx => + { + if (KnownSymbols.TryCreate(ctx.Compilation) is not { } known) return; + + // read once per compilation, not per block: it cannot change within one + var declaredMinVersion = ServerVersion.FromOptions(ctx.Options); + ctx.RegisterOperationBlockAction(blockCtx => Analyze(blockCtx, known, declaredMinVersion)); + }); + } + + private sealed class KnownSymbols + { + private KnownSymbols(INamedTypeSymbol condition, INamedTypeSymbol? transaction, INamedTypeSymbol? transactionAsync, INamedTypeSymbol? commandFlags) + { + Condition = condition; + Transaction = transaction; + TransactionAsync = transactionAsync; + CommandFlags = commandFlags; + } + + public INamedTypeSymbol Condition { get; } + public INamedTypeSymbol? Transaction { get; } + public INamedTypeSymbol? TransactionAsync { get; } + + /// + /// CommandFlags, which every command takes and no suggestion mentions. + /// + /// + /// Singled out because the argument audit below treats an argument the suggestion does not carry as a + /// reason to stay quiet, and flags would otherwise silence every rule for anyone who passes them. They + /// are carried over verbatim instead, which is what the help pages say to do. + /// + public INamedTypeSymbol? CommandFlags { get; } + + public static KnownSymbols? TryCreate(Compilation compilation) + { + // no Condition type => not our library, or a version without it; either way there is nothing here + if (compilation.GetTypeByMetadataName("StackExchange.Redis.Condition") is not { } condition) return null; + + var transaction = compilation.GetTypeByMetadataName("StackExchange.Redis.ITransaction"); + var transactionAsync = compilation.GetTypeByMetadataName("StackExchange.Redis.ITransactionAsync"); + if (transaction is null && transactionAsync is null) return null; + + return new KnownSymbols(condition, transaction, transactionAsync, compilation.GetTypeByMetadataName("StackExchange.Redis.CommandFlags")); + } + + public bool IsCommandFlags(ITypeSymbol? type) + => CommandFlags is not null && SymbolEqualityComparer.Default.Equals(type, CommandFlags); + + public bool IsTransaction(ITypeSymbol? type) + => type is not null + && ((Transaction is not null && SymbolEqualityComparer.Default.Equals(type, Transaction)) + || (TransactionAsync is not null && SymbolEqualityComparer.Default.Equals(type, TransactionAsync))); + } + + private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols known, ServerVersion declaredMinVersion) + { + foreach (var block in context.OperationBlocks) + { + // one pass, gathering per-transaction-local usage; most blocks contain nothing and fall straight out + Dictionary? usages = null; + + foreach (var operation in block.Descendants()) + { + // the transaction is identified by the local it was assigned to; anything else (a field, a + // fluent chain) is out of scope by design. Walking local *references* rather than invocations + // is what lets us see the uses that are not calls at all - see the escape check below. + if (operation is not ILocalReferenceOperation { Local: { } local }) continue; + context.CancellationToken.ThrowIfCancellationRequested(); + + if (!known.IsTransaction(local.Type)) continue; + + usages ??= new Dictionary(SymbolEqualityComparer.Default); + if (!usages.TryGetValue(local, out var usage)) usages[local] = usage = new Usage(); + + if (operation.Parent is IInvocationOperation invocation + && invocation.Instance is ILocalReferenceOperation { Local: { } instanceLocal } + && SymbolEqualityComparer.Default.Equals(instanceLocal, local)) + { + // tran.Something(...) - a queued command, a condition, or the terminator + var repeats = !TryGetBranch(invocation, block, out var branch); + usage.Add(invocation, known, branch, repeats); + } + else + { + // The transaction is used as a value: passed to a helper, stored, captured, returned. We + // cannot see what that other code queues, so our counts are no longer the whole story and + // any suggestion would be based on a partial view. Give up on this local entirely. + usage.Disqualify(); + } + } + + if (usages is null) continue; + + // Locals that are written somewhere in this block, which is what makes comparing key expressions by + // text unsound: "key" and "key" are the same text but not the same key if it was reassigned in + // between. Declarations do not count - only later writes - so the common case stays clean. + // + // Deliberately a second walk, and deliberately after the bail-out above rather than before it: this + // analyzer ships to everyone who references the package, where the overwhelming majority of blocks + // hold no transaction at all. Those blocks now pay one walk instead of two, and this one runs only + // for the handful that have something to say. + HashSet? reassignedLocals = null; + foreach (var operation in block.Descendants()) + { + if (LocalWrittenBy(operation) is { } written) + { + reassignedLocals ??= new HashSet(SymbolEqualityComparer.Default); + reassignedLocals.Add(written); + } + } + + foreach (var pair in usages) + { + if (pair.Value.TryGetSuggestion(reassignedLocals) is not { } found) continue; + + // The suggestion is only actionable on a server that has the command, and we cannot see the + // server - so if the project has told us its floor, respect it. Unset shows everything. + if (!found.MinVersion.IsSatisfiedBy(declaredMinVersion)) continue; + + var location = pair.Value.LocationFor(found.Rule); + context.ReportDiagnostic(found.Rule switch + { + Rule.NewerAtomicOperation => Diagnostic.Create( + Diagnostics.PreferNewerAtomicOperation, + location, + found.First, + found.Second, + found.Suggestion, + found.MinVersion.ToString()), + + Rule.RedundantCondition => Diagnostic.Create( + Diagnostics.RedundantCondition, + location, + found.First, + found.Second, + found.Suggestion), + + // family D's versions vary from "any" (SMOVE) to 8.0 (HGETDEL), so the clause is built + // rather than baked into the format - see Diagnostics.PreferCompoundCommand + Rule.VariadicOverload => Diagnostic.Create( + Diagnostics.PreferVariadicOverload, + location, + found.First, + found.Second, + found.Suggestion, + VersionClause(found.MinVersion)), + + Rule.CompoundCommand => Diagnostic.Create( + Diagnostics.PreferCompoundCommand, + location, + found.First, + found.Second, + found.Suggestion, + VersionClause(found.MinVersion)), + + _ => Diagnostic.Create( + Diagnostics.PreferConditionalArgument, + location, + found.First, + found.Second, + found.Suggestion), + }); + } + } + } + + /// + /// The local this operation writes to, if it writes to one. + /// + /// + /// A variable *declaration* is not a write for this purpose - the interesting case is a local that held one + /// key when a command was queued and a different one by the time the next was, which only a later assignment + /// can produce. ref/out arguments count, because the callee may do exactly that. + /// + private static ISymbol? LocalWrittenBy(IOperation operation) => operation switch + { + ISimpleAssignmentOperation { Target: ILocalReferenceOperation { Local: { } local } } => local, + ICompoundAssignmentOperation { Target: ILocalReferenceOperation { Local: { } local } } => local, + IIncrementOrDecrementOperation { Target: ILocalReferenceOperation { Local: { } local } } => local, + IArgumentOperation + { + Parameter.RefKind: RefKind.Ref or RefKind.Out, + Value: ILocalReferenceOperation { Local: { } local }, + } => local, + _ => null, + }; + + /// + /// The trailing " (requires server x.y or later)", or nothing where the suggestion needs no particular one. + /// + private static string VersionClause(ServerVersion version) + => version.IsSpecified ? " (requires server " + version + " or later)" : ""; + + /// + /// Where a call sits within the block: which branch of it, and whether it can run more than once. + /// + /// + /// + /// Counting call sites is a syntactic approximation, and this is where it breaks. Two ways, needing two + /// different answers. A call that can run repeatedly - inside a loop, or inside a lambda or local + /// function whose invocation count we cannot see at all - is one call site and N queued commands, so it is + /// not collapsible into anything: false, and the caller disqualifies the whole transaction. + /// + /// + /// 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 + /// them; the same two in opposite arms of an if/else never queue together at all, and + /// "collapsing" them would queue a command the code deliberately did not. Hence the innermost enclosing + /// branch rather than a plain "is it conditional" flag - and the branch, not the branching + /// operation, or the two arms of one if would compare equal. + /// + /// + private static bool TryGetBranch(IOperation operation, IOperation block, out SyntaxNode? branch) + { + branch = null; + var previous = operation; + for (var node = operation.Parent; node is not null && node != block; node = node.Parent) + { + switch (node) + { + case ILoopOperation: + case IAnonymousFunctionOperation: + case ILocalFunctionOperation: + return false; + + // keep walking after finding one: an enclosing loop still trumps it + case IConditionalOperation: + case ISwitchOperation: + case ISwitchExpressionOperation: + case ITryOperation: + branch ??= previous.Syntax; + break; + } + + previous = node; + } + + return true; + } + + /// + /// Which kind of rewrite this is, and so which diagnostic ID reports it. + /// + /// + /// Kept distinct from on purpose. The ID is about the *kind* of fix, which + /// is what a consumer configures severity on and what they read a doc page about; the version is data about + /// one mapping and moves as servers ship. + /// + private enum Rule + { + /// SER300 - the command already takes this condition as an argument. + ConditionalArgument, + + /// SER301 - a newer single command subsumes the condition and the write. + NewerAtomicOperation, + + /// SER302 - the condition tells the caller nothing the write does not already report. + RedundantCondition, + + /// SER303 - no condition at all; two queued operations that are one command. + CompoundCommand, + + /// SER304 - the same command queued repeatedly, where one variadic call does the lot. + VariadicOverload, + } + + /// + /// A rewrite we are prepared to suggest, and what it needs. + /// + private readonly struct Rewrite + { + public Rewrite(Rule rule, string first, string second, string suggestion, ServerVersion minVersion) + { + Rule = rule; + First = first; + Second = second; + Suggestion = suggestion; + MinVersion = minVersion; + } + + public Rule Rule { get; } + + /// + /// The condition; for the first queued operation, and for + /// the operation that was repeated. + /// + public string First { get; } + + /// + /// The queued operation; for the second one, and for + /// how many times it was queued. + /// + public string Second { get; } + + /// The suggested call, as shown to the user. + public string Suggestion { get; } + + public ServerVersion MinVersion { get; } + } + + /// + /// The method name as the mapping tables spell it: the sync name, since the tables describe commands rather + /// than overloads and both surfaces map to the same suggestion. + /// + private static string Trim(string name) + => name.EndsWith("Async", StringComparison.Ordinal) ? name.Substring(0, name.Length - 5) : name; + + /// + /// One command queued on the transaction, reduced to what the mappings need to match on. + /// + private readonly struct QueuedOperation + { + public QueuedOperation(string name, string? key, string? member, List? reads, List? supplied) + { + DisplayName = name; + Name = Trim(name); + Key = key; + Member = member; + Reads = reads; + Supplied = supplied; + } + + /// The method name with any Async suffix removed, for matching against the tables. + public string Name { get; } + + /// + /// The method name as written, for the message. + /// + /// + /// The suffix matters here even though it does not for matching: the reader is looking for this call in + /// their own code, so naming StringSetAsync when that is what they wrote saves them a beat. + /// + public string DisplayName { get; } + + /// Source text of the first argument - the key, for every command we map. + public string? Key { get; } + + /// Source text of the second argument: a hash field, or a set member, where there is one. + public string? Member { get; } + + /// + /// Locals read by the key/member expressions, so we can tell whether comparing them by text is sound. + /// + public List? Reads { get; } + + /// + /// Parameter names the caller actually wrote an argument for, other than CommandFlags. + /// + /// + /// Every mapping says which of these its suggestion still carries; anything else the caller wrote would + /// be silently dropped by the rewrite, so it declines instead. Omitted optional arguments are not + /// listed - they carry no intent and are what the suggested form would default to anyway. + /// + public List? Supplied { get; } + } + + /// + /// What we saw done with one transaction local. + /// + private sealed class Usage + { + /// + /// Beyond this many queued commands, stop recording and stay quiet. + /// + /// + /// A backstop rather than a meaningful limit: the variadic shape is unbounded in principle, so this + /// exists only to keep one pathological method from holding an arbitrarily long list. Exceeding it costs + /// a missed suggestion, never a wrong one, and 32 queued commands in a single hand-written transaction + /// is already well past what this rule is for. + /// + private const int MaxInterestingOperations = 32; + + private readonly List _operations = new(); + private int _conditionCount; + private string? _conditionFactory, _conditionKey, _conditionMember; + private List? _conditionReads; + private bool _disqualified; + private Location? _condition, _firstOperation; + + /// + /// The branch every call on this transaction so far was in; null means the block itself. + /// + /// + /// Only meaningful once is set, because null is a real value here - + /// "not inside any branch" is the common case and has to compare equal to itself. + /// + private SyntaxNode? _branch; + private bool _branchKnown; + + /// + /// Where to report, which depends on the rule: the condition is the thing to remove for most of them, + /// but family D has no condition at all, so its report goes on the first queued command. + /// + public Location? LocationFor(Rule rule) + => rule is Rule.CompoundCommand or Rule.VariadicOverload ? _firstOperation : _condition; + + /// + /// Something about this usage puts it beyond what we can reason about; stay silent regardless of counts. + /// + public void Disqualify() => _disqualified = true; + + public void Add(IInvocationOperation invocation, KnownSymbols known, SyntaxNode? branch, bool repeats) + { + if (repeats) + { + Disqualify(); + return; + } + + // The terminator is Execute/ExecuteAsync *as declared on the transaction interface*. Matching the + // name alone also swallowed IDatabaseAsync.ExecuteAsync(string command, params object[] args) - + // which is a queued command, and the one people reach for precisely when the library has no + // wrapper for what they want. A queued command we cannot see makes every count below a lie: the + // pair rules would collapse two operations that had a third between them. + if (invocation.TargetMethod.Name is "Execute" or "ExecuteAsync" + && known.IsTransaction(invocation.TargetMethod.ContainingType)) + { + return; + } + + // Deliberately after the terminator check and not before it: the terminator is allowed to sit + // somewhere else entirely - "queue it all, then commit it inside an if" is ordinary code, and says + // nothing about whether the queued commands belong together. + if (_branchKnown && _branch != branch) + { + Disqualify(); + return; + } + + _branch = branch; + _branchKnown = true; + + switch (invocation.TargetMethod.Name) + { + case "AddCondition": + _conditionCount++; + _condition ??= invocation.Syntax.GetLocation(); + + // the argument is expected to be a Condition.Xxx(...) factory call; if it is anything else + // (a variable, a helper method) we cannot know what it tests, so leave the names null and + // the mapping below will decline + if (invocation.Arguments.Length == 1 + && Unwrap(invocation.Arguments[0].Value) is IInvocationOperation factory + && SymbolEqualityComparer.Default.Equals(factory.TargetMethod.ContainingType, known.Condition)) + { + _conditionFactory = factory.TargetMethod.Name; + _conditionKey = ArgumentText(factory, 0); + _conditionMember = ArgumentText(factory, 1); + _conditionReads = LocalsRead(factory); + } + + break; + + default: + // everything else queued on the transaction is a redis operation - including a raw + // ExecuteAsync("SOMECMD", ...), which maps to nothing and so can only ever suppress + _firstOperation ??= invocation.Syntax.GetLocation(); + if (_operations.Count < MaxInterestingOperations) + { + _operations.Add(new QueuedOperation( + invocation.TargetMethod.Name, + ArgumentText(invocation, 0), + ArgumentText(invocation, 1), + LocalsRead(invocation), + SuppliedArguments(invocation, known))); + } + else + { + Disqualify(); + } + + break; + } + } + + public Rewrite? TryGetSuggestion(HashSet? reassignedLocals) + { + if (_disqualified) return null; + + // Every shape below decides by comparing key/member expressions as text. That is only sound while + // the locals involved hold the same value throughout: if one was reassigned between the two calls, + // identical text means two different keys, and the suggestion would silently change behaviour. + if (reassignedLocals is not null && ReadsAny(reassignedLocals)) return null; + + if (_conditionCount == 1 && _operations.Count == 1) + { + // families A, B and C: one guard over one command + return TryGuardedOperation(_operations[0]); + } + + if (_conditionCount != 0 || _operations.Count < 2) return null; + + // family D, two flavours. A pair of *different* commands that one compound command covers, or the + // same command repeated, which the variadic overload covers. They cannot both match, because one + // wants the names to differ and the other wants them identical. + return (_operations.Count == 2 ? TryCommandPair(_operations[0], _operations[1]) : null) + ?? TryVariadic(); + } + + private bool ReadsAny(HashSet reassignedLocals) + { + if (Contains(_conditionReads, reassignedLocals)) return true; + foreach (var operation in _operations) + { + if (Contains(operation.Reads, reassignedLocals)) return true; + } + + return false; + + static bool Contains(List? reads, HashSet reassigned) + { + if (reads is null) return false; + foreach (var read in reads) + { + if (reassigned.Contains(read)) return true; + } + + return false; + } + } + + private Rewrite? TryGuardedOperation(QueuedOperation operation) + { + if (_conditionFactory is null) return null; + + // the same key expression in both; see ArgumentText for why this is syntactic + if (_conditionKey is null || operation.Key is null || _conditionKey != operation.Key) return null; + + if (Map(_conditionFactory, operation.Name) is not { } mapped) return null; + + // Where the condition names a hash field or a set member, it has to be the *same* one the command + // touches: a condition about member "a" says nothing about removing member "b", and collapsing the + // two would silently drop a real guard. Only some mappings have a member at all, hence the flag. + if (mapped.SameMember + && (_conditionMember is null || operation.Member is null || _conditionMember != operation.Member)) + { + return null; + } + + if (!IsCovered(operation, mapped.Covered)) return null; + + return new Rewrite( + mapped.Rule, + "Condition." + _conditionFactory, + operation.DisplayName, + mapped.Suggestion, + mapped.MinVersion); + } + + private static Rewrite? TryCommandPair(QueuedOperation first, QueuedOperation second) + { + if (MapPair(first, second) is not { } mapped) return null; + if (!IsCovered(first, mapped.CoveredFirst) || !IsCovered(second, mapped.CoveredSecond)) return null; + return new Rewrite(Rule.CompoundCommand, first.DisplayName, second.DisplayName, mapped.Suggestion, mapped.MinVersion); + } + + /// + /// The same command queued several times over, where one variadic call does the lot. + /// + private Rewrite? TryVariadic() + { + var first = _operations[0]; + for (var i = 1; i < _operations.Count; i++) + { + if (_operations[i].Name != first.Name) return null; + } + + if (MapVariadic(first.Name) is not { } mapped) return null; + + // Which keys the variadic form takes is the whole distinction here. SADD and friends take one key + // and many values, so every call has to be on the *same* key - N calls across different keys have no + // single-command form. MSET/MGET/DEL take many keys, so those must be different keys, which also + // avoids arguing about what a repeated key would mean. + for (var i = 0; i < _operations.Count; i++) + { + if (_operations[i].Key is null) return null; + if (!IsCovered(_operations[i], mapped.Covered)) return null; + if (mapped.ManyKeys) + { + if (mapped.RequiresMember && _operations[i].Member is null) return null; + for (var j = i + 1; j < _operations.Count; j++) + { + if (_operations[i].Key == _operations[j].Key) return null; + } + } + else + { + if (_operations[i].Key != first.Key) return null; + if (mapped.RequiresMember && _operations[i].Member is null) return null; + } + } + + return new Rewrite( + Rule.VariadicOverload, + first.DisplayName, + _operations.Count.ToString(CultureInfo.InvariantCulture), + mapped.Suggestion, + mapped.MinVersion); + } + + /// + /// Commands with a variadic overload that subsumes N separate calls. + /// + /// + /// + /// Versions are for all but one. The variadic forms are old - 2.4 for the + /// one-key-many-values group, 3.0.3 for multi-key EXISTS, 1.0 for MSET/MGET/DEL - and all of it predates + /// anything realistically in service, so naming a version would be noise rather than information. + /// SMISMEMBER is the exception at 6.2, recent enough that somebody might actually be below it. + /// + /// + /// Deliberately absent: N x ListLeftPop across keys is *not* LMPOP. LMPOP pops from the first + /// non-empty key of those given, not from each of them, so it is a different operation however similar + /// the argument lists look. Same for ZMPOP. + /// + /// + private static (string Suggestion, bool ManyKeys, bool RequiresMember, ServerVersion MinVersion, string Covered)? MapVariadic(string operation) + => operation switch + { + // one key, many values + "SetAdd" => ("SetAdd[Async](key, values)", false, true, ServerVersion.Any, "key,value"), + "SetRemove" => ("SetRemove[Async](key, values)", false, true, ServerVersion.Any, "key,value"), + "SortedSetAdd" => ("SortedSetAdd[Async](key, entries)", false, true, ServerVersion.Any, "key,member,score"), + "SortedSetRemove" => ("SortedSetRemove[Async](key, members)", false, true, ServerVersion.Any, "key,member"), + "HashSet" => ("HashSet[Async](key, entries)", false, true, ServerVersion.Any, "key,hashField,value"), + "HashDelete" => ("HashDelete[Async](key, fields)", false, true, ServerVersion.Any, "key,hashField"), + "ListLeftPush" => ("ListLeftPush[Async](key, values)", false, true, ServerVersion.Any, "key,value"), + "ListRightPush" => ("ListRightPush[Async](key, values)", false, true, ServerVersion.Any, "key,value"), + + // SMISMEMBER, which unlike the rest of these is recent; it has no RedisFeatures gate to cite + "SetContains" => ("SetContains[Async](key, values), which returns a bool per value", false, true, new ServerVersion(6, 2), "key,value"), + + // many keys + "KeyDelete" => ("KeyDelete[Async](keys)", true, false, ServerVersion.Any, "key"), + "KeyExists" => ("KeyExists[Async](keys), which returns how many exist", true, false, ServerVersion.Any, "key"), + "StringGet" => ("StringGet[Async](keys)", true, false, ServerVersion.Any, "key"), + // MSET takes one expiry and one when for the whole batch, not one per key; the variadic + // overload's own expiry:/when: cannot express what N separate calls each said, so Covered + // stops at the pair that MSET does carry + "StringSet" => ("StringSet[Async](KeyValuePair[])", true, false, ServerVersion.Any, "key,value"), + + _ => null, + }; + + /// + /// The condition/operation pairs that have an exact single-command equivalent. + /// + /// + /// The version is the server the *suggestion* needs, not the one the flagged code needs. Family A is + /// because the conditional argument has existed as long as the command + /// (and where it has not quite - ZADD NX arrived in 3.0.2 - it predates the oldest server this library + /// supports, so saying so would be noise). + /// + private static (Rule Rule, string Suggestion, ServerVersion MinVersion, bool SameMember, string? Covered)? Map(string condition, string operation) + => (condition, operation) switch + { + // -- family A: the command already takes this condition as an argument; any server version -- + // Covered omits "when": these suggestions *are* a when: argument, so a caller who wrote their + // own has said something we would be overwriting rather than moving. + ("KeyNotExists", "StringSet") => (Rule.ConditionalArgument, "StringSet[Async](key, value, When.NotExists)", ServerVersion.Any, false, "key,value,expiry,keepTtl"), + ("KeyExists", "StringSet") => (Rule.ConditionalArgument, "StringSet[Async](key, value, When.Exists)", ServerVersion.Any, false, "key,value,expiry,keepTtl"), + ("HashNotExists", "HashSet") => (Rule.ConditionalArgument, "HashSet[Async](key, field, value, When.NotExists)", ServerVersion.Any, true, "key,hashField,value"), + // SortedSetWhen, not When: the When overload is [EditorBrowsable(Never)] and the SortedSetWhen + // one is the canonical spelling, so suggesting When would push callers at a hidden overload + ("SortedSetNotContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd[Async](key, member, score, SortedSetWhen.NotExists)", ServerVersion.Any, true, "key,member,score"), + ("SortedSetContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd[Async](key, member, score, SortedSetWhen.Exists)", ServerVersion.Any, true, "key,member,score"), + ("KeyNotExists", "KeyRename") => (Rule.ConditionalArgument, "KeyRename[Async](key, newKey, When.NotExists)", ServerVersion.Any, false, "key,newKey"), + + // -- family B: a newer single command subsumes condition and write -- + // 8.4: SET IFEQ/IFNE and DELIFEQ; see RedisFeatures.SetWithValueCheck / DeleteWithValueCheck + ("StringEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet[Async](key, value, ValueCondition.Equal(expected))", new ServerVersion(8, 4), false, "key,value,expiry"), + ("StringNotEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet[Async](key, value, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false, "key,value,expiry"), + ("StringEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete[Async](key, ValueCondition.Equal(expected)), or LockRelease[Async]", new ServerVersion(8, 4), false, "key"), + ("StringNotEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete[Async](key, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false, "key"), + + // -- family C: the write already reports what the condition was checking -- + // These have always worked this way, so no version applies. The fix deletes the transaction + // rather than moving an argument, and what the caller observes changes: Execute() returning + // false ("the guard failed") becomes the command itself returning false ("I did nothing"). + // Covered is null throughout: the command is kept exactly as written, so there is no argument + // the rewrite could drop, however exotic. + ("SetNotContains", "SetAdd") => (Rule.RedundantCondition, "SetAdd[Async](key, value), which returns false if the member was already there", ServerVersion.Any, true, null), + ("SetContains", "SetRemove") => (Rule.RedundantCondition, "SetRemove[Async](key, value), which returns false if the member was not there", ServerVersion.Any, true, null), + ("SortedSetContains", "SortedSetRemove") => (Rule.RedundantCondition, "SortedSetRemove[Async](key, member), which returns false if the member was not there", ServerVersion.Any, true, null), + ("HashExists", "HashDelete") => (Rule.RedundantCondition, "HashDelete[Async](key, field), which returns false if the field was not there", ServerVersion.Any, true, null), + ("KeyExists", "KeyDelete") => (Rule.RedundantCondition, "KeyDelete[Async](key), which returns false if the key did not exist", ServerVersion.Any, false, null), + ("KeyExists", "KeyExpire") => (Rule.RedundantCondition, "KeyExpire[Async](key, expiry), which returns false if the key did not exist", ServerVersion.Any, false, null), + + // Deliberately absent from family C: ListIndexExists + ListSetByIndex. LSET reports an + // out-of-range index by failing, not by returning false (ListSetByIndex returns Task, not + // Task), so dropping the condition turns an aborted transaction into an exception - + // a change of behaviour, not a simplification. + + // Deliberately absent, because no atomic equivalent exists and suggesting one would be wrong: + // HashExists + HashSet - there is no HSETXX; the nearest thing is a different method + // (HashFieldSet with ValueCondition.Exists, HSETEX FXX, 8.0+) + // HashEqual/HashNotEqual - no server-side hash compare-and-set at all + // ListIndexEqual + ListSet - likewise + // *Length* conditions - likewise + _ => null, + }; + + /// + /// Family D: two queued commands, no condition, that are one compound command between them. + /// + /// + /// + /// Order matters here in a way it did not for the guarded families, because these commands return a + /// value: SET ... GET hands back the value from *before* the write, so it matches a queued get + /// followed by a set, and not the other way round. + /// + /// + /// A read whose result feeds the write is impossible to express here at all - inside a transaction the + /// read's result is an unresolved Task, so the caller cannot use it. That rules out the pairing + /// that looks most tempting, ListRightPop + ListLeftPush = LMOVE: whatever value is + /// being pushed, it is not the one that was popped, so LMOVE would not do the same thing. SMOVE below is + /// fine by contrast, because the member is a value the caller already has and passes to both calls. + /// + /// + private static (string Suggestion, ServerVersion MinVersion, string CoveredFirst, string CoveredSecond)? MapPair(QueuedOperation first, QueuedOperation second) + { + // 6.2: GETDEL / GETEX / SET ... GET; see RedisFeatures.GetDelete and SetAndGet + var v6_2 = new ServerVersion(6, 2); + + if (SameKey(first, second)) + { + switch (first.Name, second.Name) + { + case ("StringGet", "KeyDelete"): + return ("StringGetDelete[Async](key)", v6_2, "key", "key"); + + // GETEX has no NX/XX, so KeyExpire's ExpireWhen is not covered and a caller who wrote + // one keeps their transaction + case ("StringGet", "KeyExpire"): + return ("StringGetSetExpiry[Async](key, expiry)", v6_2, "key", "key,expiry"); + case ("StringGet", "KeyPersist"): + return ("StringGetSetExpiry[Async](key, null)", v6_2, "key", "key"); + case ("StringGet", "StringSet"): + return ("StringSetAndGet[Async](key, value)", v6_2, "key", "key,value,expiry,keepTtl,when"); + + // SET ... EX, which is why this one needs no particular server: setting a value and its + // lifetime in one command is as old as SET's options (2.6.12). The order is load-bearing + // in the other direction to the reads above - SET *clears* any TTL, so an EXPIRE followed + // by a SET leaves no expiry at all and is emphatically not this. + // + // "expiry" is absent from the first coverage set on purpose: a StringSet that already + // carries one, followed by an EXPIRE that overrides it, is not one command with one + // lifetime and we should not be guessing which of the two the caller meant. + case ("StringSet", "KeyExpire"): + return ("StringSet[Async](key, value, expiry)", ServerVersion.Any, "key,value,when", "key,expiry"); + + // HGETDEL is 8.0; it has no RedisFeatures gate to point at + case ("HashGet", "HashDelete") when SameMember(first, second): + return ("HashFieldGetAndDelete[Async](key, field)", new ServerVersion(8, 0), "key,hashField", "key,hashField"); + } + + return null; + } + + // SMOVE, which is as old as sets themselves. Two different keys by definition - and the same member + // in both calls, or it is not one move. Either order queues the same pair of effects. + if (SameMember(first, second) + && ((first.Name == "SetRemove" && second.Name == "SetAdd") + || (first.Name == "SetAdd" && second.Name == "SetRemove"))) + { + return ("SetMove[Async](source, destination, value)", ServerVersion.Any, "key,value", "key,value"); + } + + return null; + + static bool SameKey(QueuedOperation a, QueuedOperation b) + => a.Key is not null && b.Key is not null && a.Key == b.Key; + + static bool SameMember(QueuedOperation a, QueuedOperation b) + => a.Member is not null && b.Member is not null && a.Member == b.Member; + } + + /// + /// The source text of an argument, used as a cheap "same key?" / "same member?" test. + /// + /// + /// Deliberately syntactic. Comparing keys semantically is not possible in general (they are values, + /// not symbols), so requiring the *same expression text* keeps false positives near zero at the cost + /// of missing cases where the same key is spelled two different ways. That trade is the right way + /// round for a shipped analyzer. + /// + private static List? LocalsRead(IInvocationOperation invocation) + { + List? locals = null; + for (var i = 0; i < 2 && i < invocation.Arguments.Length; i++) + { + foreach (var node in invocation.Arguments[i].Value.DescendantsAndSelf()) + { + if (node is ILocalReferenceOperation { Local: { } local }) + { + (locals ??= new List()).Add(local); + } + } + } + + return locals; + } + + /// + /// Does the suggestion still carry everything the caller wrote? + /// + /// + /// The suggestions are sketches, so this is not about spelling every argument back out - it is about + /// arguments the suggested command cannot express at all, which the rewrite would therefore drop in + /// silence. N x StringSet(key, value, expiry) is not MSET: taking that advice makes the keys + /// permanent. Declining costs a suggestion, which is the cheap direction, and the help pages list the + /// shapes it gives up on. + /// + private static bool IsCovered(QueuedOperation operation, string? covered) + { + // null covers everything: family C keeps the command exactly as written and only drops the + // condition, so no argument of it can go missing + if (covered is null || operation.Supplied is not { } supplied) return true; + + foreach (var name in supplied) + { + if (!Covers(covered, name)) return false; + } + + return true; + + static bool Covers(string covered, string name) + { + foreach (var candidate in covered.Split(',')) + { + if (candidate == name) return true; + } + + return false; + } + } + + /// + /// The parameter names the caller actually wrote an argument for, other than CommandFlags. + /// + private static List? SuppliedArguments(IInvocationOperation invocation, KnownSymbols known) + { + List? names = null; + foreach (var argument in invocation.Arguments) + { + if (argument.ArgumentKind != ArgumentKind.Explicit) continue; + if (argument.Parameter is not { } parameter) continue; + + // Flags never bear on any of this: they are on every command, no suggestion mentions them, and + // the rewrite carries them over verbatim. Recognised by name as well as by type, because the + // consequence of failing to recognise them is not a missed exclusion but silence everywhere - + // every command takes flags, so one unrecognised spelling would suppress every rule for anyone + // who passes them. Belt and braces is cheap here; the type lookup is the one that can be null. + if (parameter.Name == "flags" || known.IsCommandFlags(parameter.Type)) continue; + + (names ??= new List()).Add(parameter.Name); + } + + return names; + } + + private static string? ArgumentText(IInvocationOperation invocation, int index) + { + if (invocation.Arguments.Length <= index) return null; + + // An omitted optional argument reports the *invocation* as its syntax, so two of them from one + // call site compare equal to each other and to nothing the caller wrote. Only text somebody + // actually typed is a key or a member. + var argument = invocation.Arguments[index]; + return argument.ArgumentKind == ArgumentKind.Explicit ? argument.Value.Syntax.ToString() : null; + } + + private static IOperation Unwrap(IOperation operation) + { + // implicit RedisKey/RedisValue conversions wrap almost every argument in this API + while (operation is IConversionOperation { Operand: { } inner }) operation = inner; + return operation; + } + } +} diff --git a/src/StackExchange.Redis/RedisDatabase.cs b/src/StackExchange.Redis/RedisDatabase.cs index 4e2dd61be..4abe62cd5 100644 --- a/src/StackExchange.Redis/RedisDatabase.cs +++ b/src/StackExchange.Redis/RedisDatabase.cs @@ -2,6 +2,7 @@ using System.Buffers; using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Net; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -4311,6 +4312,9 @@ private Message GetSortedSetMultiPopMessage(RedisKey[] keys, Order order, long c return tran; } + // The analyzer is right that this is DELEX, but this *is* the fallback: LockRelease prefers the atomic + // form (see GetStringDeleteMessage) and only lands here when the server does not support it. + [SuppressMessage("Usage", "SER301:Transaction can be replaced by a single atomic operation", Justification = "Deliberate fallback for servers without DELEX.")] private ITransaction? GetLockReleaseTransaction(RedisKey key, RedisValue value) { var tran = CreateTransactionIfAvailable(asyncState); diff --git a/src/StackExchange.Redis/StackExchange.Redis.csproj b/src/StackExchange.Redis/StackExchange.Redis.csproj index 51d4ca902..c7acece30 100644 --- a/src/StackExchange.Redis/StackExchange.Redis.csproj +++ b/src/StackExchange.Redis/StackExchange.Redis.csproj @@ -37,6 +37,9 @@ + + + @@ -61,6 +64,38 @@ + + + + + + + + + + + + + + + MultiGroupDatabase.cs diff --git a/src/StackExchange.Redis/build/StackExchange.Redis.props b/src/StackExchange.Redis/build/StackExchange.Redis.props new file mode 100644 index 000000000..48ecadc11 --- /dev/null +++ b/src/StackExchange.Redis/build/StackExchange.Redis.props @@ -0,0 +1,29 @@ + + + + + + + + diff --git a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs new file mode 100644 index 000000000..83d302268 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs @@ -0,0 +1,655 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Negatives that are not about one rule but about the shape the analyzer is willing to reason about at all; +/// they suppress SER300 and SER301 alike, so they do not belong in either ID's file. +/// +/// +/// These matter more than the positive cases. Every one of them is correct code that a keener analyzer would +/// "helpfully" suggest breaking, in a diagnostic shipped to every consumer of the package. +/// +public class DetectionShape : Verifier +{ + [Fact] + // two conditions is a genuine multi-guard transaction; no single command takes both + public Task TwoConditions_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + tran.AddCondition(Condition.HashNotExists(key, "field")); + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // two queued writes need the transaction for atomicity even though the condition maps cleanly + public Task TwoOperations_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + _ = tran.StringSetAsync(key, "value"); + _ = tran.KeyExpireAsync(key, System.TimeSpan.FromMinutes(1)); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // no condition at all: this is family D territory (a compound command), not a conditional rewrite + public Task NoCondition_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // one call site, N queued commands. Counting syntax says "one operation"; the runtime says otherwise, and + // a suggestion to collapse would be flatly wrong + public Task OperationInLoop_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, RedisValue[] values) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + foreach (var value in values) + { + _ = tran.StringSetAsync(key, value); + } + + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // Opposite arms of one if/else: exactly one of these is ever queued, so there is no pair to collapse. + // SetMove here would queue a removal the code deliberately did not. + public Task OperationsInOppositeBranches_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b, RedisValue member, bool flag) + { + var tran = db.CreateTransaction(); + if (flag) { _ = tran.SetAddAsync(a, member); } + else { _ = tran.SetRemoveAsync(b, member); } + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // the asymmetric version: the second command is queued only sometimes, so the "pair" is not always a pair + public Task ConditionallyQueuedOperation_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, bool flag) + { + var tran = db.CreateTransaction(); + _ = tran.StringGetAsync(key); + if (flag) { _ = tran.KeyDeleteAsync(key); } + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // and a condition that guards from outside the branch its command is in + public Task ConditionOutsideOperationBranch_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, bool flag) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + if (flag) { _ = tran.StringSetAsync(key, "value"); } + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // The control for the three above, and the reason this is branch-matching rather than a blanket "anything + // conditional is out": two commands in the *same* branch always queue together, so the pair is real. A + // whole transaction inside an if or a try is ordinary code and must not go silent. + public Task OperationsInTheSameBranch_AreStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, bool flag) + { + var tran = db.CreateTransaction(); + if (flag) + { + _ = {|#0:tran.StringGetAsync(key)|}; + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "StringGetAsync", + "KeyDeleteAsync", + "StringGetDelete[Async](key)", + " (requires server 6.2 or later)")); + + [Fact] + // A lambda is the loop case wearing a hat: one call site, and no way to see how many times it runs - or + // whether it runs at all. Three SetAdds are queued here, not the two the syntax shows. + public Task OperationInLambda_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + System.Action add = () => { _ = tran.SetAddAsync(key, "a"); }; + add(); + add(); + _ = tran.SetAddAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // the same for a local function, which is the shape somebody actually writes + public Task OperationInLocalFunction_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, RedisKey other) + { + var tran = db.CreateTransaction(); + _ = tran.KeyDeleteAsync(key); + Queue(); + Queue(); + await tran.ExecuteAsync(); + + void Queue() => _ = tran.KeyDeleteAsync(other); + } + } + """); + + [Fact] + // the helper may queue anything at all; our counts describe only the part we can see + public Task TransactionPassedToAnotherMethod_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + _ = tran.StringSetAsync(key, "value"); + QueueMore(tran, key); + await tran.ExecuteAsync(); + } + + private static void QueueMore(ITransaction tran, RedisKey key) + => _ = tran.KeyExpireAsync(key, System.TimeSpan.FromMinutes(1)); + } + """); + + [Fact] + // stored away, so the queueing is unbounded in both time and place + public Task TransactionStoredInField_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + private ITransaction? _pending; + + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + _ = tran.StringSetAsync(key, "value"); + _pending = tran; + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // the condition comes from somewhere we cannot inspect, so we do not know what it tests + public Task ConditionFromVariable_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, Condition condition) + { + var tran = db.CreateTransaction(); + tran.AddCondition(condition); + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // same key, spelled differently. A miss, not a false positive - deliberately the safe direction, and + // pinned here so that "improving" the key comparison is a conscious decision + public Task SameKeyDifferentSpelling_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db) + { + RedisKey key = "k"; + var alias = key; + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + _ = tran.StringSetAsync(alias, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // The same unsoundness as SER304's reassignment case, on the guarded shape: the condition names key "a" and + // the write lands on "b", so the transaction is a real guard and collapsing it would change behaviour. + public Task ConditionKeyReassignedBeforeOperation_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db) + { + RedisKey key = "a"; + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + key = "b"; + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // and on the compound-pair shape + public Task PairKeyReassignedBetweenOperations_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db) + { + RedisKey key = "a"; + var tran = db.CreateTransaction(); + _ = tran.StringGetAsync(key); + key = "b"; + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // A local that is reassigned but plays no part in any key or member expression must not suppress anything - + // ordinary methods are full of counters and accumulators. + public Task UnrelatedLocalReassigned_IsStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var count = 0; + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyNotExists(key))|}; + _ = tran.StringSetAsync(key, "value"); + count = 1; + await tran.ExecuteAsync(); + return count; + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet[Async](key, value, When.NotExists)")); + + [Fact] + // A raw command queued through IDatabaseAsync.ExecuteAsync(string, ...) is still a queued command. It was + // once invisible - skipped by name alongside the transaction's own ExecuteAsync() terminator - and these + // two queued operations were "collapsed" into GETDEL with a PERSIST silently dropped in between. + public Task RawExecuteAsyncBetweenOperations_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringGetAsync(key); + _ = tran.ExecuteAsync("PERSIST", key); + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // the same, on the guarded shape: a second queued command means the transaction is doing more than the + // condition, whether or not we have a name for what it does + public Task RawExecuteAsyncBesideGuardedOperation_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + _ = tran.StringSetAsync(key, "value"); + _ = tran.ExecuteAsync("PFADD", key, "x"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // the control for the two above: the terminator itself must still be recognised, or nothing is ever + // flagged. Sync Execute() as well as ExecuteAsync(), since both spellings reach here. + public Task SyncExecuteTerminator_IsStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyNotExists(key))|}; + _ = tran.StringSetAsync(key, "value"); + tran.Execute(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet[Async](key, value, When.NotExists)")); + + [Fact] + // The worst of the dropped-argument cases, because the damage outlives the build: MSET takes one expiry + // for the whole batch, not one per key, so collapsing these would make both keys permanent. + public Task VariadicWouldDropExpiry_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(a, "1", TimeSpan.FromMinutes(1)); + _ = tran.StringSetAsync(b, "2", TimeSpan.FromMinutes(5)); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // HSET's variadic form has no NX + public Task VariadicWouldDropWhen_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.HashSetAsync(key, "f1", "v1", When.NotExists); + _ = tran.HashSetAsync(key, "f2", "v2", When.NotExists); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // GETEX has no NX/XX, so the ExpireWhen has nowhere to go + public Task PairWouldDropExpireWhen_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringGetAsync(key); + _ = tran.KeyExpireAsync(key, TimeSpan.FromMinutes(1), ExpireWhen.HasNoExpiry); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // The caller's own when: is not an argument to move but a statement to overwrite - and this pairing says + // "only if absent, and only if present", which is code we should not be rewriting on a guess. + public Task GuardedOperationWithItsOwnWhen_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyNotExists(key)); + _ = tran.StringSetAsync(key, "value", when: When.Exists); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // The control, and the reason this is per-mapping coverage rather than "any extra argument is out": + // SET does take an expiry alongside NX, so the commonest lock-acquire shape there is must still be + // flagged. CommandFlags likewise - it appears on every command, and is carried over rather than dropped. + public Task GuardedOperationWithExpiryAndFlags_IsStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyNotExists(key))|}; + _ = tran.StringSetAsync(key, "value", TimeSpan.FromMinutes(1), flags: CommandFlags.DemandMaster); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet[Async](key, value, When.NotExists)")); + + [Fact] + // CommandFlags is on every single command, no suggestion mentions it, and the rewrite carries it over + // verbatim - so it is never a reason to go quiet. Deliberately the *only* extra argument in these three, + // where GuardedOperationWithExpiryAndFlags_IsStillFlagged has an expiry beside it and so would still pass + // if flags alone suppressed everything. One per family, because the audit runs in three separate places. + public Task FlagsAloneOnGuardedOperation_IsStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyNotExists(key))|}; + _ = tran.StringSetAsync(key, "value", flags: CommandFlags.DemandMaster); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet[Async](key, value, When.NotExists)")); + + [Fact] + public Task FlagsAloneOnCommandPair_IsStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringGetAsync(key, CommandFlags.DemandMaster)|}; + _ = tran.KeyDeleteAsync(key, CommandFlags.DemandMaster); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "StringGetAsync", + "KeyDeleteAsync", + "StringGetDelete[Async](key)", + " (requires server 6.2 or later)")); + + [Fact] + public Task FlagsAloneOnRepeatedCommand_IsStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.SetAddAsync(key, "a", CommandFlags.DemandMaster)|}; + _ = tran.SetAddAsync(key, "b", CommandFlags.FireAndForget); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "SetAddAsync", + "2", + "SetAdd[Async](key, values)", + "")); + + [Fact] + // family C keeps the command exactly as written, so no argument of it can be dropped and none suppresses + public Task RedundantConditionWithExtraArguments_IsStillFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyExists(key))|}; + _ = tran.KeyExpireAsync(key, TimeSpan.FromMinutes(1), ExpireWhen.HasNoExpiry); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER302").WithLocation(0).WithArguments( + "Condition.KeyExists", + "KeyExpireAsync", + "KeyExpire[Async](key, expiry), which returns false if the key did not exist")); + + [Fact] + // two independent transactions in one method must be tracked separately, not pooled into one set of counts + public Task TwoIndependentTransactions_AreFlaggedIndependently() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var first = db.CreateTransaction(); + {|#0:first.AddCondition(Condition.KeyNotExists(a))|}; + _ = first.StringSetAsync(a, "value"); + await first.ExecuteAsync(); + + var second = db.CreateTransaction(); + {|#1:second.AddCondition(Condition.StringEqual(b, "old"))|}; + _ = second.StringSetAsync(b, "new"); + await second.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet[Async](key, value, When.NotExists)"), + Diagnostic("SER301").WithLocation(1).WithArguments( + "Condition.StringEqual", + "StringSetAsync", + "StringSet[Async](key, value, ValueCondition.Equal(expected))", + "8.4")); +} diff --git a/tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs b/tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs new file mode 100644 index 000000000..98d1246ee --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs @@ -0,0 +1,107 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Version gating: an analyzer cannot see the server, so a project can declare its floor and get only the +/// suggestions it can act on. +/// +public class MinServerVersion : Verifier +{ + private const string CompareAndSet = + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.StringEqual(key, "old"))|}; + _ = tran.StringSetAsync(key, "new"); + await tran.ExecuteAsync(); + } + } + """; + + private const string ConditionalArgument = + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyNotExists(key))|}; + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """; + + [Fact] + // the default: nobody has said anything about servers, so show the suggestion. Silence by default would + // hide the rule from exactly the people who have not thought about this yet + public Task Unset_ShowsVersionGatedSuggestion() => VerifyAsync( + CompareAndSet, + Diagnostic("SER301").WithLocation(0)); + + [Fact] + public Task NewerThanRequired_ShowsSuggestion() => VerifyWithMinServerVersionAsync( + CompareAndSet, + "8.6", + Diagnostic("SER301").WithLocation(0)); + + [Fact] + // exactly the required version counts as supported + public Task ExactlyRequired_ShowsSuggestion() => VerifyWithMinServerVersionAsync( + CompareAndSet, + "8.4", + Diagnostic("SER301").WithLocation(0)); + + [Fact] + // the point of the whole exercise: compare-and-set needs 8.4, so do not suggest it to someone on 7.4 + public Task OlderThanRequired_HidesSuggestion() => VerifyWithMinServerVersionAsync( + CompareAndSet, + "7.4"); + + [Fact] + // ... but the version-free family must survive the same setting, which is why they have separate IDs + public Task OlderThanRequired_StillShowsVersionFreeSuggestion() => VerifyWithMinServerVersionAsync( + ConditionalArgument, + "2.8", + Diagnostic("SER300").WithLocation(0)); + + [Fact] + // a major-only value is a reasonable thing to write + public Task MajorOnly_IsUnderstood() => VerifyWithMinServerVersionAsync( + CompareAndSet, + "7"); + + [Fact] + // a patch component is accepted and ignored rather than rejected + public Task PatchComponent_IsIgnored() => VerifyWithMinServerVersionAsync( + CompareAndSet, + "8.4.1", + Diagnostic("SER301").WithLocation(0)); + + [Fact] + // an unreadable value falls back to showing everything: silently hiding suggestions over a typo would be + // near-impossible to diagnose from the outside + public Task Unparseable_ShowsEverything() => VerifyWithMinServerVersionAsync( + CompareAndSet, + "not-a-version", + Diagnostic("SER301").WithLocation(0)); + + [Fact] + // the version reaches the message, so the reader knows what "newer" means without following the link + public Task Message_NamesTheRequiredVersion() => VerifyAsync( + CompareAndSet, + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringEqual", + "StringSetAsync", + "StringSet[Async](key, value, ValueCondition.Equal(expected))", + "8.4")); +} diff --git a/tests/StackExchange.Redis.Build.Tests/NoLibrary.cs b/tests/StackExchange.Redis.Build.Tests/NoLibrary.cs new file mode 100644 index 000000000..8e76d2d51 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/NoLibrary.cs @@ -0,0 +1,54 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// The analyzer ships to every consumer of the package, including projects that reference it only +/// transitively and never touch a transaction. Those compilations must get nothing at all. +/// +public class NoLibrary : Verifier +{ + [Fact] + // The decoy is the point: identical member names, identical shape, different symbols. If the analyzer ever + // matched on names instead of resolved types, this would fire - and would fire on unrelated user code. + public Task LookalikeApiInAnotherNamespace_IsNotFlagged() => VerifyWithoutLibraryAsync( + """ + using System.Threading.Tasks; + namespace NotRedis + { + public static class Condition + { + public static object StringEqual(string key, string value) => new object(); + public static object KeyNotExists(string key) => new object(); + } + + public interface ITransaction + { + void AddCondition(object condition); + Task StringSetAsync(string key, string value); + Task ExecuteAsync(); + } + + class C + { + public async Task M(ITransaction tran, string key) + { + tran.AddCondition(Condition.StringEqual(key, "old")); + _ = tran.StringSetAsync(key, "new"); + await tran.ExecuteAsync(); + } + } + } + """); + + [Fact] + // the ordinary case for nearly every compilation on earth: no such library, nothing to say + public Task UnrelatedCode_IsNotFlagged() => VerifyWithoutLibraryAsync( + """ + class C + { + public int M(int x) => x + 1; + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER300.cs b/tests/StackExchange.Redis.Build.Tests/SER300.cs new file mode 100644 index 000000000..3819cdb5f --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER300.cs @@ -0,0 +1,280 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Family A: the condition duplicates a when: argument the queued command already has. Version-free, +/// so every one of these is a pure mechanical rewrite. +/// +public class SER300 : Verifier +{ + [Fact] + public Task KeyNotExistsGuardingStringSet_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, 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] + public Task KeyExistsGuardingStringSet_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyExists(key))|}; + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyExists", + "StringSetAsync", + "StringSet[Async](key, value, When.Exists)")); + + [Fact] + public Task HashNotExistsGuardingHashSet_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.HashNotExists(key, "field"))|}; + _ = tran.HashSetAsync(key, "field", "value"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.HashNotExists", + "HashSetAsync", + "HashSet[Async](key, field, value, When.NotExists)")); + + [Fact] + public Task SortedSetNotContainsGuardingSortedSetAdd_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.SortedSetNotContains(key, "member"))|}; + _ = tran.SortedSetAddAsync(key, "member", 1.0); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.SortedSetNotContains", + "SortedSetAddAsync", + "SortedSetAdd[Async](key, member, score, SortedSetWhen.NotExists)")); + + [Fact] + public Task SortedSetContainsGuardingSortedSetAdd_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.SortedSetContains(key, "member"))|}; + _ = tran.SortedSetAddAsync(key, "member", 1.0); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.SortedSetContains", + "SortedSetAddAsync", + "SortedSetAdd[Async](key, member, score, SortedSetWhen.Exists)")); + + [Fact] + // the condition is on the *destination*, which is KeyRename's first argument's counterpart - so this is + // also the case that proves the key comparison uses the renamed-to key, not just "some key matched" + public Task KeyNotExistsGuardingKeyRename_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, RedisKey other) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyNotExists(key))|}; + _ = tran.KeyRenameAsync(key, other); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "KeyRenameAsync", + "KeyRename[Async](key, newKey, When.NotExists)")); + + [Fact] + // synchronous surface: ITransaction is both IDatabaseAsync and the sync-shaped queueing API, and the + // mapping trims the Async suffix - so the non-suffixed spelling has to land on the same rule + public Task SyncOverload_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyNotExists(key))|}; + _ = tran.StringSetAsync(key, "value"); + tran.Execute(); + } + } + """, + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet[Async](key, value, When.NotExists)")); + + [Fact] + // ITransactionAsync, not ITransaction: IDatabase hides IDatabaseAsync.CreateTransaction to refine the + // return type, so code written against IDatabaseAsync gets the async-only interface. Both are resolved by + // the analyzer, and this is what proves the second one is actually wired rather than just mentioned. + public Task AsyncOnlyTransactionInterface_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + // IDatabaseAsync.CreateTransaction is itself [Experimental] (SER007); opted in here rather than in the + // shared harness, so the gate keeps working for every other case + #pragma warning disable SER007 + class C + { + public async Task M(IDatabaseAsync db, 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] + // family A needs the same field too, not just the same key: a condition about field "a" does not guard a + // write to field "b", so the transaction is doing real work + public Task DifferentHashField_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.HashNotExists(key, "a")); + _ = tran.HashSetAsync(key, "b", "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // likewise a sorted-set member + public Task DifferentSortedSetMember_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.SortedSetNotContains(key, "a")); + _ = tran.SortedSetAddAsync(key, "b", 1.0); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // HashExists + HashSet has no HSETXX to collapse into; the nearest thing is a different method entirely + // (HashFieldSet with ValueCondition.Exists), so this deliberately stays quiet rather than mis-suggesting + public Task HashExistsGuardingHashSet_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.HashExists(key, "field")); + _ = tran.HashSetAsync(key, "field", "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // no server-side hash compare-and-set exists at all + public Task HashEqualGuardingHashSet_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.HashEqual(key, "field", "old")); + _ = tran.HashSetAsync(key, "field", "new"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // likewise for list index writes - LSET has no conditional form + public Task ListIndexEqualGuardingListSetByIndex_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.ListIndexEqual(key, 0, "old")); + _ = tran.ListSetByIndexAsync(key, 0, "new"); + await tran.ExecuteAsync(); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER301.cs b/tests/StackExchange.Redis.Build.Tests/SER301.cs new file mode 100644 index 000000000..fca390c6f --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER301.cs @@ -0,0 +1,118 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Family B: compare-and-set, where a newer single command subsumes both the condition and the write. Separate +/// from SER300 because these need an 8.4+ server and SER300 does not. +/// +public class SER301 : Verifier +{ + [Fact] + public Task StringEqualGuardingStringSet_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.StringEqual(key, "old"))|}; + _ = tran.StringSetAsync(key, "new"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringEqual", + "StringSetAsync", + "StringSet[Async](key, value, ValueCondition.Equal(expected))", + "8.4")); + + [Fact] + public Task StringNotEqualGuardingStringSet_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.StringNotEqual(key, "old"))|}; + _ = tran.StringSetAsync(key, "new"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringNotEqual", + "StringSetAsync", + "StringSet[Async](key, value, ValueCondition.NotEqual(expected))", + "8.4")); + + [Fact] + // the canonical lock-release, and the highest-frequency real-world hit in this family + public Task StringEqualGuardingKeyDelete_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.StringEqual(key, "token"))|}; + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringEqual", + "KeyDeleteAsync", + "StringDelete[Async](key, ValueCondition.Equal(expected)), or LockRelease[Async]", + "8.4")); + + [Fact] + public Task StringNotEqualGuardingKeyDelete_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.StringNotEqual(key, "token"))|}; + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringNotEqual", + "KeyDeleteAsync", + "StringDelete[Async](key, ValueCondition.NotEqual(expected))", + "8.4")); + + [Fact] + // cross-key compare-and-set genuinely needs the transaction; must never fire + public Task DifferentKeys_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.StringEqual(a, "old")); + _ = tran.StringSetAsync(b, "new"); + await tran.ExecuteAsync(); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER302.cs b/tests/StackExchange.Redis.Build.Tests/SER302.cs new file mode 100644 index 000000000..96be86b9e --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER302.cs @@ -0,0 +1,194 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Family C: the condition checks what the queued command already reports, so the transaction buys nothing. +/// +public class SER302 : Verifier +{ + [Fact] + public Task SetNotContainsGuardingSetAdd_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.SetNotContains(key, "member"))|}; + _ = tran.SetAddAsync(key, "member"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER302").WithLocation(0).WithArguments( + "Condition.SetNotContains", + "SetAddAsync", + "SetAdd[Async](key, value), which returns false if the member was already there")); + + [Fact] + public Task SetContainsGuardingSetRemove_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.SetContains(key, "member"))|}; + _ = tran.SetRemoveAsync(key, "member"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER302").WithLocation(0).WithArguments( + "Condition.SetContains", + "SetRemoveAsync", + "SetRemove[Async](key, value), which returns false if the member was not there")); + + [Fact] + public Task SortedSetContainsGuardingSortedSetRemove_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.SortedSetContains(key, "member"))|}; + _ = tran.SortedSetRemoveAsync(key, "member"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER302").WithLocation(0).WithArguments( + "Condition.SortedSetContains", + "SortedSetRemoveAsync", + "SortedSetRemove[Async](key, member), which returns false if the member was not there")); + + [Fact] + public Task HashExistsGuardingHashDelete_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.HashExists(key, "field"))|}; + _ = tran.HashDeleteAsync(key, "field"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER302").WithLocation(0).WithArguments( + "Condition.HashExists", + "HashDeleteAsync", + "HashDelete[Async](key, field), which returns false if the field was not there")); + + [Fact] + public Task KeyExistsGuardingKeyDelete_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyExists(key))|}; + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER302").WithLocation(0).WithArguments( + "Condition.KeyExists", + "KeyDeleteAsync", + "KeyDelete[Async](key), which returns false if the key did not exist")); + + [Fact] + public Task KeyExistsGuardingKeyExpire_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:tran.AddCondition(Condition.KeyExists(key))|}; + _ = tran.KeyExpireAsync(key, TimeSpan.FromMinutes(1)); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER302").WithLocation(0).WithArguments( + "Condition.KeyExists", + "KeyExpireAsync", + "KeyExpire[Async](key, expiry), which returns false if the key did not exist")); + + [Fact] + // LSET reports an out-of-range index by throwing, not by returning false - ListSetByIndex returns Task, + // not Task - so dropping the condition would turn an aborted transaction into an exception. That is + // a behaviour change, not a simplification, so this stays quiet. + public Task ListIndexExistsGuardingListSetByIndex_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.ListIndexExists(key, 0)); + _ = tran.ListSetByIndexAsync(key, 0, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // Same key, different member: the condition asks about "a" and the command removes "b", so it is a real + // guard and dropping it would change behaviour. The key matching is not enough on its own. + public Task DifferentMember_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.SetContains(key, "a")); + _ = tran.SetRemoveAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // and the same for a hash field + public Task DifferentHashField_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.HashExists(key, "a")); + _ = tran.HashDeleteAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER303.cs b/tests/StackExchange.Redis.Build.Tests/SER303.cs new file mode 100644 index 000000000..dbd5145d0 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER303.cs @@ -0,0 +1,347 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Family D: no condition at all - a transaction used purely to make two commands atomic, where one compound +/// command already does both. +/// +public class SER303 : Verifier +{ + [Fact] + public Task StringGetThenKeyDelete_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringGetAsync(key)|}; + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "StringGetAsync", + "KeyDeleteAsync", + "StringGetDelete[Async](key)", + " (requires server 6.2 or later)")); + + [Fact] + public Task StringGetThenKeyExpire_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringGetAsync(key)|}; + _ = tran.KeyExpireAsync(key, TimeSpan.FromMinutes(1)); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "StringGetAsync", + "KeyExpireAsync", + "StringGetSetExpiry[Async](key, expiry)", + " (requires server 6.2 or later)")); + + [Fact] + // SET ... EX. No version clause on this one: setting a value and its lifetime in one command is as old + // as SET's options, so naming a version would be noise. + public Task StringSetThenKeyExpire_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringSetAsync(key, "value")|}; + _ = tran.KeyExpireAsync(key, TimeSpan.FromMinutes(1)); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "StringSetAsync", + "KeyExpireAsync", + "StringSet[Async](key, value, expiry)", + "")); + + [Fact] + // an absolute expiry works the same way: Expiration converts implicitly from DateTime as well as TimeSpan + public Task StringSetThenKeyExpireAtDateTime_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, DateTime when) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringSetAsync(key, "value")|}; + _ = tran.KeyExpireAsync(key, when); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "StringSetAsync", + "KeyExpireAsync", + "StringSet[Async](key, value, expiry)", + "")); + + [Fact] + // the other order is a different program: SET clears any TTL, so EXPIRE-then-SET leaves no expiry at all + public Task KeyExpireThenStringSet_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.KeyExpireAsync(key, TimeSpan.FromMinutes(1)); + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // two expiries, one of which overrides the other: which of them the single command should carry is a + // guess, and this rule does not guess + public Task StringSetWithExpiryThenKeyExpire_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value", TimeSpan.FromMinutes(1)); + _ = tran.KeyExpireAsync(key, TimeSpan.FromMinutes(5)); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + public Task StringGetThenStringSet_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringGetAsync(key)|}; + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "StringGetAsync", + "StringSetAsync", + "StringSetAndGet[Async](key, value)", + " (requires server 6.2 or later)")); + + [Fact] + public Task HashGetThenHashDelete_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.HashGetAsync(key, "field")|}; + _ = tran.HashDeleteAsync(key, "field"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "HashGetAsync", + "HashDeleteAsync", + "HashFieldGetAndDelete[Async](key, field)", + " (requires server 8.0 or later)")); + + [Fact] + // SMOVE is as old as sets, so this one carries no version clause at all - which is why the clause is built + // per-mapping rather than baked into the message format + public Task SetRemoveThenSetAdd_IsFlaggedWithoutVersion() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey source, RedisKey destination) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.SetRemoveAsync(source, "member")|}; + _ = tran.SetAddAsync(destination, "member"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "SetRemoveAsync", + "SetAddAsync", + "SetMove[Async](source, destination, value)", + "")); + + [Fact] + // the effects are order-independent within a transaction, so the reverse order is the same move + public Task SetAddThenSetRemove_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey source, RedisKey destination) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.SetAddAsync(destination, "member")|}; + _ = tran.SetRemoveAsync(source, "member"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER303").WithLocation(0).WithArguments( + "SetAddAsync", + "SetRemoveAsync", + "SetMove[Async](source, destination, value)", + "")); + + [Fact] + // SMOVE moves one member; two different members is not one move + public Task SetMoveWithDifferentMembers_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey source, RedisKey destination) + { + var tran = db.CreateTransaction(); + _ = tran.SetRemoveAsync(source, "a"); + _ = tran.SetAddAsync(destination, "b"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // SET ... GET returns the value from *before* the write, so it matches get-then-set. Set-then-get asks for + // the value *after* the write, which is a different thing, and must not be collapsed. + public Task StringSetThenStringGet_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value"); + _ = tran.StringGetAsync(key); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // Tempting but wrong: LMOVE moves the element it popped, and inside a transaction the pop's result is an + // unresolved Task the caller cannot pass to the push - so whatever is being pushed is some other value. + public Task ListRightPopThenListLeftPush_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey source, RedisKey destination) + { + var tran = db.CreateTransaction(); + _ = tran.ListRightPopAsync(source); + _ = tran.ListLeftPushAsync(destination, "value"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // different keys: two unrelated commands that genuinely want the transaction + public Task DifferentKeys_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = tran.StringGetAsync(a); + _ = tran.KeyDeleteAsync(b); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // a condition present means this is families A-C's territory, not a compound collapse + public Task WithCondition_IsNotFlaggedAsCompound() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyExists(key)); + _ = tran.StringGetAsync(key); + _ = tran.KeyDeleteAsync(key); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // three commands is not a pair + public Task ThreeOperations_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringGetAsync(key); + _ = tran.KeyDeleteAsync(key); + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER304.cs b/tests/StackExchange.Redis.Build.Tests/SER304.cs new file mode 100644 index 000000000..a7c536e74 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER304.cs @@ -0,0 +1,344 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Family D, second flavour: the same command queued over and over, where one variadic call does the lot. +/// +public class SER304 : Verifier +{ + [Fact] + public Task RepeatedSetAddOnOneKey_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.SetAddAsync(key, "a")|}; + _ = tran.SetAddAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "SetAddAsync", + "2", + "SetAdd[Async](key, values)", + "")); + + [Fact] + // more than two, to prove the shape is not secretly pair-only + public Task ThreeRepeatedHashSets_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.HashSetAsync(key, "f1", "v1")|}; + _ = tran.HashSetAsync(key, "f2", "v2"); + _ = tran.HashSetAsync(key, "f3", "v3"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "HashSetAsync", + "3", + "HashSet[Async](key, entries)", + "")); + + [Fact] + public Task RepeatedListRightPush_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.ListRightPushAsync(key, "a")|}; + _ = tran.ListRightPushAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "ListRightPushAsync", + "2", + "ListRightPush[Async](key, values)", + "")); + + [Fact] + // SMISMEMBER is recent enough that the version clause appears + public Task RepeatedSetContains_IsFlaggedWithVersion() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.SetContainsAsync(key, "a")|}; + _ = tran.SetContainsAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "SetContainsAsync", + "2", + "SetContains[Async](key, values), which returns a bool per value", + " (requires server 6.2 or later)")); + + [Fact] + // the many-keys direction: MSET + public Task RepeatedStringSetAcrossKeys_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringSetAsync(a, "1")|}; + _ = tran.StringSetAsync(b, "2"); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "StringSetAsync", + "2", + "StringSet[Async](KeyValuePair[])", + "")); + + [Fact] + public Task RepeatedStringGetAcrossKeys_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.StringGetAsync(a)|}; + _ = tran.StringGetAsync(b); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "StringGetAsync", + "2", + "StringGet[Async](keys)", + "")); + + [Fact] + public Task RepeatedKeyDeleteAcrossKeys_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = {|#0:tran.KeyDeleteAsync(a)|}; + _ = tran.KeyDeleteAsync(b); + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER304").WithLocation(0).WithArguments( + "KeyDeleteAsync", + "2", + "KeyDelete[Async](keys)", + "")); + + [Fact] + // SADD takes one key and many values, so calls on different keys have no single-command form + public Task RepeatedSetAddAcrossKeys_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = tran.SetAddAsync(a, "m"); + _ = tran.SetAddAsync(b, "m"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // HSET takes one key and many field/value pairs, so calls across keys have no single-command form + public Task RepeatedHashSetAcrossKeys_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = tran.HashSetAsync(a, "f1", "v1"); + _ = tran.HashSetAsync(b, "f2", "v2"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // The key comparison is textual, so a local that is reassigned between the calls would read as "the same + // key" when it is nothing of the sort - collapsing these into one HashSet would write both fields to "b". + public Task KeyLocalReassignedBetweenCalls_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db) + { + RedisKey key = "a"; + var tran = db.CreateTransaction(); + _ = tran.HashSetAsync(key, "f1", "v1"); + key = "b"; + _ = tran.HashSetAsync(key, "f2", "v2"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // ... and the same where the reassignment is a compound one rather than a plain assignment + public Task KeyLocalMutatedByRef_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + private static void Change(ref RedisKey key) => key = "b"; + + public async Task M(IDatabase db) + { + RedisKey key = "a"; + var tran = db.CreateTransaction(); + _ = tran.HashSetAsync(key, "f1", "v1"); + Change(ref key); + _ = tran.HashSetAsync(key, "f2", "v2"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // ... and conversely MSET wants distinct keys; two writes to one key is not what this rule is about + public Task RepeatedStringSetOnOneKey_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "1"); + _ = tran.StringSetAsync(key, "2"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // Tempting but wrong: LMPOP pops from the first *non-empty* key of those given, not from each of them, so + // it is a different operation however similar the argument list looks. Same for ZMPOP. + public Task RepeatedListLeftPopAcrossKeys_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey a, RedisKey b) + { + var tran = db.CreateTransaction(); + _ = tran.ListLeftPopAsync(a); + _ = tran.ListLeftPopAsync(b); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // different commands: not a variadic collapse, and not one of the compound pairs either + public Task DifferentCommands_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.SetAddAsync(key, "a"); + _ = tran.ListRightPushAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // a condition present takes this out of family D entirely + public Task WithCondition_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + tran.AddCondition(Condition.KeyExists(key)); + _ = tran.SetAddAsync(key, "a"); + _ = tran.SetAddAsync(key, "b"); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + // The most common way to write this in practice, and deliberately still quiet: a loop body is one call site + // whose key expression we cannot prove is loop-invariant, so we cannot tell a same-key collapse from a + // per-key one. Left for a later pass rather than guessed at. + public Task RepeatedInLoop_IsNotFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, RedisValue[] values) + { + var tran = db.CreateTransaction(); + foreach (var value in values) + { + _ = tran.SetAddAsync(key, value); + } + + await tran.ExecuteAsync(); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj b/tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj new file mode 100644 index 000000000..8abbe3b2c --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj @@ -0,0 +1,28 @@ + + + net10.0 + Exe + enable + + $(NoWarn);NU1608;NU1701 + + + + + + + + + + + + + + + + + + diff --git a/tests/StackExchange.Redis.Build.Tests/Verifier.cs b/tests/StackExchange.Redis.Build.Tests/Verifier.cs new file mode 100644 index 000000000..c18d74020 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/Verifier.cs @@ -0,0 +1,98 @@ +using System.IO; +using System.Text; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Testing; +using Microsoft.CodeAnalysis.Text; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Base for analyzer verification, in the shape used by DapperAOT: a source string with {|#0:...|} +/// markers, plus the diagnostics expected at those locations. +/// +public abstract class Verifier + where TAnalyzer : DiagnosticAnalyzer, new() +{ + /// + /// Reference assemblies matching the library build we load below. + /// + /// + /// The harness only ships well-known sets up to a point, and mismatching them against the + /// StackExchange.Redis build we reference gives CS1705 (assembly wants a newer System.Runtime), so + /// describe the current target explicitly rather than pinning to whatever the harness happens to know. + /// + private static readonly ReferenceAssemblies Net10 = new( + "net10.0", + new PackageIdentity("Microsoft.NETCore.App.Ref", "10.0.0"), + Path.Combine("ref", "net10.0")); + + /// Expect a diagnostic with this id at the marked location. + /// + /// Defaults to because that is what the rules ship as; the harness + /// checks severity, so this is also what stops the default being changed without anyone noticing. + /// + protected static DiagnosticResult Diagnostic(string id, DiagnosticSeverity severity = DiagnosticSeverity.Warning) + => new(id, severity); + + /// Verify that produces exactly . + protected static Task VerifyAsync(string source, params DiagnosticResult[] expected) + => RunAsync(source, referenceLibrary: true, minServerVersion: null, expected); + + /// + /// As , but with the project declaring a minimum server version. + /// + /// + /// Written as a .globalconfig entry, which is also how the MSBuild property arrives once + /// CompilerVisibleProperty has translated it - so this covers both spellings' consumption path. + /// + protected static Task VerifyWithMinServerVersionAsync(string source, string minServerVersion, params DiagnosticResult[] expected) + => RunAsync(source, referenceLibrary: true, minServerVersion, expected); + + /// + /// As , but with no reference to StackExchange.Redis at all. + /// + /// + /// For asserting the no-op path: the analyzer resolves its types by metadata name and does nothing when + /// they are absent, and that has to keep working (and not throw) in the overwhelming majority of + /// compilations, which have never heard of this library. + /// + protected static Task VerifyWithoutLibraryAsync(string source) + => RunAsync(source, referenceLibrary: false, minServerVersion: null); + + private static Task RunAsync(string source, bool referenceLibrary, string? minServerVersion, params DiagnosticResult[] expected) + { + // Test sources use string literals for keys/values, which trips the library's own [Experimental] + // gate on the implicit string -> RedisValue conversion. That is unrelated to what we are testing, and + // surfaces as an error in the test compilation, so opt out of it for every case. + var test = new CSharpAnalyzerTest + { + TestCode = "#pragma warning disable StringToRedisValue" + System.Environment.NewLine + source, + ReferenceAssemblies = Net10, + }; + + // The analyzer resolves StackExchange.Redis.Condition by metadata name and does nothing at all if it + // is absent - so without this reference the positive cases would fail to compile rather than silently + // pass, but the *negative* cases would trivially "pass" by finding no diagnostics. Hence both this and + // NoLibrary.cs, which asserts the absent case deliberately rather than by accident. + if (referenceLibrary) + { + test.TestState.AdditionalReferences.Add( + MetadataReference.CreateFromFile(typeof(StackExchange.Redis.ConnectionMultiplexer).Assembly.Location)); + } + + if (minServerVersion is not null) + { + test.TestState.AnalyzerConfigFiles.Add(("/.globalconfig", SourceText.From( + "is_global = true" + System.Environment.NewLine + + "redis.min_server_version = " + minServerVersion + System.Environment.NewLine, + Encoding.UTF8))); + } + + test.ExpectedDiagnostics.AddRange(expected); + return test.RunAsync(TestContext.Current.CancellationToken); + } +} diff --git a/tests/StackExchange.Redis.Tests/ConstraintsTests.cs b/tests/StackExchange.Redis.Tests/ConstraintsTests.cs index 6740fe2b3..878d0ed7c 100644 --- a/tests/StackExchange.Redis.Tests/ConstraintsTests.cs +++ b/tests/StackExchange.Redis.Tests/ConstraintsTests.cs @@ -35,8 +35,14 @@ public async Task TestManualIncr() var newVal = (oldVal ?? 0) + 1; var tran = connection.CreateTransaction(); { // check hasn't changed + // Deliberately the long way round: this exercises the optimistic-concurrency path (read, compare, + // conditional write, observe the abort), which is the thing under test. StringIncrement would be + // the right answer in real code, and a single compare-and-set write would remove the abort we + // are here to provoke. +#pragma warning disable SER301 // Transaction can be replaced by a single atomic operation tran.AddCondition(Condition.StringEqual(key, oldVal)); _ = tran.StringSetAsync(key, newVal); +#pragma warning restore SER301 if (!await tran.ExecuteAsync().ForAwait()) return null; // aborted return newVal; } diff --git a/tests/StackExchange.Redis.Tests/RetryTests/RetryEndToEndTests.cs b/tests/StackExchange.Redis.Tests/RetryTests/RetryEndToEndTests.cs index 44ce533f1..aa9614027 100644 --- a/tests/StackExchange.Redis.Tests/RetryTests/RetryEndToEndTests.cs +++ b/tests/StackExchange.Redis.Tests/RetryTests/RetryEndToEndTests.cs @@ -6,6 +6,11 @@ using StackExchange.Redis.Server; using Xunit; +// The whole point of this file is what a WATCH-based transaction does when EXEC is retried, so the analyzer's +// advice to collapse these into a single atomic command is exactly what must not happen here: there would be no +// WATCH left to retry, and nothing to test. Suppressed file-wide rather than per-site for that reason. +#pragma warning disable SER301 // Transaction can be replaced by a single atomic operation + namespace StackExchange.Redis.Tests.RetryTests; [RunPerProtocol]