Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3376

@claude
Copy link
Contributor

claude bot commented Oct 14, 2025

Claude finished @thomhurst's task —— View job


Code Review Complete

Reviewing PR: feat: add support for generic method assertions with non-inferable type parameters

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Analyze main implementation changes in MethodAssertionGenerator.cs
  • Review new test data and test cases
  • Examine snapshot test updates
  • Check code quality and best practices
  • Identify potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage

Review Summary

This PR adds support for generic method assertions with non-inferable type parameters in the TUnit assertion source generator. The implementation looks solid overall with good test coverage.

Code Quality and Best Practices ✅

Strengths:

  • Clean, focused implementation with a single responsibility change
  • Follows existing code patterns and conventions in the codebase
  • Proper XML documentation and code comments
  • Uses modern C# syntax (string.Join, LINQ methods)
  • Includes appropriate suppression attributes for trimming warnings
  • Follows the repository's dual-mode requirement (source-gen mode implemented)

Areas for improvement:

  • The change only addresses source-generated mode - per CLAUDE.md guidelines, this feature also needs to be implemented in reflection mode (TUnit.Engine) to maintain dual-mode compatibility

Potential Bugs or Issues ⚠️

  1. Missing reflection mode implementation - The CLAUDE.md file explicitly states: "ALL changes must work identically in both source-generated and reflection modes". This PR only implements the feature in the source generator.

  2. Type parameter constraint handling - The test case uses where TError : System.Exception but I don't see explicit handling of generic constraints in the generator. This could potentially cause compilation issues in complex constraint scenarios.

Performance Considerations ✅

  • No performance regressions detected - The changes are minimal and only add type argument handling when needed
  • String concatenation is appropriately handled with StringBuilder and string.Join
  • No additional allocations in hot paths
  • The generic type parameter handling is done at compile-time, so no runtime impact

Security Concerns ✅

  • No security issues identified
  • Uses UnconditionalSuppressMessage appropriately for known-safe generic type usage
  • No dynamic code generation or reflection usage that could impact AOT/trimming
  • Input validation is already handled by the existing method validation logic

Test Coverage ✅

Well-covered:

  • New test case GenericMethodWithNonInferableTypeParameter with comprehensive verification
  • Snapshot tests updated across all target frameworks (.NET 4.7, 8.0, 9.0, 10.0)
  • Test verifies both the generated assertion class and extension method signatures
  • Validates that type arguments are correctly included in method calls

Missing:

  • Tests for reflection mode (since it's not implemented)
  • Edge cases with complex generic constraints
  • Multiple generic type parameters with mixed inference capabilities

Specific Technical Issues

File: MethodAssertionGenerator.cs:485-504

The GetGenericTypeParameters method has good logic for handling generic type parameters, but the comment on line 487-488 suggests this is specifically for extension methods, yet the logic applies to all methods. Consider clarifying this comment.

File: MethodAssertionGenerator.cs:386-391

The type argument building logic correctly handles generic methods, but should also validate that all type parameters are properly constrained in the generated code.

Recommendations

  1. Critical: Implement the reflection mode counterpart in TUnit.Engine to satisfy dual-mode requirements
  2. Medium: Add test cases for complex generic constraints
  3. Low: Consider adding validation for generic constraint compatibility
  4. Low: Clarify comments around generic type parameter handling

Compliance with Repository Guidelines

  • ✅ Snapshot tests properly updated and committed
  • Missing dual-mode implementation - only source-gen mode implemented
  • ✅ Modern C# syntax used appropriately
  • ✅ Performance considerations met
  • ✅ AOT/trimming compatibility maintained

Overall Assessment: Good implementation for source-generated mode, but needs reflection mode implementation to be complete per repository requirements.

@thomhurst thomhurst merged commit 9811463 into main Oct 14, 2025
12 checks passed
@thomhurst thomhurst deleted the bug/3376 branch October 14, 2025 19:11
This was referenced Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Carry generics to method call in assertion source generator

2 participants