feat: add TUnit.Assertions.Should package

[thomhurst]

Summary

Adds TUnit.Assertions.Should, a source-generated FluentAssertions-style Should() API layered on top of TUnit.Assertions with zero duplication of assertion logic. Every [AssertionExtension] class in the referenced assembly gets a Should-flavored counterpart (IsEqualTo→BeEqualTo, Contains→Contain, IsNotNull→NotBeNull, Throws→Throw, etc.) emitted into TUnit.Assertions.Should.dll at build time.

What's in the box

• Runtime (TUnit.Assertions.Should):
  • IShouldSource<T> marker, ShouldSource<T> (struct entry for values), ShouldCollectionSource<T> (collection entry exposing element-typed instance methods BeInOrder/BeInDescendingOrder/All/Any/HaveSingleItem/HaveDistinctItems), ShouldDelegateSource<T> (delegate entry with Throw<E>/ThrowExactly<E>).
  • ShouldAssertion<T> per-call wrapper — awaitable, exposes .And/.Or returning ShouldContinuation<T> so chains stay Should-flavored throughout.
  • Should() extension overloads on T, string, IEnumerable<T>, Action, Func<T?>, Func<Task>, Func<Task<T?>> — entry-overload narrowing mirrors Assert.That's pattern (priority-resolved, string doesn't bind to IEnumerable<char>, etc.).
  • [ShouldName("BeFoo")] — per-class override hatch for irregular names. The Should generator's name-conjugation rule turns Is*→Be*, IsNot*→NotBe*, Has*→Have*, DoesNot*→Not*, Does*→[strip prefix], and drops the 3rd-person -s/-es ending; classes whose names land awkwardly under those rules opt out per-class with [ShouldName]. Negated forms ([AssertionExtension(NegatedMethodName = "...")]) get their own conjugation pass and can be overridden by placing a separate [ShouldName] on the negated assertion class.
• Source generator (TUnit.Assertions.Should.SourceGenerator):
  • Cross-assembly metadata scan for IAssertionSource<T> extension methods. Per-reference walks cached by MetadataReference instance via ConditionalWeakTable so Roslyn drops stale entries automatically when references change.
  • Conjugation table: Is→Be, IsNot→NotBe, Has→Have, DoesNot→Not, Does→[strip], 3rd-person -s/-es drop. CamelCase boundary-aware so Issue doesn't become Besue.
  • [ShouldGeneratePartial(typeof(...))] marks Should-flavored wrapper classes (e.g. ShouldCollectionSource<TItem>) for which the generator emits instance-method partials covering each public instance method on the wrapped TUnit assertion type.
  • Forwards [Obsolete] and [EditorBrowsable] from [AssertionExtension] and [GenerateAssertion] source methods through to the Should-flavored API surface so deprecation/visibility decisions stay consistent across both entry styles.
  • Skips referenced assertions whose Should{Name}Extensions is already baked in another reference (prevents CS0121 duplicates) — user-authored assertions still emit fresh.
  • Honors [RequiresUnreferencedCode] propagation from class & ctor levels.
  • Incremental-cache friendly via EquatableArray<T> wrappers; per-compilation lookups hoisted to one-time hash sets.
  • Shares CovarianceHelper and AttributeForwardingFormatters with TUnit.Assertions.SourceGenerator via linked compile.

Test plan

• 103 Should consumer tests pass on net10.0
• 34 Should source generator tests (5 scenarios × 3 TFMs .verified.txt snapshots) pass on net8/9/10
• Value, string, collection (List/Array/HashSet/Stack/Queue), DateTime/DateTimeOffset/DateOnly/TimeOnly/TimeSpan
• Numeric: BeBetween, BeCloseTo, BeWithinPercentOf for int/long/decimal/float/double
• Chaining: .And 2-deep / 3-deep, .Or short-circuit, mixed And/Or throws MixedAndOrAssertionsException, Because propagates to failure message
• Assert.Multiple aggregation works
• Delegate: Throw<E>, ThrowExactly<E> strictness, Func<Task> async, single-evaluation, no-exception-throws, generic exception type names render without backtick mangling
• Element-type inference works without explicit generics for collection shapes (List, IReadOnlyCollection, HashSet, Stack, Queue, custom IEnumerable)
• User-defined [AssertionExtension] class in test assembly gets generated Should counterpart
• Failure messages include the Should-flavored method name in the expression chain
• Range obsolete shim: IsAll→BeAll flagged with [Obsolete], replaced by IsFullRange/BeFullRange
• All 810 analyzer tests pass (TUnit.Analyzers.Tests, TUnit.AspNetCore.Analyzers.Tests, TUnit.Mocks.Analyzers.Tests, TUnit.Assertions.Analyzers.Tests, TUnit.Assertions.Analyzers.CodeFixers.Tests)
• Full TUnit.slnx builds clean, no new warnings

[codacy-production]

Not up to standards ⛔

🔴 Issues 4 medium · 22 minor

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

Results:
26 new issues

Category	Results
UnusedCode	1 minor
BestPractice	1 medium
CodeStyle	21 minor
Performance	3 medium

View in Codacy

🟢 Metrics 333 complexity

Metric	Results
Complexity	333

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

This is a substantial and well-designed addition. The layered approach — zero duplication of assertion logic, source-generated conjugation, incremental-pipeline-friendly data structures — is architecturally sound. A few real issues and design concerns worth addressing before merge:

---

🔴 High: ShouldEntryPointAttribute is declared but never consumed

File: TUnit.Assertions.Should/Attributes/ShouldEntryPointAttribute.cs
File: TUnit.Assertions.Should.SourceGenerator/ShouldExtensionGenerator.cs

The PR description specifically calls out [assembly: ShouldEntryPoint(false)] as a "coexistence escape hatch for FluentAssertions users," but ShouldExtensionGenerator never reads this attribute. The ShouldExtensionGenerator.CollectAssertions method scans CompilationProvider without ever looking for this assembly attribute.

Why this matters: Any user who tries to use the documented escape hatch to prevent Should() extension method collisions will find it silently does nothing, leading to a confusing CS0121 ambiguity error.

Suggested fix:

// In CollectAssertions, after resolving symbols:
var entryPointAttr = compilation.GetTypeByMetadataName("TUnit.Assertions.Should.Attributes.ShouldEntryPointAttribute");
if (entryPointAttr is not null)
{
    var assemblyAttr = compilation.Assembly.GetAttributes()
        .FirstOrDefault(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, entryPointAttr));
    if (assemblyAttr is { ConstructorArguments: [{ Value: false }] })
        return ImmutableArray<AssertionData>.Empty;
}

---

🔴 High: EquatableArray<T> is the 4th copy in this repo

File: TUnit.Assertions.Should.SourceGenerator/EquatableArray.cs

The same type already exists at:

• TUnit.Core.SourceGenerator/Models/EquatableArray.cs
• TUnit.Mocks.SourceGenerator/Models/EquatableArray.cs
• TUnit.Assertions.SourceGenerator/Models/ImmutableEquatableArray.cs (slight variant)

The PR already uses the linked-file pattern for CovarianceHelper — the same approach should be used here. Four diverging copies mean any fix (e.g., a hash collision bug, a span equality edge case) must be applied in four places.

Better approach: Link the file from TUnit.Core.SourceGenerator/Models/EquatableArray.cs the same way CovarianceHelper is linked, rather than adding a new copy.

---

🟡 Medium: NeedsMethodLevelCovariance is permanently false — dead code in EmitMethod

File: TUnit.Assertions.Should.SourceGenerator/ShouldExtensionGenerator.cs

needsMethodLevelCovariance is set to false and never reassigned in CollectFromType. Yet EmitMethod has a full branch gated on data.NeedsMethodLevelCovariance:

if (data.NeedsMethodLevelCovariance)
{
    // This branch is unreachable today
    var covariantParam = CovarianceHelper.GetCovariantTypeParamName(classGenericList);
    ...
}

TypeParamConstraintName is also computed and stored but only used inside this dead branch. The same applies to TypeParamIsTypeParameter — populated but never read anywhere.

Why this matters: Dead code makes readers unsure what invariants hold, and CovarianceHelper logic is subtle. If the covariance path is intentionally deferred, document it with a TODO. If it's not needed, remove it (and the two unused fields from AssertionData).

---

🟡 Medium: ShouldCollectionSource mixes hand-crafted and generated methods without clear contract

File: TUnit.Assertions.Should/Core/ShouldCollectionSource.cs

ShouldCollectionSource<TItem> hard-codes BeInOrder, BeInDescendingOrder, All, Any, HaveSingleItem, HaveDistinctItems as instance methods, while other assertions are generated via IShouldSource<T> extensions. This inconsistency has two risks:

1. If CollectionAssertion<TItem> has matching [AssertionExtension] classes, the generator will also emit IShouldSource<IEnumerable<TItem>> extension methods for the same names, causing CS0121 ambiguity between the instance method and the extension method.
2. If CollectionAssertion does NOT expose those operations as assertion extension classes, the hand-crafted wiring will fall out of sync with future assertion additions silently.

The cleaner approach is to either: (a) ensure all collection-specific assertions are driven by [AssertionExtension] and remove the hand-crafted methods, or (b) document explicitly that collection entry methods are intentionally hand-crafted and exclude the corresponding assertion classes from generation.

---

🟡 Medium: ShouldCollectionSource passes null! to CollectionAssertion

File: TUnit.Assertions.Should/Core/ShouldCollectionSource.cs

_inner = new CollectionAssertion<TItem>(value!, expression);

The constructor signature accepts IEnumerable<TItem>? but passes it with a null-forgiving operator. If asserting that a collection is null is a valid use case (e.g., nullList.Should().BeNull()), this is intentional — but then CollectionAssertion must handle null internally. If it doesn't, users get an NRE instead of a meaningful assertion failure. This should either be explicitly tested or the behavior documented.

---

🟠 Minor: typeof(TException).Name produces ugly generic names

File: TUnit.Assertions.Should/Core/ShouldDelegateSource.cs

Context.ExpressionBuilder.Append($".Throw<{typeof(TException).Name}>()");

For a generic exception like SpecificException<Foo>, typeof(TException).Name returns SpecificException1` — the raw reflection name with backtick. Consider a small helper or a friendly-name utility so failure messages stay readable.

---

✅ What's well done

• The conjugation table in NameConjugator is thoughtful — CamelCase-boundary-aware prefix replacement avoids false matches like Issue → Besue. The TryDropTrailingS double-s guard is a nice defensive touch.
• Incremental pipeline design is correct: EquatableArray<T> wrappers on the records, per-compilation lookups hoisted out of the hot path via CollectAlreadyBakedShouldExtensionNames.
• The [assembly: ShouldEntryPoint(false)] concept is the right API shape for FluentAssertions co-existence — it just needs to be wired up in the generator.
• ShouldAssertion<T>.Because correctly delegates to _inner.Because which mutates state rather than returning a new instance — no fluency bug here.
• 87-test matrix across 3 TFMs with coverage across types, chaining, delegates, and user-defined assertions is solid test coverage.

---

Summary: The two high-priority items (ShouldEntryPointAttribute unread by generator, and EquatableArray duplication) need addressing. The medium items (dead covariance code, mixed collection pattern, null propagation) should be resolved to avoid future confusion.

[thomhurst]

Addressed in 5b89e90:

• HIGH 1 — ShouldEntryPointAttribute unread: removed the attribute entirely. After re-checking, the generated extensions live on IShouldSource<T> (not T) and never could have collided with FluentAssertions' Should() — only the hand-written Should() overloads in ShouldExtensions collide, and an attribute can't suppress a using directive's bindings. The attribute's stated purpose wasn't achievable, so dropped rather than half-wired.
• HIGH 2 — EquatableArray 4th copy: deleted local copy, linked from TUnit.Core.SourceGenerator/Models/EquatableArray.cs via <Compile Include="..." Link="Shared\..." /> (same pattern already in use for CovarianceHelper).
• MED 3 — dead covariance code: stripped the unreachable if (data.NeedsMethodLevelCovariance) branch from EmitMethod, removed NeedsMethodLevelCovariance / TypeParamIsTypeParameter / TypeParamConstraintName fields from AssertionData. Emission is now unconditionally IShouldSource<typeParam>.
• MED 4 — collection mixing: added InstanceMethodAssertions skip-set covering the seven assertion classes whose Should counterpart is a hand-crafted instance method on ShouldCollectionSource<T> (CollectionIsInOrderAssertion, CollectionAllAssertion, HasSingleItemAssertion, etc.). Generator skips them, so drift / shadowing isn't possible.
• MED 5 — null propagation: added NullSourceTests exercising null string, null list .Contain, null list .BeInOrder. All produce AssertionException (not NRE) — CollectionAssertion handles null internally as expected.
• MIN 6 — typeof(T).Name: kept as-is; matches the existing FuncAssertion.Throws / AsyncDelegateAssertion.Throws pattern in TUnit.Assertions. Worth a separate pass across all of TUnit, not Should-specific.

Test count: 90/TFM × 3 TFMs = 270 (was 261).

[claude]

Code Review: feat: add TUnit.Assertions.Should package

What This Does

Adds a new TUnit.Assertions.Should package that layers a FluentAssertions-style Should() API on top of the existing TUnit.Assertions system via source generation. A source generator (TUnit.Assertions.Should.SourceGenerator) scans [AssertionExtension]-decorated classes and emits conjugated Should-flavored extension methods (IsEqualTo → BeEqualTo, Contains → Contain, etc.) on IShouldSource<T>. Zero assertion logic duplication — pure adaptation.

---

Previous Review Items — Addressed ✅

All four items from the prior review have been addressed in 5b89e90:

• ShouldEntryPointAttribute dropped (unachievable purpose)
• EquatableArray no longer duplicated — linked from TUnit.Core.SourceGenerator
• Dead covariance branch removed
• Collection mixing fixed via InstanceMethodAssertions skip set

---

Blocking Issues

1. New projects missing from TUnit.CI.slnx — zero CI coverage

TUnit.Assertions.Should, TUnit.Assertions.Should.SourceGenerator, TUnit.Assertions.Should.Tests, and TUnit.Assertions.Should.SourceGenerator.Tests are added to TUnit.slnx only. The CI workflow builds and tests exclusively from TUnit.CI.slnx. As a result:

• The new library is not built in CI
• The 90 runtime tests never execute against PRs
• The library won't be packed/published with other packages

Add entries to TUnit.CI.slnx for at minimum TUnit.Assertions.Should.csproj and TUnit.Assertions.Should.Tests.csproj.

2. TUnit.Assertions.Should.SourceGenerator.Tests is an empty shell

The source generator test project has only a .csproj — no .cs files, no .verified.txt snapshots. The equivalent TUnit.Assertions.SourceGenerator.Tests project has ~20 test files with verified snapshots per TFM. Without snapshot tests, regressions in the generator's output (wrong conjugations, missing global:: prefix, dropped constraints, incorrect CallerArgumentExpression parameter names) will be invisible to CI. This is especially important since conjugation is the core value proposition of this package.

---

Medium Issues

3. ShouldCollectionSource silently omits the IEqualityComparer<T> overload of HaveDistinctItems

CollectionAssertionBase<TCollection, TItem>.HasDistinctItems(IEqualityComparer<TItem>) exists. The InstanceMethodAssertions skip set blocks the generator from emitting a Should version, and ShouldCollectionSource.HaveDistinctItems() only wraps the parameterless overload. Users needing case-insensitive distinct checks (HaveDistinctItems(StringComparer.OrdinalIgnoreCase)) will find the Should API silently incapable of expressing it. Either add the overload explicitly to ShouldCollectionSource, or document the limitation.

4. Cross-collection-type chaining breaks at the type system level

After a generated extension returns ShouldAssertion<IEnumerable<TItem>>, the .And continuation is ShouldContinuation<IEnumerable<TItem>> which implements IShouldSource<IEnumerable<TItem>>. Generated extensions for that type resolve fine, but ShouldCollectionSource<TItem> instance methods (BeInOrder, All, Any, etc.) are not available since they only exist on the concrete class, not the interface. So this silently doesn't compile:

await list.Should().Contain(1).And.BeInOrder();
// Error: ShouldContinuation<IEnumerable<int>> has no BeInOrder()

The reverse (BeInOrder().And.Contain(1)) works. This is an inherent limitation of the design but a surprising usability cliff — worth documenting or adding a test that captures it explicitly.

---

Minor Issues

5. Unnecessary ! null-suppressor in ShouldExtensions.cs

return new ShouldSource<string>(new AssertionContext<string>(value!, sb));

AssertionContext<string> already accepts string?, so the ! is not needed. Harmless but slightly misleading.

6. FormatDefaultValue has no type suffixes for float/decimal/long literals

If a future assertion class adds a float tolerance = 0.5f parameter, FormatDefaultValue emits "0.5" (compiles as double, wrong type). No existing assertion classes are affected, but adding suffix handling (float → 'f', decimal → 'm', long → 'L') would preempt a latent correctness gap.

7. Generator is not truly incremental

The generator uses CompilationProvider.Select(...) which re-runs the full cross-assembly walk on every compilation change (every keystroke). The EquatableArray<T> wrappers make the downstream RegisterSourceOutput step cache-efficient, but not the upstream scan. This is pragmatic since cross-assembly scanning can't use SyntaxProvider, but it means the "incremental-cache friendly" framing should be qualified.

---

What Is Well Done

• The fundamental design — zero assertion logic duplication, pure adaptation layer via IShouldSource<T> — is the right architecture. It means the entire generated API is chainable on any wrapper that implements the interface.
• ShouldContinuation<T> as a readonly struct is zero-allocation.
• Context threading is correct: ShouldAssertion<T>.And calls _inner.And (which calls SetPendingLink on the shared context), then wraps only the .Context in ShouldContinuation<T>. Chain semantics are preserved end-to-end.
• NameConjugator handles all the irregular English conjugations correctly. Longest-prefix-first ordering prevents IsNot/Is ambiguity. All existing assertion method names trace correctly through the logic.
• CollectAlreadyBakedShouldExtensionNames correctly hoists the O(n) namespace scan out of the per-type hot path.
• ShouldExtensions.Should(this string?) at priority 2 correctly prevents string binding to IEnumerable<char>.
• 90 runtime tests with solid type coverage — string, collection, numeric, DateTime, delegate, chaining, Assert.Multiple, user-defined assertions, null sources.
• Null-source tests confirm CollectionAssertion<T> returns AssertionResult.Failed("collection was null") rather than throwing NRE — correct.

---

Summary

The architecture is sound and the implementation quality is high. The two blocking items before merge are: (1) adding the new projects to TUnit.CI.slnx and (2) adding snapshot test files to TUnit.Assertions.Should.SourceGenerator.Tests. Everything else is either a documented limitation or minor polish.

[claude]

Code Review: Round 3

Previous Review Items — Status

From Round 1: All addressed in commit 5b89e90 ✅

From Round 2: Two blocking items still not addressed after the latest commit (32bdd5d), plus the new commit introduces one new issue.

---

Still Blocking

1. New projects still absent from TUnit.CI.slnx

TUnit.CI.slnx has no entries for any of the four new projects. The CI workflow builds and tests exclusively from that file. The 90 runtime tests per TFM and the source generator tests never execute against PRs or the main branch.

This has been noted in two prior reviews and is still unaddressed. Minimum required additions:

<Folder Name="/Libraries/">
  <Project Path="TUnit.Assertions.Should/TUnit.Assertions.Should.csproj" />
</Folder>
<!-- and in /Tests/ -->
<Project Path="TUnit.Assertions.Should.Tests/TUnit.Assertions.Should.Tests.csproj" />

2. TUnit.Assertions.Should.SourceGenerator.Tests still contains zero test files

The directory still has only its .csproj. The equivalent TUnit.Assertions.SourceGenerator.Tests has ~20 verified snapshot files. Generator regressions (wrong conjugations, dropped constraints, missing global:: prefix, broken CallerArgumentExpression forwarding) would be invisible. The new extension-method-scan path added in 32bdd5d makes snapshot coverage even more important since the discovery logic is now substantially more complex.

---

New Issue from Commit 32bdd5d

3. ShouldNameAttribute.Negated is a documented no-op

TryGetShouldNameOverride parses the Negated named argument from the attribute and returns it as a tuple, but every call site discards it with _:

// ShouldExtensionGenerator.cs:263
var (overrideName, _) = TryGetShouldNameOverride(returnType, ctx.ShouldNameAttribute);

The attribute's XML doc and example explicitly promise:

[ShouldName("BeOdd", Negated = "NotBeOdd")]

Users who follow that example will find the Negated value silently discarded — no negated override, no diagnostic. Either wire up Negated in the emitter (which requires correlating the negated extension method to the same return type), or remove the property and its documentation until it's implemented.

---

Pre-existing Minor Issues (for completeness)

4. NameConjugator's TUSHOULD001 diagnostic is referenced but never emitted

The NameConjugator doc says:

When Matched is false the original name is returned unchanged so the caller can emit a TUSHOULD001 diagnostic.

The Matched flag is discarded everywhere:

var positiveName = m.ShouldNameOverride ?? NameConjugator.Conjugate(m.MethodName).Name;

Methods with unrecognized naming conventions (neither Is*/Has*/Does* nor trailing-s) get emitted with their original, un-Should-flavored name. This is acceptable behaviour but the dead diagnostic reference is misleading. Either remove the mention of TUSHOULD001 from the doc comment, or emit the diagnostic.

---

What the New Commit Does Well

The switch to extension-method-scan (32bdd5d) is a meaningful improvement and solves a real gap. The old class-scan missed [GenerateAssertion]-generated assertions (e.g., IsZero, IsEven, BoolIsTrue) entirely. The constructor-matching heuristic ("simple factory" guard) is sound — it correctly defers the hard cases (context-mapping extensions like ImplicitConversionEqualityExtensions) rather than emitting broken wrappers. The 9 new GenerateAssertionTests validate the newly discovered path. The [DynamicallyAccessedMembers] and [UnconditionalSuppressMessage] propagation on type parameters is a good AOT-compatibility detail that would have been easy to miss.

---

Summary

The architectural foundation is solid and the implementation quality is high. The two items that were blocking in the previous review remain unresolved: CI coverage and snapshot tests. The ShouldNameAttribute.Negated silent no-op is a new issue introduced by the attribute still being in the public API after the generator logic for it was never completed. Address those three and this is ready to merge.

[claude]

Round 4 Review — commit b4f2494c

The latest commit (b4f2494c — "ci: wire TUnit.Assertions.Should into pipeline as beta package") adds pipeline modules (RunAssertionsShouldTestsModule.cs, pack/project list entries) but all three blocking items from Round 3 remain unresolved.

---

Blocking — 3 items still open

1. New projects absent from TUnit.CI.slnx (raised in Round 2, Round 3)

TUnit.CI.slnx is untouched in this PR. The four new projects are added only to TUnit.slnx (the developer solution). Since the CI workflow builds and tests exclusively from TUnit.CI.slnx, all runtime tests and source-generator tests never run in CI, and the packages will never be built or published through the standard pipeline.

Fix: Add all four projects to TUnit.CI.slnx in the appropriate solution folders, mirroring the structure used in TUnit.slnx.

---

2. TUnit.Assertions.Should.SourceGenerator.Tests contains zero test files (raised in Round 2, Round 3)

The project file exists with all the right package references (Verify, Verify.TUnit, Microsoft.CodeAnalysis.SourceGenerators.Testing, etc.) but there are no .cs test files and no .verified.txt snapshot files. The equivalent TUnit.Assertions.SourceGenerator.Tests has ~20 test files with verified snapshots per TFM. Per CLAUDE.md: "Changes to source generator output or public APIs require running snapshot tests. Commit .verified.txt files."

Why this matters: Without snapshot tests, any future refactor of ShouldExtensionGenerator can silently regress the generated output. The infrastructure is already wired up — it just needs tests.

Fix: Add at minimum one [Test] method per major generator scenario (simple value assertion, collection assertion, delegate assertion, [ShouldName] override, user-defined [AssertionExtension]) using VerifyGenerator or equivalent, and commit the resulting .verified.txt snapshots.

---

3. ShouldNameAttribute.Negated is a documented no-op (raised in Round 3)

TryGetShouldNameOverride correctly parses the Negated named argument and returns it — but the only call site still discards it:

MethodData has no NegatedNameOverride field, and EmitMethod only uses the positive name. A user who writes:

will find the Negated parameter silently ignored. This is a broken API contract.

Fix (option A — simplest): Remove the Negated property from ShouldNameAttribute and strip it from the XML doc until a negation-generation feature is actually implemented. Shipping a documented but non-functional API property is worse than not shipping it.

Fix (option B — full implementation): Add string? NegatedNameOverride to MethodData, propagate it through EmitMethod, and generate a Not{Name} companion method (or however negation is intended to work). This is the right long-term answer but is larger scope.

---

What changed in b4f2494c

The new commit correctly wires the package into the pipeline (GetPackageProjectsModule, PackTUnitFilesModule, new RunAssertionsShouldTestsModule). The pipeline module structure is consistent with other assertion-package pipeline entries. No new issues introduced.

---

Summary

The overall architecture remains sound and the implementation quality is high. Three items need to land before merge:

#	Issue	Status
1	All 4 projects in TUnit.CI.slnx	Still missing
2	Snapshot tests in TUnit.Assertions.Should.SourceGenerator.Tests	Still empty
3	ShouldNameAttribute.Negated — fix or remove	Still silently discarded

[claude]

Round 4 Review — commit b4f2494c

The latest commit (b4f2494c — "ci: wire TUnit.Assertions.Should into pipeline as beta package") adds pipeline modules (RunAssertionsShouldTestsModule.cs, pack/project list entries) but all three blocking items from Round 3 remain unresolved.

---

Blocking — 3 items still open

1. New projects absent from TUnit.CI.slnx (raised in Round 2, Round 3)

TUnit.CI.slnx is untouched in this PR. The four new projects are added only to TUnit.slnx (the developer solution). Since the CI workflow builds and tests exclusively from TUnit.CI.slnx, all runtime tests and source-generator tests never run in CI, and the packages will never be built or published through the standard pipeline.

Fix: Add all four projects to TUnit.CI.slnx in the appropriate solution folders, mirroring the structure used in TUnit.slnx.

---

2. TUnit.Assertions.Should.SourceGenerator.Tests contains zero test files (raised in Round 2, Round 3)

The project file exists with all the right package references (Verify, Verify.TUnit, Microsoft.CodeAnalysis.SourceGenerators.Testing, etc.) but there are no .cs test files and no .verified.txt snapshot files. The equivalent TUnit.Assertions.SourceGenerator.Tests has ~20 test files with verified snapshots per TFM. Per CLAUDE.md: "Changes to source generator output or public APIs require running snapshot tests. Commit .verified.txt files."

Why this matters: Without snapshot tests, any future refactor of ShouldExtensionGenerator can silently regress the generated output. The infrastructure is already wired up — it just needs tests.

Fix: Add at minimum one [Test] method per major generator scenario (simple value assertion, collection assertion, delegate assertion, [ShouldName] override, user-defined [AssertionExtension]) using VerifyGenerator or equivalent, and commit the resulting .verified.txt snapshots.

---

3. ShouldNameAttribute.Negated is a documented no-op (raised in Round 3)

TryGetShouldNameOverride correctly parses the Negated named argument and returns it — but the only call site still discards it:

// ShouldExtensionGenerator.cs
var (overrideName, _) = TryGetShouldNameOverride(returnType, ctx.ShouldNameAttribute);

MethodData has no NegatedNameOverride field, and EmitMethod only uses the positive name. A user who writes:

[ShouldName("BeSuccessful", Negated = "NotBeSuccessful")]
public class MySuccessAssertion : Assertion<T> { }

will find the Negated parameter silently ignored. This is a broken API contract.

Fix (option A — simplest): Remove the Negated property from ShouldNameAttribute and strip it from the XML doc until a negation-generation feature is actually implemented. Shipping a documented but non-functional API property is worse than not shipping it.

Fix (option B — full implementation): Add string? NegatedNameOverride to MethodData, propagate it through EmitMethod, and generate a Not{Name} companion method. This is the right long-term answer but is larger scope.

---

What changed in b4f2494c

The new commit correctly wires the package into the pipeline (GetPackageProjectsModule, PackTUnitFilesModule, new RunAssertionsShouldTestsModule). The pipeline module structure is consistent with other assertion-package pipeline entries. No new issues introduced.

---

Summary

The overall architecture remains sound and the implementation quality is high. Three items need to land before merge:

#	Issue	Status
1	All 4 projects in TUnit.CI.slnx	Still missing
2	Snapshot tests in TUnit.Assertions.Should.SourceGenerator.Tests	Still empty
3	ShouldNameAttribute.Negated — fix or remove	Still silently discarded

[claude]

Code Review — Round 17

This is a well-executed, architecturally clean PR that has been substantially refined through 16 prior rounds. The zero-duplication goal (source-gen bridging IAssertionSource<T> → IShouldSource<T>) is the right design, and most of the prior feedback has been addressed. A few items remain.

---

Medium — HasNot* missing conjugation rule

The conjugation table handles IsNot* → NotBe* correctly, but has no entry for HasNot*. Under the current rules, HasNotBeenCalled would conjugate via the Has* → Have* branch to HaveNotBeenCalled instead of the correct NotHaveBeenCalled.

TUnit.Assertions currently uses IsNot* for negation, so no existing assertion triggers this today — but if any HasNot* method ever lands, users silently get the wrong name with no diagnostic. The fix is one line, inserted before the Has rule:

if (TryReplacePrefix(methodName, "HasNot", "NotHave", out s)) return s;  // must come before "Has"
if (TryReplacePrefix(methodName, "Has", "Have", out s)) return s;

And a test: [Arguments("HasNotBeenCalled", "NotHaveBeenCalled")].

Why this matters: The IsNot* → NotBe* pattern sets user expectation that negation prefixes always move to the front. A HasNot* method breaking that expectation would be a confusing API inconsistency and a breaking change to fix later.

---

Medium — -ies endings conjugate incorrectly

Words ending in consonant + -ies are not handled by the 3rd-person-singular drop. Applies → Applie (wrong), correct form is Apply. The test matrix covers Fixes → Fix but not the -ies → -y irregular case.

Why this matters: The bug is silent — no error, just a semantically wrong method name generated. A comment in the algorithm documenting "Words ending in consonant+ies (apply/applies) are not handled; use [ShouldName] for those" would at least make the limitation visible.

---

Low — ShouldDelegateSource<T> is a sealed class, not a readonly struct

ShouldSource<T> is a readonly struct (pure router, stack-allocated). ShouldDelegateSource<T> is a sealed class with no observable reason for the asymmetry — it also holds only an AssertionContext<T> reference (already a class) and is constructed once per Should() call. Making it a readonly struct would save one heap allocation per delegate assertion invocation.

The asymmetry isn't fatal, but it's unexplained. Either make ShouldDelegateSource<T> a struct (consistent with ShouldSource<T>) or add a comment explaining why the delegate entry point specifically needs a class.

---

Low — CollectFromContainer silently skips generic static classes

if (type.IsGenericType)
    return;

This is correct for the [AssertionExtension] pattern (non-generic static extension classes), but user-authored assertions in a generic static container would silently produce no Should counterpart with no diagnostic. A single comment noting this limitation would prevent future confusion: // Generic static containers are not supported; [AssertionExtension] requires non-generic classes.

---

What is well-executed

• ConditionalWeakTable caching design: The choice over ConcurrentDictionary (which would pin stale IAssemblySymbol graphs for the IDE session) is correct, and the comment justifying it is exemplary.
• EquatableArray<T> on all pipeline data types: Correct for Roslyn incremental cache invalidation.
• [OverloadResolutionPriority] on string / IEnumerable<T> overloads: Both necessary, both correct.
• AttributeForwardingFormatters as a shared linked file: Prevents [Obsolete]/[EditorBrowsable] from diverging between the two generators — this is the right DRY decision.
• IsAll → BeAll deprecation handled end-to-end: [Obsolete] propagates through both generators to the user-callable surface cleanly.
• AnalyzerTestCompatibility fix: The netstandard2.0 copy + GetCompatibleDllPath pattern is the right pragmatic fix for the MCT 1.1.2 / net10.0 mismatch, following the existing linked-source convention in the repo.
• ShouldAssertion<T> as sealed class: The added comment explaining why a struct would break GetAwaiter mutation semantics makes this a defensible documented trade-off.
• Prior round feedback addressed: Redundant per-project wrappers removed, one-level-depth assumption documented on ReferencesAssertionsAssembly, FormatTypeName CLR/C# alias asymmetry documented, split-variable false positive shared limitation documented.

---

Summary

The HasNot* conjugation gap is the highest-priority remaining item: it's a one-liner fix that closes a silent API correctness hole before it can become a breaking change. The -ies edge case warrants at minimum a comment. Everything else is low-priority polish.

Overall this is solid v1 work. The architecture is clean, the incremental pipeline design is correct, and the test coverage is thorough.

[claude]

Code Review — Round 18

This PR has been substantially refined through 17 prior rounds. All the major feedback has been addressed: HasNot* conjugation is fixed, -ies endings work (Applies → Apply), ShouldDelegateSource<T> is a readonly struct, EquatableArray<T> is linked not copied, per-project GetCompatibleDllPath wrappers have been removed, and all four documentation gaps (class vs struct rationale, split-variable limitation, one-level depth assumption, CLR/C# alias asymmetry) have clear explanatory comments.

Two genuine issues remain.

---

Medium — -izes verbs produce a mangled name

File: TUnit.Assertions.Should.SourceGenerator/NameConjugator.cs, GetTrailingSDropCount

The sibilant rule that correctly handles Fixes → Fix (x+es → drop 2) and Buzzes → Buzz (z+z+es → drop 2) also fires for Normalizes, Analyzes, Authorizes, and any other -izes verb where the z is part of the infinitive stem rather than a sibilant doubling:

"Normalizes" (N-o-r-m-a-l-i-z-e-s, len=10)
  name[7]='z' → c=='z' → GetTrailingSDropCount returns 2
  result = name.Substring(0, 8) = "Normaliz"   // wrong; should be "Normalize"

The key linguistic distinction: buzz → buzzes (the z is the final stem consonant, -es is the inflection) vs normalize → normalizes (the stem already ends in silent e, only -s is added). The difference is detectable: if the character before z is a vowel (i, e, a, o, u) the z is not a final sibilant — it is inside the stem.

Suggested fix in GetTrailingSDropCount:

if (c == 'x' || c == 'o')
{
    return 2;
}
if (c == 'z')
{
    // Only treat 'z' as a sibilant stem-ender (returning 2) when it is consonant-preceded,
    // e.g. "buzz" (u-z) → "buzzes". If the char before 'z' is a vowel (e.g. "normalize",
    // i-z) the 'z' is inside the stem; "normalizes" = "normalize" + "s" → drop 1 only.
    if (firstWordEnd >= 4 && IsVowel(name[firstWordEnd - 4]))
        return 1;  // vowel+z+es: "normalizes" → "normalize"
    return 2;      // consonant+z+es: "buzzes" → "buzz"
}

And tests to lock it in:

[Arguments("Normalizes", "Normalize")]
[Arguments("Analyzes",   "Analyze")]
[Arguments("Authorizes", "Authorize")]
[Arguments("Buzzes",     "Buzz")]   // ensure the existing case still passes

Why this matters: TUnit.Assertions's own assertions don't currently trigger this (they use Is*/Has* prefixes which resolve before TryDropTrailingS). But user-authored assertion methods like Validates, Normalizes, or Authorizes — which don't carry Is* prefixes — will silently generate Validat, Normaliz, Authoriz instead of Validate, Normalize, Authorize. The breakage is silent (no compilation error, just a wrong method name), and the [ShouldName] override exists as an escape hatch, but the conjugation algorithm itself should not require per-method annotation for common English patterns.

---

Low — No public API snapshot for TUnit.Assertions.Should

File: TUnit.PublicAPI/Tests.cs

Every other shipped package (TUnit.Core, TUnit.Assertions, TUnit.Playwright, TUnit.OpenTelemetry) has a Tests.*_Library_Has_No_API_Changes.*.verified.txt snapshot that acts as an accidental-breaking-change guard. TUnit.Assertions.Should is absent, even though it is NuGet-packaged as of this PR.

The "beta" label does not remove the value of an API snapshot — it is more valuable during beta, when the surface is still settling, precisely because it makes every intentional shape change explicit.

Suggested addition to TUnit.PublicAPI/Tests.cs:

#if NET
[Test]
public Task Should_Library_Has_No_API_Changes()
    => VerifyPublicApi(typeof(TUnit.Assertions.Should.ShouldExtensions).Assembly);
#endif

Plus the corresponding .verified.txt snapshot files (generated by running the test once and accepting the output).

Why this matters: Without this, a refactor that accidentally removes ShouldContinuation<T>, renames a ShouldCollectionSource<T> method, or changes ShouldAssertion<T>.Because's return type would not be caught until a downstream user reports a compile error after upgrading.

---

What remains well-executed

• HasNot* → NotHave* rule: Added correctly before the Has* rule and covered by [Arguments("HasNotBeenCalled", "NotHaveBeenCalled")]. Good.
• -ies endings: Applies → Apply is now tested and the TryReplaceTrailingIes implementation correctly detects consonant+ies endings, handles 4-char words without erroring (no IndexOutOfRange), and routes vowel+ies through the generic -es drop.
• ShouldDelegateSource<T> as readonly struct: Consistent with ShouldSource<T> and ShouldContinuation<T>. No unexplained asymmetry.
• EquatableArray<T> via linked file: The <Compile Include="...\EquatableArray.cs" Link="Shared\EquatableArray.cs" /> pattern correctly addresses the duplication concern without introducing a shared project dependency, consistent with how CovarianceHelper and AttributeForwardingFormatters are shared.
• ShouldAssertion<T> comment: The comment explaining why GetAwaiter semantics require a class rather than a struct is clear and defensible. The entry types staying as structs is the right trade-off split.
• FormatTypeName CLR/alias note: The asymmetry is documented and accepted — reasonable for exception types.
• Chain threading: ShouldAssertion.And correctly delegates to _inner.And (which calls ThrowIfMixingCombiner<OrAssertion<T>>()), so mixed-combiner detection propagates correctly through the Should layer without any duplication of that logic.
• IsTUnitMethod semantic check in IsNotNullAssertionSuppressor: FindShouldInChain is intentionally parentName: null (correct — Should() is an extension method on any receiver) but then backs the result with GetSymbolInfo semantic resolution to confirm it's specifically ShouldExtensions.Should. This two-step approach prevents false suppressions from unrelated third-party Should() extensions while staying purely syntactic for the chain walk.
• Shared AnalyzerTestCompatibility.GetCompatibleDllPath: The netstandard2.0 copy + fallback pattern is the correct pragmatic fix for the MCT 1.1.2 / net10.0 reference-assembly mismatch.

---

Summary

Priority	Item
Medium	Fix -izes verb conjugation in GetTrailingSDropCount: Normalizes, Analyzes, Authorizes should produce Normalize, Analyze, Authorize, not Normaliz/Analyz/Authoriz. One-condition fix + 3 test cases.
Low	Add Should_Library_Has_No_API_Changes public API snapshot test to TUnit.PublicAPI/Tests.cs, consistent with all other shipped packages.

The -izes fix is the only substantive code change needed. The public API snapshot is process hygiene that's cleanly addable here or in a follow-up. Everything else from prior rounds is properly addressed.

[claude]

Code Review: PR #5785 — TUnit.Assertions.Should

Overall this is an architecturally ambitious and well-executed PR. The zero-duplication layering is correct, the incremental generator pipeline is thoughtfully designed, and the test coverage is solid. There are a few real issues worth addressing before merge, ranging from potential crash-in-generator to API gaps.

---

🔴 High: TypeArguments[0] accessed without length guard in HasMatchingConstructor

In ShouldExtensionGenerator.cs — HasMatchingConstructor:

var firstCtorParam = ctor.Parameters[0].Type as INamedTypeSymbol;
if (firstCtorParam is null
    || !SymbolEqualityComparer.Default.Equals(firstCtorParam.OriginalDefinition, assertionContextSymbol)
    || !SymbolEqualityComparer.Default.Equals(firstCtorParam.TypeArguments[0], assertionTypeArg))

firstCtorParam.TypeArguments[0] is accessed without checking firstCtorParam.TypeArguments.Length > 0 first. If the first constructor parameter's named type has zero type arguments (e.g., a non-generic type that happens to match the metadata name — contrived but possible under unusual multi-assembly setups, or if AssertionContext<T> is ever refactored), this throws an IndexOutOfRangeException inside the generator, silently crashing it for the whole compilation with no user-visible error.

Fix:

|| firstCtorParam.TypeArguments.Length != 1
|| !SymbolEqualityComparer.Default.Equals(firstCtorParam.TypeArguments[0], assertionTypeArg)

---

🔴 High: EnumerateInstanceMethods can emit duplicate partial methods for inherited overrides

The inheritance walk in EnumerateInstanceMethods yields methods from both the concrete type and all base types. If a derived assertion class overrides a method declared in Assertion<T> (the override and the base declaration both pass the HasMatchingConstructor check), two identical partial methods get emitted → CS0111 compile error in the generated output.

This doesn't currently trigger because none of the tested assertion types happen to override base assertion methods, but the invariant is fragile.

Fix: Deduplicate by method signature before yielding:

var seen = new HashSet<string>(StringComparer.Ordinal);
for (var current = type; current is not null; current = current.BaseType)
{
    foreach (var member in current.GetMembers())
    {
        if (member is IMethodSymbol m && m.MethodKind == MethodKind.Ordinary
            && !m.IsStatic && m.DeclaredAccessibility == Accessibility.Public
            && seen.Add(m.Name))
        {
            yield return m;
        }
    }
}

---

🔴 High: Because is missing on ShouldContinuation<T> and ShouldSource<T>

ShouldAssertion<T> exposes .Because(string), but ShouldContinuation<T> and ShouldSource<T> do not. This means:

// Pre-chain pattern (common FluentAssertions habit) — won't compile:
await value.Should().Because("reason").BeEqualTo(x);

// Post-chain after .And — also silently unavailable:
await value.Should().BeEqualTo(x).And.BeEqualTo(y).Because("reason");

Users migrating from FluentAssertions will try both of these immediately. The Assert.That(value).Because("reason").IsEqualTo(x) pre-chain pattern works in TUnit proper because IAssertionBuilder<T> has Because.

Fix: Add Because to IShouldSource<T> (returns this after setting the because-message on the context) and propagate it through ShouldContinuation<T>.

---

🟡 Medium: AnalyzerTestCompatibility fallback silently reintroduces the problem it was designed to fix

In SharedTestHelpers/AnalyzerTestCompatibility.cs:

public static string GetCompatibleDllPath(string assemblyName, Assembly fallback)
{
    var ns20Path = Path.Combine(AppContext.BaseDirectory, $"{assemblyName}.netstandard2.0.dll");
    return File.Exists(ns20Path) ? ns20Path : fallback.Location;
}

If the CopyToOutputDirectory item hasn't executed (clean checkout, out-of-order build targets), the method silently falls back to fallback.Location — loading the net10.0 build, triggering CS1705 suppression, breaking symbol resolution, and causing "expected 1 diagnostic, got 0" failures with no indication of the root cause. This is exactly the bug the helper was designed to prevent.

Better approach: Throw with a clear diagnostic instead of falling back:

if (!File.Exists(ns20Path))
    throw new FileNotFoundException(
        $"netstandard2.0 build of {assemblyName} not found at '{ns20Path}'. " +
        $"Run 'dotnet build' before running tests.",
        ns20Path);
return ns20Path;

---

🟡 Medium: [ShouldName] doesn't apply to [GenerateAssertion]-based methods — undocumented

[ShouldName] is read from the assertion class (returnType in TryGetShouldNameOverride). For [AssertionExtension]-based classes this works perfectly. But for [GenerateAssertion]-produced methods, the generated class is file-scoped and not user-owned — there is no class to annotate.

The limitation itself is acceptable, but it should be documented on [ShouldName] with a note like "applies only to [AssertionExtension] classes; [GenerateAssertion]-based methods use automatic conjugation and cannot be overridden". Users who hit awkward conjugations on generated methods will otherwise have no path forward.

---

🟡 Medium: Cache correctness comment missing — future footgun risk

The ConditionalWeakTable-based reference cache stores ReferenceData records that were computed using symbol instances from a previous compilation. This is safe only because ReferenceData contains string values extracted from those symbols, not live ISymbol references. This invariant is non-obvious and a future contributor adding a symbol reference to ReferenceData would silently introduce stale-symbol bugs.

Suggested comment:

// IMPORTANT: ReferenceData must contain only compilation-independent values (strings, etc.).
// Never cache live ISymbol references — they are tied to the compilation that produced them
// and become stale when the compilation changes.

---

🟢 Minor: EnumerateInstanceMethods walks all the way to object

The walk stops at null, meaning it visits object and yields GetHashCode/Equals/ToString. These are correctly filtered out by TryDescribeWrapperMethod, but the unnecessary traversal adds overhead across every compilation. Worth stopping at assertionBase.BaseType:

for (var current = type;
     current is not null && !SymbolEqualityComparer.Default.Equals(current, assertionBase.BaseType);
     current = current.BaseType)

---

🟢 Minor: OverloadResolutionPriority SDK version

[OverloadResolutionPriority] is a C# 13 attribute (SDK 9+). On net8.0 with an older SDK it is silently ignored, and the string / IEnumerable<char> disambiguation reverts to standard overload resolution. Worth a comment in ShouldExtensions.cs noting this, or a #if NET9_0_OR_GREATER guard.

---

Missing Test Coverage

1. nullList.Should().All(predicate)/.Any(predicate)/.HaveSingleItem(predicate) on null — the three hand-written predicate methods in ShouldCollectionSource are not covered by NullSourceTests (unlike the generated extension-method overloads).
2. Pre-chain .Because("reason") — no test for value.Should().Because("reason").BeEqualTo(x) (and it likely doesn't compile, which is the API gap noted above).
3. [ShouldName] with user-defined assertion — UserDefinedAssertionTests covers basic user-defined assertions but not one decorated with [ShouldName] to exercise the override path.
4. [Obsolete] forwarding from [AssertionExtension] — tested indirectly via the IsAll/BeAll range shim, but no targeted generator snapshot test for the [Obsolete] emission path in isolation.

---

AOT Compatibility

The AOT story is sound. RequiresUnreferencedCode, DynamicallyAccessedMembers, and UnconditionalSuppressMessage forwarding from source methods through to the generated API surface is correct. ShouldDelegateSource.FormatTypeName uses runtime reflection only during failure message formatting (not hot path), and only on Exception-constrained types known to the compiler, so it's AOT-safe as written.

---

Summary

Issue	Severity
TypeArguments[0] without length guard in HasMatchingConstructor	🔴 High (crash-in-generator)
Duplicate partial methods for inherited overrides in EnumerateInstanceMethods	🔴 High (compile error)
Because not on ShouldContinuation<T> / ShouldSource<T>	🔴 High (API gap)
AnalyzerTestCompatibility fallback silently reintroduces the problem it fixes	🟡 Medium
[ShouldName] limitation for [GenerateAssertion]-based methods undocumented	🟡 Medium
Cache correctness comment missing — future footgun	🟡 Medium
EnumerateInstanceMethods walks to object unnecessarily	🟢 Minor
OverloadResolutionPriority SDK version note	🟢 Minor
Missing null-collection test for predicate overloads	🟢 Minor

The core design — zero-duplication layering, incremental cache via ConditionalWeakTable, conjugation table, [ShouldGeneratePartial] for wrapper types — is well thought out. The three High issues are worth fixing before merge; the rest can be tracked as follow-ups.

[claude]

Round 17 Review — TUnit.Assertions.Should

Across 16 rounds of feedback this PR has gone from a rough draft to an impressively polished feature. Almost everything raised has been addressed thoughtfully. This round I'm flagging three small remaining items and one design observation, then giving an overall thumbs-up.

---

What's been done well

The zero-duplication architecture — layering Should() on top of the existing Assert.That assertion engine rather than reimplementing it — is the right call and well-executed. The key design decisions hold up:

• ConditionalWeakTable<MetadataReference, ReferenceData> for cross-assembly caching is correct. Weak keys mean Roslyn's reference GC automatically invalidates stale entries; no manual eviction needed.
• ShouldAssertion<T> as a sealed class is justified. The comment in the class (lines 14-21) is exactly the right level of explanation — mutation semantics during await genuinely prevent a struct here.
• NameConjugator with 38 parameterized cases is a solid regression harness for the conjugation rules.
• Snapshot tests (5 scenarios × 3 TFMs = 15 .verified.txt files) satisfy the project's snapshot testing requirement.
• AttributeForwardingFormatters linked compile is the right way to share logic between two generators without a NuGet dependency.
• Pipeline gating: RunEngineTestsModule now [DependsOn] both Should test modules — nothing can ship without them.

---

Remaining concerns

1. ConditionalWeakTable cache — GetValue(key, factory) is the atomic idiom (minor)

ShouldExtensionGenerator.cs lines 215–233:

if (s_referenceCache.TryGetValue(metadataRef, out var cached))
    return cached;

var fresh = ScanReference(...);
try { s_referenceCache.Add(metadataRef, fresh); }
catch (ArgumentException) { }
return fresh;

The comment explains why Add + catch is used ("no TryAdd in netstandard2.0"), but ConditionalWeakTable<TKey, TValue>.GetValue(TKey key, CreateValueCallback createValueCallback) is available in netstandard2.0 and is the idiomatic atomic single-scan pattern:

// Both threads may call the factory under concurrency — same as the current approach —
// but only one result is stored, and there's no thrown exception to suppress.
return s_referenceCache.GetValue(metadataRef,
    _ => ScanReference(refAssembly, compilation, assertionSource, assertionBase, assertionContext, shouldNameAttr, partialMarker));

The CreateValueCallback delegate signature is TValue(TKey key), so the extra parameters need to be captured in a closure (which allocates once per uncached reference — acceptable). The behavior is identical to the current code, but the intent is clearer and there's no exception path to reason about. Not blocking, but worth a follow-up.

2. Silent skipping of non-factory assertions — discoverability gap (low)

ShouldExtensionGenerator silently skips extension methods whose body doesn't match the simple-factory template. The generator summary doc says:

"Methods that don't fit the factory template … are silently skipped"

For user-authored [AssertionExtension] classes that happen to use a non-factory pattern (context transformation, multi-step setup, etc.), the user gets no Should counterpart and no diagnostic explaining why. This is a usability cliff — they'll just notice the method is absent.

A TUSHOULD001 diagnostic was mentioned early in the PR history and then removed. Reopening it as an informational (not a warning/error) diagnostic only for user-assembly extensions (skipping referenced assemblies) would be much friendlier: "MyCustomAssertion was not wrapped as a Should extension because its constructor does not match the factory template." The current approach is fine for v1 but worth tracking as a follow-up issue.

3. ShouldCollectionSource<TItem> — ExpressionBuilder mutation before Because (design note)

In ShouldCollectionSource.All/Any/HaveSingleItem the expression builder is mutated (Context.ExpressionBuilder.Append(...)) before ApplyBecause is called. This means if a user calls .Because("reason") on the collection source and then calls .All(pred), the expression in the failure message will already have been extended. This ordering is intentional (the assertion gets the full expression), but it's a subtle invariant — the existing comment cross-referencing ExpressionBuilder.Clear() is good, but a brief note on the ordering assumption in ApplyBecause's summary would help future readers.

4. Codacy's remaining 33 issues (informational)

Codacy still flags 5 medium issues (4 Performance, 1 BestPractice). Without seeing the exact violations it's hard to be prescriptive, but given the generator's netstandard2.0 constraint and the runtime's hot-path sensitivity, the Performance ones are worth a scan before merge. If they're LINQ-in-generator-hot-path issues, they're worth fixing; if they're in test or pipeline code, they can be deferred.

---

Summary

This is a large, carefully designed feature with excellent test coverage (103 consumer + 34 generator + 810 analyzer + 2104 assertions tests) and no remaining blocking issues. The three items above are all minor or informational — none warrants holding the merge. The ConditionalWeakTable.GetValue refactor and the silent-skip diagnostic are both good follow-up issues to file.

Approved. Great work across 16 rounds of iteration.

[claude]

Round 19 Review — Post-Approval Verification Pass

After reviewing all 18 prior rounds and checking the current state of the files, here is a final assessment. This pass verifies items raised in REVIEW 4198038018 and REVIEW 4198340438 (both fresh passes from the same cycle) against the actual code on disk.

---

✅ Previously Open Items — All Resolved

Item	Status
TypeArguments[0] without length guard	✅ Safe — IsAssertionSourceInterface checks TypeArguments.Length == 1 before access at line 611
EnumerateInstanceMethods duplicate partial methods	✅ Fixed — seen.Add(GetMethodSignatureKey(m)) deduplication in place
Because missing on ShouldContinuation<T> / ShouldSource<T>	✅ Fixed — all four source types expose Because returning the concrete type
AnalyzerTestCompatibility fallback silently reintroducing CS1705	✅ Fixed — now throws FileNotFoundException with a clear message
Cache correctness comment missing on ReferenceData	✅ Fixed — explicit "IMPORTANT: keep ReferenceData compilation-independent" comment at line 98
-izes verbs mangled (Normalizes → Normaliz)	✅ Fixed — IsVowelOrY check correctly yields Normalizes → Normalize, Analyzes → Analyze, Authorizes → Authorize; test cases confirm
Public API snapshot missing for TUnit.Assertions.Should	✅ Fixed — three .verified.txt files present, Should_Library_Has_No_API_Changes() test wired
GetCompatibleDllPath duplicated across 4 projects	✅ Fixed — centralized in SharedTestHelpers/AnalyzerTestCompatibility.cs, documented
ExpressionBuilder.Clear() coupling in ShouldCollectionSource	✅ Fixed — ApplyBecause helper cleanly consumes _becauseMessage; no external mutation
Release gate not wired for Should test modules	✅ Fixed — RunEngineTestsModule depends on both Should test modules
ShouldDelegateSource<T> was a class, inconsistent with other struct sources	✅ Fixed — now readonly struct

---

🟡 One Remaining Minor Item

ConditionalWeakTable.Add + catch instead of GetValue(key, factory) (lines 215–233 of ShouldExtensionGenerator.cs)

The current pattern:

if (s_referenceCache.TryGetValue(metadataRef, out var cached)) return cached;
var fresh = ScanReference(...);
try { s_referenceCache.Add(metadataRef, fresh); } catch (ArgumentException) { }
return fresh;

The idiomatic netstandard2.0-compatible form:

return s_referenceCache.GetValue(metadataRef, _ => ScanReference(...));

GetValue(TKey, CreateValueCallback) is available in netstandard2.0, is atomic by contract, and eliminates the catch-for-flow-control pattern. In the current form, a race between two Roslyn worker threads both missing the cache causes two redundant ScanReference calls. This is harmless for correctness but worth a follow-up issue.

---

Overall Assessment

This is an exceptionally well-designed feature. After 18 rounds of review and dozens of refinements, the implementation is clean, well-tested, and architecturally sound:

• Zero duplication of assertion logic — the layered source-gen approach is correct
• ConditionalWeakTable caching with the documented netstandard2.0 rationale
• EquatableArray<T> used throughout the incremental pipeline
• CompilationProvider limitation explicitly documented with a TODO for v2
• NameConjugator handles all edge cases (-ies, -izes, HasNot*, Does*) with a thorough 40-case test matrix
• All three TFMs covered by snapshot tests

This PR is ready to merge. The ConditionalWeakTable.GetValue refactor can follow as a cleanup issue if desired.

[claude]

Code Review: TUnit.Assertions.Should (#5785)

This is a substantial, well-engineered PR. The core design — zero-duplication layering over TUnit.Assertions via a source generator — is sound, and the implementation shows careful thinking on several hard problems. Notes below are primarily architectural observations, not blockers.

---

What works well

Unified assertion infrastructure. Both Assert.That() and .Should() share the same AssertionContext<T>, expression builder, and async state machine. No logic duplication across the two APIs.

Generator caching strategy. Using ConditionalWeakTable<MetadataReference, ReferenceData> with compilation-independent data only is the right approach for long IDE sessions. Weak keys ensure entries are GC'd when Roslyn drops the reference, preventing unbounded growth.

Cross-assembly dedup. The three-phase collection (cached per-reference scan → union dedup → uncached local walk) with AlreadyBakedNames cleanly prevents CS0121 when consumers depend on packages that already baked their Should extensions.

OverloadResolutionPriority ordering. Correctly gates string above IEnumerable<char> and IEnumerable<T> above T, preserving the Assert.That-style priority hierarchy.

Attribute forwarding. [Obsolete], [RequiresUnreferencedCode], and [DynamicallyAccessedMembers] are forwarded through generation — this is easy to miss and gets the AOT story right.

Analyzer test infrastructure fix. The netstandard2.0 DLL copy + AnalyzerTestCompatibility helper is a pragmatic fix for the CS1705 / silent symbol-resolution failure in the Microsoft.CodeAnalysis.Testing framework. The detailed comment explaining why this is needed (CS1705 suppressed → extension methods invisible → zero diagnostics) will save future maintainers from rediscovering the issue.

---

Architectural concerns

1. OverloadResolutionPriority SDK requirement is a silent footgun

[OverloadResolutionPriority] is a .NET 9 SDK feature. On SDK <9, it compiles without errors but the attribute is silently ignored, so string.Should() will hit the IEnumerable<char> overload or an ambiguity error depending on what else is in scope.

Why this matters: Users who install TUnit.Assertions.Should on older SDKs may see confusing string-assertion behaviour with no compiler warning. The NuGet package metadata and the ShouldExtensions summary docs should call out the SDK 9+ requirement explicitly — ideally with a #pragma warning or #if guard so older-SDK projects fail fast rather than silently misbehaving.

// Consider: fail-fast shim on SDK <9 so users get a build error, not silent wrong behaviour
#if !NET9_0_OR_GREATER
#error TUnit.Assertions.Should requires .NET SDK 9.0 or later for correct string.Should() overload resolution.
#endif

This is more useful than discovering it at runtime.

2. Hand-written collection methods are a maintenance trap

ShouldCollectionSource<T>.All(), Any(), and HaveSingleItem(predicate) are manually authored because the source generator's "simple factory" template can't express the predicateDescription literal-fallback pattern. That's a reasonable short-term decision, but the code doesn't document why these are exceptions.

If the CollectionAssertion<T> constructors change (e.g., new required parameter), these three methods will silently diverge from the generated counterparts. A comment explaining the constraint would at minimum make future breakage visible:

// Not generated: These constructors take a predicateDescription literal that CAE handling
// in the simple-factory template can't express. Keep in sync with CollectionAssertion<T> ctors.

3. Per-keystroke uncached current-compilation walk

The three-phase generator intentionally leaves the current-compilation walk uncached. The PR description acknowledges a future SyntaxProvider refactor is deferred. That's fine for a beta, but the comment in the generator should flag this explicitly so it's not forgotten:

// TODO: refactor to Roslyn SyntaxProvider to cache current-compilation walk;
// currently re-runs on every keystroke in IDEs with large current-assembly assertion sets.

4. ShouldAssertion<T> being a class (not a concern, but worth stating for reviewers)

The PR body explains this but it's easy to miss in review: ShouldAssertion<T> is a sealed class rather than a readonly struct because the inner Assertion<T> state machine mutates during await. Boxing a struct copy would break the await. The current design is intentional and correct — worth a one-line comment on the class declaration so future readers don't try to convert it.

---

Analyzer integration

The extensions to AwaitAssertionAnalyzer and MixAndOrOperatorsAnalyzer look correct. Both documented false-positive edge cases (split-variable pattern for await detection; conditional assertion execution for null suppression) are accepted tradeoffs — the alternative (full dataflow analysis) would be significantly more complex for genuinely niche patterns.

One thing worth verifying: the IsNotNullAssertionSuppressor extension to recognize Should().NotBeNull() uses statement-order matching, not control-flow analysis. This is fine and matches the existing Assert.That().IsNotNull() implementation, but it means:

if (condition) await obj.Should().NotBeNull();
obj.Method(); // ← false suppression: nullable warning suppressed even if assertion doesn't run

This is already the case for Assert.That paths, so it's consistent — just worth noting the suppressor has this inherent limitation on both code paths.

---

Minor observations

• The AnalyzerTestCompatibility.GetCompatibleDllPath helper takes an Assembly fallback parameter that is never used in the method body (only assemblyName is used for the path). The parameter name implies it might fall back to fallback.Location on failure, but it doesn't — it throws instead. The signature is slightly misleading; either remove the fallback parameter or use it.

• GetSystemTextJson9DllPath() is only needed on net8.0 but the method has no #if guard. If called from non-net8 code it will either fail at runtime (path won't exist) or throw. This is currently only called from within #if NET8_0 blocks, so it's safe as written, but the method itself should either be gated or throw a more descriptive error.

• The #elif NET9_0_OR_GREATER consolidation in AnalyzerTestHelpers.cs (removing the separate NET9_0 / NET10_0_OR_GREATER branches) is a good simplification.

---

Summary

This is a well-designed addition. The source generator approach is the right architectural choice — it ensures the Should API stays in sync with assertions without manual maintenance overhead. The main action item before GA is making the SDK 9+ requirement visible to users before they hit the string overload ambiguity.

Recommended actions before shipping:

1. Document/enforce the SDK 9+ requirement in NuGet metadata and/or with a build error.
2. Add comments on the three hand-written collection methods explaining why they aren't generated.
3. Add a TODO comment on the current-compilation walk for the future SyntaxProvider refactor.
4. Remove unused fallback parameter from GetCompatibleDllPath or use it.