feat: support string-to-parseable type conversions in [Arguments]

[thomhurst]

Summary

• Allow string arguments in [Arguments] attributes to be automatically parsed into types implementing IParsable<TSelf> (e.g. DateTime, TimeSpan, Guid, DateTimeOffset, DateOnly, TimeOnly)
• Previously [Arguments("2022-5-31")] with a DateTime parameter would emit TUnit0001 at compile time, even though the runtime could handle it
• Changes span all three layers: analyzer (suppresses false TUnit0001), source generator (emits Type.Parse(...) calls), and runtime CastHelper (handles conversion via IParsable<T> reflection with caching)

Changes

• TUnit.Analyzers/TestDataAnalyzer.cs — CanConvert now recognizes IParsable<TSelf> types (with well-known type fallback for older TFMs)
• TUnit.Core.SourceGenerator/.../TypedConstantFormatter.cs — Generates Type.Parse("value", CultureInfo.InvariantCulture) for parseable target types
• TUnit.Core/Helpers/CastHelper.cs — Runtime IParsable<T> support with cached Parse method lookup (+ netstandard2.0 fallback)
• TUnit.TestProject/StringToParsableArgumentsTests.cs — Tests for DateTime, TimeSpan, Guid, DateTimeOffset, DateOnly, TimeOnly

Closes #5186

Test plan

• All 7 new StringToParsableArgumentsTests pass on net9.0
• All 663 existing analyzer tests pass
• All 115 existing source generator tests pass
• No TUnit0001 false positive for string-to-parseable conversions

[claude]

Code Review: feat: support string-to-parseable type conversions in [Arguments]

This is a well-scoped feature that addresses a real pain point (#5186). The three-layer approach (analyzer suppression → source gen → runtime fallback) is the right architecture. A few issues worth addressing before merge:

---

Bug: Unescaped string literal in source generator

**** — both the DateTime special case and the general IsParsableFromString branch interpolate the raw string value directly into a C# string literal:

return $"{fullyQualifiedName}.Parse(\"{strForParsing}\", global::System.Globalization.CultureInfo.InvariantCulture)";

If strForParsing contains \\ (backslash) or " (quote), this produces invalid C#. While date/time strings don't typically have these, it's a latent bug for any future parseable type that might accept such strings. Use SymbolDisplay.FormatLiteral or SyntaxFactory.Literal to properly escape:

var escapedValue = SymbolDisplay.FormatLiteral(strForParsing, quote: true); // produces "\"value\""
return $"{fullyQualifiedName}.Parse({escapedValue}, global::System.Globalization.CultureInfo.InvariantCulture)";

---

AOT concern: reflection placed inside TryAotSafeConversion

The new TryParseFromString call is inserted inside TryAotSafeConversion, but the underlying TryParsableConvert uses reflection (GetInterfaces(), GetMethod(), Invoke()). This is misleading and violates the method contract. The [UnconditionalSuppressMessage("Trimming", "IL2070")] silences a legitimate warning — in Native AOT builds, the Parse method could be trimmed and fail silently at runtime.

Two improvements:

1. Move the string-to-parseable check to after TryAotSafeConversion (or into the reflection layer), since it uses reflection. The placement matters for the correctness of error messages (the AOT diagnostic path would be reached first before attempting parse).

2. Replace [UnconditionalSuppressMessage] with a proper [DynamicallyAccessedMembers] annotation on the targetType parameter to make the trimmer aware of what metadata must be preserved:

private static bool TryParseFromString(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicStaticMethods)] Type targetType,
    string value,
    out object? result)

This propagates the annotation correctly and satisfies the trimmer without suppressing the diagnostic. The caller in Cast(Type, object?) already has [DynamicallyAccessedMembers(PublicParameterlessConstructor)] on its type param — this should be widened to include PublicStaticMethods as well.

---

Redundant DateTime special case in TypedConstantFormatter

The case SpecialType.System_DateTime: block added inside the switch is fully redundant — on .NET 7+ DateTime implements IParsable<DateTime>, so IsParsableFromString below catches it already. The special case adds noise and a second escaping-bug opportunity. Remove it and let the unified path handle it.

---

Duplicated IsParsableFromString logic

The same method appears in both TestDataAnalyzer.cs and TypedConstantFormatter.cs. Since these are separate projects, sharing is harder, but it's worth examining whether TypedConstantFormatter can call into the analyzer (or a common extension method in TUnit.Core.SourceGenerator.Extensions) to reduce drift. If a new parseable type is added in the future (e.g., Half, NFloat, BigInteger), both copies must be updated simultaneously — easy to miss.

---

Silent parse failures hide user errors

In TryParseFromString (both the netstandard path and TryParsableConvert), parse failures are swallowed silently:

catch
{
    // Parse failed
}

If a user writes [Arguments("not-a-date")] targeting DateTime, the parse silently fails, the method returns false, and execution falls through to an opaque type-mismatch error rather than a helpful "could not parse 'not-a-date' as DateTime" message. Consider catching FormatException / OverflowException specifically and rethrowing with context, or at minimum logging it before falling through.

---

Test assertions verify non-default but not actual value

await Assert.That(testDate).IsNotEqualTo(default(DateTime));

This proves parsing succeeded but not that the right value was parsed. A test like "2022-5-31" should produce new DateTime(2022, 5, 31) — asserting the actual expected value catches both silent misparsing and TZ-related regressions.

---

Overall

The feature is architecturally sound and the three-layer approach is correct. The string-escaping bug is the most important to fix before merge (it can generate uncompilable code). The AOT annotation issue is important given TUnit's AOT-first stance. The rest are lower priority but worth addressing.

[claude]

Code Review: feat: support string-to-parseable type conversions in [Arguments]

This is a well-scoped feature that addresses a real pain point (#5186). The three-layer approach (analyzer suppression → source gen → runtime fallback) is the right architecture. A few issues worth addressing before merge:

---

Bug: Unescaped string literal in source generator

TypedConstantFormatter.cs — both the DateTime special case and the general IsParsableFromString branch interpolate the raw string value directly into a C# string literal:

return $"{fullyQualifiedName}.Parse(\"{strForParsing}\", global::System.Globalization.CultureInfo.InvariantCulture)";

If strForParsing contains \ (backslash) or " (quote), this produces invalid C#. While date/time strings don't typically have these, it's a latent bug for any future parseable type that might accept such strings. Use SymbolDisplay.FormatLiteral or SyntaxFactory.Literal to properly escape:

var escapedValue = SymbolDisplay.FormatLiteral(strForParsing, quote: true); // produces "\"value\""
return $"{fullyQualifiedName}.Parse({escapedValue}, global::System.Globalization.CultureInfo.InvariantCulture)";

---

AOT concern: reflection placed inside TryAotSafeConversion

The new TryParseFromString call is inserted inside TryAotSafeConversion, but the underlying TryParsableConvert uses reflection (GetInterfaces(), GetMethod(), Invoke()). This is misleading and violates the method contract. The [UnconditionalSuppressMessage("Trimming", "IL2070")] silences a legitimate warning — in Native AOT builds, the Parse method could be trimmed and fail silently at runtime.

Two improvements:

1. Move the string-to-parseable check after TryAotSafeConversion (or into the reflection layer), since it uses reflection. The placement also matters for correctness of the AOT diagnostic path.

2. Replace [UnconditionalSuppressMessage] with a proper [DynamicallyAccessedMembers] annotation to make the trimmer aware of what metadata must be preserved:

private static bool TryParseFromString(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicStaticMethods)] Type targetType,
    string value,
    out object? result)

The caller Cast(Type, object?) already has [DynamicallyAccessedMembers(PublicParameterlessConstructor)] on its type param — widening it to include PublicStaticMethods would satisfy the trimmer without suppressing the diagnostic.

---

Redundant DateTime special case in TypedConstantFormatter

The case SpecialType.System_DateTime: block added inside the switch is fully redundant — on .NET 7+ DateTime implements IParsable<DateTime>, so IsParsableFromString below catches it already. The special case adds noise and a second escaping-bug opportunity. Remove it and let the unified path handle it.

---

Duplicated IsParsableFromString logic

The same method appears in both TestDataAnalyzer.cs and TypedConstantFormatter.cs. Since these are separate projects, sharing is harder, but it's worth examining whether a common extension method in TUnit.Core.SourceGenerator.Extensions (or a shared source file linked to both) can reduce drift. If a new parseable type is added in the future (e.g., Half, NFloat, BigInteger), both copies must be updated simultaneously — easy to miss.

---

Silent parse failures hide user errors

In TryParseFromString (both the netstandard path and TryParsableConvert), parse failures are swallowed silently:

catch
{
    // Parse failed
}

If a user writes [Arguments("not-a-date")] targeting DateTime, the parse silently fails, falls through to an opaque type-mismatch error rather than a helpful "could not parse 'not-a-date' as DateTime" message. Consider catching FormatException/OverflowException specifically and rethrowing with context.

---

Test assertions verify non-default but not actual value

await Assert.That(testDate).IsNotEqualTo(default(DateTime));

This proves parsing succeeded but not that the right value was parsed. Asserting the actual expected value (e.g., new DateTime(2022, 5, 31)) would catch silent misparsing and TZ-related regressions.

---

Overall

The feature is architecturally sound and the three-layer approach is correct. The string-escaping bug is the most important to fix before merge (it can generate uncompilable code for any parseable type whose format string happens to include backslashes). The AOT annotation issue is important given TUnit's AOT-first stance. The rest are lower priority.

[claude]

Follow-up Review

The refactor commit (9463f38) addressed several of the issues from my previous review — good progress:

• Redundant special case in removed ✅
• Netstandard2.0 path converted from Parse-and-catch to TryParse ✅
• Catch clause narrowed in the .NET path to expected exception types ✅

Three issues from the previous review are still outstanding:

---

🔴 Unescaped string literal (can generate invalid C#) — still present

TypedConstantFormatter.cs still interpolates the raw user-supplied string directly:

return $"{fullyQualifiedName}.Parse(\"{strForParsing}\", ...)";

If strForParsing ever contains a backslash or double-quote (e.g. a custom IParsable<T> whose canonical string form uses them), the emitted C# will be syntactically invalid and fail to compile with a mysterious error. Fix with SymbolDisplay.FormatLiteral:

var escapedValue = SymbolDisplay.FormatLiteral(strForParsing, quote: true);
return $"{fullyQualifiedName}.Parse({escapedValue}, global::System.Globalization.CultureInfo.InvariantCulture)";

This is the highest-priority fix because it's a correctness issue, not just a style concern.

---

🟡 AOT: reflection inside TryAotSafeConversion + suppressed trimmer warning

TryParsableConvert uses GetInterfaces(), GetMethod(), and Invoke() — all reflection — but is called from inside TryAotSafeConversion, which by name implies AOT-safe behaviour. The [UnconditionalSuppressMessage("Trimming", "IL2070")] suppresses a legitimate trimmer diagnostic rather than satisfying it.

The better fix is to annotate the parameter so the trimmer knows what to preserve:

private static bool TryParseFromString(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicStaticMethods)] Type targetType,
    string value,
    out object? result)

And widen the corresponding annotation on the targetType parameter in Cast(Type, object?) (currently only PublicParameterlessConstructor). This makes the trimmer-required metadata explicit and avoids the blanket suppress. Moving the call out of TryAotSafeConversion into its own pre-reflection step would also clarify intent.

---

🟡 Duplicated IsParsableFromString across two projects

The same method body exists in both TestDataAnalyzer.cs and TypedConstantFormatter.cs. When a new parseable type (e.g. Half, NFloat, BigInteger) needs to be added to the fallback list, both copies must be updated. A shared source file linked into both projects, or an extension method in TUnit.Core.SourceGenerator.Extensions visible to both, would eliminate the drift risk.

---

🟢 Test assertions (lower priority)

Tests still assert IsNotEqualTo(default) rather than the exact expected value. For example:

// Current
await Assert.That(testDate).IsNotEqualTo(default(DateTime));

// Better
await Assert.That(testDate).IsEqualTo(new DateTime(2022, 5, 31));

Asserting the actual value would catch silent misparsing (e.g. locale-dependent parsing swapping month/day) and timezone-related regressions.

---

The unescaped string literal is the one change I'd call blocking — the rest can be addressed as follow-ups if preferred.

[claude]

Follow-up Review

The refactor commit (9463f38) addressed several of the issues from my previous review — good progress:

• Redundant DateTime special case in TypedConstantFormatter removed ✅
• Netstandard2.0 path converted from Parse-and-catch to TryParse ✅
• Catch clause narrowed in the .NET path to expected exception types ✅

Three issues from the previous review are still outstanding:

---

🔴 Unescaped string literal (can generate invalid C#) — still present

TypedConstantFormatter.cs still interpolates the raw user-supplied string directly:

return $"{fullyQualifiedName}.Parse(\"{strForParsing}\", ...)";

If strForParsing ever contains a backslash or double-quote (e.g. a custom IParsable<T> whose canonical string form uses them), the emitted C# will be syntactically invalid. Fix with SymbolDisplay.FormatLiteral:

var escapedValue = SymbolDisplay.FormatLiteral(strForParsing, quote: true);
return $"{fullyQualifiedName}.Parse({escapedValue}, global::System.Globalization.CultureInfo.InvariantCulture)";

This is the highest-priority fix — it's a correctness issue, not a style concern.

---

🟡 AOT: reflection inside TryAotSafeConversion + suppressed trimmer warning

TryParsableConvert uses GetInterfaces(), GetMethod(), and Invoke() — all reflection — but is called from inside TryAotSafeConversion, which by name implies AOT-safe behaviour. The [UnconditionalSuppressMessage("Trimming", "IL2070")] suppresses a legitimate trimmer diagnostic rather than satisfying it.

The better fix is to annotate the parameter so the trimmer knows what to preserve:

private static bool TryParseFromString(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicStaticMethods)] Type targetType,
    string value,
    out object? result)

And widen the corresponding annotation on the targetType parameter in Cast(Type, object?) (currently only PublicParameterlessConstructor). This makes the trimmer-required metadata explicit and avoids the blanket suppress. Moving the call out of TryAotSafeConversion into its own step would also clarify intent.

---

🟡 Duplicated IsParsableFromString across two projects

The same method body exists in both TestDataAnalyzer.cs and TypedConstantFormatter.cs. When a new parseable type needs to be added to the fallback list, both copies must be updated simultaneously. A shared extension method or linked source file would eliminate the drift risk.

---

🟢 Test assertions (lower priority)

Tests still assert IsNotEqualTo(default) rather than the exact expected value. Asserting the actual value (e.g. new DateTime(2022, 5, 31)) would catch silent misparsing and timezone-related regressions.

---

The unescaped string literal is the one change I'd call blocking — the rest can be addressed as follow-ups if preferred.

[claude]

Follow-up Review (post-fix commit e457d87)

All three blocking/major issues from the previous review have been addressed — excellent iteration:

• String escaping via SymbolDisplay.FormatLiteral ✅
• TryParseFromString moved out of TryAotSafeConversion (now its own "Layer 1.5") ✅
• IsParsableFromString deduplicated into shared ParsableTypeExtensions.cs ✅
• Test assertions now verify exact expected values ✅

---

🟡 [UnconditionalSuppressMessage] still used instead of [DynamicallyAccessedMembers]

TryParsableConvert still suppresses the trimmer diagnostic rather than satisfying it:

[UnconditionalSuppressMessage("Trimming", "IL2070:...")]
private static bool TryParsableConvert(Type targetType, string value, out object? result)

The more correct fix (mentioned in the previous review) is to annotate the parameter so the trimmer knows what metadata to preserve:

private static bool TryParsableConvert(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicStaticMethods | DynamicallyAccessedMemberTypes.Interfaces)] Type targetType,
    string value,
    out object? result)

And widen the annotation on targetType in Cast(Type, object?) accordingly. A [DynamicallyAccessedMembers] annotation actively informs the linker what to keep; a suppress just hides the warning. Given TUnit's AOT-first stance this matters — if Parse is trimmed at AOT compile time, the runtime fallback silently returns false with no diagnostic.

---

🟡 Potential duplicate Compile include for ParsableTypeExtensions.cs

ParsableTypeExtensions.cs is included twice for TUnit.Analyzers:

1. In Roslyn.props inside <ItemGroup Condition="$(_BaseProjectName.EndsWith('.Analyzers'))">:

   <Compile Include="..\TUnit.Core.SourceGenerator\Extensions\ParsableTypeExtensions.cs" />

2. Directly in TUnit.Analyzers.csproj:

   <Compile Include="..\TUnit.Core.SourceGenerator\Extensions\ParsableTypeExtensions.cs" Link="Extensions\ParsableTypeExtensions.cs" />

If both are active for the same project this would cause a "duplicate compile item" build warning (or error with EnableDefaultItems). Worth verifying this builds cleanly — if Roslyn.props is only used by other analyzer projects (not TUnit.Analyzers itself), then this is fine, but the condition EndsWith('.Analyzers') would match TUnit.Analyzers.

---

🟢 Namespace oddity (cosmetic)

ParsableTypeExtensions.cs lives in TUnit.Core.SourceGenerator/Extensions/ but declares namespace TUnit.Analyzers.Extensions;. This works because it's a linked file, but it's slightly surprising — a reader browsing the source generator project finds a file with an Analyzers namespace. No action needed, just worth noting.

---

🟢 Two extra blank lines in TypedConstantFormatter.cs

+
+
     private string FormatArrayForCode(...)

Trivial — remove one blank line for consistency with the surrounding style.

---

Overall

This is in good shape. All previously blocking issues have been resolved. The [DynamicallyAccessedMembers] annotation is the one remaining item I'd recommend addressing before merge given TUnit's Native AOT support commitment — the suppress masks a real potential trimming failure. The duplicate-include concern is worth a quick build verification. Everything else is minor polish.

[claude]

Follow-up Review (post-fix commit f83bdc6)

This is the fourth review pass. All previously blocking issues have been resolved.

✅ All previous issues addressed

• String escaping via SymbolDisplay.FormatLiteral ✅
• TryParseFromString moved out of TryAotSafeConversion ✅
• IsParsableFromString deduplicated into shared ParsableTypeExtensions.cs ✅
• Test assertions verify exact expected values ✅
• [DynamicallyAccessedMembers] replacing [UnconditionalSuppressMessage] ✅ — TryParseFromString, TryParsableConvert, and FindParseMethod are all properly annotated, and Cast<T>/Cast(Type, object?) have been widened accordingly.

---

🟡 ParseMethodCache uses manual TryGetValue + TryAdd instead of GetOrAdd

TryParsableConvert does:

if (!ParseMethodCache.TryGetValue(targetType, out var parseMethod))
{
    parseMethod = FindParseMethod(targetType);
    ParseMethodCache.TryAdd(targetType, parseMethod);
}

Under concurrent load, two threads can both miss the cache and call FindParseMethod for the same type before either calls TryAdd. The result is correct (both threads compute the same MethodInfo?) but the work is duplicated unnecessarily. The existing ConversionMethodCache in the same class already uses the preferred pattern:

return ConversionMethodCache.GetOrAdd(
    (sourceType, targetType),
    static key => FindConversionMethod(key.Item1, key.Item2));

Prefer the same GetOrAdd approach here for consistency and to eliminate the redundant computation window:

var parseMethod = ParseMethodCache.GetOrAdd(targetType, FindParseMethod);

---

🟡 Duplicate Compile include for ParsableTypeExtensions.cs — confirmed non-issue

The previous review flagged a potential double-include. After examining Roslyn.props and TUnit.Analyzers.csproj:

• Roslyn.props is imported by test projects that set _BaseProjectName, not by TUnit.Analyzers.csproj itself (which only imports Polyfill.targets).
• TUnit.Analyzers.csproj includes the file directly with a Link attribute; the glob in Roslyn.props wouldn't re-pick it up since the file doesn't physically live in the TUnit.Analyzers folder.

No action required — the concern was unfounded.

---

🟢 Namespace cosmetic note (no action needed)

ParsableTypeExtensions.cs lives under TUnit.Core.SourceGenerator/Extensions/ but declares namespace TUnit.Analyzers.Extensions;. This is intentional (it's a linked file consumed by the analyzer namespace), but a reader browsing the source generator project may find it briefly surprising. A comment would clarify but isn't required.

---

Overall

The implementation is clean, well-layered, and correctly handles AOT/trimming constraints. The GetOrAdd consolidation is the one remaining suggestion worth acting on before merge; everything else is green. This is a nice quality-of-life improvement.

[claude]

Follow-up Review (post-fix commit 8b4d97a)

This is the fifth review pass. The previous review raised one remaining yellow issue — the ParseMethodCache.TryGetValue + TryAdd pattern vs GetOrAdd. On closer inspection, this pattern is intentionally correct for AOT, so no further changes are needed.

---

✅ All previous issues fully resolved

• String escaping via SymbolDisplay.FormatLiteral ✅
• TryParseFromString moved out of TryAotSafeConversion ("Layer 1.5") ✅
• IsParsableFromString deduplicated into shared ParsableTypeExtensions.cs ✅
• [DynamicallyAccessedMembers] replacing [UnconditionalSuppressMessage] on TryParseFromString, TryParsableConvert, FindParseMethod, and both Cast overloads ✅
• Test assertions verify exact expected values ✅
• Public API snapshots updated for all three TFMs ✅

---

ℹ️ TryGetValue + TryAdd pattern is correct here (not a regression from the GetOrAdd suggestion)

The previous review suggested ParseMethodCache.GetOrAdd(targetType, FindParseMethod) for consistency. However, the current manual pattern is actually the safer choice for AOT:

When FindParseMethod is passed as a method-group delegate to GetOrAdd, the [DynamicallyAccessedMembers] annotation on FindParseMethod's type parameter cannot flow through the Func<Type, MethodInfo?> delegate wrapper. The trimmer would see an un-annotated Type arriving at GetInterfaces() and GetMethod(), producing a valid IL2070 warning that would require suppression.

By calling FindParseMethod(targetType) directly, the annotation on the local targetType parameter (which inherits from the caller's [DynamicallyAccessedMembers(...Interfaces | ...PublicMethods)] annotation) flows correctly to the callee. The manual pattern avoids the problem rather than papering over it. The slight race condition (two threads both computing FindParseMethod before either TryAdd) is inconsequential since both threads compute identical results.

---

Summary

The implementation is clean, the layered architecture (analyzer → source generator → runtime fallback) is correct, AOT annotations are properly propagated, and the feature is well-tested. Ready to merge.