From ce4d8c6455c884d2faf738e0c77a91a160435a6e Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 13:39:46 +0100 Subject: [PATCH 01/11] first stab at transaction analyzer --- Directory.Packages.props | 15 +- .../AnalyzerReleases.Shipped.md | 2 + .../AnalyzerReleases.Unshipped.md | 13 + .../AsciiHashGenerator.cs | 46 +++- .../AutoDatabaseGenerator.cs | 16 ++ eng/StackExchange.Redis.Build/Diagnostics.cs | 82 +++++++ eng/StackExchange.Redis.Build/RoslynShims.cs | 19 ++ .../StackExchange.Redis.Build.csproj | 14 +- .../TransactionAnalyzer.cs | 228 ++++++++++++++++++ src/StackExchange.Redis/RedisDatabase.cs | 4 + .../StackExchange.Redis.Build.Tests/SER301.cs | 43 ++++ .../StackExchange.Redis.Build.Tests.csproj | 28 +++ .../Verifier.cs | 56 +++++ 13 files changed, 557 insertions(+), 9 deletions(-) create mode 100644 eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md create mode 100644 eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md create mode 100644 eng/StackExchange.Redis.Build/Diagnostics.cs create mode 100644 eng/StackExchange.Redis.Build/RoslynShims.cs create mode 100644 eng/StackExchange.Redis.Build/TransactionAnalyzer.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/SER301.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj create mode 100644 tests/StackExchange.Redis.Build.Tests/Verifier.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index c5261d0e1..2a391e64a 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -13,11 +13,22 @@ - - + + + + + diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md new file mode 100644 index 000000000..b4de231a4 --- /dev/null +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md @@ -0,0 +1,2 @@ +; Shipped analyzer releases; see AnalyzerReleases.Unshipped.md for the convention. +; Nothing has shipped yet - move rules here when a release goes out. diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md new file mode 100644 index 000000000..52e490bfe --- /dev/null +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md @@ -0,0 +1,13 @@ +; 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 + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +SER300 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a conditional argument (any server) +SER301 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) +SER350 | Build | Warning | AsciiHashGenerator: generated code requires a newer C# language version, so nothing was generated diff --git a/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs b/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs index b00675c40..b205b09a3 100644 --- a/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs +++ b/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs @@ -12,6 +12,11 @@ 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 = LanguageVersion.CSharp11; + public void Initialize(IncrementalGeneratorInitializationContext context) { // looking for [AsciiHash] partial static class Foo { } @@ -49,10 +54,39 @@ public void Initialize(IncrementalGeneratorInitializationContext context) .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)); + { + if (content.Right < MinimumLanguageVersion) + { + // 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; + } + + 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); @@ -219,7 +253,7 @@ private static string GetRawValue(string name, AttributeData? asciiHashAttribute 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 +317,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, }) { @@ -348,7 +382,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 +881,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..7f95a37dd --- /dev/null +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -0,0 +1,82 @@ +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. Analyzer rules default to - the code they flag is correct, just not optimal, and a shipped warning +/// would break builds that set TreatWarningsAsErrors. +/// +/// +internal static class Diagnostics +{ + private const string UsageCategory = "Usage", BuildCategory = "Build"; + + /// + /// 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 can be replaced by a conditional argument", + messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2} - the condition duplicates an argument the command already has", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "A transaction whose only purpose is to make one operation conditional can be replaced by the command's own conditional argument, which is a single round-trip and cannot abort under contention."); + + /// + /// 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. + /// + public static readonly DiagnosticDescriptor PreferNewerAtomicOperation = new( + id: "SER301", + title: "Transaction can be replaced by a single atomic operation", + messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2}, which is atomic on the server and needs no WATCH (requires a newer server)", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "A transaction implementing compare-and-set can be replaced by the equivalent conditional command on servers that support it, which is a single round-trip and cannot abort under contention."); + + /// + /// 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); +} diff --git a/eng/StackExchange.Redis.Build/RoslynShims.cs b/eng/StackExchange.Redis.Build/RoslynShims.cs new file mode 100644 index 000000000..41280a3c5 --- /dev/null +++ b/eng/StackExchange.Redis.Build/RoslynShims.cs @@ -0,0 +1,19 @@ +using Microsoft.CodeAnalysis; + +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; +} diff --git a/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj b/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj index 3cde6f5f6..b78bc65f4 100644 --- a/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj +++ b/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj @@ -8,7 +8,19 @@ - + + + + + + + + diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs new file mode 100644 index 000000000..d63a2c4d4 --- /dev/null +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -0,0 +1,228 @@ +using System.Collections.Immutable; +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. +/// +/// +/// Deliberately conservative. It only fires on the unambiguous shape - exactly one condition guarding exactly +/// one queued operation, on a syntactically identical key - because this ships to every consumer of the +/// package, and a false positive on correct code is worse than staying quiet. Anything cleverer (several +/// operations, a condition on a different key, a transaction whose result feeds back into control flow) 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); + + /// + 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; + ctx.RegisterOperationBlockAction(blockCtx => Analyze(blockCtx, known)); + }); + } + + private sealed class KnownSymbols + { + private KnownSymbols(INamedTypeSymbol condition, INamedTypeSymbol? transaction, INamedTypeSymbol? transactionAsync) + { + Condition = condition; + Transaction = transaction; + TransactionAsync = transactionAsync; + } + + public INamedTypeSymbol Condition { get; } + public INamedTypeSymbol? Transaction { get; } + public INamedTypeSymbol? TransactionAsync { 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); + } + + 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) + { + 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()) + { + if (operation is not IInvocationOperation invocation) continue; + context.CancellationToken.ThrowIfCancellationRequested(); + + // the transaction is identified by the local it was assigned to; anything else (a field, a + // fluent chain, a transaction passed between methods) is out of scope by design + if (invocation.Instance is not ILocalReferenceOperation { Local: { } local }) continue; + if (!known.IsTransaction(local.Type)) continue; + + usages ??= new Dictionary(SymbolEqualityComparer.Default); + if (!usages.TryGetValue(local, out var usage)) usages[local] = usage = new Usage(); + usage.Add(invocation, known); + } + + if (usages is null) continue; + + foreach (var pair in usages) + { + if (pair.Value.TryGetSuggestion(out var conditionName, out var operationName, out var suggestion, out var needsNewerServer)) + { + context.ReportDiagnostic(Diagnostic.Create( + needsNewerServer ? Diagnostics.PreferNewerAtomicOperation : Diagnostics.PreferConditionalArgument, + pair.Value.ReportAt, + conditionName, + operationName, + suggestion)); + } + } + } + } + + /// + /// What we saw done with one transaction local. + /// + private sealed class Usage + { + private int _conditionCount, _operationCount; + private string? _conditionFactory, _conditionKey; + private string? _operationName, _operationKey; + + public Location? ReportAt { get; private set; } + + public void Add(IInvocationOperation invocation, KnownSymbols known) + { + switch (invocation.TargetMethod.Name) + { + case "AddCondition": + _conditionCount++; + ReportAt ??= 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 = FirstArgumentText(factory); + } + + break; + + case "Execute": + case "ExecuteAsync": + break; // the terminator, not a queued operation + + default: + // everything else queued on the transaction is a redis operation + _operationCount++; + _operationName = invocation.TargetMethod.Name; + _operationKey = FirstArgumentText(invocation); + break; + } + } + + public bool TryGetSuggestion(out string conditionName, out string operationName, out string suggestion, out bool needsNewerServer) + { + conditionName = operationName = suggestion = ""; + needsNewerServer = false; + + // only the unambiguous shape: one guard, one operation, and the same key in both + if (_conditionCount != 1 || _operationCount != 1) return false; + if (_conditionFactory is null || _operationName is null) return false; + if (_conditionKey is null || _operationKey is null || _conditionKey != _operationKey) return false; + + if (Map(_conditionFactory, _operationName) is not { } mapped) return false; + + conditionName = "Condition." + _conditionFactory; + operationName = _operationName; + (suggestion, needsNewerServer) = mapped; + return true; + } + + /// + /// The condition/operation pairs that have an exact single-command equivalent. + /// + private static (string Suggestion, bool NeedsNewerServer)? Map(string condition, string operation) + { + var op = Trim(operation); + return (condition, op) switch + { + // -- family A: the command already takes this condition as an argument; any server version -- + ("KeyNotExists", "StringSet") => ("StringSet(key, value, When.NotExists)", false), + ("KeyExists", "StringSet") => ("StringSet(key, value, When.Exists)", false), + ("HashNotExists", "HashSet") => ("HashSet(key, field, value, When.NotExists)", false), + ("SortedSetNotContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, When.NotExists)", false), + ("SortedSetContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, When.Exists)", false), + ("KeyNotExists", "KeyRename") => ("KeyRename(key, newKey, When.NotExists)", false), + + // -- family B: a newer single command subsumes condition and write (compare-and-set, 8.4+) -- + ("StringEqual", "StringSet") => ("StringSet(key, value, ValueCondition.Equal(expected))", true), + ("StringNotEqual", "StringSet") => ("StringSet(key, value, ValueCondition.NotEqual(expected))", true), + ("StringEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", true), + ("StringNotEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.NotEqual(expected))", true), + + // 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, + }; + + static string Trim(string name) + => name.EndsWith("Async", StringComparison.Ordinal) ? name.Substring(0, name.Length - 5) : name; + } + + /// + /// The source text of the first argument, used as a cheap "same key?" 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 string? FirstArgumentText(IInvocationOperation invocation) + => invocation.Arguments.Length == 0 ? null : invocation.Arguments[0].Value.Syntax.ToString(); + + 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/tests/StackExchange.Redis.Build.Tests/SER301.cs b/tests/StackExchange.Redis.Build.Tests/SER301.cs new file mode 100644 index 000000000..836bc0136 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER301.cs @@ -0,0 +1,43 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +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)); + + [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/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..1658c7324 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/Verifier.cs @@ -0,0 +1,56 @@ +using System.IO; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Testing; +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. + protected static DiagnosticResult Diagnostic(string id, DiagnosticSeverity severity = DiagnosticSeverity.Info) + => new(id, severity); + + /// Verify that produces exactly . + protected static Task VerifyAsync(string source, 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 every test would trivially "pass" by finding no diagnostics. + // NoDiagnosticsWhenLibraryAbsent covers the other side of that. + test.TestState.AdditionalReferences.Add( + MetadataReference.CreateFromFile(typeof(StackExchange.Redis.ConnectionMultiplexer).Assembly.Location)); + + test.ExpectedDiagnostics.AddRange(expected); + return test.RunAsync(TestContext.Current.CancellationToken); + } +} From 5596554375c3702b6c4fb86dc9d3264fab2c128b Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 14:02:41 +0100 Subject: [PATCH 02/11] intermediate --- Directory.Packages.props | 5 + docs/index.md | 1 + docs/rules/SER300.md | 61 ++++++ docs/rules/SER301.md | 58 +++++ docs/rules/SER350.md | 29 +++ docs/rules/index.md | 35 +++ .../AsciiHashGenerator.cs | 2 +- eng/StackExchange.Redis.Build/Diagnostics.cs | 22 +- eng/StackExchange.Redis.Build/RoslynShims.cs | 15 ++ .../StackExchange.Redis.Build.csproj | 12 +- .../TransactionAnalyzer.cs | 64 +++++- .../DetectionShape.cs | 200 ++++++++++++++++++ .../NoLibrary.cs | 54 +++++ .../StackExchange.Redis.Build.Tests/SER300.cs | 195 +++++++++++++++++ .../StackExchange.Redis.Build.Tests/SER301.cs | 59 ++++++ .../Verifier.cs | 26 ++- 16 files changed, 815 insertions(+), 23 deletions(-) create mode 100644 docs/rules/SER300.md create mode 100644 docs/rules/SER301.md create mode 100644 docs/rules/SER350.md create mode 100644 docs/rules/index.md create mode 100644 tests/StackExchange.Redis.Build.Tests/DetectionShape.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/NoLibrary.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/SER300.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 2a391e64a..66df6c65b 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -22,6 +22,11 @@ 4.3 is where ForAttributeWithMetadataName arrives, and the generators need that to stay cheap on consumer code, so going lower is not useful. Raising it drops consumer tooling; do so deliberately. --> + + 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..be31aeec0 --- /dev/null +++ b/docs/rules/SER300.md @@ -0,0 +1,61 @@ +# SER300: transaction can be replaced 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, anything queued in a loop, a transaction passed to another method or stored in a field, and pairings +with no atomic equivalent (`HashExists` + `HashSet`, `HashEqual`, `ListIndexEqual`, the `*Length*` conditions). + +See also [Transactions](../Transactions). + +## Suppressing + +The flagged code is correct, just not optimal, so this is reported as information and never fails a build. To +silence it anyway: + +```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..dff0e5ac6 --- /dev/null +++ b/docs/rules/SER301.md @@ -0,0 +1,58 @@ +# SER301: transaction can be replaced by a single atomic operation + +A transaction implementing compare-and-set can 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. + +## 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. + +## Suppressing + +Reported as information; it never fails a build. To silence: + +```xml +$(NoWarn);SER301 +``` + +or locally: + +```c# +#pragma warning disable SER301 +``` 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..527501baa --- /dev/null +++ b/docs/rules/index.md @@ -0,0 +1,35 @@ +# 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 (reported as *information*; never fails a build) | +| `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". + +## Usage + +- [SER300](SER300) - transaction can be replaced by a conditional argument (any server version) +- [SER301](SER301) - transaction can be replaced by a single atomic operation (needs a newer server) + +## Build + +- [SER350](SER350) - language version too low for generated code + +## Why these are only information + +The code these rules flag is correct - it works, and it will keep working. They point at a form that is a single +round-trip instead of two and cannot abort under contention. Shipping them as warnings would break every +consumer building with `TreatWarningsAsErrors`, so they are informational by default; raise the severity in +`.editorconfig` if you want them enforced: + +```ini +dotnet_diagnostic.SER300.severity = warning +``` diff --git a/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs b/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs index b205b09a3..9b97fa913 100644 --- a/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs +++ b/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs @@ -15,7 +15,7 @@ public class AsciiHashGenerator : IIncrementalGenerator /// /// The emitted code uses UTF-8 string literals, which are C# 11. /// - private const LanguageVersion MinimumLanguageVersion = LanguageVersion.CSharp11; + private const LanguageVersion MinimumLanguageVersion = LanguageVersions.CSharp11; public void Initialize(IncrementalGeneratorInitializationContext context) { diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index 7f95a37dd..f1aecb9b5 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -26,6 +26,17 @@ 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. /// @@ -41,7 +52,8 @@ internal static class Diagnostics category: UsageCategory, defaultSeverity: DiagnosticSeverity.Info, isEnabledByDefault: true, - description: "A transaction whose only purpose is to make one operation conditional can be replaced by the command's own conditional argument, which is a single round-trip and cannot abort under contention."); + description: "A transaction whose only purpose is to make one operation conditional can 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. @@ -60,7 +72,8 @@ internal static class Diagnostics category: UsageCategory, defaultSeverity: DiagnosticSeverity.Info, isEnabledByDefault: true, - description: "A transaction implementing compare-and-set can be replaced by the equivalent conditional command on servers that support it, which is a single round-trip and cannot abort under contention."); + description: "A transaction implementing compare-and-set can 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")); /// /// The generated code cannot be compiled at the language version in effect, so nothing was generated. @@ -78,5 +91,8 @@ internal static class Diagnostics 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); + 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 index 41280a3c5..30ad00828 100644 --- a/eng/StackExchange.Redis.Build/RoslynShims.cs +++ b/eng/StackExchange.Redis.Build/RoslynShims.cs @@ -1,4 +1,5 @@ using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; namespace StackExchange.Redis.Build; @@ -17,3 +18,17 @@ 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/StackExchange.Redis.Build.csproj b/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj index b78bc65f4..1d4959e24 100644 --- a/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj +++ b/eng/StackExchange.Redis.Build/StackExchange.Redis.Build.csproj @@ -8,13 +8,11 @@ - - + + + + diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs index d63a2c4d4..bdf6fa168 100644 --- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -78,17 +78,31 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols foreach (var operation in block.Descendants()) { - if (operation is not IInvocationOperation invocation) continue; + // 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(); - // the transaction is identified by the local it was assigned to; anything else (a field, a - // fluent chain, a transaction passed between methods) is out of scope by design - if (invocation.Instance is not ILocalReferenceOperation { Local: { } local }) continue; if (!known.IsTransaction(local.Type)) continue; usages ??= new Dictionary(SymbolEqualityComparer.Default); if (!usages.TryGetValue(local, out var usage)) usages[local] = usage = new Usage(); - usage.Add(invocation, known); + + 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 + usage.Add(invocation, known, insideLoop: IsInsideLoop(invocation, block)); + } + 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; @@ -108,6 +122,24 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols } } + /// + /// Is this call inside a loop, and so potentially queueing many commands from one call site? + /// + /// + /// Counting call sites is a syntactic approximation, and a loop is where it breaks: one + /// tran.StringSetAsync(key, value) in a foreach is one call site but N queued commands, which + /// is emphatically not collapsible into a single command. Cheap to check and it removes the whole class. + /// + private static bool IsInsideLoop(IOperation operation, IOperation block) + { + for (var node = operation; node is not null && node != block; node = node.Parent) + { + if (node is ILoopOperation) return true; + } + + return false; + } + /// /// What we saw done with one transaction local. /// @@ -116,11 +148,23 @@ private sealed class Usage private int _conditionCount, _operationCount; private string? _conditionFactory, _conditionKey; private string? _operationName, _operationKey; + private bool _disqualified; public Location? ReportAt { get; private set; } - public void Add(IInvocationOperation invocation, KnownSymbols known) + /// + /// 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, bool insideLoop) { + if (insideLoop) + { + Disqualify(); + return; + } + switch (invocation.TargetMethod.Name) { case "AddCondition": @@ -158,6 +202,8 @@ public bool TryGetSuggestion(out string conditionName, out string operationName, conditionName = operationName = suggestion = ""; needsNewerServer = false; + if (_disqualified) return false; + // only the unambiguous shape: one guard, one operation, and the same key in both if (_conditionCount != 1 || _operationCount != 1) return false; if (_conditionFactory is null || _operationName is null) return false; @@ -183,8 +229,10 @@ private static (string Suggestion, bool NeedsNewerServer)? Map(string condition, ("KeyNotExists", "StringSet") => ("StringSet(key, value, When.NotExists)", false), ("KeyExists", "StringSet") => ("StringSet(key, value, When.Exists)", false), ("HashNotExists", "HashSet") => ("HashSet(key, field, value, When.NotExists)", false), - ("SortedSetNotContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, When.NotExists)", false), - ("SortedSetContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, When.Exists)", false), + // 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") => ("SortedSetAdd(key, member, score, SortedSetWhen.NotExists)", false), + ("SortedSetContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, SortedSetWhen.Exists)", false), ("KeyNotExists", "KeyRename") => ("KeyRename(key, newKey, When.NotExists)", false), // -- family B: a newer single command subsumes condition and write (compare-and-set, 8.4+) -- diff --git a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs new file mode 100644 index 000000000..c5f4f480d --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs @@ -0,0 +1,200 @@ +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] + // 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] + // 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), + Diagnostic("SER301").WithLocation(1)); +} 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..522934ab6 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER300.cs @@ -0,0 +1,195 @@ +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)); + + [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)); + + [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)); + + [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)); + + [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)); + + [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)); + + [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)); + + [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 index 836bc0136..27b77f2a5 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER301.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER301.cs @@ -3,6 +3,10 @@ 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] @@ -23,6 +27,61 @@ public async Task M(IDatabase db, RedisKey key) """, Diagnostic("SER301").WithLocation(0)); + [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)); + + [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)); + + [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)); + [Fact] // cross-key compare-and-set genuinely needs the transaction; must never fire public Task DifferentKeys_IsNotFlagged() => VerifyAsync( diff --git a/tests/StackExchange.Redis.Build.Tests/Verifier.cs b/tests/StackExchange.Redis.Build.Tests/Verifier.cs index 1658c7324..815af6b89 100644 --- a/tests/StackExchange.Redis.Build.Tests/Verifier.cs +++ b/tests/StackExchange.Redis.Build.Tests/Verifier.cs @@ -34,6 +34,20 @@ protected static DiagnosticResult Diagnostic(string id, DiagnosticSeverity sever /// Verify that produces exactly . protected static Task VerifyAsync(string source, params DiagnosticResult[] expected) + => RunAsync(source, referenceLibrary: true, 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); + + private static Task RunAsync(string source, bool referenceLibrary, 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 @@ -45,10 +59,14 @@ protected static Task VerifyAsync(string source, params DiagnosticResult[] expec }; // The analyzer resolves StackExchange.Redis.Condition by metadata name and does nothing at all if it - // is absent - so without this reference every test would trivially "pass" by finding no diagnostics. - // NoDiagnosticsWhenLibraryAbsent covers the other side of that. - test.TestState.AdditionalReferences.Add( - MetadataReference.CreateFromFile(typeof(StackExchange.Redis.ConnectionMultiplexer).Assembly.Location)); + // 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)); + } test.ExpectedDiagnostics.AddRange(expected); return test.RunAsync(TestContext.Current.CancellationToken); From abf210c4a839bf3b0ab90fb5071f580bdbe90437 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 14:18:54 +0100 Subject: [PATCH 03/11] server version logic --- docs/rules/SER301.md | 25 ++++ docs/rules/index.md | 20 +++ eng/StackExchange.Redis.Build/Diagnostics.cs | 7 +- .../ServerVersion.cs | 91 +++++++++++++ .../TransactionAnalyzer.cs | 127 ++++++++++++------ .../StackExchange.Redis.csproj | 3 + .../build/StackExchange.Redis.props | 29 ++++ .../MinServerVersion.cs | 107 +++++++++++++++ .../Verifier.cs | 26 +++- 9 files changed, 393 insertions(+), 42 deletions(-) create mode 100644 eng/StackExchange.Redis.Build/ServerVersion.cs create mode 100644 src/StackExchange.Redis/build/StackExchange.Redis.props create mode 100644 tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs diff --git a/docs/rules/SER301.md b/docs/rules/SER301.md index dff0e5ac6..c8df025bb 100644 --- a/docs/rules/SER301.md +++ b/docs/rules/SER301.md @@ -25,6 +25,31 @@ an older server you want to silence this one while keeping SER300, and a shared 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 diff --git a/docs/rules/index.md b/docs/rules/index.md index 527501baa..20a2ef173 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -23,6 +23,26 @@ which mean "this API is preview", not "consider changing this code". - [SER350](SER350) - language version too low for generated code +## 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. + ## Why these are only information The code these rules flag is correct - it works, and it will keep working. They point at a form that is a single diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index f1aecb9b5..a7d14d95b 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -64,11 +64,16 @@ internal static class Diagnostics /// 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 - Redis_MinServerVersion, 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 can be replaced by a single atomic operation", - messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2}, which is atomic on the server and needs no WATCH (requires a newer server)", + messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2}, which is atomic on the server and needs no WATCH (requires server {3} or later)", category: UsageCategory, defaultSeverity: DiagnosticSeverity.Info, isEnabledByDefault: true, diff --git a/eng/StackExchange.Redis.Build/ServerVersion.cs b/eng/StackExchange.Redis.Build/ServerVersion.cs new file mode 100644 index 000000000..e8357d81e --- /dev/null +++ b/eng/StackExchange.Redis.Build/ServerVersion.cs @@ -0,0 +1,91 @@ +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; + } + + if (global.TryGetValue("build_property.Redis_MinServerVersion", 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; + + var parts = text!.Trim().Split('.'); + if (!int.TryParse(parts[0], out var major) || major <= 0) return false; + + var minor = 0; + if (parts.Length > 1 && !int.TryParse(parts[1], out minor)) return false; + if (minor < 0) return false; + + version = new ServerVersion(major, minor); + return true; + } +} diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs index bdf6fa168..f175fe89a 100644 --- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -34,7 +34,10 @@ public override void Initialize(AnalysisContext context) context.RegisterCompilationStartAction(static ctx => { if (KnownSymbols.TryCreate(ctx.Compilation) is not { } known) return; - ctx.RegisterOperationBlockAction(blockCtx => Analyze(blockCtx, known)); + + // 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)); }); } @@ -69,7 +72,7 @@ public bool IsTransaction(ITypeSymbol? type) || (TransactionAsync is not null && SymbolEqualityComparer.Default.Equals(type, TransactionAsync))); } - private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols known) + private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols known, ServerVersion declaredMinVersion) { foreach (var block in context.OperationBlocks) { @@ -109,15 +112,26 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols foreach (var pair in usages) { - if (pair.Value.TryGetSuggestion(out var conditionName, out var operationName, out var suggestion, out var needsNewerServer)) - { - context.ReportDiagnostic(Diagnostic.Create( - needsNewerServer ? Diagnostics.PreferNewerAtomicOperation : Diagnostics.PreferConditionalArgument, + if (pair.Value.TryGetSuggestion() 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; + + context.ReportDiagnostic(found.NeedsNewerServer + ? Diagnostic.Create( + Diagnostics.PreferNewerAtomicOperation, pair.Value.ReportAt, - conditionName, - operationName, - suggestion)); - } + found.ConditionName, + found.OperationName, + found.Suggestion, + found.MinVersion.ToString()) + : Diagnostic.Create( + Diagnostics.PreferConditionalArgument, + pair.Value.ReportAt, + found.ConditionName, + found.OperationName, + found.Suggestion)); } } } @@ -140,6 +154,37 @@ private static bool IsInsideLoop(IOperation operation, IOperation block) return false; } + /// + /// A rewrite we are prepared to suggest, and what it needs. + /// + private readonly struct Rewrite + { + public Rewrite(string conditionName, string operationName, string suggestion, bool needsNewerServer, ServerVersion minVersion) + { + ConditionName = conditionName; + OperationName = operationName; + Suggestion = suggestion; + NeedsNewerServer = needsNewerServer; + MinVersion = minVersion; + } + + public string ConditionName { get; } + public string OperationName { get; } + + /// The suggested call, as shown to the user. + public string Suggestion { get; } + + /// Which rule this is: the version-dependent one, or the version-free one. + /// + /// Kept distinct from on purpose. This picks the diagnostic ID, and the ID is + /// about the *kind* of fix (move an argument vs adopt a newer command), which is what a consumer + /// configures severity on. The version is data about one mapping and may change as servers ship. + /// + public bool NeedsNewerServer { get; } + + public ServerVersion MinVersion { get; } + } + /// /// What we saw done with one transaction local. /// @@ -197,49 +242,55 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside } } - public bool TryGetSuggestion(out string conditionName, out string operationName, out string suggestion, out bool needsNewerServer) + public Rewrite? TryGetSuggestion() { - conditionName = operationName = suggestion = ""; - needsNewerServer = false; - - if (_disqualified) return false; + if (_disqualified) return null; // only the unambiguous shape: one guard, one operation, and the same key in both - if (_conditionCount != 1 || _operationCount != 1) return false; - if (_conditionFactory is null || _operationName is null) return false; - if (_conditionKey is null || _operationKey is null || _conditionKey != _operationKey) return false; - - if (Map(_conditionFactory, _operationName) is not { } mapped) return false; - - conditionName = "Condition." + _conditionFactory; - operationName = _operationName; - (suggestion, needsNewerServer) = mapped; - return true; + if (_conditionCount != 1 || _operationCount != 1) return null; + if (_conditionFactory is null || _operationName is null) return null; + if (_conditionKey is null || _operationKey is null || _conditionKey != _operationKey) return null; + + if (Map(_conditionFactory, _operationName) is not { } mapped) return null; + + return new Rewrite( + "Condition." + _conditionFactory, + _operationName, + mapped.Suggestion, + mapped.NeedsNewerServer, + mapped.MinVersion); } /// /// The condition/operation pairs that have an exact single-command equivalent. /// - private static (string Suggestion, bool NeedsNewerServer)? Map(string condition, string operation) + /// + /// 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 (string Suggestion, bool NeedsNewerServer, ServerVersion MinVersion)? Map(string condition, string operation) { var op = Trim(operation); return (condition, op) switch { // -- family A: the command already takes this condition as an argument; any server version -- - ("KeyNotExists", "StringSet") => ("StringSet(key, value, When.NotExists)", false), - ("KeyExists", "StringSet") => ("StringSet(key, value, When.Exists)", false), - ("HashNotExists", "HashSet") => ("HashSet(key, field, value, When.NotExists)", false), + ("KeyNotExists", "StringSet") => ("StringSet(key, value, When.NotExists)", false, ServerVersion.Any), + ("KeyExists", "StringSet") => ("StringSet(key, value, When.Exists)", false, ServerVersion.Any), + ("HashNotExists", "HashSet") => ("HashSet(key, field, value, When.NotExists)", false, ServerVersion.Any), // 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") => ("SortedSetAdd(key, member, score, SortedSetWhen.NotExists)", false), - ("SortedSetContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, SortedSetWhen.Exists)", false), - ("KeyNotExists", "KeyRename") => ("KeyRename(key, newKey, When.NotExists)", false), - - // -- family B: a newer single command subsumes condition and write (compare-and-set, 8.4+) -- - ("StringEqual", "StringSet") => ("StringSet(key, value, ValueCondition.Equal(expected))", true), - ("StringNotEqual", "StringSet") => ("StringSet(key, value, ValueCondition.NotEqual(expected))", true), - ("StringEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", true), - ("StringNotEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.NotEqual(expected))", true), + ("SortedSetNotContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, SortedSetWhen.NotExists)", false, ServerVersion.Any), + ("SortedSetContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, SortedSetWhen.Exists)", false, ServerVersion.Any), + ("KeyNotExists", "KeyRename") => ("KeyRename(key, newKey, When.NotExists)", false, ServerVersion.Any), + + // -- family B: a newer single command subsumes condition and write -- + // 8.4: SET IFEQ/IFNE and DELIFEQ; see RedisFeatures.SetWithValueCheck / DeleteWithValueCheck + ("StringEqual", "StringSet") => ("StringSet(key, value, ValueCondition.Equal(expected))", true, new ServerVersion(8, 4)), + ("StringNotEqual", "StringSet") => ("StringSet(key, value, ValueCondition.NotEqual(expected))", true, new ServerVersion(8, 4)), + ("StringEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", true, new ServerVersion(8, 4)), + ("StringNotEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.NotEqual(expected))", true, new ServerVersion(8, 4)), // 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 diff --git a/src/StackExchange.Redis/StackExchange.Redis.csproj b/src/StackExchange.Redis/StackExchange.Redis.csproj index 51d4ca902..6c7a2ac65 100644 --- a/src/StackExchange.Redis/StackExchange.Redis.csproj +++ b/src/StackExchange.Redis/StackExchange.Redis.csproj @@ -37,6 +37,9 @@ + + + diff --git a/src/StackExchange.Redis/build/StackExchange.Redis.props b/src/StackExchange.Redis/build/StackExchange.Redis.props new file mode 100644 index 000000000..3ca67ab32 --- /dev/null +++ b/src/StackExchange.Redis/build/StackExchange.Redis.props @@ -0,0 +1,29 @@ + + + + + + + + + $(RedisMinServerVersion) + + + diff --git a/tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs b/tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs new file mode 100644 index 000000000..1bed1bd9a --- /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(key, value, ValueCondition.Equal(expected))", + "8.4")); +} diff --git a/tests/StackExchange.Redis.Build.Tests/Verifier.cs b/tests/StackExchange.Redis.Build.Tests/Verifier.cs index 815af6b89..c7d37a44f 100644 --- a/tests/StackExchange.Redis.Build.Tests/Verifier.cs +++ b/tests/StackExchange.Redis.Build.Tests/Verifier.cs @@ -1,9 +1,11 @@ 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; @@ -34,7 +36,17 @@ protected static DiagnosticResult Diagnostic(string id, DiagnosticSeverity sever /// Verify that produces exactly . protected static Task VerifyAsync(string source, params DiagnosticResult[] expected) - => RunAsync(source, referenceLibrary: true, 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. @@ -45,9 +57,9 @@ protected static Task VerifyAsync(string source, params DiagnosticResult[] expec /// compilations, which have never heard of this library. /// protected static Task VerifyWithoutLibraryAsync(string source) - => RunAsync(source, referenceLibrary: false); + => RunAsync(source, referenceLibrary: false, minServerVersion: null); - private static Task RunAsync(string source, bool referenceLibrary, params DiagnosticResult[] expected) + 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 @@ -68,6 +80,14 @@ private static Task RunAsync(string source, bool referenceLibrary, params Diagno 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); } From ea6432cd0c19df5a13d95273b76c78abeeaac269 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 14:28:59 +0100 Subject: [PATCH 04/11] packaging; verifier arg validation; CI packing test --- .github/workflows/CI.yml | 22 ++++++++++++ eng/StackExchange.Redis.Build/Diagnostics.cs | 2 +- .../ServerVersion.cs | 4 ++- .../StackExchange.Redis.csproj | 32 +++++++++++++++++ .../build/StackExchange.Redis.props | 16 ++++----- .../DetectionShape.cs | 11 ++++-- .../StackExchange.Redis.Build.Tests/SER300.cs | 35 +++++++++++++++---- .../StackExchange.Redis.Build.Tests/SER301.cs | 24 ++++++++++--- 8 files changed, 123 insertions(+), 23 deletions(-) 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/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index a7d14d95b..be930e6ec 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -66,7 +66,7 @@ internal static class Diagnostics /// 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 - Redis_MinServerVersion, or + /// 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. /// /// diff --git a/eng/StackExchange.Redis.Build/ServerVersion.cs b/eng/StackExchange.Redis.Build/ServerVersion.cs index e8357d81e..faf584e11 100644 --- a/eng/StackExchange.Redis.Build/ServerVersion.cs +++ b/eng/StackExchange.Redis.Build/ServerVersion.cs @@ -57,7 +57,9 @@ public static ServerVersion FromOptions(AnalyzerOptions? options) return version; } - if (global.TryGetValue("build_property.Redis_MinServerVersion", out value) && TryParse(value, out 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; } diff --git a/src/StackExchange.Redis/StackExchange.Redis.csproj b/src/StackExchange.Redis/StackExchange.Redis.csproj index 6c7a2ac65..c7acece30 100644 --- a/src/StackExchange.Redis/StackExchange.Redis.csproj +++ b/src/StackExchange.Redis/StackExchange.Redis.csproj @@ -64,6 +64,38 @@ + + + + + + + + + + + + + + + MultiGroupDatabase.cs diff --git a/src/StackExchange.Redis/build/StackExchange.Redis.props b/src/StackExchange.Redis/build/StackExchange.Redis.props index 3ca67ab32..48ecadc11 100644 --- a/src/StackExchange.Redis/build/StackExchange.Redis.props +++ b/src/StackExchange.Redis/build/StackExchange.Redis.props @@ -14,16 +14,16 @@ The equivalent .editorconfig / .globalconfig spelling is `redis.min_server_version`, which takes precedence. Major.minor is what is read; anything more precise is accepted and ignored. - Note the property is deliberately renamed on the way through: MSBuild properties reach an analyzer as - `build_property.`, and CompilerVisibleProperty is what puts it there at all - without this file the - property would be silently ignored. + CompilerVisibleProperty is what makes the property readable at all - without it the analyzer simply never + sees the value. The property is surfaced under its own name rather than being copied to some internal one: + this file is imported *before* the consuming project's own PropertyGroup, so anything we read here at + evaluation time is still empty. The item declaration is safe because the value is read later, at compile + time, by which point the consumer's setting is in effect. + + It reaches the analyzer as `build_property.RedisMinServerVersion`. --> - + - - $(RedisMinServerVersion) - - diff --git a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs index c5f4f480d..94cccfd7c 100644 --- a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs +++ b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs @@ -195,6 +195,13 @@ public async Task M(IDatabase db, RedisKey a, RedisKey b) } } """, - Diagnostic("SER300").WithLocation(0), - Diagnostic("SER301").WithLocation(1)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet(key, value, When.NotExists)"), + Diagnostic("SER301").WithLocation(1).WithArguments( + "Condition.StringEqual", + "StringSetAsync", + "StringSet(key, value, ValueCondition.Equal(expected))", + "8.4")); } diff --git a/tests/StackExchange.Redis.Build.Tests/SER300.cs b/tests/StackExchange.Redis.Build.Tests/SER300.cs index 522934ab6..6a342ba6e 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER300.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER300.cs @@ -25,7 +25,10 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER300").WithLocation(0)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet(key, value, When.NotExists)")); [Fact] public Task KeyExistsGuardingStringSet_IsFlagged() => VerifyAsync( @@ -43,7 +46,10 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER300").WithLocation(0)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyExists", + "StringSetAsync", + "StringSet(key, value, When.Exists)")); [Fact] public Task HashNotExistsGuardingHashSet_IsFlagged() => VerifyAsync( @@ -61,7 +67,10 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER300").WithLocation(0)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.HashNotExists", + "HashSetAsync", + "HashSet(key, field, value, When.NotExists)")); [Fact] public Task SortedSetNotContainsGuardingSortedSetAdd_IsFlagged() => VerifyAsync( @@ -79,7 +88,10 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER300").WithLocation(0)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.SortedSetNotContains", + "SortedSetAddAsync", + "SortedSetAdd(key, member, score, SortedSetWhen.NotExists)")); [Fact] public Task SortedSetContainsGuardingSortedSetAdd_IsFlagged() => VerifyAsync( @@ -97,7 +109,10 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER300").WithLocation(0)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.SortedSetContains", + "SortedSetAddAsync", + "SortedSetAdd(key, member, score, SortedSetWhen.Exists)")); [Fact] // the condition is on the *destination*, which is KeyRename's first argument's counterpart - so this is @@ -117,7 +132,10 @@ public async Task M(IDatabase db, RedisKey key, RedisKey other) } } """, - Diagnostic("SER300").WithLocation(0)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "KeyRenameAsync", + "KeyRename(key, newKey, When.NotExists)")); [Fact] // synchronous surface: ITransaction is both IDatabaseAsync and the sync-shaped queueing API, and the @@ -136,7 +154,10 @@ public void M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER300").WithLocation(0)); + Diagnostic("SER300").WithLocation(0).WithArguments( + "Condition.KeyNotExists", + "StringSetAsync", + "StringSet(key, value, When.NotExists)")); [Fact] // HashExists + HashSet has no HSETXX to collapse into; the nearest thing is a different method entirely diff --git a/tests/StackExchange.Redis.Build.Tests/SER301.cs b/tests/StackExchange.Redis.Build.Tests/SER301.cs index 27b77f2a5..b153464b7 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER301.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER301.cs @@ -25,7 +25,11 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER301").WithLocation(0)); + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringEqual", + "StringSetAsync", + "StringSet(key, value, ValueCondition.Equal(expected))", + "8.4")); [Fact] public Task StringNotEqualGuardingStringSet_IsFlagged() => VerifyAsync( @@ -43,7 +47,11 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER301").WithLocation(0)); + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringNotEqual", + "StringSetAsync", + "StringSet(key, value, ValueCondition.NotEqual(expected))", + "8.4")); [Fact] // the canonical lock-release, and the highest-frequency real-world hit in this family @@ -62,7 +70,11 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER301").WithLocation(0)); + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringEqual", + "KeyDeleteAsync", + "StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", + "8.4")); [Fact] public Task StringNotEqualGuardingKeyDelete_IsFlagged() => VerifyAsync( @@ -80,7 +92,11 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER301").WithLocation(0)); + Diagnostic("SER301").WithLocation(0).WithArguments( + "Condition.StringNotEqual", + "KeyDeleteAsync", + "StringDelete(key, ValueCondition.NotEqual(expected))", + "8.4")); [Fact] // cross-key compare-and-set genuinely needs the transaction; must never fire From d2b6802992c744f383bcba4b999cb3e844be9e61 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 14:46:45 +0100 Subject: [PATCH 05/11] redundant conditions and compound operations --- docs/rules/SER302.md | 58 ++++ docs/rules/SER303.md | 70 ++++ docs/rules/index.md | 2 + .../AnalyzerReleases.Unshipped.md | 2 + eng/StackExchange.Redis.Build/Diagnostics.cs | 38 +++ .../TransactionAnalyzer.cs | 323 +++++++++++++++--- .../StackExchange.Redis.Build.Tests/SER300.cs | 37 ++ .../StackExchange.Redis.Build.Tests/SER302.cs | 194 +++++++++++ .../StackExchange.Redis.Build.Tests/SER303.cs | 259 ++++++++++++++ 9 files changed, 927 insertions(+), 56 deletions(-) create mode 100644 docs/rules/SER302.md create mode 100644 docs/rules/SER303.md create mode 100644 tests/StackExchange.Redis.Build.Tests/SER302.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/SER303.cs diff --git a/docs/rules/SER302.md b/docs/rules/SER302.md new file mode 100644 index 000000000..3f0661649 --- /dev/null +++ b/docs/rules/SER302.md @@ -0,0 +1,58 @@ +# SER302: transaction condition is 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. + +See also [Transactions](../Transactions). + +## Suppressing + +Reported as information; it never fails a build. + +```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..b8921ec1b --- /dev/null +++ b/docs/rules/SER303.md @@ -0,0 +1,70 @@ +# SER303: transaction can be replaced 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 | +| `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. + +## Order matters + +These commands return a value, so which way round the pair is queued is part of the meaning. `SET ... GET` +returns 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. + +`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. +- **Three or more queued commands**, and anything queued in a loop. + +See also [Transactions](../Transactions). + +## Suppressing + +Reported as information; it never fails a build. + +```xml +$(NoWarn);SER303 +``` + +or locally: + +```c# +#pragma warning disable SER303 +``` diff --git a/docs/rules/index.md b/docs/rules/index.md index 20a2ef173..f96088196 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -18,6 +18,8 @@ which mean "this API is preview", not "consider changing this code". - [SER300](SER300) - transaction can be replaced by a conditional argument (any server version) - [SER301](SER301) - transaction can be replaced by a single atomic operation (needs a newer server) +- [SER302](SER302) - condition is redundant; the command already reports whether it acted (any server version) +- [SER303](SER303) - two queued operations are a single compound command (varies by pair) ## Build diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md index 52e490bfe..7df6e36bb 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md @@ -10,4 +10,6 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- SER300 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a conditional argument (any server) SER301 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) +SER302 | Usage | Info | TransactionAnalyzer: condition is redundant; the queued command already reports whether it acted +SER303 | Usage | Info | TransactionAnalyzer: two queued operations are a single compound command SER350 | Build | Warning | AsciiHashGenerator: generated code requires a newer C# language version, so nothing was generated diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index be930e6ec..d584373f1 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -80,6 +80,44 @@ internal static class Diagnostics description: "A transaction implementing compare-and-set can 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 is redundant", + messageFormat: "This transaction ({0} guarding {1}) is redundant - use {2}", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "A condition that checks what the queued command already reports through its return value 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 can be replaced by a single compound command", + messageFormat: "These two queued operations ({0} then {1}) are one command: use {2}{3}", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "A transaction used only to make two operations atomic can be replaced by the single command that does both, which is one round-trip and cannot abort.", + helpLinkUri: HelpLink("SER303")); + /// /// The generated code cannot be compiled at the language version in effect, so nothing was generated. /// diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs index f175fe89a..c533f7231 100644 --- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -20,7 +20,11 @@ public sealed class TransactionAnalyzer : DiagnosticAnalyzer { /// public override ImmutableArray SupportedDiagnostics { get; } - = ImmutableArray.Create(Diagnostics.PreferConditionalArgument, Diagnostics.PreferNewerAtomicOperation); + = ImmutableArray.Create( + Diagnostics.PreferConditionalArgument, + Diagnostics.PreferNewerAtomicOperation, + Diagnostics.RedundantCondition, + Diagnostics.PreferCompoundCommand); /// public override void Initialize(AnalysisContext context) @@ -118,20 +122,41 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols // server - so if the project has told us its floor, respect it. Unset shows everything. if (!found.MinVersion.IsSatisfiedBy(declaredMinVersion)) continue; - context.ReportDiagnostic(found.NeedsNewerServer - ? Diagnostic.Create( + var location = pair.Value.LocationFor(found.Rule); + context.ReportDiagnostic(found.Rule switch + { + Rule.NewerAtomicOperation => Diagnostic.Create( Diagnostics.PreferNewerAtomicOperation, - pair.Value.ReportAt, - found.ConditionName, - found.OperationName, + 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.CompoundCommand => Diagnostic.Create( + Diagnostics.PreferCompoundCommand, + location, + found.First, + found.Second, found.Suggestion, - found.MinVersion.ToString()) - : Diagnostic.Create( + found.MinVersion.IsSpecified ? " (requires server " + found.MinVersion + " or later)" : ""), + + _ => Diagnostic.Create( Diagnostics.PreferConditionalArgument, - pair.Value.ReportAt, - found.ConditionName, - found.OperationName, - found.Suggestion)); + location, + found.First, + found.Second, + found.Suggestion), + }); } } } @@ -154,35 +179,94 @@ private static bool IsInsideLoop(IOperation operation, IOperation block) return false; } + /// + /// 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, + } + /// /// A rewrite we are prepared to suggest, and what it needs. /// private readonly struct Rewrite { - public Rewrite(string conditionName, string operationName, string suggestion, bool needsNewerServer, ServerVersion minVersion) + public Rewrite(Rule rule, string first, string second, string suggestion, ServerVersion minVersion) { - ConditionName = conditionName; - OperationName = operationName; + Rule = rule; + First = first; + Second = second; Suggestion = suggestion; - NeedsNewerServer = needsNewerServer; MinVersion = minVersion; } - public string ConditionName { get; } - public string OperationName { get; } + public Rule Rule { get; } + + /// The condition, or for the first queued operation. + public string First { get; } + + /// The queued operation, or for the second one. + public string Second { get; } /// The suggested call, as shown to the user. public string Suggestion { get; } - /// Which rule this is: the version-dependent one, or the version-free one. + 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) + { + DisplayName = name; + Name = Trim(name); + Key = key; + Member = member; + } + + /// 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. + /// /// - /// Kept distinct from on purpose. This picks the diagnostic ID, and the ID is - /// about the *kind* of fix (move an argument vs adopt a newer command), which is what a consumer - /// configures severity on. The version is data about one mapping and may change as servers ship. + /// 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 bool NeedsNewerServer { get; } + public string DisplayName { get; } - public ServerVersion MinVersion { 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; } } /// @@ -190,12 +274,24 @@ public Rewrite(string conditionName, string operationName, string suggestion, bo /// private sealed class Usage { - private int _conditionCount, _operationCount; - private string? _conditionFactory, _conditionKey; - private string? _operationName, _operationKey; + /// + /// Beyond this many queued commands nothing here can apply, so stop recording and stay quiet. + /// + /// The largest shape we map is family D's pair; a third command rules everything out. + private const int MaxInterestingOperations = 3; + + private readonly List _operations = new(MaxInterestingOperations); + private int _conditionCount; + private string? _conditionFactory, _conditionKey, _conditionMember; private bool _disqualified; + private Location? _condition, _firstOperation; - public Location? ReportAt { get; private set; } + /// + /// 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 == Rule.CompoundCommand ? _firstOperation : _condition; /// /// Something about this usage puts it beyond what we can reason about; stay silent regardless of counts. @@ -214,7 +310,7 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside { case "AddCondition": _conditionCount++; - ReportAt ??= invocation.Syntax.GetLocation(); + _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 @@ -224,7 +320,8 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside && SymbolEqualityComparer.Default.Equals(factory.TargetMethod.ContainingType, known.Condition)) { _conditionFactory = factory.TargetMethod.Name; - _conditionKey = FirstArgumentText(factory); + _conditionKey = ArgumentText(factory, 0); + _conditionMember = ArgumentText(factory, 1); } break; @@ -235,9 +332,19 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside default: // everything else queued on the transaction is a redis operation - _operationCount++; - _operationName = invocation.TargetMethod.Name; - _operationKey = FirstArgumentText(invocation); + _firstOperation ??= invocation.Syntax.GetLocation(); + if (_operations.Count < MaxInterestingOperations) + { + _operations.Add(new QueuedOperation( + invocation.TargetMethod.Name, + ArgumentText(invocation, 0), + ArgumentText(invocation, 1))); + } + else + { + Disqualify(); + } + break; } } @@ -246,21 +353,50 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside { if (_disqualified) return null; - // only the unambiguous shape: one guard, one operation, and the same key in both - if (_conditionCount != 1 || _operationCount != 1) return null; - if (_conditionFactory is null || _operationName is null) return null; - if (_conditionKey is null || _operationKey is null || _conditionKey != _operationKey) return null; + return _conditionCount switch + { + // families A, B and C: one guard over one command + 1 when _operations.Count == 1 => TryGuardedOperation(_operations[0]), + + // family D: no guard at all, just two commands queued for atomicity + 0 when _operations.Count == 2 => TryCommandPair(_operations[0], _operations[1]), - if (Map(_conditionFactory, _operationName) is not { } mapped) return null; + _ => null, + }; + } + + 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; + } return new Rewrite( + mapped.Rule, "Condition." + _conditionFactory, - _operationName, + operation.DisplayName, mapped.Suggestion, - mapped.NeedsNewerServer, mapped.MinVersion); } + private static Rewrite? TryCommandPair(QueuedOperation first, QueuedOperation second) + { + if (MapPair(first, second) is not { } mapped) return null; + return new Rewrite(Rule.CompoundCommand, first.DisplayName, second.DisplayName, mapped.Suggestion, mapped.MinVersion); + } + /// /// The condition/operation pairs that have an exact single-command equivalent. /// @@ -270,27 +406,43 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside /// (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 (string Suggestion, bool NeedsNewerServer, ServerVersion MinVersion)? Map(string condition, string operation) + private static (Rule Rule, string Suggestion, ServerVersion MinVersion, bool SameMember)? Map(string condition, string operation) { var op = Trim(operation); return (condition, op) switch { // -- family A: the command already takes this condition as an argument; any server version -- - ("KeyNotExists", "StringSet") => ("StringSet(key, value, When.NotExists)", false, ServerVersion.Any), - ("KeyExists", "StringSet") => ("StringSet(key, value, When.Exists)", false, ServerVersion.Any), - ("HashNotExists", "HashSet") => ("HashSet(key, field, value, When.NotExists)", false, ServerVersion.Any), + ("KeyNotExists", "StringSet") => (Rule.ConditionalArgument, "StringSet(key, value, When.NotExists)", ServerVersion.Any, false), + ("KeyExists", "StringSet") => (Rule.ConditionalArgument, "StringSet(key, value, When.Exists)", ServerVersion.Any, false), + ("HashNotExists", "HashSet") => (Rule.ConditionalArgument, "HashSet(key, field, value, When.NotExists)", ServerVersion.Any, true), // 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") => ("SortedSetAdd(key, member, score, SortedSetWhen.NotExists)", false, ServerVersion.Any), - ("SortedSetContains", "SortedSetAdd") => ("SortedSetAdd(key, member, score, SortedSetWhen.Exists)", false, ServerVersion.Any), - ("KeyNotExists", "KeyRename") => ("KeyRename(key, newKey, When.NotExists)", false, ServerVersion.Any), + ("SortedSetNotContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd(key, member, score, SortedSetWhen.NotExists)", ServerVersion.Any, true), + ("SortedSetContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd(key, member, score, SortedSetWhen.Exists)", ServerVersion.Any, true), + ("KeyNotExists", "KeyRename") => (Rule.ConditionalArgument, "KeyRename(key, newKey, When.NotExists)", ServerVersion.Any, false), // -- family B: a newer single command subsumes condition and write -- // 8.4: SET IFEQ/IFNE and DELIFEQ; see RedisFeatures.SetWithValueCheck / DeleteWithValueCheck - ("StringEqual", "StringSet") => ("StringSet(key, value, ValueCondition.Equal(expected))", true, new ServerVersion(8, 4)), - ("StringNotEqual", "StringSet") => ("StringSet(key, value, ValueCondition.NotEqual(expected))", true, new ServerVersion(8, 4)), - ("StringEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", true, new ServerVersion(8, 4)), - ("StringNotEqual", "KeyDelete") => ("StringDelete(key, ValueCondition.NotEqual(expected))", true, new ServerVersion(8, 4)), + ("StringEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet(key, value, ValueCondition.Equal(expected))", new ServerVersion(8, 4), false), + ("StringNotEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet(key, value, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), + ("StringEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", new ServerVersion(8, 4), false), + ("StringNotEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete(key, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), + + // -- 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"). + ("SetNotContains", "SetAdd") => (Rule.RedundantCondition, "SetAdd(key, value), which returns false if the member was already there", ServerVersion.Any, true), + ("SetContains", "SetRemove") => (Rule.RedundantCondition, "SetRemove(key, value), which returns false if the member was not there", ServerVersion.Any, true), + ("SortedSetContains", "SortedSetRemove") => (Rule.RedundantCondition, "SortedSetRemove(key, member), which returns false if the member was not there", ServerVersion.Any, true), + ("HashExists", "HashDelete") => (Rule.RedundantCondition, "HashDelete(key, field), which returns false if the field was not there", ServerVersion.Any, true), + ("KeyExists", "KeyDelete") => (Rule.RedundantCondition, "KeyDelete(key), which returns false if the key did not exist", ServerVersion.Any, false), + ("KeyExists", "KeyExpire") => (Rule.RedundantCondition, "KeyExpire(key, expiry), which returns false if the key did not exist", ServerVersion.Any, false), + + // 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 @@ -301,12 +453,71 @@ private static (string Suggestion, bool NeedsNewerServer, ServerVersion MinVersi _ => null, }; - static string Trim(string name) - => name.EndsWith("Async", StringComparison.Ordinal) ? name.Substring(0, name.Length - 5) : name; } /// - /// The source text of the first argument, used as a cheap "same key?" test. + /// 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)? 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(key)", v6_2); + case ("StringGet", "KeyExpire"): + return ("StringGetSetExpiry(key, expiry)", v6_2); + case ("StringGet", "KeyPersist"): + return ("StringGetSetExpiry(key, null)", v6_2); + case ("StringGet", "StringSet"): + return ("StringSetAndGet(key, value)", v6_2); + + // HGETDEL is 8.0; it has no RedisFeatures gate to point at + case ("HashGet", "HashDelete") when SameMember(first, second): + return ("HashFieldGetAndDelete(key, field)", new ServerVersion(8, 0)); + } + + 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(source, destination, value)", ServerVersion.Any); + } + + 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, @@ -314,8 +525,8 @@ static string Trim(string name) /// 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 string? FirstArgumentText(IInvocationOperation invocation) - => invocation.Arguments.Length == 0 ? null : invocation.Arguments[0].Value.Syntax.ToString(); + private static string? ArgumentText(IInvocationOperation invocation, int index) + => invocation.Arguments.Length <= index ? null : invocation.Arguments[index].Value.Syntax.ToString(); private static IOperation Unwrap(IOperation operation) { diff --git a/tests/StackExchange.Redis.Build.Tests/SER300.cs b/tests/StackExchange.Redis.Build.Tests/SER300.cs index 6a342ba6e..2332202c8 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER300.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER300.cs @@ -159,6 +159,43 @@ public void M(IDatabase db, RedisKey key) "StringSetAsync", "StringSet(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 diff --git a/tests/StackExchange.Redis.Build.Tests/SER302.cs b/tests/StackExchange.Redis.Build.Tests/SER302.cs new file mode 100644 index 000000000..6e623266c --- /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(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(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(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(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(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(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..0084b0daf --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER303.cs @@ -0,0 +1,259 @@ +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(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(key, expiry)", + " (requires server 6.2 or later)")); + + [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(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(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(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(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(); + } + } + """); +} From 83273dc2db1040d0138a9d423e350c3e5af32d99 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 14:54:37 +0100 Subject: [PATCH 06/11] build cleanup --- .../AnalyzerReleases.Shipped.md | 16 +++- .../AnalyzerReleases.Unshipped.md | 13 +--- .../AsciiHashGenerator.cs | 78 ++++++++++--------- 3 files changed, 60 insertions(+), 47 deletions(-) diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md index b4de231a4..0ab65a3b2 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md @@ -1,2 +1,16 @@ ; Shipped analyzer releases; see AnalyzerReleases.Unshipped.md for the convention. -; Nothing has shipped yet - move rules here when a release goes out. +; 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 | Info | TransactionAnalyzer: transaction can be replaced by a conditional argument (any server) +SER301 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) +SER302 | Usage | Info | TransactionAnalyzer: condition is redundant; the queued command already reports whether it acted +SER303 | Usage | Info | TransactionAnalyzer: two queued operations are a single compound command +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 index 7df6e36bb..b9d09f18c 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md @@ -3,13 +3,6 @@ ; 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 - -### New Rules - -Rule ID | Category | Severity | Notes ---------|----------|----------|------- -SER300 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a conditional argument (any server) -SER301 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) -SER302 | Usage | Info | TransactionAnalyzer: condition is redundant; the queued command already reports whether it acted -SER303 | Usage | Info | TransactionAnalyzer: two queued operations are a single compound command -SER350 | Build | Warning | AsciiHashGenerator: generated code requires a newer C# language version, so nothing was generated +; +; 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 9b97fa913..5b97d6af6 100644 --- a/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs +++ b/eng/StackExchange.Redis.Build/AsciiHashGenerator.cs @@ -17,39 +17,52 @@ public class AsciiHashGenerator : IIncrementalGenerator /// 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(); @@ -90,19 +103,6 @@ public void Initialize(IncrementalGeneratorInitializationContext context) 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 (attrib.Name.ToString() is nameof(AsciiHashAttribute) or nameof(AsciiHash)) return true; - } - } - - return false; - } } private static string GetName(INamedTypeSymbol type) @@ -148,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; @@ -184,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) @@ -229,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, @@ -246,7 +250,9 @@ 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); @@ -355,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, @@ -372,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); From 9f437a6dabd674f7a47e41b5ef579de615b56993 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 15:12:29 +0100 Subject: [PATCH 07/11] final category --- docs/rules/SER303.md | 5 +- docs/rules/SER304.md | 84 +++++ docs/rules/index.md | 1 + .../AnalyzerReleases.Shipped.md | 1 + eng/StackExchange.Redis.Build/Diagnostics.cs | 19 + .../TransactionAnalyzer.cs | 258 +++++++++++-- .../DetectionShape.cs | 67 ++++ .../StackExchange.Redis.Build.Tests/SER304.cs | 344 ++++++++++++++++++ 8 files changed, 754 insertions(+), 25 deletions(-) create mode 100644 docs/rules/SER304.md create mode 100644 tests/StackExchange.Redis.Build.Tests/SER304.cs diff --git a/docs/rules/SER303.md b/docs/rules/SER303.md index b8921ec1b..a75acf318 100644 --- a/docs/rules/SER303.md +++ b/docs/rules/SER303.md @@ -51,7 +51,10 @@ spelling is flagged. 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. -- **Three or more queued commands**, and anything queued in a loop. +- **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. See also [Transactions](../Transactions). diff --git a/docs/rules/SER304.md b/docs/rules/SER304.md new file mode 100644 index 000000000..28062f553 --- /dev/null +++ b/docs/rules/SER304.md @@ -0,0 +1,84 @@ +# SER304: repeated queued operations can use 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. + +## Suppressing + +Reported as information; it never fails a build. + +```xml +$(NoWarn);SER304 +``` + +or locally: + +```c# +#pragma warning disable SER304 +``` diff --git a/docs/rules/index.md b/docs/rules/index.md index f96088196..afd7ae936 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -20,6 +20,7 @@ which mean "this API is preview", not "consider changing this code". - [SER301](SER301) - transaction can be replaced by a single atomic operation (needs a newer server) - [SER302](SER302) - condition is redundant; the command already reports whether it acted (any server version) - [SER303](SER303) - two queued operations are a single compound command (varies by pair) +- [SER304](SER304) - the same operation queued repeatedly can use the variadic overload (mostly any server) ## Build diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md index 0ab65a3b2..e07b259fe 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md @@ -13,4 +13,5 @@ SER300 | Usage | Info | TransactionAnalyzer: transaction can be replaced SER301 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) SER302 | Usage | Info | TransactionAnalyzer: condition is redundant; the queued command already reports whether it acted SER303 | Usage | Info | TransactionAnalyzer: two queued operations are a single compound command +SER304 | Usage | Info | TransactionAnalyzer: repeated queued operations can use 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/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index d584373f1..aedda7280 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -118,6 +118,25 @@ internal static class Diagnostics description: "A transaction used only to make two operations atomic can 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 can use the variadic overload", + messageFormat: "These {1} queued {0} calls are one command: use {2}{3}", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "The same command queued several times over can 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. /// diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs index c533f7231..2f21f113b 100644 --- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -1,4 +1,5 @@ using System.Collections.Immutable; +using System.Globalization; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations; @@ -9,11 +10,17 @@ namespace StackExchange.Redis.Build; /// Spots ITransaction/ITransactionAsync usage that a single conditional command does better. /// /// -/// Deliberately conservative. It only fires on the unambiguous shape - exactly one condition guarding exactly -/// one queued operation, on a syntactically identical key - because this ships to every consumer of the -/// package, and a false positive on correct code is worse than staying quiet. Anything cleverer (several -/// operations, a condition on a different key, a transaction whose result feeds back into control flow) is -/// left alone on purpose: partial inference that works inconsistently would be more confusing than none. +/// +/// 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 @@ -24,7 +31,8 @@ public sealed class TransactionAnalyzer : DiagnosticAnalyzer Diagnostics.PreferConditionalArgument, Diagnostics.PreferNewerAtomicOperation, Diagnostics.RedundantCondition, - Diagnostics.PreferCompoundCommand); + Diagnostics.PreferCompoundCommand, + Diagnostics.PreferVariadicOverload); /// public override void Initialize(AnalysisContext context) @@ -83,6 +91,20 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols // one pass, gathering per-transaction-local usage; most blocks contain nothing and fall straight out Dictionary? usages = null; + // 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. + HashSet? reassignedLocals = null; + + foreach (var operation in block.Descendants()) + { + if (LocalWrittenBy(operation) is { } written) + { + reassignedLocals ??= new HashSet(SymbolEqualityComparer.Default); + reassignedLocals.Add(written); + } + } + foreach (var operation in block.Descendants()) { // the transaction is identified by the local it was assigned to; anything else (a field, a @@ -116,7 +138,7 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols foreach (var pair in usages) { - if (pair.Value.TryGetSuggestion() is not { } found) continue; + 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. @@ -142,13 +164,21 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols // 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, - found.MinVersion.IsSpecified ? " (requires server " + found.MinVersion + " or later)" : ""), + VersionClause(found.MinVersion)), _ => Diagnostic.Create( Diagnostics.PreferConditionalArgument, @@ -161,6 +191,33 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols } } + /// + /// 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)" : ""; + /// /// Is this call inside a loop, and so potentially queueing many commands from one call site? /// @@ -200,6 +257,9 @@ private enum Rule /// 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, } /// @@ -218,10 +278,16 @@ public Rewrite(Rule rule, string first, string second, string suggestion, Server public Rule Rule { get; } - /// The condition, or for the first queued operation. + /// + /// The condition; for the first queued operation, and for + /// the operation that was repeated. + /// public string First { get; } - /// The queued operation, or for the second one. + /// + /// 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. @@ -242,12 +308,13 @@ private static string Trim(string name) /// private readonly struct QueuedOperation { - public QueuedOperation(string name, string? key, string? member) + public QueuedOperation(string name, string? key, string? member, List? reads) { DisplayName = name; Name = Trim(name); Key = key; Member = member; + Reads = reads; } /// The method name with any Async suffix removed, for matching against the tables. @@ -267,6 +334,11 @@ public QueuedOperation(string name, string? key, string? member) /// 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; } } /// @@ -275,14 +347,20 @@ public QueuedOperation(string name, string? key, string? member) private sealed class Usage { /// - /// Beyond this many queued commands nothing here can apply, so stop recording and stay quiet. + /// Beyond this many queued commands, stop recording and stay quiet. /// - /// The largest shape we map is family D's pair; a third command rules everything out. - private const int MaxInterestingOperations = 3; + /// + /// 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(MaxInterestingOperations); + private readonly List _operations = new(); private int _conditionCount; private string? _conditionFactory, _conditionKey, _conditionMember; + private List? _conditionReads; private bool _disqualified; private Location? _condition, _firstOperation; @@ -291,7 +369,7 @@ private sealed class Usage /// but family D has no condition at all, so its report goes on the first queued command. /// public Location? LocationFor(Rule rule) - => rule == Rule.CompoundCommand ? _firstOperation : _condition; + => 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. @@ -322,6 +400,7 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside _conditionFactory = factory.TargetMethod.Name; _conditionKey = ArgumentText(factory, 0); _conditionMember = ArgumentText(factory, 1); + _conditionReads = LocalsRead(factory); } break; @@ -338,7 +417,8 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside _operations.Add(new QueuedOperation( invocation.TargetMethod.Name, ArgumentText(invocation, 0), - ArgumentText(invocation, 1))); + ArgumentText(invocation, 1), + LocalsRead(invocation))); } else { @@ -349,20 +429,50 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside } } - public Rewrite? TryGetSuggestion() + public Rewrite? TryGetSuggestion(HashSet? reassignedLocals) { if (_disqualified) return null; - return _conditionCount switch + // 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 - 1 when _operations.Count == 1 => TryGuardedOperation(_operations[0]), + return TryGuardedOperation(_operations[0]); + } - // family D: no guard at all, just two commands queued for atomicity - 0 when _operations.Count == 2 => TryCommandPair(_operations[0], _operations[1]), + if (_conditionCount != 0 || _operations.Count < 2) return null; - _ => 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) @@ -397,6 +507,89 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside 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 (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 arrived in 2.4, which + /// predates anything anyone is running and well predates the oldest server this library supports, so + /// saying so would be noise. SMISMEMBER is the exception at 6.2 - recent enough to matter. + /// + /// + /// 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)? MapVariadic(string operation) + => operation switch + { + // one key, many values + "SetAdd" => ("SetAdd(key, values)", false, true, ServerVersion.Any), + "SetRemove" => ("SetRemove(key, values)", false, true, ServerVersion.Any), + "SortedSetAdd" => ("SortedSetAdd(key, entries)", false, true, ServerVersion.Any), + "SortedSetRemove" => ("SortedSetRemove(key, members)", false, true, ServerVersion.Any), + "HashSet" => ("HashSet(key, entries)", false, true, ServerVersion.Any), + "HashDelete" => ("HashDelete(key, fields)", false, true, ServerVersion.Any), + "ListLeftPush" => ("ListLeftPush(key, values)", false, true, ServerVersion.Any), + "ListRightPush" => ("ListRightPush(key, values)", false, true, ServerVersion.Any), + + // SMISMEMBER, which unlike the rest of these is recent; it has no RedisFeatures gate to cite + "SetContains" => ("SetContains(key, values), which returns a bool per value", false, true, new ServerVersion(6, 2)), + + // many keys + "KeyDelete" => ("KeyDelete(keys)", true, false, ServerVersion.Any), + "KeyExists" => ("KeyExists(keys), which returns how many exist", true, false, ServerVersion.Any), + "StringGet" => ("StringGet(keys)", true, false, ServerVersion.Any), + "StringSet" => ("StringSet(KeyValuePair[])", true, false, ServerVersion.Any), + + _ => null, + }; + /// /// The condition/operation pairs that have an exact single-command equivalent. /// @@ -525,6 +718,23 @@ static bool SameMember(QueuedOperation a, QueuedOperation b) /// 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; + } + private static string? ArgumentText(IInvocationOperation invocation, int index) => invocation.Arguments.Length <= index ? null : invocation.Arguments[index].Value.Syntax.ToString(); diff --git a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs index 94cccfd7c..2eac7ceca 100644 --- a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs +++ b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs @@ -173,6 +173,73 @@ public async Task M(IDatabase db) } """); + [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(key, value, When.NotExists)")); + [Fact] // two independent transactions in one method must be tracked separately, not pooled into one set of counts public Task TwoIndependentTransactions_AreFlaggedIndependently() => VerifyAsync( diff --git a/tests/StackExchange.Redis.Build.Tests/SER304.cs b/tests/StackExchange.Redis.Build.Tests/SER304.cs new file mode 100644 index 000000000..87714ddd6 --- /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(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(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(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(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(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(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(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(); + } + } + """); +} From ec89c736c8f45ebe76760ddf8f73610ffc16a846 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 15:15:12 +0100 Subject: [PATCH 08/11] update transaction docs --- docs/Transactions.md | 32 +++++++++++++++++++ .../TransactionAnalyzer.cs | 7 ++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/docs/Transactions.md b/docs/Transactions.md index 4d8deca27..0d72d3c09 100644 --- a/docs/Transactions.md +++ b/docs/Transactions.md @@ -116,3 +116,35 @@ 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 suggestions +(information severity - they never fail a build). 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/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs index 2f21f113b..6e9aab7dd 100644 --- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -555,9 +555,10 @@ static bool Contains(List? reads, HashSet reassigned) /// /// /// - /// Versions are for all but one: the variadic forms arrived in 2.4, which - /// predates anything anyone is running and well predates the oldest server this library supports, so - /// saying so would be noise. SMISMEMBER is the exception at 6.2 - recent enough to matter. + /// 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 From 8bf545bfeb8341aa0c95eb0f24abec62a44ecdad Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 15:32:06 +0100 Subject: [PATCH 09/11] upgrading to "warning"; "information" is too invisible --- docs/Transactions.md | 3 +- docs/rules/SER300.md | 4 +-- docs/rules/SER301.md | 3 +- docs/rules/SER302.md | 2 +- docs/rules/SER303.md | 2 +- docs/rules/SER304.md | 2 +- docs/rules/index.md | 35 +++++++++++++++---- .../AnalyzerReleases.Shipped.md | 10 +++--- eng/StackExchange.Redis.Build/Diagnostics.cs | 24 ++++++++----- .../Verifier.cs | 6 +++- .../ConstraintsTests.cs | 6 ++++ .../RetryTests/RetryEndToEndTests.cs | 5 +++ 12 files changed, 73 insertions(+), 29 deletions(-) diff --git a/docs/Transactions.md b/docs/Transactions.md index 0d72d3c09..7f78db126 100644 --- a/docs/Transactions.md +++ b/docs/Transactions.md @@ -136,8 +136,7 @@ if (await tran.ExecuteAsync()) { /* ... */ } 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 suggestions -(information severity - they never fail a build). It covers conditions that duplicate a `when:` argument, +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. diff --git a/docs/rules/SER300.md b/docs/rules/SER300.md index be31aeec0..da1ccb16a 100644 --- a/docs/rules/SER300.md +++ b/docs/rules/SER300.md @@ -47,8 +47,8 @@ See also [Transactions](../Transactions). ## Suppressing -The flagged code is correct, just not optimal, so this is reported as information and never fails a build. To -silence it anyway: +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 diff --git a/docs/rules/SER301.md b/docs/rules/SER301.md index c8df025bb..7b3452557 100644 --- a/docs/rules/SER301.md +++ b/docs/rules/SER301.md @@ -70,7 +70,8 @@ fields or list indices, so `HashEqual` and `ListIndexEqual` are left alone. ## Suppressing -Reported as information; it never fails a build. To silence: +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. To +silence: ```xml $(NoWarn);SER301 diff --git a/docs/rules/SER302.md b/docs/rules/SER302.md index 3f0661649..7e31104b2 100644 --- a/docs/rules/SER302.md +++ b/docs/rules/SER302.md @@ -45,7 +45,7 @@ See also [Transactions](../Transactions). ## Suppressing -Reported as information; it never fails a build. +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. ```xml $(NoWarn);SER302 diff --git a/docs/rules/SER303.md b/docs/rules/SER303.md index a75acf318..ddf94b2c7 100644 --- a/docs/rules/SER303.md +++ b/docs/rules/SER303.md @@ -60,7 +60,7 @@ See also [Transactions](../Transactions). ## Suppressing -Reported as information; it never fails a build. +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. ```xml $(NoWarn);SER303 diff --git a/docs/rules/SER304.md b/docs/rules/SER304.md index 28062f553..b7f5f9aea 100644 --- a/docs/rules/SER304.md +++ b/docs/rules/SER304.md @@ -71,7 +71,7 @@ suggestion is not for you - suppress it. ## Suppressing -Reported as information; it never fails a build. +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. ```xml $(NoWarn);SER304 diff --git a/docs/rules/index.md b/docs/rules/index.md index afd7ae936..6b3db28c5 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -8,7 +8,7 @@ separately: | Range | Meaning | |---|---| -| `SER300`-`SER349` | usage guidance about your code (reported as *information*; never fails a build) | +| `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), @@ -46,13 +46,34 @@ 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. -## Why these are only information +## Severity, and turning it down -The code these rules flag is correct - it works, and it will keep working. They point at a form that is a single -round-trip instead of two and cannot abort under contention. Shipping them as warnings would break every -consumer building with `TreatWarningsAsErrors`, so they are informational by default; raise the severity in -`.editorconfig` if you want them enforced: +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 = warning +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 index e07b259fe..cf19e9a0b 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md @@ -9,9 +9,9 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- -SER300 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a conditional argument (any server) -SER301 | Usage | Info | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) -SER302 | Usage | Info | TransactionAnalyzer: condition is redundant; the queued command already reports whether it acted -SER303 | Usage | Info | TransactionAnalyzer: two queued operations are a single compound command -SER304 | Usage | Info | TransactionAnalyzer: repeated queued operations can use the variadic overload +SER300 | Usage | Warning | TransactionAnalyzer: transaction can be replaced by a conditional argument (any server) +SER301 | Usage | Warning | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) +SER302 | Usage | Warning | TransactionAnalyzer: condition is redundant; the queued command already reports whether it acted +SER303 | Usage | Warning | TransactionAnalyzer: two queued operations are a single compound command +SER304 | Usage | Warning | TransactionAnalyzer: repeated queued operations can use 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/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index aedda7280..a33a8f25e 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -17,9 +17,17 @@ namespace StackExchange.Redis.Build; /// /// /// These are a public contract: once shipped, an ID cannot be reused or re-pointed, because consumers put -/// them in NoWarn and .editorconfig. Analyzer rules default to - the code they flag is correct, just not optimal, and a shipped warning -/// would break builds that set TreatWarningsAsErrors. +/// 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. /// /// internal static class Diagnostics @@ -50,7 +58,7 @@ internal static class Diagnostics title: "Transaction can be replaced by a conditional argument", messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2} - the condition duplicates an argument the command already has", category: UsageCategory, - defaultSeverity: DiagnosticSeverity.Info, + defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "A transaction whose only purpose is to make one operation conditional can be replaced by the command's own conditional argument, which is a single round-trip and cannot abort under contention.", helpLinkUri: HelpLink("SER300")); @@ -75,7 +83,7 @@ internal static class Diagnostics title: "Transaction can be replaced by a single atomic operation", messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2}, which is atomic on the server and needs no WATCH (requires server {3} or later)", category: UsageCategory, - defaultSeverity: DiagnosticSeverity.Info, + defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "A transaction implementing compare-and-set can 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")); @@ -95,7 +103,7 @@ internal static class Diagnostics title: "Transaction condition is redundant", messageFormat: "This transaction ({0} guarding {1}) is redundant - use {2}", category: UsageCategory, - defaultSeverity: DiagnosticSeverity.Info, + defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "A condition that checks what the queued command already reports through its return value buys nothing: the transaction costs an extra round-trip and can abort, and the command alone says whether it acted.", helpLinkUri: HelpLink("SER302")); @@ -113,7 +121,7 @@ internal static class Diagnostics title: "Transaction can be replaced by a single compound command", messageFormat: "These two queued operations ({0} then {1}) are one command: use {2}{3}", category: UsageCategory, - defaultSeverity: DiagnosticSeverity.Info, + defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "A transaction used only to make two operations atomic can be replaced by the single command that does both, which is one round-trip and cannot abort.", helpLinkUri: HelpLink("SER303")); @@ -132,7 +140,7 @@ internal static class Diagnostics title: "Repeated queued operations can use the variadic overload", messageFormat: "These {1} queued {0} calls are one command: use {2}{3}", category: UsageCategory, - defaultSeverity: DiagnosticSeverity.Info, + defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "The same command queued several times over can be a single variadic call, which is one round-trip and needs no transaction to be atomic.", helpLinkUri: HelpLink("SER304")); diff --git a/tests/StackExchange.Redis.Build.Tests/Verifier.cs b/tests/StackExchange.Redis.Build.Tests/Verifier.cs index c7d37a44f..c18d74020 100644 --- a/tests/StackExchange.Redis.Build.Tests/Verifier.cs +++ b/tests/StackExchange.Redis.Build.Tests/Verifier.cs @@ -31,7 +31,11 @@ public abstract class Verifier Path.Combine("ref", "net10.0")); /// Expect a diagnostic with this id at the marked location. - protected static DiagnosticResult Diagnostic(string id, DiagnosticSeverity severity = DiagnosticSeverity.Info) + /// + /// 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 . 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] From a40016e361bdc7072c10cfe3331d2446ebb2cf2e Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 5 Aug 2026 15:50:38 +0100 Subject: [PATCH 10/11] advertise Foo[Async] instead of just Foo; handle ITransactionAsync --- docs/rules/index.md | 14 ++++ .../TransactionAnalyzer.cs | 70 +++++++++---------- .../DetectionShape.cs | 6 +- .../MinServerVersion.cs | 2 +- .../StackExchange.Redis.Build.Tests/SER300.cs | 41 +++++++++-- .../StackExchange.Redis.Build.Tests/SER301.cs | 8 +-- .../StackExchange.Redis.Build.Tests/SER302.cs | 12 ++-- .../StackExchange.Redis.Build.Tests/SER303.cs | 12 ++-- .../StackExchange.Redis.Build.Tests/SER304.cs | 14 ++-- 9 files changed, 110 insertions(+), 69 deletions(-) diff --git a/docs/rules/index.md b/docs/rules/index.md index 6b3db28c5..81e74af15 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -14,6 +14,20 @@ separately: 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 can be replaced by a conditional argument (any server version) diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs index 6e9aab7dd..e89c459c4 100644 --- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -570,23 +570,23 @@ private static (string Suggestion, bool ManyKeys, bool RequiresMember, ServerVer => operation switch { // one key, many values - "SetAdd" => ("SetAdd(key, values)", false, true, ServerVersion.Any), - "SetRemove" => ("SetRemove(key, values)", false, true, ServerVersion.Any), - "SortedSetAdd" => ("SortedSetAdd(key, entries)", false, true, ServerVersion.Any), - "SortedSetRemove" => ("SortedSetRemove(key, members)", false, true, ServerVersion.Any), - "HashSet" => ("HashSet(key, entries)", false, true, ServerVersion.Any), - "HashDelete" => ("HashDelete(key, fields)", false, true, ServerVersion.Any), - "ListLeftPush" => ("ListLeftPush(key, values)", false, true, ServerVersion.Any), - "ListRightPush" => ("ListRightPush(key, values)", false, true, ServerVersion.Any), + "SetAdd" => ("SetAdd[Async](key, values)", false, true, ServerVersion.Any), + "SetRemove" => ("SetRemove[Async](key, values)", false, true, ServerVersion.Any), + "SortedSetAdd" => ("SortedSetAdd[Async](key, entries)", false, true, ServerVersion.Any), + "SortedSetRemove" => ("SortedSetRemove[Async](key, members)", false, true, ServerVersion.Any), + "HashSet" => ("HashSet[Async](key, entries)", false, true, ServerVersion.Any), + "HashDelete" => ("HashDelete[Async](key, fields)", false, true, ServerVersion.Any), + "ListLeftPush" => ("ListLeftPush[Async](key, values)", false, true, ServerVersion.Any), + "ListRightPush" => ("ListRightPush[Async](key, values)", false, true, ServerVersion.Any), // SMISMEMBER, which unlike the rest of these is recent; it has no RedisFeatures gate to cite - "SetContains" => ("SetContains(key, values), which returns a bool per value", false, true, new ServerVersion(6, 2)), + "SetContains" => ("SetContains[Async](key, values), which returns a bool per value", false, true, new ServerVersion(6, 2)), // many keys - "KeyDelete" => ("KeyDelete(keys)", true, false, ServerVersion.Any), - "KeyExists" => ("KeyExists(keys), which returns how many exist", true, false, ServerVersion.Any), - "StringGet" => ("StringGet(keys)", true, false, ServerVersion.Any), - "StringSet" => ("StringSet(KeyValuePair[])", true, false, ServerVersion.Any), + "KeyDelete" => ("KeyDelete[Async](keys)", true, false, ServerVersion.Any), + "KeyExists" => ("KeyExists[Async](keys), which returns how many exist", true, false, ServerVersion.Any), + "StringGet" => ("StringGet[Async](keys)", true, false, ServerVersion.Any), + "StringSet" => ("StringSet[Async](KeyValuePair[])", true, false, ServerVersion.Any), _ => null, }; @@ -606,32 +606,32 @@ private static (Rule Rule, string Suggestion, ServerVersion MinVersion, bool Sam return (condition, op) switch { // -- family A: the command already takes this condition as an argument; any server version -- - ("KeyNotExists", "StringSet") => (Rule.ConditionalArgument, "StringSet(key, value, When.NotExists)", ServerVersion.Any, false), - ("KeyExists", "StringSet") => (Rule.ConditionalArgument, "StringSet(key, value, When.Exists)", ServerVersion.Any, false), - ("HashNotExists", "HashSet") => (Rule.ConditionalArgument, "HashSet(key, field, value, When.NotExists)", ServerVersion.Any, true), + ("KeyNotExists", "StringSet") => (Rule.ConditionalArgument, "StringSet[Async](key, value, When.NotExists)", ServerVersion.Any, false), + ("KeyExists", "StringSet") => (Rule.ConditionalArgument, "StringSet[Async](key, value, When.Exists)", ServerVersion.Any, false), + ("HashNotExists", "HashSet") => (Rule.ConditionalArgument, "HashSet[Async](key, field, value, When.NotExists)", ServerVersion.Any, true), // 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(key, member, score, SortedSetWhen.NotExists)", ServerVersion.Any, true), - ("SortedSetContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd(key, member, score, SortedSetWhen.Exists)", ServerVersion.Any, true), - ("KeyNotExists", "KeyRename") => (Rule.ConditionalArgument, "KeyRename(key, newKey, When.NotExists)", ServerVersion.Any, false), + ("SortedSetNotContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd[Async](key, member, score, SortedSetWhen.NotExists)", ServerVersion.Any, true), + ("SortedSetContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd[Async](key, member, score, SortedSetWhen.Exists)", ServerVersion.Any, true), + ("KeyNotExists", "KeyRename") => (Rule.ConditionalArgument, "KeyRename[Async](key, newKey, When.NotExists)", ServerVersion.Any, false), // -- 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(key, value, ValueCondition.Equal(expected))", new ServerVersion(8, 4), false), - ("StringNotEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet(key, value, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), - ("StringEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", new ServerVersion(8, 4), false), - ("StringNotEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete(key, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), + ("StringEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet[Async](key, value, ValueCondition.Equal(expected))", new ServerVersion(8, 4), false), + ("StringNotEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet[Async](key, value, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), + ("StringEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete[Async](key, ValueCondition.Equal(expected)), or LockRelease[Async]", new ServerVersion(8, 4), false), + ("StringNotEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete[Async](key, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), // -- 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"). - ("SetNotContains", "SetAdd") => (Rule.RedundantCondition, "SetAdd(key, value), which returns false if the member was already there", ServerVersion.Any, true), - ("SetContains", "SetRemove") => (Rule.RedundantCondition, "SetRemove(key, value), which returns false if the member was not there", ServerVersion.Any, true), - ("SortedSetContains", "SortedSetRemove") => (Rule.RedundantCondition, "SortedSetRemove(key, member), which returns false if the member was not there", ServerVersion.Any, true), - ("HashExists", "HashDelete") => (Rule.RedundantCondition, "HashDelete(key, field), which returns false if the field was not there", ServerVersion.Any, true), - ("KeyExists", "KeyDelete") => (Rule.RedundantCondition, "KeyDelete(key), which returns false if the key did not exist", ServerVersion.Any, false), - ("KeyExists", "KeyExpire") => (Rule.RedundantCondition, "KeyExpire(key, expiry), which returns false if the key did not exist", ServerVersion.Any, false), + ("SetNotContains", "SetAdd") => (Rule.RedundantCondition, "SetAdd[Async](key, value), which returns false if the member was already there", ServerVersion.Any, true), + ("SetContains", "SetRemove") => (Rule.RedundantCondition, "SetRemove[Async](key, value), which returns false if the member was not there", ServerVersion.Any, true), + ("SortedSetContains", "SortedSetRemove") => (Rule.RedundantCondition, "SortedSetRemove[Async](key, member), which returns false if the member was not there", ServerVersion.Any, true), + ("HashExists", "HashDelete") => (Rule.RedundantCondition, "HashDelete[Async](key, field), which returns false if the field was not there", ServerVersion.Any, true), + ("KeyExists", "KeyDelete") => (Rule.RedundantCondition, "KeyDelete[Async](key), which returns false if the key did not exist", ServerVersion.Any, false), + ("KeyExists", "KeyExpire") => (Rule.RedundantCondition, "KeyExpire[Async](key, expiry), which returns false if the key did not exist", ServerVersion.Any, false), // Deliberately absent from family C: ListIndexExists + ListSetByIndex. LSET reports an // out-of-range index by failing, not by returning false (ListSetByIndex returns Task, not @@ -676,17 +676,17 @@ private static (string Suggestion, ServerVersion MinVersion)? MapPair(QueuedOper switch (first.Name, second.Name) { case ("StringGet", "KeyDelete"): - return ("StringGetDelete(key)", v6_2); + return ("StringGetDelete[Async](key)", v6_2); case ("StringGet", "KeyExpire"): - return ("StringGetSetExpiry(key, expiry)", v6_2); + return ("StringGetSetExpiry[Async](key, expiry)", v6_2); case ("StringGet", "KeyPersist"): - return ("StringGetSetExpiry(key, null)", v6_2); + return ("StringGetSetExpiry[Async](key, null)", v6_2); case ("StringGet", "StringSet"): - return ("StringSetAndGet(key, value)", v6_2); + return ("StringSetAndGet[Async](key, value)", v6_2); // HGETDEL is 8.0; it has no RedisFeatures gate to point at case ("HashGet", "HashDelete") when SameMember(first, second): - return ("HashFieldGetAndDelete(key, field)", new ServerVersion(8, 0)); + return ("HashFieldGetAndDelete[Async](key, field)", new ServerVersion(8, 0)); } return null; @@ -698,7 +698,7 @@ private static (string Suggestion, ServerVersion MinVersion)? MapPair(QueuedOper && ((first.Name == "SetRemove" && second.Name == "SetAdd") || (first.Name == "SetAdd" && second.Name == "SetRemove"))) { - return ("SetMove(source, destination, value)", ServerVersion.Any); + return ("SetMove[Async](source, destination, value)", ServerVersion.Any); } return null; diff --git a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs index 2eac7ceca..998ebd42e 100644 --- a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs +++ b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs @@ -238,7 +238,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.KeyNotExists", "StringSetAsync", - "StringSet(key, value, When.NotExists)")); + "StringSet[Async](key, value, When.NotExists)")); [Fact] // two independent transactions in one method must be tracked separately, not pooled into one set of counts @@ -265,10 +265,10 @@ public async Task M(IDatabase db, RedisKey a, RedisKey b) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.KeyNotExists", "StringSetAsync", - "StringSet(key, value, When.NotExists)"), + "StringSet[Async](key, value, When.NotExists)"), Diagnostic("SER301").WithLocation(1).WithArguments( "Condition.StringEqual", "StringSetAsync", - "StringSet(key, value, ValueCondition.Equal(expected))", + "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 index 1bed1bd9a..98d1246ee 100644 --- a/tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs +++ b/tests/StackExchange.Redis.Build.Tests/MinServerVersion.cs @@ -102,6 +102,6 @@ public Task Message_NamesTheRequiredVersion() => VerifyAsync( Diagnostic("SER301").WithLocation(0).WithArguments( "Condition.StringEqual", "StringSetAsync", - "StringSet(key, value, ValueCondition.Equal(expected))", + "StringSet[Async](key, value, ValueCondition.Equal(expected))", "8.4")); } diff --git a/tests/StackExchange.Redis.Build.Tests/SER300.cs b/tests/StackExchange.Redis.Build.Tests/SER300.cs index 2332202c8..3819cdb5f 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER300.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER300.cs @@ -28,7 +28,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.KeyNotExists", "StringSetAsync", - "StringSet(key, value, When.NotExists)")); + "StringSet[Async](key, value, When.NotExists)")); [Fact] public Task KeyExistsGuardingStringSet_IsFlagged() => VerifyAsync( @@ -49,7 +49,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.KeyExists", "StringSetAsync", - "StringSet(key, value, When.Exists)")); + "StringSet[Async](key, value, When.Exists)")); [Fact] public Task HashNotExistsGuardingHashSet_IsFlagged() => VerifyAsync( @@ -70,7 +70,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.HashNotExists", "HashSetAsync", - "HashSet(key, field, value, When.NotExists)")); + "HashSet[Async](key, field, value, When.NotExists)")); [Fact] public Task SortedSetNotContainsGuardingSortedSetAdd_IsFlagged() => VerifyAsync( @@ -91,7 +91,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.SortedSetNotContains", "SortedSetAddAsync", - "SortedSetAdd(key, member, score, SortedSetWhen.NotExists)")); + "SortedSetAdd[Async](key, member, score, SortedSetWhen.NotExists)")); [Fact] public Task SortedSetContainsGuardingSortedSetAdd_IsFlagged() => VerifyAsync( @@ -112,7 +112,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.SortedSetContains", "SortedSetAddAsync", - "SortedSetAdd(key, member, score, SortedSetWhen.Exists)")); + "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 @@ -135,7 +135,7 @@ public async Task M(IDatabase db, RedisKey key, RedisKey other) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.KeyNotExists", "KeyRenameAsync", - "KeyRename(key, newKey, When.NotExists)")); + "KeyRename[Async](key, newKey, When.NotExists)")); [Fact] // synchronous surface: ITransaction is both IDatabaseAsync and the sync-shaped queueing API, and the @@ -157,7 +157,34 @@ public void M(IDatabase db, RedisKey key) Diagnostic("SER300").WithLocation(0).WithArguments( "Condition.KeyNotExists", "StringSetAsync", - "StringSet(key, value, When.NotExists)")); + "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 diff --git a/tests/StackExchange.Redis.Build.Tests/SER301.cs b/tests/StackExchange.Redis.Build.Tests/SER301.cs index b153464b7..fca390c6f 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER301.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER301.cs @@ -28,7 +28,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER301").WithLocation(0).WithArguments( "Condition.StringEqual", "StringSetAsync", - "StringSet(key, value, ValueCondition.Equal(expected))", + "StringSet[Async](key, value, ValueCondition.Equal(expected))", "8.4")); [Fact] @@ -50,7 +50,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER301").WithLocation(0).WithArguments( "Condition.StringNotEqual", "StringSetAsync", - "StringSet(key, value, ValueCondition.NotEqual(expected))", + "StringSet[Async](key, value, ValueCondition.NotEqual(expected))", "8.4")); [Fact] @@ -73,7 +73,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER301").WithLocation(0).WithArguments( "Condition.StringEqual", "KeyDeleteAsync", - "StringDelete(key, ValueCondition.Equal(expected)), or LockRelease", + "StringDelete[Async](key, ValueCondition.Equal(expected)), or LockRelease[Async]", "8.4")); [Fact] @@ -95,7 +95,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER301").WithLocation(0).WithArguments( "Condition.StringNotEqual", "KeyDeleteAsync", - "StringDelete(key, ValueCondition.NotEqual(expected))", + "StringDelete[Async](key, ValueCondition.NotEqual(expected))", "8.4")); [Fact] diff --git a/tests/StackExchange.Redis.Build.Tests/SER302.cs b/tests/StackExchange.Redis.Build.Tests/SER302.cs index 6e623266c..96be86b9e 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER302.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER302.cs @@ -27,7 +27,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER302").WithLocation(0).WithArguments( "Condition.SetNotContains", "SetAddAsync", - "SetAdd(key, value), which returns false if the member was already there")); + "SetAdd[Async](key, value), which returns false if the member was already there")); [Fact] public Task SetContainsGuardingSetRemove_IsFlagged() => VerifyAsync( @@ -48,7 +48,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER302").WithLocation(0).WithArguments( "Condition.SetContains", "SetRemoveAsync", - "SetRemove(key, value), which returns false if the member was not there")); + "SetRemove[Async](key, value), which returns false if the member was not there")); [Fact] public Task SortedSetContainsGuardingSortedSetRemove_IsFlagged() => VerifyAsync( @@ -69,7 +69,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER302").WithLocation(0).WithArguments( "Condition.SortedSetContains", "SortedSetRemoveAsync", - "SortedSetRemove(key, member), which returns false if the member was not there")); + "SortedSetRemove[Async](key, member), which returns false if the member was not there")); [Fact] public Task HashExistsGuardingHashDelete_IsFlagged() => VerifyAsync( @@ -90,7 +90,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER302").WithLocation(0).WithArguments( "Condition.HashExists", "HashDeleteAsync", - "HashDelete(key, field), which returns false if the field was not there")); + "HashDelete[Async](key, field), which returns false if the field was not there")); [Fact] public Task KeyExistsGuardingKeyDelete_IsFlagged() => VerifyAsync( @@ -111,7 +111,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER302").WithLocation(0).WithArguments( "Condition.KeyExists", "KeyDeleteAsync", - "KeyDelete(key), which returns false if the key did not exist")); + "KeyDelete[Async](key), which returns false if the key did not exist")); [Fact] public Task KeyExistsGuardingKeyExpire_IsFlagged() => VerifyAsync( @@ -133,7 +133,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER302").WithLocation(0).WithArguments( "Condition.KeyExists", "KeyExpireAsync", - "KeyExpire(key, expiry), which returns false if the key did not exist")); + "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, diff --git a/tests/StackExchange.Redis.Build.Tests/SER303.cs b/tests/StackExchange.Redis.Build.Tests/SER303.cs index 0084b0daf..c234aee19 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER303.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER303.cs @@ -28,7 +28,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER303").WithLocation(0).WithArguments( "StringGetAsync", "KeyDeleteAsync", - "StringGetDelete(key)", + "StringGetDelete[Async](key)", " (requires server 6.2 or later)")); [Fact] @@ -51,7 +51,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER303").WithLocation(0).WithArguments( "StringGetAsync", "KeyExpireAsync", - "StringGetSetExpiry(key, expiry)", + "StringGetSetExpiry[Async](key, expiry)", " (requires server 6.2 or later)")); [Fact] @@ -73,7 +73,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER303").WithLocation(0).WithArguments( "StringGetAsync", "StringSetAsync", - "StringSetAndGet(key, value)", + "StringSetAndGet[Async](key, value)", " (requires server 6.2 or later)")); [Fact] @@ -95,7 +95,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER303").WithLocation(0).WithArguments( "HashGetAsync", "HashDeleteAsync", - "HashFieldGetAndDelete(key, field)", + "HashFieldGetAndDelete[Async](key, field)", " (requires server 8.0 or later)")); [Fact] @@ -119,7 +119,7 @@ public async Task M(IDatabase db, RedisKey source, RedisKey destination) Diagnostic("SER303").WithLocation(0).WithArguments( "SetRemoveAsync", "SetAddAsync", - "SetMove(source, destination, value)", + "SetMove[Async](source, destination, value)", "")); [Fact] @@ -142,7 +142,7 @@ public async Task M(IDatabase db, RedisKey source, RedisKey destination) Diagnostic("SER303").WithLocation(0).WithArguments( "SetAddAsync", "SetRemoveAsync", - "SetMove(source, destination, value)", + "SetMove[Async](source, destination, value)", "")); [Fact] diff --git a/tests/StackExchange.Redis.Build.Tests/SER304.cs b/tests/StackExchange.Redis.Build.Tests/SER304.cs index 87714ddd6..a7c536e74 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER304.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER304.cs @@ -27,7 +27,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER304").WithLocation(0).WithArguments( "SetAddAsync", "2", - "SetAdd(key, values)", + "SetAdd[Async](key, values)", "")); [Fact] @@ -51,7 +51,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER304").WithLocation(0).WithArguments( "HashSetAsync", "3", - "HashSet(key, entries)", + "HashSet[Async](key, entries)", "")); [Fact] @@ -73,7 +73,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER304").WithLocation(0).WithArguments( "ListRightPushAsync", "2", - "ListRightPush(key, values)", + "ListRightPush[Async](key, values)", "")); [Fact] @@ -96,7 +96,7 @@ public async Task M(IDatabase db, RedisKey key) Diagnostic("SER304").WithLocation(0).WithArguments( "SetContainsAsync", "2", - "SetContains(key, values), which returns a bool per value", + "SetContains[Async](key, values), which returns a bool per value", " (requires server 6.2 or later)")); [Fact] @@ -119,7 +119,7 @@ public async Task M(IDatabase db, RedisKey a, RedisKey b) Diagnostic("SER304").WithLocation(0).WithArguments( "StringSetAsync", "2", - "StringSet(KeyValuePair[])", + "StringSet[Async](KeyValuePair[])", "")); [Fact] @@ -141,7 +141,7 @@ public async Task M(IDatabase db, RedisKey a, RedisKey b) Diagnostic("SER304").WithLocation(0).WithArguments( "StringGetAsync", "2", - "StringGet(keys)", + "StringGet[Async](keys)", "")); [Fact] @@ -163,7 +163,7 @@ public async Task M(IDatabase db, RedisKey a, RedisKey b) Diagnostic("SER304").WithLocation(0).WithArguments( "KeyDeleteAsync", "2", - "KeyDelete(keys)", + "KeyDelete[Async](keys)", "")); [Fact] From 5ddbc1230d7607df2cc562e0ea8fb890db391e1e Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 6 Aug 2026 09:40:40 +0100 Subject: [PATCH 11/11] Marc/tran analyzer review fixes (#3163) * Identify the transaction terminator by symbol, not by method name ITransaction inherits IDatabaseAsync.ExecuteAsync(string command, params object[] args) - a raw command, very much queued, and the one people reach for exactly when the library has no wrapper for what they want. The switch discarded it by name alongside the transaction's own ExecuteAsync() terminator, so it was invisible to the counts every rule below depends on: _ = tran.StringGetAsync(key); _ = tran.ExecuteAsync("PERSIST", key); // invisible _ = tran.KeyDeleteAsync(key); reported SER303 "use StringGetDelete[Async](key)", and taking that advice drops the PERSIST. The guarded families were wrong the same way: a raw command beside a conditional write still made the transaction a one-command transaction as far as the analyzer could see. Execute/ExecuteAsync now count as the terminator only when declared on ITransaction/ITransactionAsync, which is exactly where the terminator lives (ITransaction.cs) and is not where the raw command lives. A raw ExecuteAsync is now recorded as an ordinary queued operation; it matches nothing in the mapping tables, so its only effect is to suppress, which is the whole point. The third test is the control: both spellings of the terminator must still be recognised, or the rules would go quiet everywhere and the two negatives above would pass for the wrong reason. * Require queued commands to share a branch, and treat lambdas like loops The loop check was the only thing standing between "one call site" and "one queued command", and it only covered the repetition half of that gap. Branching was not covered at all, so mutually exclusive code read as a pair: if (flag) { _ = tran.SetAddAsync(a, member); } else { _ = tran.SetRemoveAsync(b, member); } reported SER303 "use SetMove[Async](source, destination, value)" - which queues a removal the code deliberately did not. The asymmetric form was the same: a StringGet followed by a KeyDelete queued only under an if became GETDEL, and a condition guarding a command in a branch it is not itself in became a conditional argument. The fix is branch *matching* rather than a blanket "anything conditional is out", because two commands in the same if body do always queue together and a compound command really does replace them - and because a whole transaction inside an if or a try is ordinary code that must not go silent. Each call records the innermost enclosing branch (the arm, not the if: the two arms of one if/else have to compare unequal), and a transaction whose calls do not agree is disqualified. The terminator is exempt - "queue it all, then commit inside an if" says nothing about whether the queued commands belong together. Repetition also had a second, larger hole: a lambda or local function is one call site whose invocation count is not visible at all, so _ = tran.KeyDeleteAsync(key); Queue(); Queue(); void Queue() => _ = tran.KeyDeleteAsync(other); reported SER304 over "these 2 queued KeyDeleteAsync calls" when three are queued. Those bodies now disqualify exactly as a loop body does. OperationsInTheSameBranch_AreStillFlagged is the control: without it, matching could be tightened to "never fire under a conditional" and every negative here would still pass. * Decline where the suggested command cannot carry an argument the caller wrote Only arguments 0 and 1 were ever looked at, so everything after them was invisible - and several suggestions cannot express what was there. The rewrites were silently lossy: - N x StringSet(key, value, expiry) -> MSET. MSET takes one expiry for the whole batch, not one per key, so following SER304 here makes both keys permanent. This one outlives the build. - N x HashSet(key, field, value, When.NotExists) -> HSET variadic, which has no NX. - StringGet + KeyExpire(key, expiry, ExpireWhen.HasNoExpiry) -> GETEX, which has no NX/XX. - Condition.KeyNotExists + StringSet(key, value, when: When.Exists) -> "use When.NotExists", turning code that never writes into code that writes when absent. Each mapping now states which parameters its suggestion still carries, and an argument the caller wrote that is not among them declines the rewrite. Per-mapping rather than a blanket "any extra argument is out", because SET *does* take an expiry alongside NX and that is the commonest shape the rule exists for - GuardedOperationWithExpiryAndFlags_IsStillFlagged is the control that keeps it. Family C carries a null coverage set meaning "everything": it keeps the command exactly as written and only deletes the condition, so no argument of it can go missing however exotic. CommandFlags is exempt throughout - it is on every command, no suggestion mentions it, and the help pages already say to carry it over verbatim. Omitted optional arguments do not count either: they carry no intent, and are what the suggested form would default to anyway. Coverage is stated as names the suggestion keeps rather than names it drops so that the fail-safe direction is the default: a parameter added to an overload later reads as uncovered and goes quiet, instead of being silently dropped by a rewrite that has never heard of it. Also fixes a related sharp edge in ArgumentText: an omitted optional argument reports the *invocation* as its syntax, so "the second argument" of tran.KeyDeleteAsync(key) was the text of the whole call. It could only ever collide with another omission from the same call site, but it was never a member the caller wrote, and RequiresMember was asserting nothing as a result. * SER303: StringSet then KeyExpire is SET ... EX Requested as a backlog item during review, and it fits family D exactly: two queued commands, no condition, one command that does both. No version clause. Setting a value and its lifetime in one command is as old as SET''s options (2.6.12), so naming a version would be the noise the other Any entries avoid. An absolute expiry is covered too - Expiration converts implicitly from DateTime as well as TimeSpan - though the EXAT form underneath that one does want 6.2; that caveat goes on the help page rather than into the version clause, because putting 6.2 in the mapping would hide the ordinary relative case from everyone who has declared a floor below it. Order is load-bearing, in the opposite direction to the reads already here: SET *clears* any TTL, so an EXPIRE followed by a SET leaves no expiry at all. Only (StringSet, KeyExpire) maps, and KeyExpireThenStringSet_IsNotFlagged pins that. A StringSet that already carries an expiry, followed by an EXPIRE that overrides it, stays quiet: which of the two lifetimes the single command should carry is a guess. That falls out of the coverage machinery from the previous commit rather than needing its own check - "expiry" is simply absent from the first operation''s coverage set - which is what turns MapPair''s coverage from one set into one per operation. The same split lets GETEX keep rejecting an ExpireWhen while accepting the expiry beside it, where a single shared set had to spell "when" in both senses at once. Not covered, and worth its own item if anyone wants it: Condition.KeyNotExists + StringSet + KeyExpire is StringSet(key, value, expiry, When.NotExists), but that is a condition *and* a pair, which is neither family as they are drawn today. * Do not walk every block twice to answer a question most blocks never ask The reassigned-locals scan ran over every operation block before anything knew whether the block held a transaction at all, so every method body in every project that references the package paid two full Descendants() walks where one would do. The compilation-level short-circuit does not help here: it only skips compilations that have never heard of StackExchange.Redis, which is precisely not the population this analyzer ships to. Moved below the "no transaction locals here" bail-out. Same answer, and the class comment''s claim of one pass is now true for the blocks that take it. Also, while here: parse the declared server version with the invariant culture, since it comes from a config file rather than from someone typing in a locale; and stop re-trimming a method name that QueuedOperation already trimmed, which read as though Map expected raw input. * Document the shapes the analyzer now declines, and the new SET ... EX pair The per-rule "deliberately not flagged" lists were repeating each other and had gone out of date in the same places, so the family-wide cases move to one section in the rules index and each page points at it. Three of them are new behaviour from this branch (a third queued command including a raw ExecuteAsync, commands in different branches, arguments the single command cannot carry) and one - SER300 declining a caller''s own when: - is worth stating next to the expiry case that is still flagged, since "extra arguments suppress" is not the rule and would be a fair thing to conclude otherwise. SER302 gets the opposite note: it keeps the command exactly as written and deletes only the condition, so the argument caveat is the one family-wide case that does not apply to it. SER303 gains the StringSet + KeyExpire row, the second (and opposite) reason order matters on this rule - SET clears the TTL, so only one direction is SET ... EX - and the absolute-expiry caveat that the version column cannot carry. * implement-resp-command: consider whether the new command replaces a transaction A surprising share of new commands are atomic compositions - GETDEL, GETEX, HGETDEL, SMOVE, SET NX/GET/IFEQ, SMISMEMBER, every variadic form - and every one of them exists because people were writing a transaction to get the same effect. TransactionAnalyzer is what tells those people to stop, and a command that is not added to its tables is invisible there: the analyzer stays quiet about exactly the code the command was written to replace. Cheap at the time, and nobody comes back for it later. So the skill now asks the question as a step, and the new section says what a mapping costs: which table by the shape of transaction it replaces, the server version the *suggestion* needs (not the flagged code), the coverage set that stops a rewrite silently dropping an argument, and the same-member / order / key-direction constraints that each table has a column for. Two things it insists on because they are what the review of that analyzer turned up. Coverage is stated as names kept rather than names dropped, so a parameter added to an overload later fails safe. And near-misses get written down where the next person will hit them - LMOVE and LMPOP are both wrong for reasons that are only obvious once you have had them explained. * Say plainly that the usage rules are guidance, and where to report a bad one These five read source text and cannot see keys, servers, or intent, so they are heuristics however carefully drawn - and they arrive as warnings, in a package the consumer did not opt into an analyzer from. Saying "this is a suggestion, not a defect report" costs a paragraph and sets the expectation the severity otherwise sets wrongly. The part that matters is the second half: a rule firing on correct code is a bug in the rule, not something for the consumer to work around, and it reaches everyone. Suppression is documented right below and is easy to reach for silently; the issue link is there so that reporting it is just as easy, and the wording pushes that way first. SER350 deliberately does not get this. It reports an actual build problem - code that was not generated - rather than offering an opinion about working code, and there is nothing heuristic about it. * Hedge the usage diagnostics: they are suggestions, and they are heuristics "can be replaced", "are one command", "is redundant" are findings of fact, and these rules are not in a position to state one - they read source text and cannot see keys, servers, or intent. Combined with a warning severity, which already overstates the case, the wording claimed more certainty than the analysis has. Titles now say "may be replaceable" / "may be redundant" / "may suit", and messages lead with "Consider" or "looks like ... - consider". Message arguments and their order are untouched, so this is wording only; the tests format through the same descriptors and are unaffected. SER350 deliberately keeps its plain phrasing. It reports that generated code was not emitted, which is a fact about the build rather than an opinion about working code. The help pages, the index list and the release-tracking notes move with the titles, since a message that hedges next to a doc heading that does not is worse than either alone. * Recognise CommandFlags by name as well as by type, and gate it with tests that fail without it Flags never bear on any of the argument audit: they are on every command, no suggestion mentions them, and the rewrite carries them over verbatim. The exclusion was by type alone, which is the check that can come back null - and the failure mode is not a missed exclusion but silence everywhere, because every command takes flags, so one unrecognised spelling would suppress every rule for anyone who passes them. Name and type both, which costs a string comparison on a path that only runs for calls on a transaction. The behaviour was technically already covered, but only by GuardedOperationWithExpiryAndFlags, which carries an expiry alongside the flags and is really about the expiry - and only for SER300, where the audit runs in three separate places. Three tests now pass flags as the *only* extra argument, one per family. Checked rather than assumed: with the exclusion disabled, exactly those three and the expiry one fail, and nothing else does. A test that would pass either way is not a gate. --- .../skills/implement-resp-command/SKILL.md | 35 +- docs/rules/SER300.md | 25 +- docs/rules/SER301.md | 19 +- docs/rules/SER302.md | 17 +- docs/rules/SER303.md | 35 +- docs/rules/SER304.md | 18 +- docs/rules/index.md | 41 +- .../AnalyzerReleases.Shipped.md | 10 +- eng/StackExchange.Redis.Build/Diagnostics.cs | 36 +- .../ServerVersion.cs | 6 +- .../TransactionAnalyzer.cs | 334 +++++++++++---- .../DetectionShape.cs | 381 ++++++++++++++++++ .../StackExchange.Redis.Build.Tests/SER303.cs | 88 ++++ 13 files changed, 927 insertions(+), 118 deletions(-) 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/docs/rules/SER300.md b/docs/rules/SER300.md index da1ccb16a..98a64f588 100644 --- a/docs/rules/SER300.md +++ b/docs/rules/SER300.md @@ -1,4 +1,4 @@ -# SER300: transaction can be replaced by a conditional argument +# 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. @@ -40,11 +40,30 @@ Read this before rewriting - the collapsed form is not a drop-in for every calle 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, anything queued in a loop, a transaction passed to another method or stored in a field, and pairings -with no atomic equivalent (`HashExists` + `HashSet`, `HashEqual`, `ListIndexEqual`, the `*Length*` conditions). +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 diff --git a/docs/rules/SER301.md b/docs/rules/SER301.md index 7b3452557..99ec41abc 100644 --- a/docs/rules/SER301.md +++ b/docs/rules/SER301.md @@ -1,7 +1,7 @@ -# SER301: transaction can be replaced by a single atomic operation +# SER301: transaction may be replaceable by a single atomic operation -A transaction implementing compare-and-set can be replaced by the equivalent conditional command on a server -that supports it. +A transaction implementing compare-and-set can usually be replaced by the equivalent conditional command on a +server that supports it. ```c# // flagged @@ -68,6 +68,19 @@ Only one condition guarding one queued command on the *same key expression* is f 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 diff --git a/docs/rules/SER302.md b/docs/rules/SER302.md index 7e31104b2..271cffc30 100644 --- a/docs/rules/SER302.md +++ b/docs/rules/SER302.md @@ -1,4 +1,4 @@ -# SER302: transaction condition is redundant +# 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. @@ -41,8 +41,23 @@ disappears, and `CommandFlags` must be carried over verbatim. `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. diff --git a/docs/rules/SER303.md b/docs/rules/SER303.md index ddf94b2c7..e4152f8d6 100644 --- a/docs/rules/SER303.md +++ b/docs/rules/SER303.md @@ -1,4 +1,4 @@ -# SER303: transaction can be replaced by a single compound command +# 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. @@ -21,17 +21,29 @@ RedisValue value = await db.StringGetDeleteAsync(key); | `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 -These commands return a value, so which way round the pair is queued is part of the meaning. `SET ... GET` -returns 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. +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. @@ -56,8 +68,23 @@ spelling is flagged. - **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. diff --git a/docs/rules/SER304.md b/docs/rules/SER304.md index b7f5f9aea..fc88f4a39 100644 --- a/docs/rules/SER304.md +++ b/docs/rules/SER304.md @@ -1,4 +1,4 @@ -# SER304: repeated queued operations can use the variadic overload +# 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. @@ -68,6 +68,22 @@ suggestion is not for you - suppress it. - **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 diff --git a/docs/rules/index.md b/docs/rules/index.md index 81e74af15..5f8115b88 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -30,16 +30,47 @@ substitute your own expressions. ## Usage -- [SER300](SER300) - transaction can be replaced by a conditional argument (any server version) -- [SER301](SER301) - transaction can be replaced by a single atomic operation (needs a newer server) -- [SER302](SER302) - condition is redundant; the command already reports whether it acted (any server version) -- [SER303](SER303) - two queued operations are a single compound command (varies by pair) -- [SER304](SER304) - the same operation queued repeatedly can use the variadic overload (mostly any server) +- [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 diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md index cf19e9a0b..9ca930621 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Shipped.md @@ -9,9 +9,9 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- -SER300 | Usage | Warning | TransactionAnalyzer: transaction can be replaced by a conditional argument (any server) -SER301 | Usage | Warning | TransactionAnalyzer: transaction can be replaced by a single atomic operation (newer server) -SER302 | Usage | Warning | TransactionAnalyzer: condition is redundant; the queued command already reports whether it acted -SER303 | Usage | Warning | TransactionAnalyzer: two queued operations are a single compound command -SER304 | Usage | Warning | TransactionAnalyzer: repeated queued operations can use the variadic overload +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/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index a33a8f25e..b930b125f 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -29,6 +29,12 @@ namespace StackExchange.Redis.Build; /// 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 { @@ -55,12 +61,12 @@ internal static class Diagnostics /// public static readonly DiagnosticDescriptor PreferConditionalArgument = new( id: "SER300", - title: "Transaction can be replaced by a conditional argument", - messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2} - the condition duplicates an argument the command already has", + 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 be replaced by the command's own conditional argument, which is a single round-trip and cannot abort under contention.", + 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")); /// @@ -80,12 +86,12 @@ internal static class Diagnostics /// public static readonly DiagnosticDescriptor PreferNewerAtomicOperation = new( id: "SER301", - title: "Transaction can be replaced by a single atomic operation", - messageFormat: "This transaction ({0} guarding {1}) can be expressed as {2}, which is atomic on the server and needs no WATCH (requires server {3} or later)", + 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 be replaced by the equivalent conditional command on servers that support it, which is a single round-trip and cannot abort under contention.", + 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")); /// @@ -100,12 +106,12 @@ internal static class Diagnostics /// public static readonly DiagnosticDescriptor RedundantCondition = new( id: "SER302", - title: "Transaction condition is redundant", - messageFormat: "This transaction ({0} guarding {1}) is redundant - use {2}", + 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 buys nothing: the transaction costs an extra round-trip and can abort, and the command alone says whether it acted.", + 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")); /// @@ -118,12 +124,12 @@ internal static class Diagnostics /// public static readonly DiagnosticDescriptor PreferCompoundCommand = new( id: "SER303", - title: "Transaction can be replaced by a single compound command", - messageFormat: "These two queued operations ({0} then {1}) are one command: use {2}{3}", + 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 be replaced by the single command that does both, which is one round-trip and cannot abort.", + 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")); /// @@ -137,12 +143,12 @@ internal static class Diagnostics /// public static readonly DiagnosticDescriptor PreferVariadicOverload = new( id: "SER304", - title: "Repeated queued operations can use the variadic overload", - messageFormat: "These {1} queued {0} calls are one command: use {2}{3}", + 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 be a single variadic call, which is one round-trip and needs no transaction to be atomic.", + 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")); /// diff --git a/eng/StackExchange.Redis.Build/ServerVersion.cs b/eng/StackExchange.Redis.Build/ServerVersion.cs index faf584e11..1e3d2feab 100644 --- a/eng/StackExchange.Redis.Build/ServerVersion.cs +++ b/eng/StackExchange.Redis.Build/ServerVersion.cs @@ -1,3 +1,4 @@ +using System.Globalization; using Microsoft.CodeAnalysis.Diagnostics; namespace StackExchange.Redis.Build; @@ -80,11 +81,12 @@ 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], out var major) || major <= 0) return false; + 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], out minor)) return false; + 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); diff --git a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs index e89c459c4..d26e34ee0 100644 --- a/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/TransactionAnalyzer.cs @@ -55,17 +55,28 @@ public override void Initialize(AnalysisContext context) private sealed class KnownSymbols { - private KnownSymbols(INamedTypeSymbol condition, INamedTypeSymbol? transaction, INamedTypeSymbol? transactionAsync) + 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 @@ -75,9 +86,12 @@ private KnownSymbols(INamedTypeSymbol condition, INamedTypeSymbol? transaction, var transactionAsync = compilation.GetTypeByMetadataName("StackExchange.Redis.ITransactionAsync"); if (transaction is null && transactionAsync is null) return null; - return new KnownSymbols(condition, transaction, transactionAsync); + 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)) @@ -91,20 +105,6 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols // one pass, gathering per-transaction-local usage; most blocks contain nothing and fall straight out Dictionary? usages = null; - // 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. - HashSet? reassignedLocals = null; - - foreach (var operation in block.Descendants()) - { - if (LocalWrittenBy(operation) is { } written) - { - reassignedLocals ??= new HashSet(SymbolEqualityComparer.Default); - reassignedLocals.Add(written); - } - } - foreach (var operation in block.Descendants()) { // the transaction is identified by the local it was assigned to; anything else (a field, a @@ -123,7 +123,8 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols && SymbolEqualityComparer.Default.Equals(instanceLocal, local)) { // tran.Something(...) - a queued command, a condition, or the terminator - usage.Add(invocation, known, insideLoop: IsInsideLoop(invocation, block)); + var repeats = !TryGetBranch(invocation, block, out var branch); + usage.Add(invocation, known, branch, repeats); } else { @@ -136,6 +137,24 @@ private static void Analyze(OperationBlockAnalysisContext context, KnownSymbols 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; @@ -219,21 +238,51 @@ private static string VersionClause(ServerVersion version) => version.IsSpecified ? " (requires server " + version + " or later)" : ""; /// - /// Is this call inside a loop, and so potentially queueing many commands from one call site? + /// 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 a loop is where it breaks: one - /// tran.StringSetAsync(key, value) in a foreach is one call site but N queued commands, which - /// is emphatically not collapsible into a single command. Cheap to check and it removes the whole class. + /// + /// 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 IsInsideLoop(IOperation operation, IOperation block) + private static bool TryGetBranch(IOperation operation, IOperation block, out SyntaxNode? branch) { - for (var node = operation; node is not null && node != block; node = node.Parent) + branch = null; + var previous = operation; + for (var node = operation.Parent; node is not null && node != block; node = node.Parent) { - if (node is ILoopOperation) return true; + 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 false; + return true; } /// @@ -308,13 +357,14 @@ private static string Trim(string name) /// private readonly struct QueuedOperation { - public QueuedOperation(string name, string? key, string? member, List? reads) + 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. @@ -339,6 +389,16 @@ public QueuedOperation(string name, string? key, string? member, List? /// 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; } } /// @@ -364,6 +424,16 @@ private sealed class Usage 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. @@ -376,14 +446,37 @@ private sealed class Usage /// public void Disqualify() => _disqualified = true; - public void Add(IInvocationOperation invocation, KnownSymbols known, bool insideLoop) + public void Add(IInvocationOperation invocation, KnownSymbols known, SyntaxNode? branch, bool repeats) { - if (insideLoop) + 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": @@ -405,12 +498,9 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside break; - case "Execute": - case "ExecuteAsync": - break; // the terminator, not a queued operation - default: - // everything else queued on the transaction is a redis operation + // 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) { @@ -418,7 +508,8 @@ public void Add(IInvocationOperation invocation, KnownSymbols known, bool inside invocation.TargetMethod.Name, ArgumentText(invocation, 0), ArgumentText(invocation, 1), - LocalsRead(invocation))); + LocalsRead(invocation), + SuppliedArguments(invocation, known))); } else { @@ -493,6 +584,8 @@ static bool Contains(List? reads, HashSet reassigned) return null; } + if (!IsCovered(operation, mapped.Covered)) return null; + return new Rewrite( mapped.Rule, "Condition." + _conditionFactory, @@ -504,6 +597,7 @@ static bool Contains(List? reads, HashSet reassigned) 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); } @@ -527,6 +621,7 @@ static bool Contains(List? reads, HashSet reassigned) 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; @@ -566,27 +661,30 @@ static bool Contains(List? reads, HashSet reassigned) /// the argument lists look. Same for ZMPOP. /// /// - private static (string Suggestion, bool ManyKeys, bool RequiresMember, ServerVersion MinVersion)? MapVariadic(string operation) + 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), - "SetRemove" => ("SetRemove[Async](key, values)", false, true, ServerVersion.Any), - "SortedSetAdd" => ("SortedSetAdd[Async](key, entries)", false, true, ServerVersion.Any), - "SortedSetRemove" => ("SortedSetRemove[Async](key, members)", false, true, ServerVersion.Any), - "HashSet" => ("HashSet[Async](key, entries)", false, true, ServerVersion.Any), - "HashDelete" => ("HashDelete[Async](key, fields)", false, true, ServerVersion.Any), - "ListLeftPush" => ("ListLeftPush[Async](key, values)", false, true, ServerVersion.Any), - "ListRightPush" => ("ListRightPush[Async](key, values)", false, true, ServerVersion.Any), + "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)), + "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), - "KeyExists" => ("KeyExists[Async](keys), which returns how many exist", true, false, ServerVersion.Any), - "StringGet" => ("StringGet[Async](keys)", true, false, ServerVersion.Any), - "StringSet" => ("StringSet[Async](KeyValuePair[])", true, false, ServerVersion.Any), + "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, }; @@ -600,38 +698,40 @@ private static (string Suggestion, bool ManyKeys, bool RequiresMember, ServerVer /// (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)? Map(string condition, string operation) - { - var op = Trim(operation); - return (condition, op) switch + 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 -- - ("KeyNotExists", "StringSet") => (Rule.ConditionalArgument, "StringSet[Async](key, value, When.NotExists)", ServerVersion.Any, false), - ("KeyExists", "StringSet") => (Rule.ConditionalArgument, "StringSet[Async](key, value, When.Exists)", ServerVersion.Any, false), - ("HashNotExists", "HashSet") => (Rule.ConditionalArgument, "HashSet[Async](key, field, value, When.NotExists)", ServerVersion.Any, true), + // 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), - ("SortedSetContains", "SortedSetAdd") => (Rule.ConditionalArgument, "SortedSetAdd[Async](key, member, score, SortedSetWhen.Exists)", ServerVersion.Any, true), - ("KeyNotExists", "KeyRename") => (Rule.ConditionalArgument, "KeyRename[Async](key, newKey, When.NotExists)", ServerVersion.Any, false), + ("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), - ("StringNotEqual", "StringSet") => (Rule.NewerAtomicOperation, "StringSet[Async](key, value, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), - ("StringEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete[Async](key, ValueCondition.Equal(expected)), or LockRelease[Async]", new ServerVersion(8, 4), false), - ("StringNotEqual", "KeyDelete") => (Rule.NewerAtomicOperation, "StringDelete[Async](key, ValueCondition.NotEqual(expected))", new ServerVersion(8, 4), false), + ("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"). - ("SetNotContains", "SetAdd") => (Rule.RedundantCondition, "SetAdd[Async](key, value), which returns false if the member was already there", ServerVersion.Any, true), - ("SetContains", "SetRemove") => (Rule.RedundantCondition, "SetRemove[Async](key, value), which returns false if the member was not there", ServerVersion.Any, true), - ("SortedSetContains", "SortedSetRemove") => (Rule.RedundantCondition, "SortedSetRemove[Async](key, member), which returns false if the member was not there", ServerVersion.Any, true), - ("HashExists", "HashDelete") => (Rule.RedundantCondition, "HashDelete[Async](key, field), which returns false if the field was not there", ServerVersion.Any, true), - ("KeyExists", "KeyDelete") => (Rule.RedundantCondition, "KeyDelete[Async](key), which returns false if the key did not exist", ServerVersion.Any, false), - ("KeyExists", "KeyExpire") => (Rule.RedundantCondition, "KeyExpire[Async](key, expiry), which returns false if the key did not exist", ServerVersion.Any, false), + // 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 @@ -647,8 +747,6 @@ private static (Rule Rule, string Suggestion, ServerVersion MinVersion, bool Sam _ => null, }; - } - /// /// Family D: two queued commands, no condition, that are one compound command between them. /// @@ -666,7 +764,7 @@ private static (Rule Rule, string Suggestion, ServerVersion MinVersion, bool Sam /// fine by contrast, because the member is a value the caller already has and passes to both calls. /// /// - private static (string Suggestion, ServerVersion MinVersion)? MapPair(QueuedOperation first, QueuedOperation second) + 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); @@ -676,17 +774,31 @@ private static (string Suggestion, ServerVersion MinVersion)? MapPair(QueuedOper switch (first.Name, second.Name) { case ("StringGet", "KeyDelete"): - return ("StringGetDelete[Async](key)", v6_2); + 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); + return ("StringGetSetExpiry[Async](key, expiry)", v6_2, "key", "key,expiry"); case ("StringGet", "KeyPersist"): - return ("StringGetSetExpiry[Async](key, null)", v6_2); + return ("StringGetSetExpiry[Async](key, null)", v6_2, "key", "key"); case ("StringGet", "StringSet"): - return ("StringSetAndGet[Async](key, value)", v6_2); + 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)); + return ("HashFieldGetAndDelete[Async](key, field)", new ServerVersion(8, 0), "key,hashField", "key,hashField"); } return null; @@ -698,7 +810,7 @@ private static (string Suggestion, ServerVersion MinVersion)? MapPair(QueuedOper && ((first.Name == "SetRemove" && second.Name == "SetAdd") || (first.Name == "SetAdd" && second.Name == "SetRemove"))) { - return ("SetMove[Async](source, destination, value)", ServerVersion.Any); + return ("SetMove[Async](source, destination, value)", ServerVersion.Any, "key,value", "key,value"); } return null; @@ -736,8 +848,74 @@ static bool SameMember(QueuedOperation a, QueuedOperation b) 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) - => invocation.Arguments.Length <= index ? null : invocation.Arguments[index].Value.Syntax.ToString(); + { + 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) { diff --git a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs index 998ebd42e..83d302268 100644 --- a/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs +++ b/tests/StackExchange.Redis.Build.Tests/DetectionShape.cs @@ -91,6 +91,131 @@ public async Task M(IDatabase db, RedisKey key, RedisValue[] values) } """); + [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( @@ -240,6 +365,262 @@ public async Task M(IDatabase db, RedisKey key) "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( diff --git a/tests/StackExchange.Redis.Build.Tests/SER303.cs b/tests/StackExchange.Redis.Build.Tests/SER303.cs index c234aee19..dbd5145d0 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER303.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER303.cs @@ -54,6 +54,94 @@ public async Task M(IDatabase db, RedisKey key) "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( """