diff --git a/CHANGELOG.md b/CHANGELOG.md index c1c7f43..1441623 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ section is the GitHub release's notes. ### Changed - Packages: the public surface is checked against 2.3.1's, so nothing 2.3.1 ships is removed or changed in 2.4.0. +- Analyzers: CMTK0005 now also warns at `CollectionsMarshal.SetCount(ids, count)` on a `List`, which makes room for `count` items without putting one there, as `new OrderId[count]` does; a constant count of zero isn't reported. In a build that treats warnings as errors, that can report code that built before ([CMTK0005](https://github.com/CaffeinatedCoder/CodoMetis.TypeKit/blob/main/src/CodoMetis.TypeKit.Analyzers/README.md#cmtk0005)). ### Fixed diff --git a/design/analyzers.md b/design/analyzers.md index a105409..6b64815 100644 --- a/design/analyzers.md +++ b/design/analyzers.md @@ -35,7 +35,7 @@ exist. | CMTK0002 | Type implements `IValue<>`/`IValidatedValue<,,>`, but the compilation does not reference `CodoMetis.TypeKit.Generators`, so it is never woven | Error | shipped in 1.0.0 | | CMTK0003 | `Result`/`Option` a call returns, dropped by an expression statement, awaited or not, or a `Task` of one converted to a plain `Task` (`Task Cancel() => CancelAsync();`); `_ =` opts out. `Tap`, `TapAsync`, `TapError` and `TapErrorAsync` on a stored result or task are exempt, since they return their receiver, and so is a chain of them on one (`r.Tap(log).TapError(warn)`, `(await pending).Tap(log)`, `pending.WaitAsync(t).TapAsync(log)`), measured as an Error's false report before 2.0.0 was tagged; so is such a tap converted to a plain `Task` or waited for (`Task Log() => pending.TapAsync(log);`, `held.TapAsync(log).Wait()`), on a `Result` and on a `Task` of one, which an independent review found reported on 2026-10-14. Also a collection of results the statement made (an array, `ICollection` or `IReadOnlyCollection`, such as `await Task.WhenAll(ids.Select(orders.CancelAsync))`), and a method group of a `Task` method converted to a delegate returning `Task`; tasks held in variables are silent, and so is a lazy `IEnumerable`, where nothing has run, unless a call that was given no result returned it and the statement materialises it. Also `Wait` on a task the expression made, tasks of results the statement made converted to plain ones (`Task.WaitAll`, `WaitAny`), and a `foreach` or `await foreach` over results its header made whose body never reads the iteration variable; a variable named `_` opts out. From 2.3.0, an assertion of `CodoMetis.TypeKit.Testing` on a `Task` or `ValueTask` of what it checks, dropped by an expression statement without `await`: `service.FindAsync(id).ShouldBeNone();`, the lowered static call, through `?.`, the body of a lambda that returns nothing, through `ConfigureAwait` or `WaitAsync` of the `Task` or `Task` it returns (every overload), and at each branch of a `?:`, `??` or `switch` expression so passed through, judged by the path every dropped call takes (#152); handed to `Task.WhenAll`, `Task.Run` or `Task.WhenEach` whose task is dropped, or to `Task.WhenAny` or `Task.WaitAny` in any use, as arguments, an array, a collection expression, a `Select`, a query, a `ToList`, a method group or an `async` lambda that awaits it with no `catch` between that keeps its failure, and a `WhenAny` of them converted to a plain `Task` or waited for, and a loop that never reads the assertions its header made (#157); `Task.WaitAll` throws, so it is silent. A combinator inside another is judged by how deep its task holds the failure: `WhenAll` and `Run` fault with a task that faults, `WhenAny`, `WhenEach` and an `async` lambda that returns the task complete with it as a value, each `await` unwraps one, and an awaited, waited-for or converted combinator observes only a failure at depth 0, so `await Task.WhenAll(Task.WhenAny(…))` and `await Task.Run(() => Task.WhenAny(…))` report, at any depth, and `await await Task.WhenAny(…)` is silent (#165). A `catch` keeps the failure only when its `try` block throws it, its type is or is a base of `TypeKitAssertionException` (resolved in the Testing assembly), and its handler can complete, which a constant `false` filter leaves unreachable: `catch { throw; }`, a log and `throw;`, `throw new …`, `catch (OperationCanceledException)` and `when (false)` hand it on and report; a filter that is no constant counts as keeping it, since an Error reports only a failure lost whatever the filter answers (#166). It fails the test only through its task. Known by the package's assembly, so a consumer's extension of the same name on a pending result is silent; awaited, held, returned or discarded with `_ =`, it is silent too, and so are held tasks combined. Also from 2.3.0, on tasks of results, forms 2.2.0 code can contain: a `Task.WhenAny` of tasks the statement made, in any use (#159; of held tasks it is silent, where 2.2.0 reported it awaited), `Wait()` on a `?:`, `??` or `switch` (#160), an array written in `Task.WaitAll` or `Task.WaitAny` (#161) and a dropped `Task.WhenEach` (#162). All held by `DroppedTaskVariantTests` over `Variants/DroppedTaskShapes`, 1837 cases. CA1806 can only enforce this per method name, not per return type | Error | shipped in 1.0.0 at Warning; Error in 2.0.0 ([decision 50](adr/0050-warnings-cmtk0003-alone-becomes-error-2.md)), its 1.x gaps closed first; the dropped pending assertion in 2.3.0, a form only 2.3.0's API writes ([decision 118](adr/0118-an-error-may-learn-a-form-only-new-api-can-write.md)), its nested combinators and catches (#165, #166) included; the 2.2-writable forms #159 to #162 in 2.3.0, gaps found before the tag ([decision 119](adr/0119-a-gap-found-before-the-tag-ships-in-that-release.md)) | | CMTK0004 | `IValueObjectMaterializer<,>.Materialize`, `ValueObjectConverter<,>.Materialize` or a hand-written value object's public `Materialize`, called or referenced (expression trees included). The satellite's own call is compiled in the satellite; EF's compiled model is generated code, whose diagnostics are not reported (measured: reported, it is six errors in the smoke test's AOT consumer). `StoredJsonConverterFactory` goes through `IStoredJsonConverterSource`, not `Materialize` | Error | shipped in 1.0.0 | -| CMTK0005 | `new T[n]`, `stackalloc T[n]`, `GC.AllocateUninitializedArray/AllocateArray` or `Array.Resize` of a no-default struct: every new slot starts as `default`. A constant length of zero (for `Array.Resize` and the `GC` methods too) or initial elements are silent | Warning | shipped in 1.0.0 | +| CMTK0005 | `new T[n]`, `stackalloc T[n]`, `GC.AllocateUninitializedArray/AllocateArray` or `Array.Resize` of a no-default struct: every new slot starts as `default`. A constant length of zero (for `Array.Resize` and the `GC` methods too) or initial elements are silent. From 2.4.0 also `CollectionsMarshal.SetCount(list, count)`, resolved by symbol, which makes room in a `List` without an item: like `Array.Resize` it adds slots only when the count grows, and a slot may hold an item removed earlier, which the rule cannot tell; a constant zero is silent (#229) | Warning | shipped in 1.0.0; `SetCount` in 2.4.0, a Warning reporting more, within [decision 34](adr/0034-errors-fixed-within-major-version-warnings.md) | | CMTK0006 | Field or auto-property of a no-default struct type in a class that no initializer, `required` or constructor sets; a non-public parameterless constructor (EF, serializers) and a record's copy constructor are exempt. Measured 2026-09-28 on the repository's EF entities and request types: 12 of 12 reports on settable properties of a type with only the implicit constructor, each a real way to a default. `required`, the fix the message names where it compiles, works with EF's compiled model, precompiled queries and Native AOT (the smoke test rerun with it). A `[SetsRequiredMembers]` constructor must assign the `required` members it promises | Warning | shipped in 1.0.0 | | CMTK0007 | `FromKnownGood` given a value straight from a parameter of the enclosing method or lambda, or a member or element of one. A value the code produced itself is legitimate and silent; a parameter copied into a local first is not seen. Info, since test theories and helpers that only receive constants are reported too | Info | shipped in 1.0.0 | | CMTK0008 | The wrapped values of two different value objects compared (`==`, `!=`, ordering, `Equals`, `CompareTo`) through `.Value`, `?.Value`, `GetValue()` or `ValueOrNull()`: the typed ids unwrapped into the bug they prevent. Recognised by syntax, since `.Value` of a same-project value object does not bind where analyzers run. Also `string.Equals`, `object.Equals`, a comparer's `Equals`, `string.Compare`/`CompareOrdinal` over two such values (the forms MA0006 rewrites `==` into), and two value objects of different types through `Equals(object)`, which is always false. Also a LINQ join on two such values: a query's `join … on a.Value equals b.Value`, with or without `into`, and `Join`/`GroupJoin`/`LeftJoin`/`RightJoin` of `Enumerable`, `Queryable` and `AsyncEnumerable` whose key selectors are lambdas returning them, resolved by symbol, with the candidates where a key selector does not bind; a composite key is not looked into. A raw value on one side, or an explicit cast, is silent | Warning | shipped in 1.0.0 | diff --git a/src/CodoMetis.TypeKit.Analyzers/DefaultFilledCollectionAnalyzer.cs b/src/CodoMetis.TypeKit.Analyzers/DefaultFilledCollectionAnalyzer.cs index 1707b0b..e6e5f77 100644 --- a/src/CodoMetis.TypeKit.Analyzers/DefaultFilledCollectionAnalyzer.cs +++ b/src/CodoMetis.TypeKit.Analyzers/DefaultFilledCollectionAnalyzer.cs @@ -10,8 +10,8 @@ namespace CodoMetis.TypeKit.Analyzers; /// -/// CMTK0005: an array or span of a struct that forbids default, created with a length, so -/// every element starts as a default instance. +/// CMTK0005: an array, span or list of a struct that forbids default, created or sized with a +/// length, so every element starts as a default instance. /// /// /// CMTK0001 sees default written out, and none of these forms writes it: new OrderId[n], @@ -21,7 +21,10 @@ namespace CodoMetis.TypeKit.Analyzers; /// Array.CreateInstance(typeof(OrderId), n), Array.CreateInstanceFromArrayType(typeof(OrderId[]), n) /// and ArrayPool<OrderId>.Rent(n) (#228): the first two only where a typeof names the element, /// since a Type variable could be anything, and a rented array is reported as a fresh one, all -/// default, though a reused one holds what it was returned with. An array with initial +/// default, though a reused one holds what it was returned with. And CollectionsMarshal.SetCount (#229), +/// which makes room in a List<OrderId> without putting an item there: like Array.Resize it adds +/// slots only when the count is larger, and like a rented array a slot may hold an item removed earlier, which +/// the rule cannot tell either. An array with initial /// elements, a constant length or size of zero in any one dimension and a collection expression start /// with no default element and are not reported. A warning, since filling such an array in a loop /// straight after is correct code the rule cannot tell apart. @@ -59,8 +62,12 @@ public override void Initialize(AnalysisContext context) var array = start.Compilation.GetTypeByMetadataName("System.Array"); var pool = start.Compilation.GetTypeByMetadataName("System.Buffers.ArrayPool`1"); + // CollectionsMarshal.SetCount(List, int), from .NET 8: the method itself, compared by symbol. + var setCount = start.Compilation.GetTypeByMetadataName("System.Runtime.InteropServices.CollectionsMarshal")?.GetMembers("SetCount").OfType().ToImmutableHashSet(SymbolEqualityComparer.Default) + ?? ImmutableHashSet.Empty; + start.RegisterOperationAction(operation => AnalyzeArrayCreation(operation, symbols), OperationKind.ArrayCreation); - start.RegisterOperationAction(operation => AnalyzeInvocation(operation, symbols, gc, array, pool), OperationKind.Invocation); + start.RegisterOperationAction(operation => AnalyzeInvocation(operation, symbols, gc, array, pool, setCount), OperationKind.Invocation); // IOperation has no public node for stackalloc; the syntax is read instead. start.RegisterSyntaxNodeAction(node => AnalyzeStackAlloc(node, symbols), SyntaxKind.StackAllocArrayCreationExpression); @@ -78,7 +85,7 @@ private static void AnalyzeArrayCreation(OperationAnalysisContext context, TypeK if (Diagnose(context.ContainingSymbol, creation.Syntax, "An array created with a length", element, symbols) is { } diagnostic) context.Report(diagnostic); } - private static void AnalyzeInvocation(OperationAnalysisContext context, TypeKitSymbols symbols, INamedTypeSymbol? gc, INamedTypeSymbol? array, INamedTypeSymbol? arrayPool) + private static void AnalyzeInvocation(OperationAnalysisContext context, TypeKitSymbols symbols, INamedTypeSymbol? gc, INamedTypeSymbol? array, INamedTypeSymbol? arrayPool, ImmutableHashSet setCount) { var invocation = (IInvocationOperation)context.Operation; var method = invocation.TargetMethod; @@ -115,6 +122,12 @@ private static void AnalyzeInvocation(OperationAnalysisContext context, TypeKitS form = "ArrayPool.Rent"; element = pool.TypeArguments[0]; } + else if (setCount.Contains(method.OriginalDefinition)) + { + // (List list, int count): every slot past the list's old count holds no item it was given. + form = "CollectionsMarshal.SetCount"; + element = method.TypeArguments[0]; + } if (form is null || element is null) return; diff --git a/src/CodoMetis.TypeKit.Analyzers/README.md b/src/CodoMetis.TypeKit.Analyzers/README.md index e692396..e44cdde 100644 --- a/src/CodoMetis.TypeKit.Analyzers/README.md +++ b/src/CodoMetis.TypeKit.Analyzers/README.md @@ -30,7 +30,7 @@ project. The two packages share a version and move together. | CMTK0002 | Error | A type implements `IValue`, `IValidatedValue<,,>` or `IClosedSet`, but the compilation doesn't reference `CodoMetis.TypeKit.Generators`. So nothing is generated for it: no field, no `Value`, no factory, or for a closed set no name and no `Match`. | | CMTK0003 | Error | A `Result` or `Option` that a call returns, or a collection or a tuple of them that the statement made, dropped. It counts as dropped as a statement, awaited or not; when its `Task` is converted to a plain `Task`, also where a method group is the conversion; through `Wait`; in a loop that never reads it; and in a branch of a conditional, a `switch` expression or a `??` that picked the call. Also an assertion of `CodoMetis.TypeKit.Testing` on a task, left without `await`. `_` says you mean the drop. | | CMTK0004 | Error | `Materialize`, which rebuilds a value object without its rules, called or referenced anywhere but the EF Core package. | -| CMTK0005 | Warning | An array or span of a value object, `Option`, `Result` or `[RequireCustomInitialization]` struct created with a length, so every slot starts as `default`. A constant length of zero, in any dimension, isn't reported. | +| CMTK0005 | Warning | An array, span or list of a value object, `Option`, `Result` or `[RequireCustomInitialization]` struct given a length, so every slot starts as `default`. A constant length of zero, in any dimension, isn't reported. | | CMTK0006 | Warning | A field or auto-property of such a type, in a class, that no initializer, `required` or constructor sets. Also a `required` one that a `[SetsRequiredMembers]` constructor doesn't set. | | CMTK0007 | Info | `FromKnownGood` given a value that comes straight from a parameter. | | CMTK0008 | Warning | The values inside two different value objects compared, `order.CustomerId.Value == product.Id.Value`, or the two value objects themselves compared through `Equals(object)`. | @@ -304,9 +304,11 @@ OrderId[] kept = [.. rows.Select(r => OrderId.From(r.Id))]; // built from valu `stackalloc`, `GC.AllocateUninitializedArray`, `GC.AllocateArray` and `Array.Resize` are reported too, and so are `Array.CreateInstance` and `Array.CreateInstanceFromArrayType` when a `typeof` names the element, and `ArrayPool.Rent`. A `Type` held in a variable could be anything, so it isn't -reported. `Array.Resize` adds default slots whenever the new size is larger, which the rule can't tell. A -constant length or size of zero creates no slot, as `new OrderId[0]` doesn't, and isn't reported. That -holds for any one dimension: `new OrderId[0, count]` holds no element either. +reported. A list counts too: `CollectionsMarshal.SetCount(ids, count)` on a `List` makes room +for `count` items without putting one there. `Array.Resize` and `SetCount` add default slots whenever +the new size is larger, which the rule can't tell. A constant length or size of zero creates no slot, as +`new OrderId[0]` doesn't, and isn't reported. That holds for any one dimension: `new OrderId[0, count]` +holds no element either. It's a warning, because an array filled in a loop straight after it is created is correct, and the rule can't tell it from one that isn't. diff --git a/test/CodoMetis.TypeKit.Analyzers.Tests/DefaultFilledCollectionAnalyzerTests.cs b/test/CodoMetis.TypeKit.Analyzers.Tests/DefaultFilledCollectionAnalyzerTests.cs index 7807f1b..c2ef697 100644 --- a/test/CodoMetis.TypeKit.Analyzers.Tests/DefaultFilledCollectionAnalyzerTests.cs +++ b/test/CodoMetis.TypeKit.Analyzers.Tests/DefaultFilledCollectionAnalyzerTests.cs @@ -6,13 +6,15 @@ namespace CodoMetis.TypeKit.Analyzers.Tests; -/// CMTK0005: arrays and spans of a no-default struct, created with a length. +/// CMTK0005: arrays, spans and lists of a no-default struct, created or sized with a length. public sealed class DefaultFilledCollectionAnalyzerTests { private const string Subjects = """ using System; using System.Buffers; + using System.Collections.Generic; + using System.Runtime.InteropServices; using CodoMetis.TypeKit; using CodoMetis.TypeKit.ValueObjects; @@ -238,10 +240,76 @@ public static void Run(int n, Type type, Plain a) .RunAsync(TestContext.Current.CancellationToken); /// - /// ArrayPool<T>.Rent and the typeof factories judge their element as + /// CollectionsMarshal.SetCount makes room in a list without putting an item there, as + /// Array.Resize does in an array, and was not reported (#229). Across the input class, a list's + /// count set by that call: by element type, with the type argument inferred or written, the count named, + /// through a type parameter, and on a list a derived class holds. + /// + [Fact] + public Task Setting_a_list_s_count_reports() => + Test( + """ + public sealed class Ids : List { } + + public static class C + { + public static void Run(int n, List plain, List> options, List> results, List guarded, Ids ids) + { + {|#0:CollectionsMarshal.SetCount(plain, n)|}; + {|#1:CollectionsMarshal.SetCount(plain, 3)|}; + {|#2:CollectionsMarshal.SetCount(count: n, list: plain)|}; + {|#3:CollectionsMarshal.SetCount(options, n)|}; + {|#4:CollectionsMarshal.SetCount(results, n)|}; + {|#5:CollectionsMarshal.SetCount(guarded, n)|}; + {|#6:CollectionsMarshal.SetCount(ids, n)|}; + } + + public static void Generic(List list, int n) where T : struct, IValue => {|#7:CollectionsMarshal.SetCount(list, n)|}; + } + """, + Cmtk0005(0, "CollectionsMarshal.SetCount", "Plain"), + Cmtk0005(1, "CollectionsMarshal.SetCount", "Plain"), + Cmtk0005(2, "CollectionsMarshal.SetCount", "Plain"), + Cmtk0005(3, "CollectionsMarshal.SetCount", "Option"), + Cmtk0005(4, "CollectionsMarshal.SetCount", "Result"), + Cmtk0005(5, "CollectionsMarshal.SetCount", "Guarded"), + Cmtk0005(6, "CollectionsMarshal.SetCount", "Plain"), + Cmtk0005(7, "CollectionsMarshal.SetCount", "T")) + .RunAsync(TestContext.Current.CancellationToken); + + /// + /// A count of zero empties the list and adds no slot, as new Plain[0] creates none, however the + /// zero is written; a list of another element type, a nullable value object's included, has no slot to judge. + /// + [Fact] + public Task Setting_a_list_s_count_to_zero_or_of_another_type_stays_silent() => + Test( + """ + public static class C + { + public static void Run(int n, List plain, List numbers, List nullable, List classes) + { + const int none = 0; + CollectionsMarshal.SetCount(plain, 0); + CollectionsMarshal.SetCount(plain, count: 0); + CollectionsMarshal.SetCount(count: none, list: plain); + CollectionsMarshal.SetCount(plain, (byte)0); + CollectionsMarshal.SetCount(plain, '\0'); + CollectionsMarshal.SetCount(numbers, n); + CollectionsMarshal.SetCount(nullable, n); + CollectionsMarshal.SetCount(classes, n); + plain.Capacity = n; + var sized = new List(n); + } + } + """) + .RunAsync(TestContext.Current.CancellationToken); + + /// + /// ArrayPool<T>.Rent, CollectionsMarshal.SetCount and the typeof factories judge their element as /// GC.AllocateArray<T> does, across every kind of : each /// reports exactly where the generic allocator does. typeof takes neither dynamic (CS1962) nor a nullable reference type - /// (CS8639), so those two kinds run Rent alone. + /// (CS8639), so those two kinds run Rent and SetCount alone. /// [Theory] [MemberData(nameof(Kinds))] @@ -253,6 +321,7 @@ public async Task The_factories_judge_each_kind_as_the_generic_allocator_does(st $$""" #nullable enable using System.Buffers; + using System.Runtime.InteropServices; public static class Use{{kind.Parameters}} {{kind.Where}} { @@ -260,6 +329,7 @@ public static void M(int n) { _ = GC.AllocateArray<{{kind.Argument}}>(n); _ = ArrayPool<{{kind.Argument}}>.Shared.Rent(n); + CollectionsMarshal.SetCount(new List<{{kind.Argument}}>(), n); {{(typeOf ? $"_ = Array.CreateInstance(typeof({kind.Argument}), n);" : "")}} {{(typeOf ? $"_ = Array.CreateInstance(typeof({kind.Argument}), n, n);" : "")}} {{(typeOf ? $"_ = Array.CreateInstanceFromArrayType(typeof({kind.Argument}[]), n);" : "")}} @@ -272,7 +342,9 @@ public static void M(int n) var reported = forms.Contains("GC.AllocateArray"); if (kind is KindOfType { Is: var type } && type.HasFlag(Is.Struct) && type.HasFlag(Is.ValueObjectMarker)) reported.ShouldBeTrue("a value object struct is the positive control"); - string[] factories = typeOf ? ["ArrayPool.Rent", "Array.CreateInstance", "Array.CreateInstance", "Array.CreateInstanceFromArrayType"] : ["ArrayPool.Rent"]; + string[] factories = typeOf + ? ["ArrayPool.Rent", "CollectionsMarshal.SetCount", "Array.CreateInstance", "Array.CreateInstance", "Array.CreateInstanceFromArrayType"] + : ["ArrayPool.Rent", "CollectionsMarshal.SetCount"]; forms.Where(form => form != "GC.AllocateArray").Order().ShouldBe(reported ? factories.Order() : []); } diff --git a/test/oracles/claims.json b/test/oracles/claims.json index 212af7c..8855858 100644 --- a/test/oracles/claims.json +++ b/test/oracles/claims.json @@ -2848,6 +2848,20 @@ "DefaultFilledCollectionAnalyzerTests" ] }, + { + "id": "K05-a-list-given-a-count", + "source": "src/CodoMetis.TypeKit.Analyzers/README.md#cmtk0005", + "quote": "A list counts too: `CollectionsMarshal.SetCount(ids, count)` on a `List` makes room for `count` items without putting one there.", + "claim": "CMTK0005 reports CollectionsMarshal.SetCount on a List of a no-default type, unless the count is a constant zero.", + "oracle": "claim", + "surfaces": [ + "IValueObject.*", + "IValidatedValue.*" + ], + "heldBy": [ + "DefaultFilledCollectionAnalyzerTests" + ] + }, { "id": "K05-a-zero-dimension-is-silent", "source": "src/CodoMetis.TypeKit.Analyzers/README.md#cmtk0005",