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