fix(should): add specialized assertion sources#5806
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #5806 — Add specialized assertion sources
This is a substantial, well-structured PR. The overall approach — extending Should() routing via source generation plus hand-written ShouldDictionarySource, ShouldSetSource, and ShouldDelegateCollectionSource — is sound. The base-class refactor (ShouldSourceBase / ShouldEnumerableSourceBase / ShouldSetSourceBase) reduces duplication nicely. Below are the specific issues I found.
1. Bug: GetShouldEntryKey appends ReceiverTypeName twice (copy-paste error)
File: TUnit.Assertions.Should.SourceGenerator/ShouldExtensionGenerator.cs
private static string GetShouldEntryKey(ShouldEntryData entry)
{
var sb = new StringBuilder(entry.ReceiverTypeName)
.Append('|')
.Append(entry.ReceiverTypeName) // <-- BUG: should be entry.SourceTypeArgDisplay
.Append('|')
.Append(entry.Priority)
...The second .Append(entry.ReceiverTypeName) is clearly intended to be .Append(entry.SourceTypeArgDisplay). Two different Assert.That overloads that share the same receiver type but project to different IAssertionSource<T> type arguments (e.g., a hypothetical dual overload for IList<T> that returns two different assertion types) will hash-collide and only the first will survive deduplication.
Fix:
var sb = new StringBuilder(entry.ReceiverTypeName)
.Append('|')
.Append(entry.SourceTypeArgDisplay) // was: entry.ReceiverTypeName
.Append('|')
.Append(entry.Priority)
...2. Key-format mismatch between GetShouldEntryKey and CreateShouldMethodSignatureKey
GetShouldEntryKey (used to build the key from a freshly-collected ShouldEntryData) and CreateShouldMethodSignatureKey (used to build the key from an already-baked IMethodSymbol in CollectExistingShouldEntryKeys) produce structurally different strings for the same logical overload:
GetShouldEntryKey |
CreateShouldMethodSignatureKey |
|
|---|---|---|
| Name segment | Not included | method.Name / methodNameOverride |
| Receiver | ReceiverTypeName (display string) |
Uses AppendTypeSignatureKey with ordinal-normalised type parameters |
| Type-arg count | MethodGenericParams.Length |
method.TypeParameters.Length |
| Subsequent params | TypeName:Name |
normalised type via AppendTypeSignatureKey |
The mismatch means the SignatureKey computed at collection time (which uses CreateShouldMethodSignatureKey) can never match a key produced by GetShouldEntryKey at dedup time. The allEntries dedup path therefore only prevents duplicate ShouldEntryData objects from showing up in the same run, but the separate currentShouldEntryKeys / bakedReferencedShouldEntryKeys checks (which use SignatureKey, i.e., the CreateShouldMethodSignatureKey form) never match anything produced by GetShouldEntryKey.
Why this matters: ShouldEntryData.SignatureKey is populated by CreateShouldMethodSignatureKey (line ~814), but DeduplicateEntries uses GetShouldEntryKey. If the intent is for SignatureKey to be the canonical key throughout, GetShouldEntryKey should simply return entry.SignatureKey (or be removed entirely), and DeduplicateEntries should key on entry.SignatureKey directly. That would also eliminate the copy-paste issue in point 1.
Suggested fix — simplify DeduplicateEntries:
private static ShouldEntryData[] DeduplicateEntries(ShouldEntryData[] entries)
{
var seen = new HashSet<string>(StringComparer.Ordinal);
var result = new List<ShouldEntryData>(entries.Length);
foreach (var entry in entries)
{
if (seen.Add(entry.SignatureKey))
result.Add(entry);
}
return result.ToArray();
}And remove GetShouldEntryKey entirely, since SignatureKey is already the canonical fingerprint.
3. ShouldDelegateCollectionSource is a readonly struct but Because(string) returns a new struct
The Because method on ShouldDelegateCollectionSource<TItem> is correct in spirit:
public ShouldDelegateCollectionSource<TItem> Because(string message)
=> new(Context, message.Trim());However, the existing class-based sources (now inheriting from ShouldSourceBase) store _becauseMessage as a field and mutate it in Because. A consumer writing:
await func.Should().Because("reason").HaveAtLeast(2);works fine, but:
var source = func.Should();
source.Because("reason"); // ← value is discarded; struct is copied
await source.HaveAtLeast(2); // ← because message is lostThis is the classic struct-mutation trap. ShouldDelegateSource<T> (the pre-existing sibling) is also readonly struct and has the same issue — so this is consistent within the design — but it is worth calling out since the other new sources (ShouldDictionarySource, ShouldSetSource) are classes and avoid the issue. A comment or XML doc warning that Because must be consumed in the same fluent chain would prevent user confusion.
4. Public ApplyBecause on ShouldEnumerableSourceBase
// ShouldSourceBase.cs
protected TAssertion ApplyBecause<TAssertion>(TAssertion assertion)
where TAssertion : Assertion<TCollection>ApplyBecause is protected, which is intentional. But the public API snapshot shows it appearing in the public surface (via the protected accessibility leaking through the abstract base). This is fine for inheritance, but it would benefit from a [EditorBrowsable(EditorBrowsableState.Never)] annotation to prevent it cluttering IntelliSense for test authors who subclass these bases.
5. ShouldSetSource<TItem> does not carry [ShouldGeneratePartial]; ShouldReadOnlySetSource<TItem> and ShouldHashSetSource<TItem> don't either
ShouldDictionarySource and ShouldMutableDictionarySource both carry [ShouldGeneratePartial(typeof(DictionaryAssertion<,>))] / [ShouldGeneratePartial(typeof(MutableDictionaryAssertion<,>))], so source-generated partial methods are emitted for those types. The set sources (ShouldSetSource, ShouldReadOnlySetSource, ShouldHashSetSource) do not have [ShouldGeneratePartial], which means they cannot pick up assertion methods from Assert.That overloads automatically. If the corresponding SetAssertion<T> family has methods beyond those hand-coded in ShouldSetSourceBase, they will be silently missing. This asymmetry should be intentional (or the attribute should be added).
Minor observations (not blocking)
-
The
BuildExpressionstatic helper is copy-pasted into every new source class (ShouldDictionarySource,ShouldMutableDictionarySource,ShouldSetSource,ShouldReadOnlySetSource,ShouldHashSetSource). Now thatShouldSourceBaseexists, a singleprotected static StringBuilder BuildExpression(string? expression)there would eliminate all the duplication. -
ShouldDelegateCollectionSource.FormatTypeName(Type t)is private and duplicated from the pre-existingShouldDelegateSource. Consider moving it to a shared internal utility.
Summary
The main actionable items are (in priority order):
- Fix the copy-paste bug in
GetShouldEntryKey(point 1). - Align the dedup key — either use
entry.SignatureKeydirectly inDeduplicateEntries, or ensureGetShouldEntryKeygenerates the same format asCreateShouldMethodSignatureKey(point 2). - Document or guard the
Becausestruct-copy trap onShouldDelegateCollectionSource(point 3).
Everything else is polish. The architecture — incremental generator specialisation, the base-class hierarchy, the snapshot test coverage — is solid work.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 2 medium |
| CodeStyle | 6 minor |
| Performance | 1 medium |
🟢 Metrics 168 complexity
Metric Results Complexity 168
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.
Code Review: PR #5806 — Add specialized assertion sourcesThis is a substantial, well-structured PR. The overall approach — extending 1. Bug:
|
GetShouldEntryKey |
CreateShouldMethodSignatureKey |
|
|---|---|---|
| Name segment | not included | method.Name / override |
| Receiver | display string | ordinal-normalised via AppendTypeSignatureKey |
| Subsequent params | TypeName:Name |
normalised type |
This means ShouldEntryData.SignatureKey (populated by CreateShouldMethodSignatureKey) can never match a key produced by GetShouldEntryKey. The currentShouldEntryKeys / bakedReferencedShouldEntryKeys guards that are meant to prevent re-emitting already-baked methods are effectively bypassed, because both sides use different encoding.
Suggested fix — Since ShouldEntryData.SignatureKey is already the canonical fingerprint, simplify DeduplicateEntries to key on it directly and remove GetShouldEntryKey:
private static ShouldEntryData[] DeduplicateEntries(ShouldEntryData[] entries)
{
var seen = new HashSet<string>(StringComparer.Ordinal);
var result = new List<ShouldEntryData>(entries.Length);
foreach (var entry in entries)
{
if (seen.Add(entry.SignatureKey))
result.Add(entry);
}
return result.ToArray();
}This also resolves issue #1 as a side-effect.
3. ShouldDelegateCollectionSource is a readonly struct — Because result is silently discarded
public ShouldDelegateCollectionSource<TItem> Because(string message)
=> new(Context, message.Trim()); // returns a NEW structAny code that writes source.Because("reason"); await source.HaveAtLeast(2); loses the message silently because the struct copy is discarded. This matches ShouldDelegateSource<T>'s existing behaviour, so it's consistent within the design — but the new class-based sources (ShouldDictionarySource, ShouldSetSource) don't have this footgun. A [MustUseReturnValue] / XML-doc warning that Because must be chained directly would prevent subtle test bugs.
4. ShouldSetSource / ShouldReadOnlySetSource / ShouldHashSetSource lack [ShouldGeneratePartial]
ShouldDictionarySource and ShouldMutableDictionarySource both carry [ShouldGeneratePartial(typeof(DictionaryAssertion<,>))], causing the generator to emit partial methods for all matching assertion overloads. The set sources do not have this attribute. If SetAssertion<T> gains new overloads in future, those overloads won't be picked up automatically. If this is intentional (set assertions are fully hand-coded), a brief comment explaining why would prevent later confusion.
Minor observations (non-blocking)
-
BuildExpressionduplication: Theprivate static StringBuilder BuildExpression(string? expression)helper is copy-pasted identically into every new source class. Now thatShouldSourceBaseexists, a singleprotected static StringBuilder BuildExpression(string? expression)there would eliminate all copies. -
FormatTypeNameduplication:ShouldDelegateCollectionSource.FormatTypeNameis a private copy of the same method inShouldDelegateSource. A shared internal utility would avoid drift.
Summary
| Priority | Issue |
|---|---|
| Must fix | Copy-paste bug in GetShouldEntryKey — ReceiverTypeName appended twice instead of SourceTypeArgDisplay |
| Must fix | GetShouldEntryKey and CreateShouldMethodSignatureKey use incompatible key formats — dedup guards are ineffective |
| Recommended | Document Because struct-copy trap on ShouldDelegateCollectionSource |
| Low | Missing [ShouldGeneratePartial] on set sources — clarify intent |
The architecture (incremental generator specialisation, base-class hierarchy, snapshot test coverage) is solid. The two key-format issues are the main things to address before merge.
Generate inherited collection methods for set-specific Should() wrappers so HashSet and ISet receivers match Assert.That(...). Also dedupe generated Should entrypoints by the canonical signature key to avoid stale key collisions.
There was a problem hiding this comment.
Code Review — PR #5806: fix(should): add specialized assertion sources
Status of previous review findings
Resolved:
- ✅
GetShouldEntryKeycopy-paste bug (double-appendingReceiverTypeName) — the method no longer exists;DeduplicateEntriesusesentry.SignatureKeydirectly. Clean fix. - ✅ Key-format mismatch between deduplication helpers — resolved by the same change.
- ✅ Set sources (
ShouldSetSource,ShouldReadOnlySetSource,ShouldHashSetSource) missing[ShouldGeneratePartial]— all three now carry the attribute.
Still present (carried over from previous review):
⚠️ BuildExpressionis still duplicated across 7 classes (ShouldCollectionSource,ShouldDelegateCollectionSource, 2 dictionary sources, 3 set sources). SinceShouldDelegateCollectionSourceis areadonly structit can't inherit, but the six class-based sources could all delegate to aprotected staticmethod onShouldSourceBase. No blocker, but worth cleaning up.⚠️ FormatTypeNameis still private inShouldDelegateCollectionSourceand duplicated fromShouldDelegateSource<T>.⚠️ ShouldDelegateCollectionSourceis areadonly struct—Because(string)returns a new copy; chaining is required or the message is silently lost. This is consistent withShouldDelegateSource<T>, so acceptable, but a one-line XML doc warning would prevent user confusion.
New concerns
1. GetWrapperMethodKey type-parameter count inconsistency (low risk)
// WrapperMethodData overload — hardcoded '0'
var sb = new StringBuilder(NameConjugator.Conjugate(method.SourceMethodName))
.Append('|').Append('0');
// IMethodSymbol overload — uses actual count
var sb = new StringBuilder(method.Name)
.Append('|').Append(method.TypeParameters.Length);If any assertion method ever carries its own type parameter (e.g., a generic helper method), the two overloads produce different keys for the same logical method, and CollectExistingWrapperMethodKeys fails to suppress re-emission, potentially causing a compile error in downstream assemblies. Today's assertion methods appear to only carry class-level type parameters, so this is latent. A Debug.Assert(data.MethodGenericParams.Length == 0) or using data.MethodGenericParams.Length instead of the literal '0' would make the invariant explicit and safe.
2. Generated Should() entry points always return ShouldSource<T>, not the specialized source (design concern)
EmitShouldEntry always emits:
var source = Assert.That<TKey, TValue>(value, expression);
var innerContext = ((IAssertionSource<IReadOnlyDictionary<TKey, TValue>>)source).Context;
return new ShouldSource<IReadOnlyDictionary<TKey, TValue>>(innerContext);This discards the ShouldDictionarySource<TKey, TValue> that Assert.That returns, wrapping it in the plain ShouldSource<T>. For the current manually-coded overloads this is harmless (deduplication skips re-emission via CollectExistingShouldEntryKeys), but for any future or downstream Assert.That overload with a specialized source, users get ShouldSource<T> with no access to the richer API.
The cleaner approach when emitting a wrapper is to return the actual returnType of Assert.That (already available as ShouldEntryData.SourceTypeArgDisplay... or rather, the return type of the detected Assert.That method) and emit a direct call without the cast-and-wrap round-trip:
// instead of:
return new ShouldSource<T>(innerContext);
// emit:
return Assert.That<...>(value, expression); // returns the real specialized typeThis would require storing the return type name in ShouldEntryData alongside SourceTypeArgDisplay, but it removes a needless downcast and future-proofs the pattern. Worth considering before this merges if there are plans to add more specialized sources.
Overall assessment
The core architecture is solid: the new ShouldSourceBase / ShouldEnumerableSourceBase / ShouldSetSourceBase hierarchy is a clear improvement over the prior copy-paste pattern in ShouldCollectionSource, and the SignatureKey-based deduplication is correct. The two "must-fix" items from the previous review are cleanly resolved.
The main outstanding concern is point 2 above: the generated entry points silently downgrade specialized source types to plain ShouldSource<T>. Whether that's acceptable now (because manual overloads cover all current specialized cases) should be an explicit, documented decision rather than an accidental gap.
Code Review — PR #5806:
|
…t copy Address PR review: - Move BuildExpression to single ShouldExpressionBuilder.Build helper; drop 6 copies across set/dictionary/collection sources and ShouldExtensions. - Extract FormatTypeName to DelegateExceptionTypeFormatter; share between ShouldDelegateSource and ShouldDelegateCollectionSource. - Document the readonly-struct copy trap on Because for both delegate sources so unchained calls are recognized as a footgun. - Comment GetWrapperMethodKey's hardcoded '0' tying it to the TryDescribeWrapperMethod filter for method-level type parameters.
|
Pushed 246251b addressing the review:
On point 2 (specialized return type), pushing back: I think there's a misread of the code. Snapshot tests (46) and Should runtime tests (130) all green locally. |
Code Review — PR #5806 (round 3): `fix(should): add specialized assertion sources`Status of all previous findingsResolved in latest push:
New findings1. `ShouldSourceBase.ResetShouldExpression` has no callers — appears to be dead code // ShouldSourceBase.cs
protected void ResetShouldExpression(string? expression)
{
Context.ExpressionBuilder.Clear();
Context.ExpressionBuilder.Append(expression ?? "?").Append(".Should()");
}None of the concrete sources (`ShouldCollectionSource`, `ShouldDictionarySource`, `ShouldSetSource`, etc.) call this method. The generated code in `EmitShouldEntry` operates on `innerContext` directly rather than through a source instance. Either remove it or add a brief comment explaining its intended use. 2. `ShouldDictionarySource` / `ShouldMutableDictionarySource` share ~9 nearly-identical methods (architectural, non-blocking) Both classes have identical signatures for `ContainKey`, `NotContainKey`, `ContainValue`, `NotContainValue`, `ContainKeyWithValue`, `AllKeys`, `AllValues`, `AnyKey`, `AnyValue` — differing only in their `TDictionary` type argument. This PR already introduced `ShouldSetSourceBase` to deduplicate exactly this kind of cross-class repetition between `ShouldSetSource`/`ShouldReadOnlySetSource`/`ShouldHashSetSource`. A `ShouldDictionarySourceBase<TDictionary, TKey, TValue, TSelf>` (similar pattern) would halve the duplicate code. That said, the assertion hierarchy (`DictionaryAssertion` vs `MutableDictionaryAssertion`) is already duplicated at the lower layer, so this is inherited structural debt rather than something introduced here — fine to leave for a follow-up. Overall assessmentAll must-fix items from previous rounds are cleanly resolved. The `ShouldSourceBase` / `ShouldEnumerableSourceBase` / `ShouldSetSourceBase` hierarchy is a genuine improvement, snapshot and API tests are updated, and the `SignatureKey`-based deduplication logic is now correct. The only actionable item before merge is the dead `ResetShouldExpression` method — either remove it or document its intended purpose. The dictionary source duplication is worth a follow-up but not a blocker. |
…ll in net8 generator tests Net8.0 BCL lacks OverloadResolutionPriorityAttribute. The Polyfill copy compiled into the test assembly is internal, so the synthetic GeneratorTest compilation can't bind constructor arguments — generator returns priority 0 and snapshot snapshot diverges from the verified output that includes [OverloadResolutionPriority(N)]. Inject a public copy into the compilation under #if NET8_0 so attribute resolves consistently across all multi-target TFMs.
There was a problem hiding this comment.
Code Review — PR #5806 (third pass): `fix(should): add specialized assertion sources`
Status of previous review findings
All previous blockers resolved:
- ✅ `GetShouldEntryKey` copy-paste bug — resolved
- ✅ Deduplication key-format mismatch — `DeduplicateEntries` now uses `entry.SignatureKey` directly
- ✅ Set sources missing `[ShouldGeneratePartial]` — all three now carry the attribute
- ✅ `GetWrapperMethodKey` type-parameter count inconsistency (low risk) — comment added clarifying the invariant
The `ShouldSourceBase` / `ShouldEnumerableSourceBase` / `ShouldSetSourceBase` hierarchy looks clean. The `ShouldExpressionBuilder` extraction eliminates the `BuildExpression` duplication mentioned in earlier passes. The `DelegateExceptionTypeFormatter` extraction resolves the `FormatTypeName` duplication. The `Because()` XML doc warning on the struct types is appreciated.
New concern: `EmitShouldEntry` omits `[CallerArgumentExpression]` on the expression parameter
File: `TUnit.Assertions.Should.SourceGenerator/ShouldExtensionGenerator.cs` — `EmitShouldEntry`
The generator correctly records `CallerArgumentExpressionTarget` in `ParameterData` and uses it in the body to build the expression string. But when emitting the method signature, the attribute is never written:
// Current emission (no [CallerArgumentExpression]):
public static ShouldSource<ISet<TItem>> Should<TItem>(
this ISet<TItem>? value, string? expression = default) // ← missing [CallerArgumentExpression("value")]Compare with `EmitMethod` / `EmitWrapperMethod` at lines ~1053–1055 and ~1175, which both do emit the attribute. The snapshot verified files confirm the omission (e.g., `...dictionary_specialization...DotNet10_0.verified.txt` shows no attribute on `expression`).
Impact: Any time `EmitShouldEntry` actually writes a `Should()` overload (i.e., dedup did not suppress it), callers get `"?.Should()"` instead of `"myValue.Should()"` in failure messages. For the current PR this only affects downstream assemblies that define their own `Assert.That` overloads for the whitelisted receiver types — unlikely today but entirely within the generator's design intent.
Fix — add the attribute emission loop in `EmitShouldEntry` analogous to the existing pattern in `EmitMethod`:
sb.Append($" public static global::TUnit.Assertions.Should.Core.ShouldSource<{entry.SourceTypeArgDisplay}> Should{genericList}(this {entry.ReceiverTypeName}? value");
foreach (var p in entry.Parameters)
{
sb.Append(", ");
if (p.CallerArgumentExpressionTarget is not null)
{
sb.Append($"[global::System.Runtime.CompilerServices.CallerArgumentExpression(\"{p.CallerArgumentExpressionTarget}\")] ");
}
sb.Append($"{p.TypeName} {p.Name}");
if (p.HasDefaultValue)
sb.Append(" = ").Append(p.DefaultValueLiteral);
}The snapshot .verified.txt files would need to be regenerated after this fix.
Minor: `ICollection` (priority 3) and `IReadOnlyList` (priority 3) share the same priority
Both new overloads in `ShouldExtensions.cs` have `[OverloadResolutionPriority(3)]`. For any type that implements both `ICollection` and `IReadOnlyList` without `IList` (unusual but valid), the compiler would produce an ambiguity error. All framework types (`List`, `ReadOnlyCollection`, `ImmutableArray`) implement `IList` as well, so priority 4 would win for them. The risk is theoretical for user types, but assigning distinct priorities (`IReadOnlyList` at 3, `ICollection` at 2, or vice versa) would close the gap at no cost.
Minor: `ResetShouldExpression` is dead protected API
`ShouldSourceBase<TValue, TSelf>.ResetShouldExpression(string? expression)` is `protected` and appears in the public API snapshot but is not called anywhere in the new or existing source. It would be cleaner to remove it, or at minimum mark it `[EditorBrowsable(EditorBrowsableState.Never)]` to hide it from IntelliSense for consumers who subclass the base.
Overall assessment
The architecture is solid and the two previous blocking issues are cleanly resolved. The `[CallerArgumentExpression]` omission in `EmitShouldEntry` is the only new correctness concern worth fixing before merge — its impact is currently limited but it will silently degrade failure messages for any future use of the generated path. Everything else is polish.
1 participant
Summary
Verification