Conversation
… and in parameter tests Add support for configuring return values on methods returning ReadOnlySpan<T>/Span<T> via .Returns() on the generated typed wrapper class. Span return values are stored as arrays via OutRefContext at index -1 and reconstructed at invocation time. Also adds comprehensive test coverage for: - Out/ref parameters of span types (ReadOnlySpan<T>, Span<T>) via array conversion - Span-returning methods (.Returns(), Callback, Throws, verification) - 'in' (readonly ref) parameters (argument matching, returns, callbacks, verification)
There was a problem hiding this comment.
Code Review — Span Return + Out/Ref Span + In Parameter Support
This is a well-motivated set of changes that fills real gaps in TUnit.Mocks. The array-conversion pattern for span storage is the right approach (ref structs can't be boxed), and the test coverage is thorough. A few architectural concerns worth addressing:
Issue 1: Magic constant -1 creates a hidden, fragile contract
The most significant concern is using integer literal -1 as the storage key for span return values across two independently-generated code locations.
In MockMembersBuilder.cs:
// generated Returns(SpanType) method:
public {wrapperName} Returns({spanReturnType} value) {{ EnsureSetup().SetsOutParameter(-1, value.ToArray()); return this; }}In MockImplBuilder.cs (EmitSpanReturnReadback):
writer.AppendLine($"if (__outRef.TryGetValue(-1, out var __spanRet)) return new {method.ReturnType}(({method.SpanReturnElementType}[])__spanRet!);");This is a silent contract between two code-generation paths. The number -1 has no documentation at either site, and there is nothing preventing a future change from colliding with it (e.g., if parameter indexing ever went negative).
Better approach: Define a named constant in a shared location so the contract is explicit:
// In OutRefContext or a sibling constants class:
/// <summary>Reserved index used to store the return value for span-returning methods.</summary>
public const int SpanReturnValueIndex = -1;Then reference OutRefContext.SpanReturnValueIndex in both generators. The generated code itself can still emit the literal, but the source of truth becomes self-documenting.
Issue 2: Code duplication between EmitOutRefReadback and EmitSpanReturnReadback
EmitSpanReturnReadback essentially reimplements the out/ref parameter reading loop from EmitOutRefReadback, then appends the span-return readback. The param-iteration block is copy-pasted nearly verbatim.
Why this matters: The two copies can drift. If a third case is added to parameter readback (e.g., a new ref struct subtype), it must be updated in both places.
Better approach: Extract the parameter-reading loop into a shared private helper, then compose:
private static void EmitOutRefParamReadback(CodeWriter writer, MockMemberModel method)
{
// loop over out/ref params — single source of truth
for (int i = 0; i < method.Parameters.Length; i++)
{
var p = method.Parameters[i];
if (p.IsRefStruct && p.SpanElementType is null) continue;
if (p.Direction == ParameterDirection.Out || p.Direction == ParameterDirection.Ref)
{
if (p.SpanElementType is not null)
writer.AppendLine($"...");
else
writer.AppendLine($"...");
}
}
}
private static void EmitOutRefReadback(CodeWriter writer, MockMemberModel method)
{
writer.AppendLine("var __outRef = global::TUnit.Mocks.Setup.OutRefContext.Consume();");
using (writer.Block("if (__outRef is not null)"))
EmitOutRefParamReadback(writer, method);
}
private static void EmitSpanReturnReadback(CodeWriter writer, MockMemberModel method)
{
writer.AppendLine("var __outRef = global::TUnit.Mocks.Setup.OutRefContext.Consume();");
using (writer.Block("if (__outRef is not null)"))
{
EmitOutRefParamReadback(writer, method); // reuse
writer.AppendLine($"if (__outRef.TryGetValue(-1, out var __spanRet)) return new {method.ReturnType}(({method.SpanReturnElementType}[])__spanRet!);");
}
writer.AppendLine("return default;");
}Issue 3: GenerateVoidUnifiedClass signature extension with optional params
Adding optional parameters to an existing method is a low-risk change here (it's private, source-generated only), but the pattern is slightly misleading — the function is now generating fundamentally different code paths depending on whether spanReturnElementType is null.
Minor suggestion: A small parameter record or breaking these into separate methods (or overloads) would make the branching explicit. This is a lower-priority concern given it's internal to the generator.
Positives
GetSpanElementType— clean implementation usingMetadataName+ namespace rather than display strings; robust against aliasing.- Thread safety —
[ThreadStatic]inOutRefContextcorrectly isolates parallel test execution. - Model equality —
SpanElementTypeandSpanReturnElementTypeare correctly included inEqualsandGetHashCodeon both model types. - Test coverage — 99 tests across 4 files covering empty spans, data, throws, callbacks, verification, arg matchers, and mixed params is excellent.
- The
#if NET9_0_OR_GREATERguard inComprehensiveOutRefSpanTestsfor ref struct arg matching is the right approach. - Fallback
return default;inEmitSpanReturnReadbackcorrectly handles the no-setup path.
Summary
| Severity | Concern |
|---|---|
| Medium | Magic -1 literal — define a named constant |
| Low | Duplicated out/ref param loop — extract shared helper |
| Nit | Optional params on GenerateVoidUnifiedClass |
The span array-conversion pattern is the right design for ref struct serialization. Addressing the -1 constant would significantly improve maintainability.
…readback - Add OutRefContext.SpanReturnValueIndex constant to replace magic -1 literal - Extract EmitOutRefParamAssignments shared helper to deduplicate the out/ref param reading loop between EmitOutRefReadback and EmitSpanReturnReadback
There was a problem hiding this comment.
Follow-up Review
Good news: both primary concerns from the previous review have been fully addressed in the second commit (d70a5a09).
Issue 1 (Magic -1 literal) — ✅ Resolved
OutRefContext.SpanReturnValueIndex = -1 is now a named constant, and both generation sites reference it via the fully qualified name:
MockMembersBuilder.cs:EnsureSetup().SetsOutParameter(global::TUnit.Mocks.Setup.OutRefContext.SpanReturnValueIndex, value.ToArray())MockImplBuilder.cs(EmitSpanReturnReadback):__outRef.TryGetValue(global::TUnit.Mocks.Setup.OutRefContext.SpanReturnValueIndex, ...)
The public documentation on the constant (Parameter indices are always >= 0, so -1 is safe from collision) makes the contract self-evident.
Issue 2 (Duplicated out/ref param loop) — ✅ Resolved
EmitOutRefParamAssignments is now the single source of truth for parameter readback, called from both EmitOutRefReadback and EmitSpanReturnReadback. The split-continue guard (if (p.IsRefStruct && p.SpanElementType is null) continue;) correctly handles the non-span ref struct skip in one place.
Issue 3 (Optional params on GenerateVoidUnifiedClass) — Accepted as-is
This was a nit; the current approach is pragmatic and low-risk for an internal generator method. No action needed.
No new concerns
The PR is clean. The GetSpanElementType detection, model equality/hash updates, and test coverage all remain solid. Approved.
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.17.25 to 1.17.29. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.17.29 <!-- Release notes generated using configuration in .github/release.yml at v1.17.29 --> ## What's Changed ### Other Changes * feat(mocks): span return .Returns() support, out span params, and in param tests by @thomhurst in thomhurst/TUnit#5007 * refactor(mocks): move Mock<T> control members to static Mock helpers by @thomhurst in thomhurst/TUnit#5006 ### Dependencies * chore(deps): update dependency rabbitmq.client to 7.2.1 by @thomhurst in thomhurst/TUnit#5005 * chore(deps): update tunit to 1.17.25 by @thomhurst in thomhurst/TUnit#5004 **Full Changelog**: thomhurst/TUnit@v1.17.25...v1.17.29 Commits viewable in [compare view](thomhurst/TUnit@v1.17.25...v1.17.29). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
.Returns()support for methods returningReadOnlySpan<T>/Span<T>— span values are stored as arrays viaOutRefContextat index-1and reconstructed at invocation time.ToArray()at setup,new Span<T>(array)at readback)in(readonly ref) parameters which were implemented but untestedChanges
Source generator (5 files):
MockMemberModel/MockParameterModel— addedSpanReturnElementTypeandSpanElementTypepropertiesMemberDiscovery— detectReadOnlySpan<T>/Span<T>viaMetadataName+ namespaceMockMembersBuilder— generateReturns(SpanType)on void wrapper class for span returns; typedSetsOut/SetsRefmethods for span out/ref paramsMockImplBuilder—EmitSpanReturnReadbackreads index-1fromOutRefContextto reconstruct span return values; updated all 3 impl paths (interface, wrap, partial)Tests (4 new files, 99 new tests):
OutRefSpanTests.cs— 4 basic outReadOnlySpan<byte>testsComprehensiveOutRefSpanTests.cs— 70 tests across 10 classes (out Span, out ReadOnlySpan, multiple out params, ref+out combos, dual spans, sequential patterns)SpanReturnTests.cs— 14 tests for span-returning methods (.Returns, Callback, Throws, verification, arg matching, large data)InParameterTests.cs— 15 tests forinparameters (matching, returns, callbacks, throws, verification, structs, predicates)Test plan
dotnet run --framework net10.0)