Unmark AOT safe IsEquivalentTo overload

[thomhurst]

Fixes #3851

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3854: "Unmark AOT safe IsEquivalentTo overload"

Todo List:

• Read and analyze modified files
• Review AOT compatibility changes
• Evaluate code quality and implementation
• Check test coverage additions
• Provide comprehensive feedback

---

Summary

This PR addresses AOT compatibility issues with IsEquivalentTo and IsNotEquivalentTo assertions by moving the RequiresUnreferencedCode attribute from the class level to the constructor level. This allows the overloads with custom comparers to be AOT-safe while properly marking the parameterless constructor that uses structural comparison as requiring unreferenced code.

Code Quality Assessment

✅ Strengths

1. Targeted AOT Compatibility Fix: The approach is well-designed - moving RequiresUnreferencedCode from class-level to constructor-level allows granular control over which overloads are AOT-safe.

2. Source Generator Enhancement: The source generator improvements at AssertionExtensionGenerator.cs:229-243 properly handle constructor-level vs class-level attributes, with correct precedence logic.

3. Comprehensive Test Coverage: The new tests in AotCompatibilityTests.cs cover both scenarios (with/without ordering) and include good documentation explaining the AOT safety.

4. Accurate Justification Messages: The updated UnconditionalSuppressMessage attributes have improved justification text that accurately explains when the path is AOT-safe vs not.

⚠️ Areas for Improvement

1. Missing Constructor in NotEquivalentToAssertion:

   • IsEquivalentToAssertion has a constructor that takes a comparer parameter (IsEquivalentToAssertion.cs:34-44)
   • NotEquivalentToAssertion is missing this constructor and only has the .Using() fluent method
   • This creates an API inconsistency and means the AOT-safe path for IsNotEquivalentTo requires the fluent syntax

   // IsEquivalentTo - both work and are AOT-safe:
   await Assert.That(list1).IsEquivalentTo(list2, comparer);  // Constructor overload
   await Assert.That(list1).IsEquivalentTo(list2).Using(comparer);  // Fluent
   
   // IsNotEquivalentTo - only fluent works and is AOT-safe:
   await Assert.That(list1).IsNotEquivalentTo(list2).Using(comparer);  // Only this works

2. Test Coverage Gap: The AOT tests don't include IsNotEquivalentTo with custom comparer scenarios, only IsEquivalentTo.

Security & Performance

• Security: ✅ No security concerns identified
• Performance: ✅ No performance impact - this is purely about compile-time attributes

Technical Analysis

Source Generator Changes (AssertionExtensionGenerator.cs:229-243)

• Good: Proper precedence handling (constructor-level overrides class-level)
• Good: Clean implementation that maintains backward compatibility
• Good: Correctly propagates the attribute to generated extension methods

Assertion Changes

• Good: Moving from class-level to constructor-level RequiresUnreferencedCode is the right approach
• Good: The parameterless constructor (using structural comparison) is properly marked as requiring unreferenced code
• Good: The custom comparer constructor remains AOT-safe

Test Coverage

• Good: Tests verify the specific issue (IsEquivalentToAssertionExtensions.IsEquivalentTo should work with Native AOT when using a custom comparer #3851) scenarios
• Good: Documentation explains the AOT compatibility expectations
• Missing: No tests for IsNotEquivalentTo with custom comparer

Recommendations

1. Add Missing Constructor (Medium Priority)

Consider adding the missing constructor to NotEquivalentToAssertion for API consistency:

public NotEquivalentToAssertion(
    AssertionContext<TCollection> context,
    IEnumerable<TItem> notExpected,
    IEqualityComparer<TItem> comparer,
    CollectionOrdering ordering = CollectionOrdering.Any)
    : base(context)
{
    _notExpected = notExpected ?? throw new ArgumentNullException(nameof(notExpected));
    _ordering = ordering;
    SetComparer(comparer);
}

2. Expand Test Coverage (Low Priority)

Add test cases for IsNotEquivalentTo with custom comparers:

[Test]
public async Task IsNotEquivalentTo_WithCustomComparer_ShouldNotTriggerAotWarnings()
{
    var list1 = new List<int> { 1, 2, 3 };
    var list2 = new List<int> { 4, 5, 6 };
    
    await Assert.That(list1).IsNotEquivalentTo(list2).Using(EqualityComparer<int>.Default);
}

Conclusion

This is a well-executed fix that properly addresses the AOT compatibility concerns raised in #3851. The implementation follows TUnit's dual-mode architecture principles and maintains backward compatibility while enabling AOT scenarios.

Approval Status: ✅ Approved with minor suggestions

The core fix is solid and ready for merge. The suggested improvements are enhancements for API consistency but not blockers for this PR.

---

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review - PR #3854: Unmark AOT safe IsEquivalentTo overload

Todo List:

• Read and analyze all modified files
• Review AOT compatibility changes
• Evaluate code quality and implementation
• Check for potential bugs and security issues
• Assess performance considerations
• Review test coverage additions
• Provide comprehensive feedback

---

Summary

This PR addresses AOT compatibility issues with IsEquivalentTo and IsNotEquivalentTo assertions by moving the RequiresUnreferencedCode attribute from the class level to the constructor level. This allows the overloads with custom comparers to be AOT-safe while properly marking the parameterless constructor that uses structural comparison as requiring unreferenced code.

Code Quality Assessment

✅ Strengths

1. Targeted AOT Compatibility Fix: The approach is well-designed - moving RequiresUnreferencedCode from class-level to constructor-level allows granular control over which overloads are AOT-safe.

2. Source Generator Enhancement: The source generator improvements at AssertionExtensionGenerator.cs:229-243 properly handle constructor-level vs class-level attributes, with correct precedence logic (constructor takes precedence over class).

3. Comprehensive Test Coverage: The new tests in AotCompatibilityTests.cs cover both scenarios (with/without ordering) and include excellent documentation explaining the AOT safety.

4. Accurate Justification Messages: The updated UnconditionalSuppressMessage attributes have improved justification text that accurately explains when the path is AOT-safe vs not.

5. API Consistency: Both IsEquivalentToAssertion and NotEquivalentToAssertion have been updated consistently.

⚠️ Observations

1. API Asymmetry:

   • IsEquivalentToAssertion has both a parameterless constructor (AOT-unsafe) and a constructor with comparer (AOT-safe)
   • NotEquivalentToAssertion only has the parameterless constructor - there's no constructor overload that takes a comparer parameter
   • This means for IsNotEquivalentTo, users must use the fluent .Using(comparer) syntax to get AOT-safe behavior

2. Public API Changes: The snapshots show that the IsEquivalentTo overload with custom comparer is no longer marked with RequiresUnreferencedCode, which is the intended fix.

Security & Performance

• Security: ✅ No security concerns identified
• Performance: ✅ No performance impact - this is purely about compile-time attributes and AOT compatibility

Technical Analysis

Source Generator Changes (AssertionExtensionGenerator.cs:229-243)

• ✅ Excellent: Proper precedence handling - constructor-level RequiresUnreferencedCode overrides class-level
• ✅ Good: Clean implementation that maintains backward compatibility
• ✅ Correct: Properly propagates the appropriate attribute to generated extension methods

Assertion Changes

• ✅ Correct Approach: Moving from class-level to constructor-level RequiresUnreferencedCode is the right solution
• ✅ Proper Attribution: The parameterless constructor (using structural comparison) is correctly marked as requiring unreferenced code
• ✅ AOT-Safe Path: The custom comparer constructor remains unmarked and is truly AOT-safe

Test Coverage

• ✅ Good Coverage: Tests verify the specific issue (IsEquivalentToAssertionExtensions.IsEquivalentTo should work with Native AOT when using a custom comparer #3851) scenarios
• ✅ Documentation: Excellent comments explaining the AOT compatibility expectations
• ✅ Multiple Scenarios: Tests cover both basic equivalency and ordering modes

Code Quality

• ✅ Modern C# Syntax: Uses appropriate collection expressions and modern patterns
• ✅ Null Safety: Proper null checks and handling
• ✅ Error Handling: Appropriate error messages and exception handling

Potential Issues

1. Missing Constructor in NotEquivalentToAssertion (Low Priority)

While both classes have been updated consistently with the attribute changes, there's an API design difference:

// IsEquivalentTo - both paths work and have appropriate AOT marking:
await Assert.That(list1).IsEquivalentTo(list2, comparer);  // Constructor overload (AOT-safe)
await Assert.That(list1).IsEquivalentTo(list2).Using(comparer);  // Fluent (AOT-safe)

// IsNotEquivalentTo - only fluent works:
await Assert.That(list1).IsNotEquivalentTo(list2).Using(comparer);  // Only this works (AOT-safe)

This is not a bug, but creates API inconsistency. However, it's outside the scope of this PR.

Recommendations

✅ Ready for Merge

This PR successfully addresses the AOT compatibility issue raised in #3851. The implementation is solid and follows best practices.

Future Enhancements (Optional)

1. Consider adding a constructor overload to NotEquivalentToAssertion for API consistency
2. The current approach correctly prioritizes AOT safety while maintaining backward compatibility

Conclusion

This is a well-executed fix that properly addresses the AOT compatibility concerns. The key improvements:

• ✅ Moves RequiresUnreferencedCode to the appropriate scope (constructor vs class)
• ✅ Enables AOT-safe usage when custom comparers are provided
• ✅ Maintains backward compatibility for existing code
• ✅ Provides clear test coverage demonstrating the fix
• ✅ Updates public API snapshots correctly

Approval Status: ✅ Approved

The implementation correctly solves the reported issue while maintaining code quality and following TUnit's architectural principles.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

Reviewing PR #3854: "Unmark AOT safe IsEquivalentTo overload"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze all modified files
• Review AOT compatibility changes
• Evaluate code quality and implementation
• Check for potential bugs and security issues
• Assess performance considerations
• Review test coverage additions
• Provide comprehensive feedback

---

Summary

This PR addresses AOT compatibility issues with IsEquivalentTo and IsNotEquivalentTo assertions by moving the RequiresUnreferencedCode attribute from the class level to the constructor level. This allows the overloads with custom comparers to be AOT-safe while properly marking the parameterless constructor that uses structural comparison as requiring unreferenced code.

Code Quality Assessment

✅ Strengths

1. Targeted AOT Compatibility Fix: The approach is well-designed - moving RequiresUnreferencedCode from class-level to constructor-level allows granular control over which overloads are AOT-safe.

2. Source Generator Enhancement: The source generator improvements at AssertionExtensionGenerator.cs:229-243 properly handle constructor-level vs class-level attributes, with correct precedence logic.

3. Comprehensive Test Coverage: The new tests in AotCompatibilityTests.cs cover both scenarios (with/without ordering) and include good documentation explaining the AOT safety.

4. Accurate Justification Messages: The updated UnconditionalSuppressMessage attributes have improved justification text that accurately explains when the path is AOT-safe vs not.

5. Proper API Design: Both IsEquivalentToAssertion and NotEquivalentToAssertion now have two constructors:

   • Parameterless constructor (AOT-unsafe, uses StructuralEqualityComparer)
   • Constructor with custom comparer (AOT-safe)

6. Follows TUnit Guidelines: Adheres to TUnit's AOT/Trimming compatibility requirements (Rule 5 from CLAUDE.md).

⚠️ Technical Analysis

Source Generator Changes (AssertionExtensionGenerator.cs:229-243)

• ✅ Excellent: Proper precedence handling - constructor-level RequiresUnreferencedCode overrides class-level
• ✅ Good: Clean implementation that maintains backward compatibility
• ✅ Correct: Properly propagates the appropriate attribute to generated extension methods

Assertion Changes

• ✅ Correct Approach: Moving from class-level to constructor-level RequiresUnreferencedCode is the right solution
• ✅ Proper Attribution: The parameterless constructor (using structural comparison) is correctly marked as requiring unreferenced code
• ✅ AOT-Safe Path: The custom comparer constructor remains unmarked and is truly AOT-safe

Test Coverage

• ✅ Good Coverage: Tests verify the specific issue (IsEquivalentToAssertionExtensions.IsEquivalentTo should work with Native AOT when using a custom comparer #3851) scenarios
• ✅ Documentation: Excellent comments explaining the AOT compatibility expectations
• ✅ Multiple Scenarios: Tests cover both basic equivalency and ordering modes

Public API Changes

The snapshot changes show the intended effect:

• The IsEquivalentTo overload with custom comparer is no longer marked with RequiresUnreferencedCode
• This enables AOT-safe usage when custom comparers are provided
• Maintains backward compatibility for existing code

Security & Performance

• Security: ✅ No security concerns identified
• Performance: ✅ No performance impact - this is purely about compile-time attributes and AOT compatibility

Potential Issues

✅ None Found - Well Executed

This implementation correctly addresses all potential concerns:

1. API Consistency: Both assertion classes have been updated consistently
2. Backward Compatibility: Existing code continues to work unchanged
3. Clear Documentation: Test cases clearly document the expected behavior
4. Proper Attribution: Only the unsafe paths are marked with RequiresUnreferencedCode

Recommendations

✅ Ready for Merge

This PR successfully addresses the AOT compatibility issue raised in #3851. The implementation is solid and follows best practices.

Technical Correctness

• ✅ Follows TUnit's dual-mode architecture principles
• ✅ Maintains API backward compatibility
• ✅ Provides clear separation between AOT-safe and AOT-unsafe code paths
• ✅ Updates snapshots correctly to reflect the API changes

Conclusion

This is a well-executed fix that properly addresses the AOT compatibility concerns. The key improvements:

• ✅ Moves RequiresUnreferencedCode to the appropriate scope (constructor vs class)
• ✅ Enables AOT-safe usage when custom comparers are provided
• ✅ Maintains backward compatibility for existing code
• ✅ Provides clear test coverage demonstrating the fix
• ✅ Updates public API snapshots correctly

The implementation correctly solves the reported issue while maintaining code quality and following TUnit's architectural principles.

Approval Status: ✅ Approved

---