fix(assertions): add nullability checks for [AssertionFrom<T>] generated assertions

[thomhurst]

Fixes #3421

[claude]

Claude finished @thomhurst's task —— View job

---

🔍 PR Review Complete

I've thoroughly reviewed this PR that fixes nullability checks for [AssertionFrom<T>] generated assertions. Here's my detailed feedback:

✅ Strengths

1. Excellent Problem Identification & Solution

• The fix correctly identifies that methods like string.IsNullOrEmpty(string? value) explicitly accept null values via nullable annotations
• Smart fallback to method name checking for .NET Framework compatibility where NullableAnnotation might not be available
• Lines AssertionMethodGenerator.cs:710-720 show well-thought-out logic

2. Comprehensive Test Coverage

• New test file covers all critical scenarios: null, empty, whitespace, and failure cases
• Tests both positive assertions (IsNullOrEmpty, IsNullOrWhiteSpace) and negated ones (IsNotNullOrEmpty)
• Good use of string? to explicitly test nullable scenarios

3. Code Quality

• Clear, well-commented logic explaining the nullability checking strategy
• Follows TUnit's coding conventions from CLAUDE.md
• Proper handling of backwards compatibility

⚠️ Areas for Improvement

1. Dual-Mode Implementation Concern

• CRITICAL: The fix only addresses source-generated mode but TUnit requires dual-mode implementation (per CLAUDE.md line 4)
• However, after investigation, AssertionFrom<T> attributes are source-generation-only, so this may be acceptable
• Recommendation: Confirm this is source-gen-only functionality and doesn't need reflection mode equivalent

2. Snapshot Tests Missing

• IMPORTANT: Per CLAUDE.md lines 11-14, source generator changes require running and accepting snapshots
• Command needed: dotnet test TUnit.Core.SourceGenerator.Tests
• If snapshots differ, rename *.received.txt to *.verified.txt

3. Minor Code Style

• AssertionMethodGenerator.cs:706-707: The variable shouldGenerateNullCheck could be more descriptive (e.g., shouldSkipNullCheckForNullableParameter)

🚀 Performance & Security

✅ Performance: No performance regressions - the additional nullable annotation check is minimal overhead during source generation

✅ Security: No security concerns identified

🧪 Test Assessment

Excellent test coverage with these scenarios:

• IsNullOrEmpty_WithNullString_Passes ✅
• IsNullOrEmpty_WithEmptyString_Passes ✅
• IsNullOrEmpty_WithNonEmptyString_Fails ✅
• All IsNullOrWhiteSpace variants ✅
• Negated assertion (IsNotNullOrEmpty) ✅

📋 Action Items Before Merge

1. Run snapshot tests: dotnet test TUnit.Core.SourceGenerator.Tests
2. Accept snapshots if needed: Rename any *.received.txt to *.verified.txt
3. Verify: Confirm AssertionFrom<T> is source-gen-only (no reflection mode needed)

🎯 Overall Assessment

This is a well-implemented bug fix that solves a real problem with nullable string assertions. The approach is sound, tests are comprehensive, and the code quality is high. The main requirement is ensuring snapshot tests are updated per TUnit's development guidelines.

Recommendation: Approve after completing snapshot test requirements ✅

---