From 03cdb4bdc5455abb6b7d2e573cfd2a2936366103 Mon Sep 17 00:00:00 2001 From: neuecc Date: Fri, 30 Oct 2026 11:57:27 +0900 Subject: [PATCH 1/2] Add [NonCopyable] so SF002 covers structs that hold a buffer by value A serializer's reader or writer is not a buffer itself but carries one by value, so copying it duplicates the buffer just as copying the buffer would. Implementing IWriteBuffer/IReadBuffer stays the implicit marker; structs marked with SerializerFoundation.CodeAnalysis.NonCopyableAttribute now get the same SF002 checks. The analyzer keys on this library's attribute type, not the name, and reads it from the generic definition so every construction is covered. Co-Authored-By: Claude Opus 5.5 --- README.md | 8 +- .../NonCopyableBufferAnalyzer.cs | 197 +++++++++++------- .../CodeAnalysis/NonCopyableAttribute.cs | 12 ++ .../NonCopyableBufferAnalyzerTest.cs | 129 +++++++++++- 4 files changed, 266 insertions(+), 80 deletions(-) create mode 100644 src/SerializerFoundation/CodeAnalysis/NonCopyableAttribute.cs diff --git a/README.md b/README.md index 386f9eb..75c42b6 100644 --- a/README.md +++ b/README.md @@ -457,7 +457,7 @@ The package bundles Roslyn analyzers that check the buffer contract at compile t | SF002 | Buffer structs are single-owner and must not be copied. A copy diverges silently (two write indexes over one window) and double-returns pooled arrays at Dispose. | | SF003 | Virtual methods marked `[RequireOverride]` must be overridden by concrete derived types that can access them. | -SF002 is the one you will meet. Implementing a buffer interface is the non-copyable marker, no attribute needed. The analyzer catches common copy mistakes in serializer code: by-value parameters and extension receivers, `in` / `ref readonly` parameters (the callee mutates a defensive copy), local and field initialization from another buffer, boxing, capture in a tuple or array initializer, `foreach`, pattern matching, `with`, by-value properties, yielding an existing buffer, and mutation through a `readonly` reference. It is a guard against common mistakes, not a complete ownership checker: callers still need to keep buffers single-owner and pass them by `ref`. +SF002 is the one you will meet. Implementing a buffer interface is the non-copyable marker, so buffers need no attribute. A struct that is not a buffer but holds one by value, such as a serializer's reader or writer, opts in with `[NonCopyable]` (`SerializerFoundation.CodeAnalysis`) and is checked by the same rules. The analyzer catches common copy mistakes in serializer code: by-value parameters and extension receivers, `in` / `ref readonly` parameters (the callee mutates a defensive copy), local and field initialization from another buffer, boxing, capture in a tuple or array initializer, `foreach`, pattern matching, `with`, by-value properties, yielding an existing buffer, and mutation through a `readonly` reference. It is a guard against common mistakes, not a complete ownership checker: callers still need to keep buffers single-owner and pass them by `ref`. ```csharp void ByValue(ArrayPoolListWriteBuffer buffer) { } // SF002 @@ -466,6 +466,12 @@ void ByRef(ref ArrayPoolListWriteBuffer buffer) { } // OK var buffer = new ArrayPoolListWriteBuffer(scratch); // OK: constructed in place var copy = buffer; // SF002: copied by assignment + +[NonCopyable] +public ref struct MyWriter where TBuffer : struct, IWriteBuffer, allows ref struct +{ + TBuffer buffer; // a copy of MyWriter would copy the buffer too, so MyWriter follows the same rules +} ``` The analyzers ship inside the SerializerFoundation package under `analyzers/dotnet/cs` and require a host compatible with Roslyn 4.3.1 or later. A direct `PackageReference` loads them automatically. Transitive references are different: NuGet's default `PrivateAssets` (`contentfiles;analyzers;build`) writes `exclude="Build,Analyzers"` into the dependency entry of any package that references SerializerFoundation, so applications that only reference that package never load the analyzers. **If you publish a library that depends on SerializerFoundation, always reference it with `PrivateAssets="none"`** so the analyzers reach your users: diff --git a/src/SerializerFoundation.Analyzers/NonCopyableBufferAnalyzer.cs b/src/SerializerFoundation.Analyzers/NonCopyableBufferAnalyzer.cs index 53aa669..31d991b 100644 --- a/src/SerializerFoundation.Analyzers/NonCopyableBufferAnalyzer.cs +++ b/src/SerializerFoundation.Analyzers/NonCopyableBufferAnalyzer.cs @@ -11,8 +11,9 @@ namespace SerializerFoundation.Analyzers; /// Every struct implementing IWriteBuffer/IReadBuffer is single-owner mutable state, /// a copy diverges silently (two write indexes over one window) and double-returns pooled arrays at Dispose. /// The BCL declined a general [NonCopyable] (dotnet/runtime#50389), -/// and for this library no attribute is needed anyway: implementing a buffer interface IS the non-copyable marker, -/// so the analyzer keys directly on that. +/// and buffers need no attribute: implementing a buffer interface IS the non-copyable marker, +/// so the analyzer keys directly on that. SerializerFoundation's own [NonCopyable] covers the one shape the +/// interfaces cannot see, a struct that is not a buffer but holds one by value (a serializer's reader or writer). /// [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class NonCopyableBufferAnalyzer : DiagnosticAnalyzer @@ -47,11 +48,11 @@ static bool IsUsingLocal(ILocalSymbol local) static readonly DiagnosticDescriptor Rule = new( DiagnosticId, "Buffer structs are single-owner and must not be copied", - "'{0}' implements a buffer interface and must not be {1}; buffers are single-owner - pass by ref or construct in place", + "'{0}' is a single-owner struct (a buffer, or marked [NonCopyable]) and must not be {1}; pass by ref or construct in place", "SerializerFoundation.Correctness", DiagnosticSeverity.Error, // this must not allow silent divergence of mutable state, so it's an error, not a warning isEnabledByDefault: true, - description: "Structs implementing IWriteBuffer/IReadBuffer (or the async flavors) hold single-owner mutable state (write indexes, rented pool arrays, pinned windows). A copy diverges silently and double-disposes pooled state. Pass buffers by ref (the formatter contract), construct them in place, and never box them. `in` and `ref readonly` parameters count as copies: the buffer's mutating members act on a defensive copy, and so does any non-readonly member called through a readonly field or a ref readonly reference."); + description: "Structs implementing IWriteBuffer/IReadBuffer (or the async flavors) hold single-owner mutable state (write indexes, rented pool arrays, pinned windows). A copy diverges silently and double-disposes pooled state. Structs marked [NonCopyable] (typically a reader or writer that holds a buffer by value) follow the same rules. Pass buffers by ref (the formatter contract), construct them in place, and never box them. `in` and `ref readonly` parameters count as copies: the buffer's mutating members act on a defensive copy, and so does any non-readonly member called through a readonly field or a ref readonly reference."); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); @@ -62,42 +63,33 @@ public override void Initialize(AnalysisContext context) context.RegisterCompilationStartAction(static startContext => { - var interfaces = ImmutableArray.CreateRange( - new[] - { - "SerializerFoundation.IWriteBuffer", - "SerializerFoundation.IReadBuffer", - } - .Select(startContext.Compilation.GetTypeByMetadataName) - .Where(static symbol => symbol is not null) - .Select(static symbol => symbol!)); - if (interfaces.IsEmpty) + if (NonCopyableTypes.Create(startContext.Compilation) is not { } types) { return; } startContext.RegisterOperationAction( - context => AnalyzeAssignment(context, interfaces), + context => AnalyzeAssignment(context, types), OperationKind.SimpleAssignment, OperationKind.VariableDeclarator, OperationKind.FieldInitializer); startContext.RegisterOperationAction( - context => AnalyzeArgument(context, interfaces), + context => AnalyzeArgument(context, types), OperationKind.Argument); startContext.RegisterOperationAction( - context => AnalyzeConversion(context, interfaces), + context => AnalyzeConversion(context, types), OperationKind.Conversion); startContext.RegisterOperationAction( - context => AnalyzeParameters(context, ((ILocalFunctionOperation)context.Operation).Symbol, interfaces), + context => AnalyzeParameters(context, ((ILocalFunctionOperation)context.Operation).Symbol, types), OperationKind.LocalFunction); startContext.RegisterOperationAction( - context => AnalyzeParameters(context, ((IAnonymousFunctionOperation)context.Operation).Symbol, interfaces), + context => AnalyzeParameters(context, ((IAnonymousFunctionOperation)context.Operation).Symbol, types), OperationKind.AnonymousFunction); startContext.RegisterSymbolAction( - context => AnalyzeParameters(context, (IMethodSymbol)context.Symbol, interfaces), + context => AnalyzeParameters(context, (IMethodSymbol)context.Symbol, types), SymbolKind.Method); startContext.RegisterSyntaxNodeAction( - context => AnalyzeExtensionReceiver(context, interfaces), + context => AnalyzeExtensionReceiver(context, types), SyntaxKind.Parameter); // delegate Invoke is implicitly declared, so the Method symbol action never // sees it; the delegate TYPE is the source-declared symbol to hook @@ -106,47 +98,47 @@ public override void Initialize(AnalysisContext context) { if (((INamedTypeSymbol)context.Symbol).DelegateInvokeMethod is { } invoke) { - AnalyzeParameters(context, invoke, interfaces); + AnalyzeParameters(context, invoke, types); } }, SymbolKind.NamedType); startContext.RegisterSymbolAction( - context => AnalyzeProperty(context, interfaces), + context => AnalyzeProperty(context, types), SymbolKind.Property); startContext.RegisterOperationAction( - context => AnalyzeReturn(context, interfaces), + context => AnalyzeReturn(context, types), OperationKind.Return, OperationKind.YieldReturn); startContext.RegisterOperationAction( - context => AnalyzeForEach(context, interfaces), + context => AnalyzeForEach(context, types), OperationKind.Loop); startContext.RegisterOperationAction( - context => AnalyzePattern(context, interfaces), + context => AnalyzePattern(context, types), OperationKind.DeclarationPattern, OperationKind.RecursivePattern); startContext.RegisterOperationAction( - context => AnalyzeWith(context, interfaces), + context => AnalyzeWith(context, types), OperationKind.With); startContext.RegisterOperationAction( - context => AnalyzeTuple(context, interfaces), + context => AnalyzeTuple(context, types), OperationKind.Tuple); // KNOWN GAP: C#12 collection-expression elements ([a]) need // ICollectionExpressionOperation, which arrived after the deliberate // Microsoft.CodeAnalysis 4.3.1 floor (see the csproj comment); wire it up when // the floor moves to 4.10+ startContext.RegisterOperationAction( - context => AnalyzeCollectionElements(context, ((IArrayInitializerOperation)context.Operation).ElementValues, interfaces), + context => AnalyzeCollectionElements(context, ((IArrayInitializerOperation)context.Operation).ElementValues, types), OperationKind.ArrayInitializer); startContext.RegisterOperationAction( - context => AnalyzeUsing(context, interfaces), + context => AnalyzeUsing(context, types), OperationKind.Using); startContext.RegisterOperationAction( - context => AnalyzeReadOnlyReceiver(context, interfaces), + context => AnalyzeReadOnlyReceiver(context, types), OperationKind.Invocation); }); } - static void AnalyzeAssignment(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeAssignment(OperationAnalysisContext context, NonCopyableTypes types) { var (target, value) = context.Operation switch { @@ -157,17 +149,17 @@ static void AnalyzeAssignment(OperationAnalysisContext context, ImmutableArray (field.InitializedFields[0].Type, field.Value), _ => (null, null), }; - if (target is null || value is null || !IsBufferStruct(target, interfaces) || IsFreshValue(value)) + if (target is null || value is null || !types.IsNonCopyable(target) || IsFreshValue(value)) { return; } context.ReportDiagnostic(Diagnostic.Create(Rule, value.Syntax.GetLocation(), target.Name, "copied by assignment")); } - static void AnalyzeArgument(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeArgument(OperationAnalysisContext context, NonCopyableTypes types) { var argument = (IArgumentOperation)context.Operation; - if (argument.Parameter is not { } parameter || argument.Value.Type is not { } type || !IsBufferStruct(type, interfaces)) + if (argument.Parameter is not { } parameter || argument.Value.Type is not { } type || !types.IsNonCopyable(type)) { return; } @@ -186,10 +178,10 @@ static void AnalyzeArgument(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeConversion(OperationAnalysisContext context, NonCopyableTypes types) { var conversion = (IConversionOperation)context.Operation; - if (conversion.Operand.Type is not { } operandType || !IsBufferStruct(operandType, interfaces)) + if (conversion.Operand.Type is not { } operandType || !types.IsNonCopyable(operandType)) { return; } @@ -205,7 +197,7 @@ static void AnalyzeConversion(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeParameters(DiagnosticReporter context, IMethodSymbol method, NonCopyableTypes types) { if (method.MethodKind is not (MethodKind.Ordinary or MethodKind.ExplicitInterfaceImplementation or MethodKind.LocalFunction or MethodKind.Constructor or MethodKind.AnonymousFunction or MethodKind.DelegateInvoke or MethodKind.UserDefinedOperator or MethodKind.Conversion)) @@ -214,11 +206,11 @@ static void AnalyzeParameters(DiagnosticReporter context, IMethodSymbol method, } foreach (var parameter in method.Parameters) { - AnalyzeParameter(context, parameter, interfaces); + AnalyzeParameter(context, parameter, types); } } - static void AnalyzeExtensionReceiver(SyntaxNodeAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeExtensionReceiver(SyntaxNodeAnalysisContext context, NonCopyableTypes types) { // A C# 14 extension receiver belongs to the extension's named type, not to // any member's parameter list. ParameterSyntax and GetDeclaredSymbol work @@ -228,15 +220,15 @@ static void AnalyzeExtensionReceiver(SyntaxNodeAnalysisContext context, Immutabl extension.GetMembers().Any(static member => !member.IsStatic)) { // Static extension members do not receive an instance or copy the buffer. - AnalyzeParameter(context, parameter, interfaces); + AnalyzeParameter(context, parameter, types); } } - static void AnalyzeParameter(DiagnosticReporter context, IParameterSymbol parameter, ImmutableArray interfaces) + static void AnalyzeParameter(DiagnosticReporter context, IParameterSymbol parameter, NonCopyableTypes types) { // ref and out are the only sanctioned shapes: a buffer's members are mutating, so an // in / ref readonly parameter acts on a hidden defensive copy when mutated. - if (parameter.RefKind is not (RefKind.None or RefKind.In or RefReadOnlyParameter) || !IsBufferStruct(parameter.Type, interfaces)) + if (parameter.RefKind is not (RefKind.None or RefKind.In or RefReadOnlyParameter) || !types.IsNonCopyable(parameter.Type)) { return; } @@ -250,10 +242,10 @@ static void AnalyzeParameter(DiagnosticReporter context, IParameterSymbol parame // the getter of a by-value property hands out a COPY on every access, so // `x.Buffer.Dispose()` silently disposes a temporary — no other rule can see that, // because no assignment/argument/conversion is involved at the use site - static void AnalyzeProperty(SymbolAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeProperty(SymbolAnalysisContext context, NonCopyableTypes types) { var property = (IPropertySymbol)context.Symbol; - if (property.RefKind != RefKind.None || !IsBufferStruct(property.Type, interfaces)) + if (property.RefKind != RefKind.None || !types.IsNonCopyable(property.Type)) { return; } @@ -267,11 +259,11 @@ static void AnalyzeProperty(SymbolAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeReturn(OperationAnalysisContext context, NonCopyableTypes types) { var returnOperation = (IReturnOperation)context.Operation; if (returnOperation.ReturnedValue is not { Type: { } type } value || - !IsBufferStruct(type, interfaces) || + !types.IsNonCopyable(type) || context.ContainingSymbol is IMethodSymbol { RefKind: not RefKind.None }) { return; @@ -301,13 +293,13 @@ static void AnalyzeReturn(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeForEach(OperationAnalysisContext context, NonCopyableTypes types) { if (context.Operation is not IForEachLoopOperation { LoopControlVariable: IVariableDeclaratorOperation { Symbol: { } local } }) { return; } - if (local.RefKind != RefKind.None || !IsBufferStruct(local.Type, interfaces)) + if (local.RefKind != RefKind.None || !types.IsNonCopyable(local.Type)) { return; } @@ -315,7 +307,7 @@ static void AnalyzeForEach(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzePattern(OperationAnalysisContext context, NonCopyableTypes types) { var declared = context.Operation switch { @@ -323,7 +315,7 @@ static void AnalyzePattern(OperationAnalysisContext context, ImmutableArray pattern.DeclaredSymbol as ILocalSymbol, _ => null, }; - if (declared is null || !IsBufferStruct(declared.Type, interfaces)) + if (declared is null || !types.IsNonCopyable(declared.Type)) { return; } @@ -331,10 +323,10 @@ static void AnalyzePattern(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeWith(OperationAnalysisContext context, NonCopyableTypes types) { var with = (IWithOperation)context.Operation; - if (with.Operand.Type is { } type && IsBufferStruct(type, interfaces)) + if (with.Operand.Type is { } type && types.IsNonCopyable(type)) { context.ReportDiagnostic(Diagnostic.Create(Rule, with.Syntax.GetLocation(), type.Name, "copied by a with-expression")); } @@ -342,24 +334,24 @@ static void AnalyzeWith(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeTuple(OperationAnalysisContext context, NonCopyableTypes types) { var tuple = (ITupleOperation)context.Operation; foreach (var element in tuple.Elements) { - if (Unwrap(element).Type is { } type && IsBufferStruct(type, interfaces)) + if (Unwrap(element).Type is { } type && types.IsNonCopyable(type)) { context.ReportDiagnostic(Diagnostic.Create(Rule, element.Syntax.GetLocation(), type.Name, "captured in a tuple (tuples copy freely and hide the buffer)")); } } } - static void AnalyzeCollectionElements(OperationAnalysisContext context, IEnumerable elements, ImmutableArray interfaces) + static void AnalyzeCollectionElements(OperationAnalysisContext context, IEnumerable elements, NonCopyableTypes types) { foreach (var element in elements) { var unwrapped = Unwrap(element); - if (unwrapped.Type is { } type && IsBufferStruct(type, interfaces) && !IsFreshValue(unwrapped)) + if (unwrapped.Type is { } type && types.IsNonCopyable(type) && !IsFreshValue(unwrapped)) { context.ReportDiagnostic(Diagnostic.Create(Rule, element.Syntax.GetLocation(), type.Name, "copied into a collection")); } @@ -368,7 +360,7 @@ static void AnalyzeCollectionElements(OperationAnalysisContext context, IEnumera // `using (a)` copies the resource into a hidden slot and disposes THAT; the // declaration form (`using var a = ...;`) is handled by the assignment rule - static void AnalyzeUsing(OperationAnalysisContext context, ImmutableArray interfaces) + static void AnalyzeUsing(OperationAnalysisContext context, NonCopyableTypes types) { var usingOperation = (IUsingOperation)context.Operation; if (usingOperation.Resources is IVariableDeclarationGroupOperation) @@ -376,7 +368,7 @@ static void AnalyzeUsing(OperationAnalysisContext context, ImmutableArray