feat(mocks): add RefStructArg<T> for ref struct parameter visibility in setup/verify API#5003
feat(mocks): add RefStructArg<T> for ref struct parameter visibility in setup/verify API#5003
Conversation
There was a problem hiding this comment.
Code Review: feat(mocks): add RefStructArg for ref struct parameter visibility in setup/verify API
Overall Assessment
This is a well-motivated feature that solves a real usability problem — ref struct parameters previously disappearing silently from the mock API was genuinely confusing. The use of the allows ref struct anti-constraint (C# 13+) is the correct approach, and the #if NET9_0_OR_GREATER guards are properly placed. The snapshot test is updated and the new tests cover the key scenarios. However, there are structural concerns around code duplication and maintainability worth addressing.
Issues Found
1. Tri-fold pattern repeated 3x in MockImplBuilder.cs (DRY violation)
In GenerateWrapMethodBody, GeneratePartialMethodBody, and GenerateEngineDispatchBody, the exact same 10-line block appears:
string argsArray;
if (HasRefStructParams(method))
{
writer.AppendLine("#if NET9_0_OR_GREATER");
writer.AppendLine($"var __args = {GetArgsArrayExpression(method, true)};");
writer.AppendLine("#else");
writer.AppendLine($"var __args = {GetArgsArrayExpression(method, false)};");
writer.AppendLine("#endif");
argsArray = "__args";
}
else
{
argsArray = GetArgsArrayExpression(method, false);
}Why this matters: This pattern will inevitably diverge if one copy is updated and the others aren't. Extract it:
private static string EmitArgsArrayVariable(CodeWriter writer, MockMemberModel method)
{
if (!HasRefStructParams(method))
return GetArgsArrayExpression(method, false);
writer.AppendLine("#if NET9_0_OR_GREATER");
writer.AppendLine($"var __args = {GetArgsArrayExpression(method, true)};");
writer.AppendLine("#else");
writer.AppendLine($"var __args = {GetArgsArrayExpression(method, false)};");
writer.AppendLine("#endif");
return "__args";
}Then each call site becomes a single line: var argsArray = EmitArgsArrayVariable(writer, method);
2. HasRefStructParams duplicated across two source generator files
Both MockImplBuilder.cs and MockMembersBuilder.cs define the same private helper:
private static bool HasRefStructParams(MockMemberModel method)
=> method.Parameters.Any(p => p.IsRefStruct && p.Direction != ParameterDirection.Out);Why this matters: These will drift. Consider either moving it to a shared static helper class (e.g., MockParameterExtensions), or adding it as a computed property on MockMemberModel itself. The model approach is preferable since it keeps logic close to the data.
3. GenerateTyped*Overload method explosion
Three methods (GenerateTypedReturnsOverload, GenerateTypedCallbackOverload, GenerateTypedThrowsOverload) are each duplicated — one without allNonOutParams and one with — giving 6 methods doing very similar work.
Better approach: Unify each pair into a single method. Since the only difference is how castArgs is computed, extract that concern:
private static string BuildCastArgs(List<MockParameterModel> nonOutParams, List<MockParameterModel>? allNonOutParams = null)
{
if (allNonOutParams is null)
return string.Join(", ", nonOutParams.Select((p, i) => $"({p.FullyQualifiedType})args[{i}]!"));
var indexMap = allNonOutParams.Select((p, i) => (p, i)).ToDictionary(x => x.p, x => x.i);
return string.Join(", ", nonOutParams.Select(p => $"({p.FullyQualifiedType})args[{indexMap[p]}]!"));
}This halves the method count and makes the "with ref struct positions" case an additive concern, not a parallel copy.
4. O(n^2) IndexOf in BuildCastArgs
private static string BuildCastArgs(List<MockParameterModel> nonRefStructParams, List<MockParameterModel> allNonOutParams)
{
return string.Join(", ", nonRefStructParams.Select(p =>
{
var realIndex = allNonOutParams.IndexOf(p); // O(n) per call => O(n^2) total
return $"({p.FullyQualifiedType})args[{realIndex}]!";
}));
}For typical parameter counts this is fine at generator runtime, but the fix is straightforward (pre-build a dictionary index map — see issue 3 suggestion above).
5. Tests disabled rather than upgraded
The tests wrapped with #if !NET9_0_OR_GREATER — Void_RefStruct_Method_Callback_Fires, NonVoid_RefStruct_Param_Verification, Mixed_Params_ArgMatching_On_NonRefStruct_Params, Mixed_Params_Verification_With_Matcher — test the old silent-drop behavior. On net9.0+ they are simply compiled out, leaving a coverage gap.
These tests document important behavioral contracts. Consider updating them to test equivalent scenarios using the new API instead of disabling them. For example, Void_RefStruct_Method_Callback_Fires could verify that .Callback(() => wasCalled = true) fires when invoked via RefStructArg<T>.Any on net9.0+.
6. Undocumented API limitation on typed overloads
On net9.0+, the typed Callback/Returns/Throws overloads generated for methods with ref struct params silently exclude those params from the delegate signature. For example, for Compute(int id, ReadOnlySpan<byte> data), the generated Returns overload takes Func<int, TReturn> — the span parameter disappears from the delegate.
This is correct and necessary (lambdas can't capture ref structs), but it is not communicated at the API level. Adding an XML <remarks> note explaining that ref struct parameters are not received in typed callbacks would prevent user confusion.
Minor Observations
__args(double-underscore) in generated code is fine functionally, but double-underscore is conventionally associated with compiler-generated names. A name liketunit_argsor_argswould be less likely to cause confusion.- The new non-generic
AnyMatcher.Matches(object? value)always returnstrue. Since ref struct positions always carrynullsentinels at runtime, this is semantically correct. A comment noting "ref struct positions always store null sentinels; this matcher unconditionally matches" would help future maintainers avoid thinking it competes withAnyMatcher<T>in a buggy way.
Summary
| # | Severity | Topic |
|---|---|---|
| 1 | Medium | DRY violation: #if block repeated 3x in MockImplBuilder |
| 2 | Medium | HasRefStructParams duplicated across two files |
| 3 | Medium | 3-pair method overload explosion in MockMembersBuilder |
| 4 | Low | O(n^2) IndexOf in BuildCastArgs |
| 5 | Medium | Tests disabled rather than upgraded to new API |
| 6 | Low | Undocumented API limitation on typed overloads |
The core design is sound and the feature fills a real gap. Issues 1-3 are the most impactful for long-term maintainability and worth addressing before merge, especially as more ref-struct edge cases arise.
- Extract EmitArgsArrayVariable helper in MockImplBuilder to deduplicate 3 identical #if NET9_0_OR_GREATER args array blocks - Move HasRefStructParams to computed property on MockMemberModel (derived from Parameters, excluded from equality) - Merge 3 pairs of GenerateTyped*Overload methods and 2 BuildCastArgs overloads into single methods with optional allNonOutParams parameter - Fix O(n^2) IndexOf in BuildCastArgs by pre-building a dictionary - Add net9.0+ equivalents of disabled tests using RefStructArg<T>.Any - Add XML remarks to RefStructArg<T> documenting limitations
There was a problem hiding this comment.
Follow-up Code Review: feat(mocks): add RefStructArg
This is a follow-up to my previous review on commit 5e713cc. Comparing the current diff against the issues raised then.
Previous Issues — Status Update
| # | Issue | Status |
|---|---|---|
| 1 | DRY: #if block repeated 3× in MockImplBuilder |
✅ Fixed — EmitArgsArrayVariable extracted |
| 2 | HasRefStructParams duplicated across two files |
✅ Fixed — moved to computed property on MockMemberModel |
| 3 | 3-pair method overload explosion in MockMembersBuilder |
✅ Resolved — unified with optional allNonOutParams parameter |
| 4 | O(n²) IndexOf in BuildCastArgs |
✅ Fixed — replaced with dictionary index map |
| 5 | Tests disabled rather than upgraded | ✅ Acceptable — see below |
| 6 | Undocumented API limitation on typed delegate overloads | ✅ Fixed — <remarks> added to RefStructArg<T> |
All six issues have been addressed. The refactoring quality is good — the changes are targeted and don't introduce new complexity.
Issue 5 — Reassessment
The four tests guarded by #if \!NET9_0_OR_GREATER test the old silent-drop API behavior, which literally does not compile on net9.0+ because the generated method signatures now include RefStructArg<T> parameters. These tests cannot be ported in-place.
The new #if NET9_0_OR_GREATER tests cover equivalent contracts via the new API:
Void_RefStruct_Method_Callback_Fires→RefStructArg_Any_Matches_Void_MethodNonVoid_RefStruct_Param_Verification→RefStructArg_NonVoid_Method_VerificationMixed_Params_ArgMatching_On_NonRefStruct_Params→RefStructArg_Mixed_Params_ArgMatching_On_NonRefStruct_ParamsMixed_Params_Verification_With_Matcher→RefStructArg_Mixed_Params_Verification_With_Matcher
The approach is correct. The old tests document the legacy TFM behavior; the new tests document the new behavior. Disabling rather than deleting preserves this documentation.
Remaining Minor Observations
__args naming in generated code — Double-underscore is a conventional signal for compiler-generated identifiers, which could cause confusion when reading generated code in user projects. A name like tunit_args or _mockArgs would be clearer. Very minor.
AnyMatcher without null-sentinel note — The non-generic AnyMatcher.Matches always returns true. The class doc says "matches any value including null" but doesn't explain why null will always arrive at ref struct positions (because the generated mock impl stores a null sentinel in __args). A single sentence would help future maintainers understand this isn't a bug.
Growing method signatures — GenerateVoidUnifiedClass and GenerateReturnUnifiedClass each now take 7 parameters including the new hasRefStructParams and allNonOutParams. If the feature continues to expand, grouping into a small GenerationContext record would be worth considering. Not an issue now, just a note for future growth.
Summary
The PR is in excellent shape. All substantive issues from the previous review are resolved. The architecture is sound — allows ref struct anti-constraint, #if NET9_0_OR_GREATER guards in both the runtime library and generated code, and null sentinels for runtime argument-array alignment are all the right choices. The remaining observations above are cosmetic and not blocking.
Approved.
There was a problem hiding this comment.
Follow-up Code Review: feat(mocks): add RefStructArg
This is a follow-up to my previous review on commit 5e713cc. Comparing the current diff against the issues raised then.
Previous Issues — Status Update
| # | Issue | Status |
|---|---|---|
| 1 | DRY: #if block repeated 3x in MockImplBuilder |
Fixed — EmitArgsArrayVariable extracted |
| 2 | HasRefStructParams duplicated across two files |
Fixed — moved to computed property on MockMemberModel |
| 3 | 3-pair method overload explosion in MockMembersBuilder |
Resolved — unified with optional allNonOutParams parameter |
| 4 | O(n^2) IndexOf in BuildCastArgs |
Fixed — replaced with dictionary index map |
| 5 | Tests disabled rather than upgraded | Acceptable — see below |
| 6 | Undocumented API limitation on typed delegate overloads | Fixed — remarks added to RefStructArg |
All six issues have been addressed. The refactoring quality is good — the changes are targeted and do not introduce new complexity.
Issue 5 — Reassessment
The four tests guarded by #if !NET9_0_OR_GREATER test the old silent-drop API behavior, which literally does not compile on net9.0+ because the generated method signatures now include RefStructArg<T> parameters. These tests cannot be ported in-place.
The new #if NET9_0_OR_GREATER tests cover equivalent contracts via the new API:
Void_RefStruct_Method_Callback_Fires->RefStructArg_Any_Matches_Void_MethodNonVoid_RefStruct_Param_Verification->RefStructArg_NonVoid_Method_VerificationMixed_Params_ArgMatching_On_NonRefStruct_Params->RefStructArg_Mixed_Params_ArgMatching_On_NonRefStruct_ParamsMixed_Params_Verification_With_Matcher->RefStructArg_Mixed_Params_Verification_With_Matcher
The approach is correct. The old tests document the legacy TFM behavior; the new tests document the new behavior. Disabling rather than deleting preserves this documentation intent.
Remaining Minor Observations
__args naming in generated code — Double-underscore is a conventional signal for compiler-generated identifiers, which could cause confusion when a user reads the generated output in their project. A name like tunit_args or _mockArgs would be clearer. Very minor.
AnyMatcher without null-sentinel note — The non-generic AnyMatcher.Matches always returns true. The class doc says "matches any value including null" but does not explain why null will always arrive at ref struct positions (because the generated mock impl stores a null sentinel in the args array). A single inline comment would help future maintainers understand this is intentional, not a bug.
Growing method signatures — GenerateVoidUnifiedClass and GenerateReturnUnifiedClass each now take 7 parameters including the new hasRefStructParams and allNonOutParams. If the feature continues to expand, grouping into a small GenerationContext record would be worth considering. Not an issue now, just a note for future growth.
Summary
The PR is in excellent shape. All substantive issues from the previous review are resolved. The architecture is sound — the allows ref struct anti-constraint, #if NET9_0_OR_GREATER guards in both the runtime library and generated code, and null sentinels for runtime argument-array alignment are all the right choices. The remaining observations above are cosmetic and not blocking.
Approved.
There was a problem hiding this comment.
Final Review: feat(mocks): add RefStructArg
This is a follow-up to my previous two reviews (on commits 5e713cc and af1ee1493). The only change since the last review is the documentation update.
Documentation Review (only change since last approval)
The new Ref Struct Parameters section in argument-matchers.md is well done:
- The table entry
RefStructArg<T>.Any | Any value of a ref struct type (.NET 9+)is placed correctly among the other matchers - The
.NET 9+ Onlyadmonition clearly communicates the compatibility constraint and legacy TFM behavior - The Limitations list accurately captures all three constraints (Any-only, no capture, excluded from typed callbacks) — directly reflecting the
<remarks>inRefStructArg<T> - Code examples for both pure ref-struct and mixed-param cases are clear and match the test scenarios
- The note directing users to the
Action<object?[]>overload for typed callbacks is a helpful escape hatch reference
No issues with the docs.
Previous Issues — All Resolved
| # | Issue | Status |
|---|---|---|
| 1 | DRY: #if block repeated 3× in MockImplBuilder |
✅ EmitArgsArrayVariable extracted |
| 2 | HasRefStructParams duplicated across two files |
✅ Moved to computed property on MockMemberModel |
| 3 | Method overload explosion in MockMembersBuilder |
✅ Unified with optional allNonOutParams |
| 4 | O(n²) IndexOf in BuildCastArgs |
✅ Dictionary index map |
| 5 | Tests disabled rather than upgraded | ✅ New equivalent tests added under #if NET9_0_OR_GREATER |
| 6 | Undocumented typed-overload limitation | ✅ <remarks> in RefStructArg<T> + docs section |
The PR is in excellent shape. The allows ref struct anti-constraint approach, null sentinels for args-array alignment, and #if NET9_0_OR_GREATER guards throughout are all the right architectural choices. Approved.
Summary
RefStructArg<T> where T : allows ref structtype (net9.0+) so ref struct parameters are visible in mock setup/verify extension methods instead of being silently dropped#if NET9_0_OR_GREATER/#else/#endifblocks: on net9.0+ extension methods includeRefStructArg<T>params; on older TFMs current behavior is preservedAnyMatcherfor ref struct arg positions (ref structs can't be generic type args forArg<T>)Before (all TFMs)
After (net9.0+)
Test plan
RefStructArg<T>tests#if !NET9_0_OR_GREATERwhere needed