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.