feat: enhance AbstractTestClassWithDataSourcesAnalyzer

[thomhurst]

to conditionally report diagnostics based on inheriting classes with [InheritsTests]

Fixes #3517

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Completed ✅

I've reviewed this PR for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage.

Todo List

• Read repository guidelines and setup instructions
• Analyze the AbstractTestClassWithDataSourcesAnalyzer changes
• Review the test implementation and coverage
• Check for potential bugs and edge cases
• Evaluate performance considerations
• Assess security implications
• Provide comprehensive feedback

Review Summary

This is a well-implemented enhancement that improves the analyzer's precision by conditionally reporting diagnostics based on the presence of concrete inheriting classes with [InheritsTests]. The implementation follows TUnit's coding standards and includes comprehensive test coverage.

✅ Strengths

Code Quality

• Modern C# syntax: Uses collection expressions, proper null checking, and modern patterns
• Clear logic flow: The conditional diagnostic reporting is well-structured and easy to follow
• Consistent naming: Follows TUnit's naming conventions (_camelCase for private, PascalCase for public)
• Proper separation of concerns: Helper methods are well-factored and focused

Architecture & Design

• Comprehensive attribute detection: Handles both name-based detection (Contains("DataSource"), ArgumentsAttribute) and interface-based detection (IDataSourceAttribute)
• Robust inheritance traversal: Correctly walks the inheritance chain to find base classes
• Efficient early returns: Multiple guard clauses prevent unnecessary processing

Test Coverage

• Excellent test scenarios: Covers all major cases including positive/negative flows
• Edge case testing: Tests scenarios with/without [InheritsTests], multiple inheriting classes, and various data source types
• Clear test names: Self-documenting test method names that describe the expected behavior

⚠️ Areas for Improvement

Performance Considerations

Issue: The GetAllNamedTypes() method performs a full compilation traversal for each abstract class analysis, which could be expensive in large codebases.

// Lines 102-103: This scans ALL types in the assembly
var allTypes = GetAllNamedTypes(context.Compilation.Assembly.GlobalNamespace);

Impact: O(n²) complexity when multiple abstract test classes exist, as each one triggers a full type scan.

Recommendation: Consider caching the type hierarchy or using Roslyn's built-in inheritance resolution methods.

Potential False Negatives

Issue: The attribute detection logic may miss custom data source attributes that don't follow naming conventions.

// Lines 69-70: Name-based detection could miss edge cases
if (typeName.Contains("DataSource") || typeName == "ArgumentsAttribute")

Scenario: A custom attribute named TestParametersAttribute implementing IDataSourceAttribute would only be caught by the interface check, but if that fails, it could be missed.

Robustness

Issue: Missing null safety in nested type traversal.

// Line 103: No null check before accessing Assembly
var allTypes = GetAllNamedTypes(context.Compilation.Assembly.GlobalNamespace);

Recommendation: Add defensive null checks:

var assembly = context.Compilation.Assembly;
if (assembly?.GlobalNamespace == null) return false;

🔒 Security Assessment

Status: ✅ No security concerns identified

• No external data processing or file I/O
• Standard Roslyn analyzer patterns with proper symbol access
• No reflection or dynamic code generation

🧪 Test Coverage Assessment

Status: ✅ Comprehensive coverage

The test suite covers all critical scenarios:

• ✅ Concrete classes with data sources (no warning)
• ✅ Abstract classes without data sources (no warning)
• ✅ Abstract classes without tests (no warning)
• ✅ Abstract classes with various data source types (warnings)
• ✅ Abstract classes with inheriting classes having [InheritsTests] (no warning)
• ✅ Abstract classes with inheriting classes missing [InheritsTests] (warning)

Suggestion: Consider adding a test for deeply nested inheritance chains to ensure the base type traversal works correctly.

🎯 Compliance with TUnit Guidelines

• ✅ Performance focus: Generally efficient, though full compilation scan could be optimized
• ✅ Modern C# usage: Uses modern syntax and patterns correctly
• ✅ Code style: Follows established conventions
• ✅ Analyzer best practices: Proper use of ConcurrentDiagnosticAnalyzer

📋 Minor Suggestions

1. Documentation: Consider adding XML documentation to the main HasConcreteInheritingClassesWithInheritsTests method
2. Constants: The magic strings "DataSource" and "ArgumentsAttribute" could be moved to the WellKnown constants class
3. Linq usage: Line 43 uses .ToList() but could use .Any() directly in the condition for better performance

🎉 Conclusion

This is a solid enhancement that correctly implements the conditional diagnostic logic. The code quality is high, follows TUnit's standards, and includes excellent test coverage. The main concern is the potential performance impact in large codebases, but this is a minor issue that can be addressed in future optimizations if needed.

Recommendation: ✅ Approve - This PR successfully addresses issue #3517 with a well-implemented solution.

---