Fix nullable IParsable type recognition in source generator and analyzer#5354
Fix nullable IParsable type recognition in source generator and analyzer#5354
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
- Unwrap nullable value types (e.g., DateTime? -> DateTime) in IsParsableFromString - Use underlying type for Parse() call in TypedConstantFormatter for nullable targets - Add test cases for nullable DateTime?, TimeSpan?, Guid? with string arguments - Add source generator snapshot test for nullable parsable arguments Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/18bddde7-3da0-4a0c-8fd4-6221aaea00bc Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/18bddde7-3da0-4a0c-8fd4-6221aaea00bc Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This PR correctly identifies and fixes a real bug: nullable parsable value types (e.g., DateTime?) were incorrectly flagged by the analyzer and not properly handled in source generation when a string argument is provided. The fix is logically sound and the tests confirm the correct behavior. A few observations follow.
Code Duplication: Existing Extension Methods Not Reused
ParsableTypeExtensions.cs (new code):
if (type is INamedTypeSymbol { IsGenericType: true, ConstructedFrom.SpecialType: SpecialType.System_Nullable_T } nullableType)
{
type = nullableType.TypeArguments[0];
}TypedConstantFormatter.cs (new code, FormatPrimitiveForCode):
if (targetType is INamedTypeSymbol { IsGenericType: true, ConstructedFrom.SpecialType: SpecialType.System_Nullable_T } nullableType)
{
parseType = nullableType.TypeArguments[0];
}Both of these inline the nullable unwrapping pattern manually. However, TypeSymbolExtensions (TUnit.Core.SourceGenerator.Extensions) already provides IsNullableValueType() and GetNullableUnderlyingType() for exactly this purpose, and TypedConstantFormatter.cs itself already uses these methods in the same file (lines 18–20, 248–253 for the enum handling). The new FormatPrimitiveForCode block should use these same extensions for consistency:
// Suggested: use existing extension methods
if (targetType.IsNullableValueType())
{
parseType = targetType.GetNullableUnderlyingType()!;
}The namespace difference (TUnit.Analyzers.Extensions vs TUnit.Core.SourceGenerator.Extensions) means ParsableTypeExtensions may not have access to TypeSymbolExtensions without a using directive, but TypedConstantFormatter.cs definitely can and should reuse them.
Missing Source Generator Snapshot Test
The PR adds integration tests in TUnit.TestProject/StringToParsableArgumentsTests.cs and an analyzer test in DataDrivenTestArgumentsAnalyzerTests.cs, but does not add a source generator snapshot test (.verified.txt) to validate the generated code for nullable parsable parameters.
Given the project's CLAUDE.md rules ("Changes to source generator output or public APIs require running snapshot tests"), a test case exercising DateTime? (and ideally TimeSpan?, Guid?) with [Arguments("...")] should be added to TUnit.Core.SourceGenerator.Tests/ alongside a .verified.txt snapshot. This would explicitly document and protect the generated global::System.DateTime.Parse(...) call.
Hardcoded Future Date in Analyzer Test
[Arguments("2026/04/03 15:45")]This is today's date at time of authoring, but it will become a past date and the slash-separated format (2026/04/03) may not parse successfully with CultureInfo.InvariantCulture for DateTime.Parse. The existing test cases in StringToParsableArgumentsTests.cs use ISO-8601 format ("2022-05-31") which is safer and more standard. The analyzer test format should be consistent with the runtime test cases.
Correctness: Reflection Engine Already Handles This
Worth noting: the runtime CastHelper.Cast() in TUnit.Core/Helpers/CastHelper.cs (line 72) already unwraps nullable via Nullable.GetUnderlyingType(type) ?? type before calling TryParseFromString. So the reflection-mode engine was already working for this case at runtime — the bug was purely in the source generator mode (analyzer false positive + incorrect generated code). The PR description and test coverage appropriately focus there.
Summary
| Item | Status |
|---|---|
| Core bug fix logic | Correct |
Analyzer fix (IsParsableFromString) |
Correct, but could reuse existing extension |
Source generator fix (TypedConstantFormatter) |
Correct, but duplicates pattern instead of reusing existing extensions |
| Integration tests (TestProject) | Good coverage (DateTime?, TimeSpan?, Guid?) |
| Analyzer test | Present, minor date format concern |
| Source generator snapshot tests | Missing |
The fix is functionally correct. The main actionable items are: (1) reuse IsNullableValueType()/GetNullableUnderlyingType() in TypedConstantFormatter.cs for consistency with the rest of that file, and (2) add a source generator snapshot test per the project's snapshot testing requirements.
Nullable<T>whereT : IParsable<T>(e.g.,DateTime?) was not recognized as parsable, causingTUnit0001errors when passing string arguments to nullable parsable parameters.Changes
ParsableTypeExtensions.IsParsableFromString— UnwrapNullable<T>before checking forIParsable<T>interface. This file is shared (via linked compile) between the source generator and analyzer, fixing both.TypedConstantFormatter.FormatPrimitiveForCode— Use the underlying type forParse()code generation when target is nullable (generatesDateTime.Parse(...)notDateTime?.Parse(...)).StringToParsableArgumentsTests— Added test cases forDateTime?,TimeSpan?,Guid?with both string and null arguments.DataDrivenTestArgumentsAnalyzerTests— Added analyzer test verifying nullable parsable types accept string arguments without diagnostics.Note: The runtime
CastHelper.Cast<T>already handled nullable unwrapping correctly viaNullable.GetUnderlyingType(), so no runtime changes were needed.