feat: add IsNotNullAssertionSuppressor to suppress nullability warnings after non-null assertions

[thomhurst]

Fixes #2679

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: IsNotNullAssertionSuppressor

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze IsNotNullAssertionSuppressor implementation
• Review test coverage and quality
• Evaluate code quality and best practices
• Check for potential issues (bugs, performance, security)
• Provide comprehensive review feedback

---

Overview

This PR adds a diagnostic suppressor that suppresses nullability warnings (CS8600, CS8602, CS8604, CS8618) for variables after they've been asserted as non-null using Assert.That(x).IsNotNull(). This is a valuable quality-of-life improvement for TUnit users.

✅ Strengths

1. Code Quality & Architecture

• Well-structured implementation following TUnit's analyzer patterns
• Consistent with existing suppressors (DynamicTestAwaitExpressionSuppressor, MarkMethodStaticSuppressor)
• Clean separation of concerns with focused helper methods
• Proper use of modern C# syntax and patterns

2. Test Coverage

• Excellent test coverage with 6 comprehensive test cases:
  • Basic suppression scenarios (CS8602, CS8604)
  • Negative testing (no suppression without assertion)
  • Multiple variable usage patterns
  • Variable isolation testing
  • Property access chains
• Tests verify both positive and negative cases effectively

3. Documentation

• Clear XML documentation explaining behavior and limitations
• Good inline comments explaining the logic
• Tests include descriptive comments about expected behavior

⚠️ Areas for Improvement

1. Performance Considerations

• Line 100: ToList() materializes all statements unnecessarily. Consider using a more efficient approach:

  // Instead of materializing the entire list
  var allStatements = containingMethod.DescendantNodes().OfType<StatementSyntax>().ToList();
  
  // Consider iterating until you find the target statement, then continue searching backwards

• Line 77: FirstOrDefault() on all descendant nodes could be expensive for large methods
• Consider caching semantic model results for repeated symbol lookups

2. Robustness Issues

• Limited scope: Only checks within method boundaries (FirstAncestorOrSelf<MethodDeclarationSyntax>() at line 93). Won't work for:
  • Assertions in nested blocks (try/catch, using statements, local functions)
  • Assertions in different scopes (properties, constructors, etc.)
• Statement ordering: Logic assumes linear statement execution but doesn't account for:
  • Control flow (if/else, loops, early returns)
  • Exception handling that might skip assertions
• Symbol comparison: Relies on SymbolEqualityComparer.Default which may not handle all edge cases

3. Edge Cases Not Covered
Missing test cases for:

• Assertions inside conditional blocks
• Multiple assertions on the same variable
• Assertions in nested methods/local functions
• Complex member access patterns (obj.Property.SubProperty)
• Generic type scenarios

4. Minor Issues

• Line 185: Uses FirstOrDefault() but should never return null given the implementation - consider First() with better error handling
• Missing validation for malformed syntax trees
• No handling of partial methods or other edge cases

🔒 Security Assessment

• No security concerns identified
• Only performs static analysis without executing code
• Proper cancellation token usage prevents hanging
• No reflection or dynamic code generation

📝 Suggestions

1. Performance Optimization:

// More efficient statement traversal
private bool WasAssertedNotNull(IdentifierNameSyntax identifierName, SemanticModel semanticModel, CancellationToken cancellationToken)
{
    var identifierStatement = identifierName.FirstAncestorOrSelf<StatementSyntax>();
    if (identifierStatement?.Parent is not BlockSyntax block)
        return false;
    
    // Search backwards from current statement
    bool foundCurrent = false;
    foreach (var statement in block.Statements.Reverse())
    {
        if (statement == identifierStatement)
        {
            foundCurrent = true;
            continue;
        }
        
        if (foundCurrent && IsNotNullAssertion(statement, symbol, semanticModel, cancellationToken))
            return true;
    }
    
    return false;
}

2. Expand Scope Support:
Consider supporting assertions in broader scopes than just method-level statements.

3. Additional Test Cases:
Add tests for control flow scenarios, nested blocks, and complex member access patterns.

🎯 Repository Compliance

• ✅ Follows TUnit code style guidelines
• ✅ Proper analyzer implementation pattern
• ✅ Comprehensive test coverage
• ✅ Modern C# syntax usage
• ⚠️ Consider running performance benchmarks for large codebases

🏁 Verdict

This is a solid implementation that provides real value to TUnit users. The core functionality is well-implemented and tested. While there are opportunities for performance optimization and scope expansion, the current implementation is production-ready for the common use cases.

Recommendation: ✅ Approve with minor suggestions

The implementation correctly addresses issue #2679 and follows TUnit's development patterns. The identified improvements are enhancements rather than blocking issues.

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: IsNotNullAssertionSuppressor

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze IsNotNullAssertionSuppressor implementation
• Review test coverage and quality
• Evaluate code quality and best practices
• Check for potential issues (bugs, performance, security)
• Provide comprehensive review feedback

---

Overview

This PR adds a diagnostic suppressor that suppresses nullability warnings (CS8600, CS8602, CS8604, CS8618) for variables after they've been asserted as non-null using Assert.That(x).IsNotNull(). This is a valuable quality-of-life improvement for TUnit users.

✅ Strengths

1. Code Quality & Architecture

• Well-structured implementation following TUnit's analyzer patterns
• Consistent with existing suppressors (DynamicTestAwaitExpressionSuppressor, MarkMethodStaticSuppressor)
• Clean separation of concerns with focused helper methods
• Proper use of modern C# syntax and patterns

2. Test Coverage

• Excellent test coverage with 12 comprehensive test cases:
  • Basic suppression scenarios (CS8602, CS8604)
  • Negative testing (no suppression without assertion)
  • Multiple variable usage patterns
  • Variable isolation testing
  • Property access chains
  • Complex assertion chain scenarios (start, middle, end, Or chains)
• Tests verify both positive and negative cases effectively

3. Documentation

• Clear XML documentation explaining behavior and limitations
• Good inline comments explaining the logic
• Tests include descriptive comments about expected behavior

⚠️ Areas for Improvement

1. Performance Considerations

• Line 100: ToList() materializes all statements unnecessarily. Consider using a more efficient approach:

  // Instead of materializing the entire list
  var allStatements = containingMethod.DescendantNodes().OfType<StatementSyntax>().ToList();
  
  // Consider iterating until you find the target statement, then continue searching backwards

• Line 77: FirstOrDefault() on all descendant nodes could be expensive for large methods
• Consider caching semantic model results for repeated symbol lookups

2. Robustness Issues

• Limited scope: Only checks within method boundaries (FirstAncestorOrSelf<MethodDeclarationSyntax>() at line 93). Won't work for:
  • Assertions in nested blocks (try/catch, using statements, local functions)
  • Assertions in different scopes (properties, constructors, etc.)
• Statement ordering: Logic assumes linear statement execution but doesn't account for:
  • Control flow (if/else, loops, early returns)
  • Exception handling that might skip assertions
• Symbol comparison: Relies on SymbolEqualityComparer.Default which may not handle all edge cases

3. Edge Cases Not Covered
Missing test cases for:

• Assertions inside conditional blocks
• Multiple assertions on the same variable
• Assertions in nested methods/local functions
• Complex member access patterns (obj.Property.SubProperty)
• Generic type scenarios

4. Minor Issues

• Line 185: Uses FirstOrDefault() but should never return null given the implementation - consider First() with better error handling
• Missing validation for malformed syntax trees
• No handling of partial methods or other edge cases

🔒 Security Assessment

• No security concerns identified
• Only performs static analysis without executing code
• Proper cancellation token usage prevents hanging
• No reflection or dynamic code generation

📝 Suggestions

1. Performance Optimization:

// More efficient statement traversal
private bool WasAssertedNotNull(IdentifierNameSyntax identifierName, SemanticModel semanticModel, CancellationToken cancellationToken)
{
    var identifierStatement = identifierName.FirstAncestorOrSelf<StatementSyntax>();
    if (identifierStatement?.Parent is not BlockSyntax block)
        return false;
    
    // Search backwards from current statement
    bool foundCurrent = false;
    foreach (var statement in block.Statements.Reverse())
    {
        if (statement == identifierStatement)
        {
            foundCurrent = true;
            continue;
        }
        
        if (foundCurrent && IsNotNullAssertion(statement, symbol, semanticModel, cancellationToken))
            return true;
    }
    
    return false;
}

2. Expand Scope Support:
Consider supporting assertions in broader scopes than just method-level statements.

3. Additional Test Cases:
Add tests for control flow scenarios, nested blocks, and complex member access patterns.

🎯 Repository Compliance

• ✅ Follows TUnit code style guidelines
• ✅ Proper analyzer implementation pattern
• ✅ Comprehensive test coverage
• ✅ Modern C# syntax usage
• ⚠️ Consider running performance benchmarks for large codebases

🏁 Verdict

This is a solid implementation that provides real value to TUnit users. The core functionality is well-implemented and tested. While there are opportunities for performance optimization and scope expansion, the current implementation is production-ready for the common use cases.

Recommendation: ✅ Approve with minor suggestions

The implementation correctly addresses issue #2679 and follows TUnit's development patterns. The identified improvements are enhancements rather than blocking issues.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: IsNotNullAssertionSuppressor

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze IsNotNullAssertionSuppressor implementation
• Review test coverage and quality
• Evaluate code quality and best practices
• Check for potential issues (bugs, performance, security)
• Provide comprehensive review feedback

---

Overview

This PR adds a diagnostic suppressor that suppresses nullability warnings (CS8600, CS8602, CS8604, CS8618) for variables after they've been asserted as non-null using Assert.That(x).IsNotNull(). This is a valuable quality-of-life improvement for TUnit users.

✅ Strengths

1. Code Quality & Architecture

• Well-structured implementation following TUnit's analyzer patterns
• Consistent with existing suppressors (DynamicTestAwaitExpressionSuppressor, MarkMethodStaticSuppressor)
• Clean separation of concerns with focused helper methods
• Proper use of modern C# syntax and patterns

2. Test Coverage

• Excellent test coverage with 12 comprehensive test cases:
  • Basic suppression scenarios (CS8602, CS8604)
  • Negative testing (no suppression without assertion)
  • Multiple variable usage patterns
  • Variable isolation testing
  • Property access chains
  • Complex assertion chain scenarios (start, middle, end, Or chains)
• Tests verify both positive and negative cases effectively

3. Documentation

• Clear XML documentation explaining behavior and limitations
• Good inline comments explaining the logic
• Tests include descriptive comments about expected behavior

⚠️ Areas for Improvement

1. Performance Considerations

• Line 100: ToList() materializes all statements unnecessarily. Consider using a more efficient approach:

  // Instead of materializing the entire list
  var allStatements = containingMethod.DescendantNodes().OfType<StatementSyntax>().ToList();
  
  // Consider iterating until you find the target statement, then continue searching backwards

• Line 77: FirstOrDefault() on all descendant nodes could be expensive for large methods
• Consider caching semantic model results for repeated symbol lookups

2. Robustness Issues

• Limited scope: Only checks within method boundaries (FirstAncestorOrSelf<MethodDeclarationSyntax>() at line 93). Won't work for:
  • Assertions in nested blocks (try/catch, using statements, local functions)
  • Assertions in different scopes (properties, constructors, etc.)
• Statement ordering: Logic assumes linear statement execution but doesn't account for:
  • Control flow (if/else, loops, early returns)
  • Exception handling that might skip assertions
• Symbol comparison: Relies on SymbolEqualityComparer.Default which may not handle all edge cases

3. Edge Cases Not Covered
Missing test cases for:

• Assertions inside conditional blocks
• Multiple assertions on the same variable
• Assertions in nested methods/local functions
• Complex member access patterns (obj.Property.SubProperty)
• Generic type scenarios

4. Minor Issues

• Line 185: Uses FirstOrDefault() but should never return null given the implementation - consider First() with better error handling
• Missing validation for malformed syntax trees
• No handling of partial methods or other edge cases

🔒 Security Assessment

• No security concerns identified
• Only performs static analysis without executing code
• Proper cancellation token usage prevents hanging
• No reflection or dynamic code generation

📝 Suggestions

1. Performance Optimization:

// More efficient statement traversal
private bool WasAssertedNotNull(IdentifierNameSyntax identifierName, SemanticModel semanticModel, CancellationToken cancellationToken)
{
    var identifierStatement = identifierName.FirstAncestorOrSelf<StatementSyntax>();
    if (identifierStatement?.Parent is not BlockSyntax block)
        return false;
    
    // Search backwards from current statement
    bool foundCurrent = false;
    foreach (var statement in block.Statements.Reverse())
    {
        if (statement == identifierStatement)
        {
            foundCurrent = true;
            continue;
        }
        
        if (foundCurrent && IsNotNullAssertion(statement, symbol, semanticModel, cancellationToken))
            return true;
    }
    
    return false;
}

2. Expand Scope Support:
Consider supporting assertions in broader scopes than just method-level statements.

3. Additional Test Cases:
Add tests for control flow scenarios, nested blocks, and complex member access patterns.

🎯 Repository Compliance

• ✅ Follows TUnit code style guidelines
• ✅ Proper analyzer implementation pattern
• ✅ Comprehensive test coverage
• ✅ Modern C# syntax usage
• ⚠️ Consider running performance benchmarks for large codebases

🏁 Verdict

This is a solid implementation that provides real value to TUnit users. The core functionality is well-implemented and tested. While there are opportunities for performance optimization and scope expansion, the current implementation is production-ready for the common use cases.

Recommendation: ✅ Approve with minor suggestions

The implementation correctly addresses issue #2679 and follows TUnit's development patterns. The identified improvements are enhancements rather than blocking issues.

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: IsNotNullAssertionSuppressor

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze IsNotNullAssertionSuppressor implementation
• Review test coverage and quality
• Evaluate code quality and best practices
• Check for potential issues (bugs, performance, security)
• Provide comprehensive review feedback

---

Overview

This PR adds a diagnostic suppressor that suppresses nullability warnings (CS8600, CS8602, CS8604, CS8618) for variables after they've been asserted as non-null using Assert.That(x).IsNotNull(). This is a valuable quality-of-life improvement for TUnit users that complements the newly added Assert.NotNull() and Assert.Null() methods.

✅ Strengths

1. Code Quality & Architecture

• Well-structured implementation following TUnit's analyzer patterns
• Consistent with existing suppressors (DynamicTestAwaitExpressionSuppressor, MarkMethodStaticSuppressor)
• Clean separation of concerns with focused helper methods
• Proper use of modern C# syntax and patterns
• Clear XML documentation explaining behavior and limitations

2. Test Coverage

• Comprehensive test coverage with 12 test cases covering:
  • Basic suppression scenarios (CS8602, CS8604)
  • Negative testing (no suppression without assertion)
  • Multiple variable usage patterns
  • Variable isolation testing (only suppresses the asserted variable)
  • Property access chains
  • Complex assertion chain scenarios (start, middle, end positions)
  • Or chains in assertions
  • Multiple variables with chained assertions
• Tests verify both positive and negative cases effectively
• Good use of descriptive test names and inline comments

3. Integration with TUnit Philosophy

• Adds valuable developer experience improvements
• Follows TUnit's modern, fast, and enjoyable testing approach
• Properly complements the existing assertion methods

4. Complementary Assert Methods

• The PR also adds Assert.NotNull() and Assert.Null() methods that properly update null-state flow analysis
• These provide an alternative when compile-time null tracking is desired over runtime suppression
• Good separation of concerns: fluent API for runtime checks, static methods for compile-time flow analysis

⚠️ Areas for Improvement

1. Performance Considerations

• Line 100: ToList() materializes all statements unnecessarily. For large methods, this could be expensive:

  var allStatements = containingMethod.DescendantNodes().OfType<StatementSyntax>().ToList();

• Line 77: FirstOrDefault() on all descendant nodes could be expensive for large methods
• Consider caching semantic model results for repeated symbol lookups

2. Scope Limitations

• Limited to method boundaries: Only searches within MethodDeclarationSyntax (line 93), missing:
  • Assertions in nested blocks (try/catch, using statements)
  • Assertions in different scopes (properties, constructors, local functions)
  • Cross-method assertions (though this is reasonable limitation)

3. Control Flow Analysis

• Linear assumption: Logic assumes linear statement execution but doesn't account for:
  • Control flow (if/else, loops, early returns)
  • Exception handling that might skip assertions
  • Variable reassignment between assertion and usage

4. Minor Implementation Issues

• Line 185: Uses FirstOrDefault() but should never return null given implementation - consider using First() with better error handling
• Missing validation for malformed syntax trees

🔒 Security Assessment

• No security concerns identified
• Only performs static analysis without executing code
• Proper cancellation token usage prevents hanging
• No reflection or dynamic code generation

📝 Suggestions for Future Enhancement

1. Performance Optimization:

// More efficient backward search from current statement
private bool WasAssertedNotNull(IdentifierNameSyntax identifierName, SemanticModel semanticModel, CancellationToken cancellationToken)
{
    var identifierStatement = identifierName.FirstAncestorOrSelf<StatementSyntax>();
    if (identifierStatement?.Parent is not BlockSyntax block)
        return false;
    
    // Search backwards from current statement (more efficient)
    bool foundCurrent = false;
    foreach (var statement in block.Statements.Reverse())
    {
        if (statement == identifierStatement)
        {
            foundCurrent = true;
            continue;
        }
        
        if (foundCurrent && IsNotNullAssertion(statement, symbol, semanticModel, cancellationToken))
            return true;
    }
    
    return false;
}

2. Potential Edge Cases to Consider:

• Assertions inside conditional blocks
• Multiple assertions on the same variable
• Assertions in nested methods/local functions
• Complex member access patterns (obj.Property.SubProperty)
• Generic type scenarios

🎯 Repository Compliance

• ✅ Follows TUnit code style guidelines perfectly
• ✅ Proper analyzer implementation pattern
• ✅ Comprehensive test coverage with both positive and negative cases
• ✅ Modern C# syntax usage (collection expressions, pattern matching)
• ✅ Appropriate XML documentation
• ✅ No dual-mode implementation needed (analyzers are compile-time only)

🏁 Verdict

This is a well-implemented feature that provides real value to TUnit users. The core functionality is solid and the test coverage is excellent. The implementation correctly addresses issue #2679 and follows TUnit's development patterns closely.

The identified performance considerations and scope limitations are opportunities for future enhancement rather than blocking issues. For the common use cases, this implementation will work reliably and efficiently.

Recommendation: ✅ Approve

The implementation demonstrates good software engineering practices and will improve the developer experience for TUnit users who prefer the fluent assertion API over the static Assert.NotNull() methods.

---

Additional Notes:

• The complementary Assert.NotNull()/Assert.Null() methods provide an excellent alternative for scenarios where compile-time null-state tracking is preferred
• The suppressor focuses on the most common usage patterns, which is a pragmatic approach
• Future enhancements could expand scope support and optimize performance for large codebases

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review: IsNotNullAssertionSuppressor

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze IsNotNullAssertionSuppressor implementation
• Review test coverage and quality
• Evaluate code quality and best practices
• Check for potential issues (bugs, performance, security)
• Provide comprehensive review feedback

---

Overview

This PR adds a diagnostic suppressor that suppresses nullability warnings (CS8600, CS8602, CS8604, CS8618) for variables after they've been asserted as non-null using Assert.That(x).IsNotNull(). This is a valuable quality-of-life improvement for TUnit users that complements the newly added Assert.NotNull() and Assert.Null() methods.

✅ Strengths

1. Code Quality & Architecture

• Well-structured implementation following TUnit's analyzer patterns
• Consistent with existing suppressors (DynamicTestAwaitExpressionSuppressor, MarkMethodStaticSuppressor)
• Clean separation of concerns with focused helper methods
• Proper use of modern C# syntax and patterns
• Clear XML documentation explaining behavior and limitations

2. Test Coverage

• Comprehensive test coverage with 12 test cases covering:
  • Basic suppression scenarios (CS8602, CS8604)
  • Negative testing (no suppression without assertion)
  • Multiple variable usage patterns
  • Variable isolation testing (only suppresses the asserted variable)
  • Property access chains
  • Complex assertion chain scenarios (start, middle, end positions)
  • Or chains in assertions
  • Multiple variables with chained assertions
• Tests verify both positive and negative cases effectively
• Good use of descriptive test names and inline comments

3. Integration with TUnit Philosophy

• Adds valuable developer experience improvements
• Follows TUnit's modern, fast, and enjoyable testing approach
• Properly complements the existing assertion methods

4. Complementary Assert Methods

• The PR also adds Assert.NotNull() and Assert.Null() methods that properly update null-state flow analysis
• These provide an alternative when compile-time null tracking is desired over runtime suppression
• Good separation of concerns: fluent API for runtime checks, static methods for compile-time flow analysis

⚠️ Areas for Improvement

1. Performance Considerations

• Line 100: ToList() materializes all statements unnecessarily. For large methods, this could be expensive:

  var allStatements = containingMethod.DescendantNodes().OfType<StatementSyntax>().ToList();

• Line 77: FirstOrDefault() on all descendant nodes could be expensive for large methods
• Consider caching semantic model results for repeated symbol lookups

2. Scope Limitations

• Limited to method boundaries: Only searches within MethodDeclarationSyntax (line 93), missing:
  • Assertions in nested blocks (try/catch, using statements)
  • Assertions in different scopes (properties, constructors, local functions)
  • Cross-method assertions (though this is reasonable limitation)

3. Control Flow Analysis

• Linear assumption: Logic assumes linear statement execution but doesn't account for:
  • Control flow (if/else, loops, early returns)
  • Exception handling that might skip assertions
  • Variable reassignment between assertion and usage

4. Minor Implementation Issues

• Line 185: Uses FirstOrDefault() but should never return null given implementation - consider using First() with better error handling
• Missing validation for malformed syntax trees

🔒 Security Assessment

• No security concerns identified
• Only performs static analysis without executing code
• Proper cancellation token usage prevents hanging
• No reflection or dynamic code generation

📝 Suggestions for Future Enhancement

1. Performance Optimization:

// More efficient backward search from current statement
private bool WasAssertedNotNull(IdentifierNameSyntax identifierName, SemanticModel semanticModel, CancellationToken cancellationToken)
{
    var identifierStatement = identifierName.FirstAncestorOrSelf<StatementSyntax>();
    if (identifierStatement?.Parent is not BlockSyntax block)
        return false;
    
    // Search backwards from current statement (more efficient)
    bool foundCurrent = false;
    foreach (var statement in block.Statements.Reverse())
    {
        if (statement == identifierStatement)
        {
            foundCurrent = true;
            continue;
        }
        
        if (foundCurrent && IsNotNullAssertion(statement, symbol, semanticModel, cancellationToken))
            return true;
    }
    
    return false;
}

2. Potential Edge Cases to Consider:

• Assertions inside conditional blocks
• Multiple assertions on the same variable
• Assertions in nested methods/local functions
• Complex member access patterns (obj.Property.SubProperty)
• Generic type scenarios

🎯 Repository Compliance

• ✅ Follows TUnit code style guidelines perfectly
• ✅ Proper analyzer implementation pattern
• ✅ Comprehensive test coverage with both positive and negative cases
• ✅ Modern C# syntax usage (collection expressions, pattern matching)
• ✅ Appropriate XML documentation
• ✅ No dual-mode implementation needed (analyzers are compile-time only)

🏁 Verdict

This is a well-implemented feature that provides real value to TUnit users. The core functionality is solid and the test coverage is excellent. The implementation correctly addresses issue #2679 and follows TUnit's development patterns closely.

The identified performance considerations and scope limitations are opportunities for future enhancement rather than blocking issues. For the common use cases, this implementation will work reliably and efficiently.

Recommendation: ✅ Approve

The implementation demonstrates good software engineering practices and will improve the developer experience for TUnit users who prefer the fluent assertion API over the static Assert.NotNull() methods.

---

Additional Notes:

• The complementary Assert.NotNull()/Assert.Null() methods provide an excellent alternative for scenarios where compile-time null-state tracking is preferred
• The suppressor focuses on the most common usage patterns, which is a pragmatic approach
• Future enhancements could expand scope support and optimize performance for large codebases