fix(assertions): fix compiler nullability analysis for IsNotNull assertions for nullable reference and value types

[thomhurst]

Fixes#3440

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: IsNotNull Nullability Analysis Fix

Reviewing PR #3442 that fixes nullability analysis for IsNotNull assertions

Summary

This PR fixes compiler nullability analysis for IsNotNull assertions by implementing proper generic type constraints and mapping contexts to return non-nullable types. The changes enable proper nullability flow analysis so the compiler can understand that after an IsNotNull assertion, the value is guaranteed to be non-null.

✅ Positive Aspects

1. Excellent Nullability Implementation

• TUnit.Assertions/Extensions/AssertionExtensions.cs:29-51: The dual overloads for IsNotNull correctly handle both nullable reference types (where TValue : class) and nullable value types (where TValue : struct)
• Line 35: Uses proper context mapping (TValue? v) => v! for reference types
• Line 49: Uses proper context mapping (TValue? v) => v!.Value for nullable value types
• Returns NotNullAssertion<TValue> (non-nullable) instead of nullable types

2. Robust Test Coverage

• TUnit.Assertions.Tests/NullabilityInferenceTests.cs: Comprehensive tests covering:
  • Reference type nullability inference (lines 6-16)
  • Value type nullability inference (lines 18-29)
  • Chaining scenarios with .And (lines 32-44)
• Tests verify that returned values can be used without nullable warnings

3. Clean Code Organization

• TUnit.Assertions/IntAssertions.cs: New file follows established patterns with [GenerateAssertion] attributes
• Proper separation of concerns between manual implementations and generated extensions

4. API Consistency

• Public API snapshots correctly updated across all target frameworks
• Addition of IntAssertions class follows TUnit's extensible assertion pattern

⚠️Areas for Improvement

1. Missing Dual-Mode Verification

• Critical: While the assertion library is separate from the engine, the PR description mentions this fixes "compiler nullability analysis" but doesn't explicitly verify both source-generated and reflection modes work identically
• Recommendation: Add tests that explicitly verify the behavior works in both execution modes

2. Documentation Could Be Enhanced

• TUnit.Assertions/Extensions/AssertionExtensions.cs:26-27, 40-41: Comments could better explain the nullability flow benefits
• Consider adding XML doc examples showing the compiler nullability benefits

3. Test Cleanup

• TUnit.Assertions.Tests/Old/DefaultAssertionTests.cs: Entire file removed - ensure these test cases are adequately covered elsewhere
• 34 lines of test coverage removed without clear replacement

🔍 Technical Analysis

Context Mapping Implementation

// Reference types: TValue? → TValuevarmappedContext=source.Context.Map((TValue?v)=>v!);// Value types: TValue? → TValue varmappedContext=source.Context.Map<TValue>((TValue?v)=>v!.Value);

This is the correct approach for nullability flow analysis. The ! operators are safe here because the NotNullAssertion<TValue> will fail the test if the value is actually null.

Public API Impact

• Net reduction of 80-100 lines in API surface across frameworks
• Addition of IntAssertions class with IsEven/IsOdd methods
• Changes appear to be refactoring rather than breaking changes

🏆 Compliance with TUnit Guidelines

✅ Modern C# Usage: Uses collection expressions, proper async patterns
✅ Performance: No performance regressions expected in assertion library
✅ Code Style: Follows established patterns and naming conventions
⚠️Dual-Mode: Assertions library should work in both modes, but explicit verification missing
✅ Snapshot Updates: Public API snapshots properly updated

🎯 Final Recommendations

1. Add explicit dual-mode test: Create a test that verifies nullability inference works identically in both source-generated and reflection execution modes
2. Verify test coverage: Ensure removed test cases from DefaultAssertionTests.cs are adequately covered elsewhere
3. Consider adding examples: XML documentation with examples showing nullability benefits would help users understand the value

📊 Overall Assessment

This is a high-quality implementation that correctly solves the nullability analysis problem. The technical approach is sound, the code follows TUnit's patterns, and the test coverage is good. The main concern is ensuring dual-mode compatibility verification.

Verdict: ✅ Approve with minor suggestions - The implementation is solid and ready for merge with the suggested enhancements.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: IsNotNull Nullability Analysis Fix ✅

Reviewed PR #3442 that fixes nullability analysis for IsNotNull assertions

Todo List:

• Read repository guidelines (CLAUDE.md)
• Examine core assertion changes
• Review nullability condition updates
• Check test coverage and new tests
• Analyze public API changes
• Verify dual-mode implementation compliance
• Provide comprehensive feedback

---

🔍 Review Summary

This PR successfully implements proper compiler nullability analysis for IsNotNull assertions for both nullable reference types and nullable value types. The implementation is well-designed and follows TUnit's architectural patterns.

✅ Strengths

1. Excellent Nullability Flow Analysis

The implementation correctly handles both scenarios:

• Reference Types: IAssertionSource<TValue> where TValue : class → NotNullAssertion<TValue>
• Value Types: IAssertionSource<TValue?> where TValue : struct → NotNullAssertion<TValue>

The context mapping is properly implemented:

// Reference types: TValue? → TValuevarmappedContext=source.Context.Map((TValue?v)=>v!);// Value types: TValue? → TValue varmappedContext=source.Context.Map<TValue>((TValue?v)=>v!.Value);

2. Strong Test Coverage

• Tests cover both reference and value types (TUnit.Assertions.Tests/NullabilityInferenceTests.cs:6-44)
• Tests verify chaining behavior works correctly after IsNotNull()
• Tests demonstrate real nullability flow by calling methods on returned values

3. Proper API Evolution

• Public API snapshots correctly updated across all target frameworks
• Manual implementation replaces generated code where needed for better type inference
• Comments clearly explain the reasoning: "manually implemented below for proper nullability inference"

4. Follows Repository Guidelines

• ✅ Dual-mode support: This is assertion library code that works in both modes
• ✅ Snapshot tests updated: All public API snapshots properly accepted
• ✅ Modern C# syntax used throughout
• ✅ Proper code organization and documentation

📝 Minor Observations

1. IntAssertions.cs Addition

The IntAssertions.cs file appears unrelated to the nullability fix:

publicstaticboolIsEven(thisintvalue)=>value%2==0;publicstaticboolIsOdd(thisintvalue)=>value%2!=0;

This might be test code that accidentally got included, though it doesn't impact the main fix.

2. Test File Cleanup

Removing TUnit.Assertions.Tests/Old/DefaultAssertionTests.cs appears to be unrelated cleanup, which is fine but could have been a separate commit.

🚀 Technical Implementation Quality

Context Mapping Approach

The use of source.Context.Map() to transform the assertion context is elegant and maintains the fluent API chain while changing the underlying type from nullable to non-nullable.

Overload Resolution

Using distinct generic constraints (TValue : class vs TValue : struct) ensures the compiler picks the correct overload, providing precise nullability behavior for each scenario.

GetAwaiter Pattern

The NotNullAssertion<TValue> class correctly implements GetAwaiter() to return TaskAwaiter<TValue>, enabling the await Assert.That(nullable).IsNotNull() pattern to return non-nullable values.

🔒 Security & Performance

• No security concerns: Pure assertion library code with no external dependencies
• No performance regressions: Uses existing mapping infrastructure
• AOT/Trimming compatible: No dynamic code generation or problematic reflection

🎯 Verdict

This is a high-quality implementation that:

• ✅ Solves the nullability analysis problem correctly
• ✅ Follows all repository guidelines and patterns
• ✅ Has comprehensive test coverage
• ✅ Maintains API compatibility
• ✅ Uses modern C# features appropriately

Recommendation: Approve - This PR successfully fixes issue #3440 and provides developers with proper nullability flow analysis when using IsNotNull assertions.

---