Use CallerArgumentExpression for TestDataRow by default.

[m-gasser]

Description

I just wrote a Test with a lambda as the Data argument of the TestDataRow<>. If DisplayName is not provided, .ToString() seems to get called. For delegates this is the type name, which is not useful.
For my test I created a Create factory method, so that [CallerArgumentExpression] is used and I do not have to provide the DisplayName.
This PR adds the Attribute directly to TestDataRow<>. I am not sure, if that causes a worse DisplayName in some cases.

Fixes #

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Performance improvement
• Refactoring (no functional changes)

Checklist

Required

• I have read the Contributing Guidelines
• If this is a new feature, I started a discussion first and received agreement
• My code follows the project's code style (modern C# syntax, proper naming conventions)
• I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

• Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH:
  • Source Generator path (TUnit.Core.SourceGenerator)
  • Reflection path (TUnit.Engine)
• Snapshot Tests: If I changed source generator output or public APIs:
  • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
  • I reviewed the .received.txt files and accepted them as .verified.txt
  • I committed the updated .verified.txt files
• Performance: If this change affects hot paths (test discovery, execution, assertions):
  • I minimized allocations and avoided LINQ in hot paths
  • I cached reflection results where appropriate
• AOT Compatibility: If this change uses reflection:
  • I added appropriate [DynamicallyAccessedMembers] annotations
  • I verified the change works with dotnet publish -p:PublishAot=true

Testing

• All existing tests pass (dotnet test)
• I have added tests that cover my changes
• I have tested both source-generated and reflection modes (if applicable)

Additional Notes

[claude]

Code Review: Use CallerArgumentExpression for TestDataRow by default

Thanks for this contribution! The motivation is solid — lambda and delegate arguments produce unhelpful display names when using .ToString(), and [CallerArgumentExpression] can provide meaningful context automatically. However, there are a few significant issues with the current approach.

---

Critical Issue: Positional Argument Ambiguity (Breaking Change)

Removing = null from DisplayName in the primary constructor + adding a new overload with string? Skip as its second parameter creates CS0121 ambiguous overload errors for existing code using positional arguments:

// BEFORE (compiled fine): DisplayName = "my test"
yield return new TestDataRow<string>("value", "my test");

// AFTER: COMPILE ERROR — ambiguous between:
//   primary ctor: (T Data, string? DisplayName, ...)
//   new overload:  (T Data, string? Skip, ...)
yield return new TestDataRow<string>("value", "my test");

This is a stronger breaking change than the PR checklist implies — it's not just a behavior change, it's a compile-time regression for any user who passed the second argument positionally.

---

Simpler Alternative: Apply [CallerArgumentExpression] Directly to DisplayName

The PR comment says this approach might cause analyzer warnings, but that's just a Roslyn suggestion (not an error), and it's entirely suppressible. The simpler design would be:

public record TestDataRow<T>(
    T Data,
    [CallerArgumentExpression(nameof(Data))]
    string? DisplayName = null,
    string? Skip = null,
    string[]? Categories = null
) : ITestDataRow
{
    object? ITestDataRow.GetData() => Data;
}

Benefits:

• No breaking change — single constructor, existing call sites still work
• No ambiguity — no overload resolution issues
• Explicit values still override the default — DisplayName: "Admin login" continues to work
• No confusingly-named public parameter (dataCallerArgumentExpression leaks into IntelliSense)

The only downside is that IDEs may show a hint like "this parameter is auto-populated by the compiler," but that's a minor UX concern compared to the correctness issues with two constructors.

---

Public API Surface: Leaking Implementation Details

The new constructor is public and exposes dataCallerArgumentExpression as a named parameter. Users can accidentally call:

new TestDataRow<T>(data, dataCallerArgumentExpression: "some string")

which is confusing. If the two-constructor approach is kept, the parameter should be named something like displayName (or the constructor should be internal/private, though that'd conflict with the record semantics).

---

Missing Tests

The checklist reflects that no tests were written. A test to verify that:

1. The inferred display name matches the source expression
2. An explicit DisplayName: overrides the inferred value

...would be important for this feature, especially given the subtlety of [CallerArgumentExpression] at call sites vs. at invocation sites.

---

Snapshot Tests

This PR modifies the public API surface of TestDataRow<T>, which may require snapshot test updates (per project requirements in CLAUDE.md). Please run the snapshot tests and commit any .verified.txt changes if the output differs.

---

Suggested Approach

The single-constructor approach with [CallerArgumentExpression] on DisplayName is strongly preferred here. It delivers the same ergonomic improvement with zero breaking changes and simpler code.

[claude]

Code Review: Use CallerArgumentExpression for TestDataRow by default

Thanks for this contribution! The motivation is solid — lambda and delegate arguments produce unhelpful display names when using .ToString(), and [CallerArgumentExpression] can provide meaningful context automatically. However, there are a few significant issues with the current approach.

---

Critical Issue: Positional Argument Ambiguity (Breaking Change)

Removing = null from DisplayName in the primary constructor + adding a new overload with string? Skip as its second parameter creates CS0121 ambiguous overload errors for existing code using positional arguments:

// BEFORE (compiled fine): "my test" mapped to DisplayName
yield return new TestDataRow<string>("value", "my test");

// AFTER: COMPILE ERROR — ambiguous between:
//   primary ctor: (T Data, string? DisplayName, ...)
//   new overload:  (T Data, string? Skip, ...)
yield return new TestDataRow<string>("value", "my test");

This is a stronger breaking change than the checklist implies — it is not just a behavior shift but a compile-time regression for any user passing the second argument positionally.

---

Simpler Alternative: Apply [CallerArgumentExpression] Directly to DisplayName

The PR comment says this might trigger analyzer warnings, but those are just Roslyn suggestions (not errors) and are suppressible. A simpler, non-breaking design:

public record TestDataRow<T>(
    T Data,
    [CallerArgumentExpression(nameof(Data))]
    string? DisplayName = null,
    string? Skip = null,
    string[]? Categories = null
) : ITestDataRow
{
    object? ITestDataRow.GetData() => Data;
}

Benefits of this approach:

• No breaking change — single constructor, all existing call sites still compile
• No ambiguity — no overload resolution confusion
• Explicit values still work — DisplayName: "Admin login" continues to override the inferred value
• No confusing public parameter name — dataCallerArgumentExpression leaks into IntelliSense and is callable by users, which is misleading

---

Public API Concern

With the current PR, users can call the new overload with the awkward parameter name explicitly:

new TestDataRow<T>(data, dataCallerArgumentExpression: "some string")

This is confusing and pollutes the public API. If the two-constructor approach is retained, the dataCallerArgumentExpression parameter should be renamed to something user-facing (e.g. displayName) or the constructor should be hidden.

---

Missing Tests and Snapshots

• No tests were written to verify the inferred display name matches the source expression, or that an explicit DisplayName: still overrides it.
• This PR modifies the public API of TestDataRow<T>, which may require snapshot test updates per the project's requirements. Please run snapshot tests and commit any .verified.txt changes.

---

Recommendation

Use the single-constructor approach with [CallerArgumentExpression] applied directly to the DisplayName parameter. It delivers the same ergonomic improvement with zero breaking changes and significantly simpler code.

[m-gasser]

I am pretty sure that claude is hallucinating here. However I definitely should add some tests. I will do that, after some human feedback, whether the change is reasonable or not.

[thomhurst]

The recommendation Claude has given of just placing caller attribute on the display parameter sounds like it might be a good idea? Can't think of any downsides to it

[m-gasser]

just placing caller attribute on the display parameter sounds like it might be a good idea? Can't think of any downsides to it

We quite often pass [CallerFilePath]through method chains and Rider complains if those are set explicitly. Therefor I assumed, that the same is true for [CallerArgumentExpression]but this does not seem to be the case. So yes, my concerns about some analyzers flagging this are unfounded. I will simplify it.

On a side note, there are those handy rules for CallerArgumentExpression:
https://www.jetbrains.com/help/resharper/RedundantCallerArgumentExpressionDefaultValue.html
https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0108.md
Both should work as a charm, if [CallerArgumentExpression] is directly on DisplayName.

[claude]

Code Review — Follow-up (PR #5135)

The previous review raised concerns about a two-constructor approach with overload ambiguity and a leaked dataCallerArgumentExpression parameter in the public API. Both of those issues have been resolved — the current diff uses the recommended single-constructor approach with [CallerArgumentExpression] applied directly to DisplayName. That is the right design. Well done on simplifying it.

---

Current State: Clean and Correct

public record TestDataRow<T>(
    T Data,
    [CallerArgumentExpression(nameof(Data))] string? DisplayName = null,
    string? Skip = null,
    string[]? Categories = null
) : ITestDataRow

• No breaking change — existing positional and named-argument call sites still compile
• Explicit DisplayName: "..." continues to override the inferred value
• Public API snapshot format does not include attributes, so no .verified.txt updates are needed here

---

Outstanding: Missing Tests

This is the main remaining gap. Two test cases would meaningfully cover this feature:

1. Inferred display name from expression:

// Assert that DisplayName is auto-populated with the source expression text
var row = new TestDataRow<(string, string)>(("admin", "secret123"));
Assert.That(row.DisplayName, Is.EqualTo("(\"admin\", \"secret123\")"));

2. Explicit DisplayName still takes priority:

var row = new TestDataRow<(string, string)>(("admin", "secret123"), DisplayName: "Admin login");
Assert.That(row.DisplayName, Is.EqualTo("Admin login"));

---

Minor Behaviour Note (not a blocker)

If a caller explicitly passes null for DisplayName, it overrides the compiler-supplied expression:

var row = new TestDataRow<string>("hello", null);
// row.DisplayName is null, not "\"hello\""

This is standard [CallerArgumentExpression] semantics and unlikely to be surprising in practice, but worth noting in the XML doc comment or a code comment if you want to be explicit about it.

---

Summary

The core change is solid and takes the simplest path to the desired behaviour. The only ask before merging is adding the two tests above to verify the feature works and guards against regressions.