add AOT mode checks to prevent unsupported reflection usage and suppress AOT warnings in reflection classes

[thomhurst]

Fixes #3238

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of PR #3239 that adds AOT mode checks and suppresses reflection warnings. Here's my detailed feedback:

Review Checklist:

• Read and understand all changed files
• Analyze reflection strategy improvements
• Review AOT compatibility enhancements
• Check metadata builder changes
• Examine test data collector modifications
• Review API surface changes
• Assess code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Check security concerns
• Provide comprehensive feedback

✅ Strengths & Positive Changes

AOT Compatibility Improvements

• Consistent runtime checks: All reflection classes now properly check RuntimeFeature.IsDynamicCodeSupported and throw clear exceptions in AOT mode
• Clear error messaging: Exception messages like "Using TUnit Reflection mechanisms isn't supported in AOT mode" provide excellent developer guidance
• Proper code organization: AOT checks are consistently placed at method entry points in reflection-heavy classes

Comprehensive Warning Suppression

• Well-documented suppressions: Each UnconditionalSuppressMessage includes clear justifications
• Proper categorization: Separate suppressions for trimming, AOT, and single-file deployment concerns
• Class-level approach: Using class-level suppressions (e.g., ReflectionGenericTypeResolver.cs:11-17) reduces code duplication

Code Quality & Architecture

• Dual execution mode design: Maintains TUnit's critical behavioral parity between source-generated and reflection modes
• Performance optimizations: Caching mechanisms (_attributeCache, _assemblyTypesCache) reduce reflection overhead
• Error handling: Graceful degradation with FailedTestMetadata for discovery failures

⚠️ Areas for Improvement

Code Duplication

• Repetitive AOT checks: The same RuntimeFeature.IsDynamicCodeSupported check appears in multiple methods. Consider extracting to a shared helper:

internal static class AotHelper
{
    public static void EnsureReflectionSupported([CallerMemberName] string? caller = null)
    {
#if NET
        if (!RuntimeFeature.IsDynamicCodeSupported)
        {
            throw new Exception($"Using TUnit Reflection mechanisms isn't supported in AOT mode (called from {caller})");
        }
#endif
    }
}

Performance Considerations

• Method-level checks: AOT checks in hot paths like ReflectionAttributeExtractor.GetAttribute<T>() could impact performance. Consider checking once per type/assembly and caching the result
• Assembly scanning: ReflectionTestDataCollector processes assemblies in parallel but still does extensive reflection work

Error Handling Concerns

• Generic exceptions: Using Exception instead of specific exception types (e.g., NotSupportedException) makes error handling less precise
• Inconsistent patterns: Some methods return early on AOT detection, others throw immediately

🔍 Specific Issues Found

ReflectionTestDataCollector.cs:86-90

if (!RuntimeFeature.IsDynamicCodeSupported)
{
    throw new Exception("Using TUnit Reflection mechanisms isn't supported in AOT mode");
}

Issue: This check happens after potentially expensive operations like GetCachedAssemblies(). Move the check earlier in the method.

ReflectionAttributeExtractor.cs:63-68

The AOT check is performed on every attribute lookup, which could be expensive. Consider caching the check result or moving it to a higher level.

TestDataCollectorFactory.cs:28-38

Missing AOT check: The factory method doesn't validate AOT compatibility before creating reflection-based collectors, potentially leading to late failures.

🛡️ Security & Reliability

Security Assessment: ✅ Good

• No security concerns identified
• Proper input validation in reflection operations
• No exposure of sensitive data in error messages

Reliability Assessment: ✅ Good

• Comprehensive error handling with fallback mechanisms
• Thread-safe caching implementations
• Proper resource management

📊 API Surface Changes

The massive changes in TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.Net4_7.verified.txt (+2511/-1299) appear to be primarily formatting/tooling related rather than breaking changes. This is typical for snapshot tests and doesn't indicate API breaking changes.

🎯 Recommendations

1. Extract common AOT checking logic to reduce duplication
2. Move AOT checks earlier in expensive operations for better performance
3. Use specific exception types (NotSupportedException) instead of generic Exception
4. Consider caching AOT capability checks at the assembly/type level
5. Add integration tests specifically for AOT mode error scenarios

📝 Minor Issues

• Unused parameters: Some TODO comments in GetDataFromSourceAsync methods
• Magic numbers: Hard-coded array sizes in some collection initializations
• Code comments: Consider adding more inline documentation for complex reflection logic

✅ Overall Assessment

This PR successfully implements the required AOT compatibility improvements while maintaining TUnit's dual execution mode architecture. The changes are well-structured, properly documented through warning suppressions, and provide clear error guidance to developers. The performance impact should be minimal, and the reliability improvements are significant.

Recommendation: ✅ Approve with minor suggestions

The code quality is high, security is maintained, and the AOT improvements are implemented correctly. The suggestions above are optimizations rather than blocking issues.

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of PR #3239 that adds AOT mode checks and suppresses reflection warnings. Here's my detailed feedback:

Review Tasks:

• Read and understand all changed files
• Analyze AOT compatibility improvements
• Review reflection strategy enhancements
• Check code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Check security concerns
• Assess test coverage implications
• Provide comprehensive feedback

---

✅ Strengths & Positive Changes

Excellent AOT Compatibility Strategy

• Consistent runtime checks: All reflection classes now properly check RuntimeFeature.IsDynamicCodeSupported and throw clear exceptions in AOT mode
• Clear error messaging: Exception messages like "Using TUnit Reflection mechanisms isn't supported in AOT mode" provide excellent developer guidance
• Proper code organization: AOT checks are consistently placed at method entry points in reflection-heavy classes

Comprehensive Warning Suppression

• Well-documented suppressions: Each UnconditionalSuppressMessage includes clear justifications explaining why reflection is needed
• Proper categorization: Separate suppressions for trimming, AOT, and single-file deployment concerns
• Class-level approach: Using class-level suppressions (e.g., ReflectionGenericTypeResolver.cs:11-17) reduces code duplication while maintaining clarity

Code Quality & Architecture

• Dual execution mode design: Maintains TUnit's critical behavioral parity between source-generated and reflection modes per CLAUDE.md requirements
• Performance optimizations: Caching mechanisms (_attributeCache, _assemblyTypesCache) reduce reflection overhead
• Error handling: Graceful degradation with FailedTestMetadata for discovery failures

⚠️ Areas for Improvement

Code Duplication

The same RuntimeFeature.IsDynamicCodeSupported check appears in multiple methods. Consider extracting to a shared helper:

internal static class AotHelper
{
    public static void EnsureReflectionSupported([CallerMemberName] string? caller = null)
    {
#if NET
        if (!RuntimeFeature.IsDynamicCodeSupported)
        {
            throw new NotSupportedException($"TUnit Reflection mechanisms are not supported in AOT mode (called from {caller})");
        }
#endif
    }
}

Performance Considerations

• Method-level checks: AOT checks in hot paths like ReflectionAttributeExtractor.GetAttribute<T>() could impact performance. Consider checking once per type/assembly and caching the result
• Late validation: In ReflectionTestDataCollector.cs:86-90, the AOT check happens after potentially expensive operations like GetCachedAssemblies()

Error Handling Concerns

• Generic exceptions: Using Exception instead of NotSupportedException makes error handling less precise
• Inconsistent patterns: Some methods return early on AOT detection, others throw immediately

🔍 Specific Issues Found

ReflectionTestDataCollector.cs:86-90

if (!RuntimeFeature.IsDynamicCodeSupported)
{
    throw new Exception("Using TUnit Reflection mechanisms isn't supported in AOT mode");
}

Issue: This check happens after potentially expensive operations. Move the check earlier in the method, ideally before GetCachedAssemblies().

ReflectionAttributeExtractor.cs:63-68

The AOT check is performed on every attribute lookup, which could be expensive. Consider caching the check result or moving it to a higher level since the capability doesn't change during execution.

TestDataCollectorFactory.cs Missing AOT Check

The factory method doesn't validate AOT compatibility before creating reflection-based collectors, potentially leading to late failures. Consider adding validation there.

🛡️ Security & Reliability Assessment

Security: ✅ Good

• No security concerns identified
• Proper input validation in reflection operations
• No exposure of sensitive data in error messages

Reliability: ✅ Good

• Comprehensive error handling with fallback mechanisms
• Thread-safe caching implementations
• Proper resource management

📊 API Surface Changes Analysis

The massive changes in TUnit.PublicAPI/Tests.Assertions_Library_Has_No_API_Changes.Net4_7.verified.txt (+2511/-1299) appear to be primarily formatting/tooling related rather than breaking changes. The additions show many RequiresDynamicCode attributes being properly documented in the public API surface, which is excellent for AOT scenarios.

🎯 Recommendations

High Priority

1. Extract common AOT checking logic to reduce duplication and improve maintainability
2. Use NotSupportedException instead of generic Exception for better error classification
3. Move AOT checks earlier in expensive operations for better performance

Medium Priority

4. Consider caching AOT capability checks at the assembly/type level rather than per-method
5. Add integration tests specifically for AOT mode error scenarios
6. Validate AOT compatibility in factory methods before creating reflection-based instances

Low Priority

7. Add more inline documentation for complex reflection logic
8. Consider using consistent exception handling patterns across all reflection classes

📝 Minor Issues

• Code style: Some inconsistent spacing in warning suppression attributes
• Magic numbers: Hard-coded array sizes in some collection initializations could be constants
• TODO comments: A few instances of TODO comments in method parameters that should be addressed

✅ Overall Assessment

This PR successfully implements comprehensive AOT compatibility improvements while maintaining TUnit's dual execution mode architecture. The changes are well-structured, thoroughly documented through warning suppressions, and provide clear error guidance to developers.

Key Achievements:

• ✅ Maintains behavioral parity between source-gen and reflection modes (critical TUnit requirement)
• ✅ Provides clear AOT mode detection and appropriate error handling
• ✅ Comprehensive warning suppression with proper justifications
• ✅ Performance considerations with caching strategies
• ✅ Good architectural separation between AOT and reflection paths

Recommendation: ✅ Approve with suggested improvements

The code quality is high, security is maintained, and the AOT improvements are implemented correctly. The suggestions above are optimizations rather than blocking issues. This PR significantly improves TUnit's AOT compatibility while maintaining its performance and reliability goals.

---