Generate collection-shape drill-in overloads (#6185)

[thomhurst]

What & why

Fixes #6185: when a dictionary value is itself a collection, await Assert.That(dict).ContainsKey(k).And.Value.Count() failed to compile (CS0411) because the .Value drill-in returned a scalar ValueAssertion lacking the collection surface, so .Count() bound to LINQ's Enumerable.Count.

Because DictionaryValueSource<T> and AssertionContext<T> are invariant, every value shape needs its own overload — hand-writing that is ~90+ methods per call site and drifts (e.g. ReadOnlyList.HasItemAt has an IEqualityComparer param the IList one doesn't). The same "one method per collection shape" boilerplate already existed in three places, all keyed off the same shape→assertion-source map.

Approach

New Roslyn incremental generator CollectionShapeFanOutGenerator — one hardcoded shape→source→seed registry (11 rows) + symbol reflection — with three emitters:

• A — .Value drill-in, full surface. [GenerateCollectionShapeDrillIns] on a drill-in source emits the full shape-specific surface (Count/Contains/HasItemAt/IsSubsetOf/ContainsKey/…) onto DictionaryValueSource<Shape>. Signatures are reflected from each shape's assertion source, so per-shape differences are always correct and new methods are picked up automatically. Reusable: any arity-1 drill-in source opts in with one attribute.
• B — replaces CollectionItemSatisfiesExtensions.cs (per-shape Satisfies).
• C — replaces the .Count(itemAssertion) per-item overload loses specialised source #5707 Count(itemAssertion) block in AssertionExtensions.cs.

Concrete value shapes (List<T>/Dictionary<K,V>) upcast their context via a generated Upcast helper that preserves PendingPreWork, so the ContainsKey pre-work still runs first (a missing key fails with the normal "contain key" message instead of reading default).

API impact

• Sites B & C reproduce their existing public API exactly — PublicAPI snapshots show zero removals across all 4 TFMs.
• Emitter A is a deliberate full-surface expansion (the .Value drill-in now exposes the complete per-shape collection surface).

Validation

• Builds on netstandard2.0 / net8.0 / net9.0 / net10.0 (only IReadOnlySet rows are #if NET5_0_OR_GREATER; the Satisfies file is #if !NETSTANDARD2_0).
• 2168 assertions tests + 69 source-generator tests pass.
• PublicAPI approval snapshots re-accepted for all TFMs (zero removals for B/C).
• New tests cover list HasItemAt, set IsSubsetOf, nested-dict ContainsKey, array, and concrete List<T>/Dictionary<K,V> incl. the missing-key pre-work path; plus a generator content-assertion test.

Notes

• The registry intentionally excludes Memory/ReadOnlyMemory/IAsyncEnumerable (not collection drill-in targets) — documented inline.
• Dictionary IsOrderedBy<TKey>/IsOrderedByDescending<TKey> are not forwarded onto dict drill-ins (method type param collides with the source's TKey) — a documented niche gap.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 1 minor

Alerts:
⚠ 2 issues (≤ 0 issues of at least minor severity)

Results:
2 new issues

Category	Results
ErrorProne	1 high
CodeStyle	1 minor

View in Codacy

🟢 Metrics 78 complexity

Metric	Results
Complexity	78

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review — PR #6218: Generate collection-shape drill-in overloads

This is a well-architected PR that solves a real problem elegantly. Using a Roslyn incremental generator to eliminate ~90+ hand-written per-shape forwarding methods per call site is the right structural approach — and the reflection-based method collection from real IMethodSymbols means per-shape signature differences (the IEqualityComparer on ReadOnlyList.HasItemAt vs. IList.HasItemAt) are always correct without manual upkeep.

Overall: approve-ready, with a few items worth considering.

---

What works well

• Incremental generator design — WithTrackingName on each pipeline stage, fully equatable ImmutableEquatableArray-based models, no ISymbol stored across the incremental boundary. This is textbook correct incremental generator practice.
• Upcast + PendingPreWork preservation — the concrete-shape upcast (List<T> → IList<T>) chains the pre-work lambdas so a missing key still surfaces the clean "contain key" failure. The sequencing of existing and preWork is correct.
• Zero removals on Emitters B & C — the PublicAPI snapshots confirm exact API parity with the hand-written code they replace.
• CollectMethods most-derived-wins walk — stops before Assertion<T> / object, deduplicates by Name + param types, and skips Obsolete members. All correct.
• Test depth — snapshot tests for generator output fidelity, integration tests covering the original bug, missing-key pre-work path, and concrete (List<T>/Dictionary<K,V>) shapes.

---

Issues

1. Emitter B / C trigger multiplicity — silent duplicate-file risk

fires once per item in the pipeline. If a second method ever carries (now or in a future refactor), Roslyn will try to add twice → CS8785 duplicate hint name diagnostic. The same applies to Emitter C.

The fix is cheap — deduplicate with and emit once:

This is low risk today since each attribute appears exactly once, but a single misplaced attribute could cause a confusing build failure.

2. Three types are now public API

The PublicAPI snapshot now includes , , and as public types. If these are only intended as internal generator triggers (no downstream library author would ever apply them to their own types), marking them would avoid confusion and API churn.

If they are intentional extensibility points — so that consumers can write their own drill-in sources — they should be documented as such. The current XML docs describe what the generator does but not whether the attribute is intended for public use.

3. is fragile without explanation of each entry

A new assertion method added to a source class whose name isn't in this set will silently appear on all drill-in surfaces. The reverse — a needed method accidentally blocked — is harder to notice. A short grouped comment categorising why each name is excluded ("generic value-shape methods", "infrastructure / lifecycle", "System.Object") would help a future maintainer decide where a new entry belongs. Alternatively, an attribute-based approach () on the sources themselves would be self-documenting and easier to audit.

4. helper is always emitted even when no upcast shapes are needed

If a drill-in source only maps to interface-shaped collection types (no / concrete shapes in the Rows registry), the helper is dead code in that file. Minor, but emitting it conditionally based on whether any row actually produced methods for this model would keep generated files cleaner.

---

Minor observation

The documented gap — "Dictionary / are not forwarded onto dict drill-ins (method type param collides with the source's )" — is correctly handled and documented inline. No action needed, just flagging it as confirmed correct behavior.

---

Summary: The architecture is sound, tests are thorough, and public API impact is controlled. Items 1 and 2 are the ones most worth addressing before merge; 3 and 4 are lower priority.

[claude]

Code Review — PR #6218: Generate collection-shape drill-in overloads

This is a well-architected PR that solves a real problem elegantly. Using a Roslyn incremental generator to eliminate ~90+ hand-written per-shape forwarding methods per call site is the right structural approach — and the reflection-based method collection from real IMethodSymbols means per-shape signature differences (the IEqualityComparer on ReadOnlyList.HasItemAt vs. IList.HasItemAt) are always correct without manual upkeep.

Overall: approve-ready, with a few items worth considering.

---

What works well

• Incremental generator design — WithTrackingName on each pipeline stage, fully equatable ImmutableEquatableArray-based models, no ISymbol stored across the incremental boundary. Textbook correct incremental generator practice.
• Upcast + PendingPreWork preservation — the concrete-shape upcast (List<T> to IList<T>) chains the pre-work lambdas so a missing key still surfaces the clean "contain key" failure. The sequencing of existing and preWork is correct.
• Zero removals on Emitters B & C — the PublicAPI snapshots confirm exact API parity with the hand-written code they replace.
• CollectMethods most-derived-wins walk — stops before Assertion<T> / object, deduplicates by Name + param types, and skips Obsolete members. All correct.
• Test depth — snapshot tests for generator output fidelity, integration tests covering the original bug, missing-key pre-work path, and concrete (List<T> / Dictionary<K,V>) shapes.

---

Issues

1. Emitter B / C trigger multiplicity — silent duplicate-file risk

RegisterSourceOutput fires once per item in the pipeline. If a second method ever carries [GenerateCollectionShapeItemSourceOverloads] (now or in a future refactor), Roslyn will try to add CollectionItemSatisfiesExtensions.g.cs twice — CS8785 duplicate hint name. The same applies to Emitter C.

The fix is cheap — deduplicate with .Collect() and emit once:

context.RegisterSourceOutput(
    itemSourceTrigger.Collect(),
    static (spc, items) => { if (items.Length > 0) EmitItemSourceOverloads(spc); });

This is low risk today since each attribute appears exactly once, but a single misplaced attribute would produce a confusing build failure with no obvious cause.

2. Three Generate*Attribute types are now public API

The PublicAPI snapshot now includes GenerateCollectionShapeDrillInsAttribute, GenerateCollectionShapeCountOverloadsAttribute, and GenerateCollectionShapeItemSourceOverloadsAttribute as public types. If these are only intended as internal generator triggers (no downstream library author would ever apply them), marking them internal would avoid API churn and consumer confusion.

If they are intentional extensibility points — so that consumers can write their own drill-in sources — they should be documented as such. The current XML docs describe what the generator does but do not clarify whether downstream use is supported.

3. ExcludedNames is fragile without rationale for each entry

A new assertion method added to a source class whose name is not in this set will silently appear on all drill-in surfaces. The reverse — a needed method accidentally blocked — is harder to notice. Grouped comments explaining why each name is excluded (e.g. "generic value-shape methods already on IAssertionSource", "infrastructure / lifecycle", "System.Object inherited") would help a future maintainer decide where a new entry belongs.

4. Upcast helper always emitted regardless of whether upcast shapes appear

The private Upcast<TFrom, TTo> helper (~15 lines) is emitted unconditionally at the top of every generated file. If a drill-in source produces no UpcastCtor-seeded methods (no List<T> or Dictionary<K,V> concrete shapes in the output), the helper is dead code in that file. Minor, but emitting it conditionally based on whether any UpcastCtor row produced methods would keep generated files cleaner.

---

Minor observation

The documented gap — "Dictionary IsOrderedBy<TKey> / IsOrderedByDescending<TKey> are not forwarded onto dict drill-ins (method type param collides with the source's TKey)" — is correctly handled and explained inline. Confirmed as expected behavior.

---

Summary: The architecture is sound, tests are thorough, and API impact is controlled. Items 1 and 2 are most worth addressing before merge; 3 and 4 are lower-priority cleanup.

[claude]

Follow-up Review — PR #6218

This is a follow-up to the two prior reviews (both posted 2026-06-10). All four issues raised remain unaddressed in the current code. Summarising the outstanding items:

---

1. Emitter B / C trigger multiplicity — still unresolved

RegisterSourceOutput still fires once per item without .Collect() deduplication:

// Current (fragile):
context.RegisterSourceOutput(satisfiesTrigger, static (spc, _) => EmitSatisfiesOverloads(spc));
context.RegisterSourceOutput(countTrigger,    static (spc, _) => EmitCountOverloads(spc));

If a second method ever carries either attribute, Roslyn will attempt to add the same hint name twice → CS8785, with no obvious error pointing back to this code. The fix is a one-liner:

context.RegisterSourceOutput(satisfiesTrigger.Collect(),
    static (spc, items) => { if (items.Length > 0) EmitSatisfiesOverloads(spc); });

2. Three Generate*Attribute types still public API

GenerateCollectionShapeAssertionsAttribute, GenerateCollectionShapeSatisfiesOverloadsAttribute, and GenerateCollectionShapeCountOverloadsAttribute are all public sealed class. If these are purely internal generator triggers (no intended downstream use), they should be internal to avoid locking in an accidental API surface. If they are intended extensibility points, they need documentation saying so.

3. ExcludedNames still lacks per-entry rationale

The block comment ("Names already present on IAssertionSource<T> / ValueAssertion<T> / Assertion<T> / object, or that are pure infrastructure") is too generic to guide future decisions. When a new assertion method is added, a maintainer can't tell from this comment whether to include or exclude it — and a wrong call silently changes the public API surface of all drill-in types. Short grouped comments (or an attribute-based opt-out on the source class itself) would make this self-documenting.

4. Upcast helper still emitted unconditionally

The private Upcast\<TFrom, TTo\> helper is emitted at the top of every EmitWrapperAssertions output file regardless of whether any SeedKind.UpcastCtor shape actually appears. Only two of the eleven rows use UpcastCtor (List<T> and Dictionary<K,V>); for drill-in sources that produce no methods from those shapes, the helper is dead code in the generated file.

---

Priority: Issues 1 and 2 are the ones most worth fixing before merge — 1 is a latent build-breaker, 2 is an API decision that's expensive to reverse. Issues 3 and 4 are lower-priority cleanup. The core architecture remains solid.

[claude]

Code Review

This is a well-structured solution to the CS0411 ambiguity bug. The approach of using a Roslyn incremental generator to replace hand-written fan-out is clearly the right architectural direction — it eliminates 3 separate copy-paste sites and makes new collection-shape assertion methods automatically available on all drill-in targets.

Correctness

Upcast + PendingPreWork transfer is correct. The Upcast helper correctly handles the two-step context flow: when .Value is accessed, Context.Map<TValue>() consumes the PendingLink (the ContainsKey assertion) and stores it as PendingPreWork on the new value context. Later, when Upcast calls context.Map(upcast) internally, it doesn't re-consume a PendingLink (already gone), but the helper manually reads context.PendingPreWork and threads it into mapped.PendingPreWork. This correctly ensures the ContainsKey check fires first regardless of which collection-shape extension is used. The test Dictionary_ContainsKey_And_Value_Collection_Fails_When_Key_Missing covers this path.

Set SeedKind.FromContext vs Ctor distinction is justified. SetAssertion, HashSetAssertion, and ReadOnlySetAssertion have private protected context constructors (not internal), so the generator can't emit new SetAssertion<T>(ctx) from the generated extension file. The FromContext static factory methods added in this PR bridge that gap cleanly without widening constructor visibility.

Emitter C constructs sources with new SourceClosed(item, expression) — this is safe. For set shapes in Emitter C, new SetAssertion<TInner>(item, expression) uses the public SetAssertion(ISet<TItem>? value, string expression) ctor which is accessible. This is consistent with how the hand-written code worked before.

Design Observations

Three-Emitter architecture is sensible but slightly surprising. Emitters B and C are triggered by attributes on methods ([GenerateCollectionShapeSatisfiesOverloads], [GenerateCollectionShapeCountOverloads]) and use a static precomputed ItemShapes table rather than symbol reflection. This means they never have per-compilation data — they'll produce identical output on every compilation where the trigger attribute is present. The Collect() trick to deduplicate the hint name is correct but slightly unusual. Consider a brief inline comment (beyond what's already there) explaining why the attribute-on-a-method trigger pattern was chosen over a simpler assembly-level attribute, since it's not immediately obvious.

GenerateCollectionShapeCountOverloadsAttribute and GenerateCollectionShapeSatisfiesOverloadsAttribute are public. These two attributes appear in the PublicAPI snapshots and are placed on interface/private members, so end users cannot usefully apply them. They're purely internal wiring. Consider making them internal sealed to avoid cluttering the public API surface. GenerateCollectionShapeAssertionsAttribute is correctly public since it's documented as reusable.

The hardcoded Rows table is the right trade-off, but the maintenance coupling is worth documenting. Rows is used in three places: BuildWrapperModel (Emitter A, via symbol reflection), BuildItemShapes (Emitter B/C, via string formatting). If a new collection shape is added to TUnit, a developer must update Rows — which is a single place, so the coupling is minimal. However, Emitters B and C additionally embed constructor call syntax (new SourceClosed(item, ...)) which assumes each arity-1 shape has a public (TCollection, string) constructor. This invariant isn't enforced anywhere. A comment alongside Rows documenting this assumption (or a validator in BuildItemShapes) would make future additions safer.

int.Parse in BuildItemShapes is reliable but fragile. The arity is parsed from the metadata name substring after the backtick (\1→1). Since Rowsis a controlled static list this won't fail in practice, but a brief comment noting the dependency (or replacing withrow.SourceMetadataName.EndsWith("``1")` pattern matching) would guard against copy-paste errors when someone adds a new row.

CancellationToken is only checked per-row in BuildWrapperModel, not inside CollectMethods. For codebases where a single assertion source has many public methods, the inner foreach in CollectMethods could be slow without cancellation checks. Adding ct.ThrowIfCancellationRequested() inside CollectMethods would make the generator more IDE-responsive.

Minor

• The context.PendingPreWork after Map is not cleared in source.Context — this is fine because Assertion.ExecuteCoreAsync clears it on first run and DictionaryValueSource is not expected to be stored and re-used.
• The CollectionShapeAssertionSource<T> test data has Context { get; } = null!; which would NPE at runtime, but it's never executed — it's only compiled for the snapshot test. This is correct and common in generator test data.
• The ExcludedNames set correctly covers all public methods of ValueAssertion<T> (the base of DictionaryValueSource) so no double-forwarding occurs.

Summary

The approach is architecturally sound, the correctness is well-tested, and the PR description is exceptionally thorough. The two points worth addressing before merge are:

1. Make GenerateCollectionShapeCountOverloadsAttribute and GenerateCollectionShapeSatisfiesOverloadsAttribute internal to avoid unnecessary public API surface.
2. Add a comment alongside Rows documenting the (TCollection, string) constructor assumption for Emitters B and C.

Everything else is either already documented or minor style.

[claude]

Code Review — PR #6218 (Follow-up: Issues Resolved)

This is a follow-up to the three prior reviews posted 2026-06-10. All critical and high-priority issues have been addressed in the current code.

---

Issues resolved since last review

✅ Issue #1 — Emitter B/C trigger multiplicity
Both satisfiesTrigger and countTrigger now use .Collect() with a triggers.Length > 0 guard:

context.RegisterSourceOutput(satisfiesTrigger, static (spc, triggers) =>
{
    if (triggers.Length > 0) EmitSatisfiesOverloads(spc);
});

A duplicate [GenerateCollectionShape*] attribute on a second method will no longer silently cause a CS8785 duplicate hint name at build time.

✅ Issue #2 — Generate*Attribute types now internal sealed
All three attributes (GenerateCollectionShapeAssertionsAttribute, GenerateCollectionShapeSatisfiesOverloadsAttribute, GenerateCollectionShapeCountOverloadsAttribute) are internal sealed. The PublicAPI snapshots no longer include them, and the API surface is smaller and cleaner.

✅ Issue #3 — ExcludedNames now has grouped rationale comments
Each exclusion group is now explained: re-forwarding-would-be-ambiguous, lifecycle/infrastructure, and System.Object inherited members. A future maintainer adding a new assertion method can now decide whether to include or exclude it with confidence.

---

Remaining minor item

Issue #4 — Upcast helper still emitted unconditionally (minor)

In EmitWrapperAssertions (line 438 of the generator), the private Upcast<TFrom, TTo> helper (~15 lines) is always emitted at the top of every generated file, even when no SeedKind.UpcastCtor-seeded shape produces any methods. Only two of the eleven Rows entries use UpcastCtor (List<T> and Dictionary<K,V>). For a drill-in source whose model.Shapes produce no upcast-seeded methods, the helper is dead code in the output file.

The fix is adding a bool HasUpcastShapes field to WrapperModel:

private sealed record WrapperModel(
    string WrapperName,
    string WrapperOpenType,
    ImmutableEquatableArray<ShapeModel> Shapes,
    bool HasUpcastShapes);  // <-- new

Set it in BuildWrapperModel when any UpcastCtor row produced at least one method, and gate the emission in EmitWrapperAssertions:

if (model.HasUpcastShapes)
{
    sb.AppendLine("    private static global::TUnit.Assertions.Core.AssertionContext<TTo> Upcast...");
    ...
}

This is purely a cleanliness concern — the generated code is functionally correct as-is and the compiler will warn on unused private members in consuming code anyway. It does not block merge.

---

Summary

The three blocking/high-priority issues from prior reviews are fully resolved. The architecture is sound, the incremental generator model is correctly equatable with no ISymbol stored across the boundary, the API surface is now clean, and test coverage is thorough.

Verdict: approve-ready. Issue #4 is a nice-to-have that can be addressed in a follow-up if desired.

[claude]

Code Review — Follow-up (Round 6)

Previous reviews flagged four issues; three were resolved in commits 2 and 3 (emit-once guard, internal sealed attributes, ExcludedNames rationale comments). This round verifies the remaining open item and checks the latest styling commit.

Issue #4 — Upcast helper always emitted (still open, still minor)

The helper is emitted unconditionally at the top of EmitWrapperAssertions before any shape is visited:

// CollectionShapeAssertionGenerator.cs — EmitWrapperAssertions
sb.AppendLine("    private static global::TUnit.Assertions.Core.AssertionContext<TTo> Upcast<TFrom, TTo>(");
// ... always appended regardless of model.Shapes contents

Because the current Rows table always includes UpcastCtor entries (List<T> and Dictionary<K,V>), in practice the helper is never dead code today. The risk is if a future caller uses [GenerateCollectionShapeAssertions] on a source that happens not to map any upcast shapes — they'd get an unused private method in the generated file. This is a minor aesthetic issue (and won't cause a compile error in C#), not a correctness concern. Can be addressed in a follow-up if desired.

A simple guard would be:

bool hasUpcastShapes = model.Shapes.Any(s => s.Methods.Any(/* seed was UpcastCtor */));
// or, since ShapeModel doesn't carry that flag, just:
bool hasUpcastShapes = model.Shapes
    .SelectMany(s => s.Methods)
    .Any(m => m.Body.Contains("Upcast<"));
if (hasUpcastShapes) { /* emit Upcast helper */ }

Latest commit — styling pass looks clean

The braces-normalization and arity adjustments in the final commit are purely mechanical and correct. No new issues introduced.

Ctor assumption

The SeedKind.Ctor branch emits new {sourceClosed}(source.Context) — silently assuming every assertion source has a single-arg (AssertionContext<T>) constructor. The Rows table comment describes what each row is, but not why the ctor is assumed to accept a single context argument. Adding a one-liner like // All Ctor seeds must have a public .ctor(AssertionContext<T>) near the SeedKind.Ctor case would make this contract explicit for future maintainers. Non-blocking.

---

Summary

All previously blocking issues (Issues 1–3) are resolved. Issue #4 (unconditional Upcast emission) and the undocumented ctor assumption are both non-blocking and can be addressed in a follow-up. The core logic — Upcast+PendingPreWork transfer, reflection-driven method surface, per-shape #if guards, and the Collect() deduplication for Emitters B/C — is correct. The PR is approve-ready.