fix(core): fill optional params when invoking MethodDataSource via reflection

[thomhurst]

Summary

• Closes [Bug]: Optional CancellationToken parameter not respected as parameter while using MethodDataSource with abstract class #5879. [InheritsTests] derived class on a generic abstract base + a [MethodDataSource] method with an optional parameter (e.g. [EnumeratorCancellation] CancellationToken ct = default) threw TargetParameterCountException at discovery.
• Root cause: MethodDataSourceAttribute.GetDataRowsAsync called methodInfo.Invoke(instance, Arguments) with Arguments == []; reflection's Invoke does not auto-fill defaults like a C# call site. The source-gen Factory path emits a direct C# call (compiler fills defaults), but the reflection fallback used for the generic abstract base case did not — so the optional CT parameter triggered the count mismatch.
• Fix: added BuildInvokeArgs(MethodInfo, object?[]) that pads the supplied args to the method's parameter count, filling missing slots with ParameterInfo.DefaultValue, then CancellationToken.None for CancellationToken-typed params without an explicit default, then falling back to the original array so genuine required-param mismatches still surface the original TargetParameterCountException.

Behaviour parity with source-gen

The source generator emits DataSource() (TestMetadataGenerator.cs:1628) — the C# compiler then fills default(CancellationToken). The reflection path now matches that exactly (CancellationToken.None). Plumbing a build-time CT through IDataSourceAttribute.GetDataRowsAsync would be a public-API change and is out of scope for this bug fix.

Test plan

• Regression test: TUnit.TestProject/Bugs/5879/Repro.cs — exact repro from the issue, marked [EngineTest(ExpectedResult.Pass)]. Throws TargetParameterCountException before the fix; passes after.
• dotnet test TUnit.TestProject --treenode-filter "/*/*/Issue5879Tests/*" → 1/1 pass.
• dotnet test TUnit.UnitTests → 191/191 pass (no regressions in adjacent paths).

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 6 complexity

Metric	Results
Complexity	6

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

The fix correctly identifies and addresses the root cause: MethodInfo.Invoke does not auto-fill optional parameters the way a C# call site does, and the reflection fallback path for generic abstract base + [InheritsTests] wasn't accounting for this.

Regression: Truncating Extra Arguments

There is a subtle behavioural regression in BuildInvokeArgs when suppliedArguments.Length > parameters.Length:

// current code — enters the loop even when too many args are supplied
var args = new object?[parameters.Length];
for (var i = 0; i < parameters.Length; i++)
{
    if (i < suppliedArguments.Length)
        args[i] = suppliedArguments[i];   // fills params.Length slots from supplied
    // ...
}
return args;  // silently drops any extra supplied arguments

Before the fix: methodInfo.Invoke(instance, suppliedArguments) with extra args → TargetParameterCountException (informative, expected).
After the fix: extra args are silently truncated and Invoke succeeds, masking a misconfigured [MethodDataSource] attribute.

Adding a guard before entering the loop restores the original behaviour for this case:

if (suppliedArguments.Length > parameters.Length)
    return suppliedArguments;   // let Invoke surface the mismatch

Suggestion: Use Type.Missing Instead of Manual Default Filling

The current approach special-cases HasDefaultValue, then separately special-cases CancellationToken. A simpler, more general solution is to use Type.Missing — specifically designed for this scenario in reflection:

private static object?[] BuildInvokeArgs(MethodInfo methodInfo, object?[] suppliedArguments)
{
    var parameters = methodInfo.GetParameters();
    if (parameters.Length == suppliedArguments.Length)
        return suppliedArguments;
    if (suppliedArguments.Length > parameters.Length)
        return suppliedArguments;   // let Invoke surface the count mismatch

    var args = new object?[parameters.Length];
    suppliedArguments.CopyTo(args, 0);
    for (var i = suppliedArguments.Length; i < parameters.Length; i++)
        args[i] = Type.Missing;
    return args;
}

Why this is better:

• Type.Missing is the .NET-documented idiom for "use the parameter's default value" in reflection — it works for all optional param types without enumerating them.
• Eliminates the CancellationToken-specific branch, so the method stays correct if future data-source methods have other optional parameter types (e.g., bool flag = true, int count = 0).
• For required parameters (no default), Invoke throws MissingMemberException rather than the less-informative TargetParameterCountException, giving clearer diagnostic output.
• Supported on net8+ (all TUnit targets) — Type.Missing for non-COM optional params has worked since .NET Framework 4.0.

What's Good

• Root cause diagnosis in the PR description is accurate and clearly written.
• The HasDefaultValue → CancellationToken.None fallback chain is correct for the reported scenario.
• The regression test in TUnit.TestProject/Bugs/5879/Repro.cs is a clean minimal repro using the exact failing pattern — [InheritsTests], generic abstract base, [EnumeratorCancellation] CancellationToken ct = default.
• The comment in the method explains why the helper exists (reflection vs compiler behaviour), which is the right kind of comment.

Summary

The fix works for the reported bug. The two items worth addressing before merge are the silent truncation regression (should be a one-liner guard) and considering Type.Missing to avoid the growing list of type-specific special cases as this code evolves.

[thomhurst]

Thanks for the review.

Truncation regression — applied in af2d29d. Changed the early-return guard from == to <=, so any surplus supplied args now fall through to Invoke and surface the original TargetParameterCountException as before.

Type.Missing — pushing back on this one. The runtime layer (MethodBase.CheckArguments / MethodInvoker) treats Type.Missing as "use the parameter's default" only when ParameterInfo.HasDefaultValue is true — it doesn't gain anything beyond what the HasDefaultValue branch already does. The explicit CancellationToken branch is specifically the safety net for [Optional]-without-[DefaultParameterValue], where HasDefaultValue is false; Type.Missing would throw there too. So the manual approach is actually slightly more general than the suggested rewrite, not less. I'd rather keep the targeted CT fallback (which matches what the source-generator emits — default(CancellationToken)) and revisit if other "no-default-attribute" optional types come up.

[claude]

Code Review — Follow-up

Previous Review Items

✅ Behavioral regression (surplus args) — addressed in af2d29d3

The updated guard if (parameters.Length <= suppliedArguments.Length) correctly handles both exact-match and surplus-arg cases by returning suppliedArguments unchanged, letting Invoke surface the TargetParameterCountException. Good fix.

⚠️ Type.Missing suggestion — partially applicable, but current code is defensible

The previous review suggested replacing the manual default-filling with Type.Missing. On reflection (pun intended), there is a scenario where Type.Missing would not be a full replacement:

• Type.Missing only works for optional parameters that have a = defaultValue in the signature (HasDefaultValue == true).
• For a CancellationToken parameter without a default value (e.g., [EnumeratorCancellation] CancellationToken ct), Type.Missing would cause Invoke to throw rather than using CancellationToken.None.

The current HasDefaultValue → CancellationToken.None chain does handle that edge case. However, the CancellationToken branch is only reachable when HasDefaultValue == false, which is not the canonical [EnumeratorCancellation] CancellationToken ct = default pattern from the issue. If a data source method has a required CancellationToken parameter, that feels like a misuse of the API. Consider whether silently injecting CancellationToken.None is the right response there, versus surfacing an actionable error.

A middle ground worth considering:

private static object?[] BuildInvokeArgs(MethodInfo methodInfo, object?[] suppliedArguments)
{
    var parameters = methodInfo.GetParameters();
    if (parameters.Length <= suppliedArguments.Length)
        return suppliedArguments;

    var args = new object?[parameters.Length];
    Array.Copy(suppliedArguments, args, suppliedArguments.Length);
    for (var i = suppliedArguments.Length; i < parameters.Length; i++)
    {
        var p = parameters[i];
        if (p.HasDefaultValue)
            args[i] = Type.Missing;          // .NET fills the declared default
        else if (p.ParameterType == typeof(CancellationToken))
            args[i] = CancellationToken.None; // pragmatic fallback for enumerator tokens
        else
            return suppliedArguments;         // required param missing — let Invoke throw
    }
    return args;
}

This uses Type.Missing for the common case (cleaner, works for all optional types without enumeration) while keeping the CancellationToken fallback as an explicit safety net. The Array.Copy + loop-from-offset avoids re-copying already-supplied args.

Overall

The fix is correct for the reported bug. The regression concern from the previous review was properly addressed. The code is in a mergeable state — the remaining suggestion is a simplification, not a correctness issue.

Approve pending the Type.Missing / loop refactor consideration (optional, not blocking).

[thomhurst]

Applied the Type.Missing + Array.Copy simplification in 5833192. Regression test still green. Thanks!

[claude]

Code Review — Final Sign-off

Previous Review Items — All Addressed

✅ Behavioral regression (surplus args) — Fixed in af2d29d. The guard if (parameters.Length <= suppliedArguments.Length) correctly passes both exact-match and surplus-arg arrays through to Invoke unchanged, preserving the original TargetParameterCountException behaviour.

✅ Type.Missing + Array.Copy refactor — Applied in 5833192. The final implementation is exactly the middle ground suggested in the previous review:

if (p.HasDefaultValue)
    args[i] = Type.Missing;          // .NET fills the declared default (works for all optional types)
else if (p.ParameterType == typeof(CancellationToken))
    args[i] = CancellationToken.None; // pragmatic fallback for enumerator tokens without explicit default
else
    return suppliedArguments;         // required param missing — let Invoke throw

This is the correct design: Type.Missing handles the common case generically (no per-type enumeration), while the explicit CancellationToken branch is a narrow safety net for the [EnumeratorCancellation]-without-= default edge case. The Array.Copy + loop-from-offset avoids re-iterating already-supplied args.

Final State Assessment

The fix is correct, well-scoped, and the implementation is clean:

• Root cause accurately identified and fixed in the reflection invoke path
• No regression introduced for surplus-arg or exact-arg cases
• Generic handling via Type.Missing with a targeted CancellationToken safety net
• Regression test covers the exact failing pattern ([InheritsTests] + generic abstract base + [EnumeratorCancellation] optional CT)
• PR description provides clear rationale for why the source-gen path wasn't affected

Approved. Ready to merge.