Fix reflection-based hook discovery

[thomhurst]

Fixes #2905

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review for PR: Fix reflection-based hook discovery

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Analyze ReflectionHookDiscoveryService.cs
• Review TUnitInitializer.cs changes
• Examine VB.NET test file
• Check for behavioral parity with source-generated mode
• Assess performance considerations
• Evaluate security concerns
• Review test coverage
• Provide comprehensive feedback

🔍 Review Summary

This PR successfully implements reflection-based hook discovery for VB.NET and other languages without source generation support. The implementation follows TUnit's architectural patterns and maintains behavioral parity with the source-generated mode.

✅ Strengths

1. Proper AOT/Trimming Attributes: Correctly uses RequiresUnreferencedCode and RequiresDynamicCode annotations on TUnitInitializer.cs:37-38
2. Intelligent Assembly Filtering: Efficient assembly scanning logic in ReflectionHookDiscoveryService.cs:33-83 that skips system assemblies and only processes TUnit-referencing assemblies
3. Complete Hook Coverage: Supports all hook types (Before/After, BeforeEvery/AfterEvery) across all scopes (Test, Class, Assembly, TestSession, TestDiscovery)
4. Thread-Safe Implementation: Uses concurrent collections and Interlocked.Increment for registration index
5. Graceful Error Handling: Empty catch block in DiscoverHooksInAssembly prevents assembly loading issues from breaking discovery

⚠️ Issues and Concerns

Critical Issues

1. Memory Leak Risk: CreateHookDelegate<T> creates new instances for non-static methods without disposal (ReflectionHookDiscoveryService.cs:595)

   object? instance = method.IsStatic ? null : Activator.CreateInstance(type);

   If the created instance implements IDisposable, it won't be disposed.

2. Exception Swallowing: The empty catch block in DiscoverHooksInAssembly (ReflectionHookDiscoveryService.cs:101-104) hides potential issues

   catch
   {
       // Ignore assemblies that can't be scanned
   }

   Consider logging at debug level or catching specific exceptions.

Performance Concerns

3. Repeated Reflection Calls: Multiple GetCustomAttributes<T> calls per method could be optimized with a single GetCustomAttributes() call
4. Assembly Scanning on Every Discovery: No caching of assembly metadata between discovery cycles
5. Parameter Array Allocation: Creates new object?[] arrays for every hook invocation

Code Quality Issues

6. Hardcoded Values: FilePath and LineNumber are hardcoded to "Unknown" and 0, losing debugging information
7. Magic Numbers: Parameter length checks (0, 1, 2) could be more explicit
8. Inconsistent Registration: Some hooks use BeforeEveryTestHooks vs type-specific collections inconsistently

🚀 Performance Recommendations

Given TUnit's performance-critical nature:

1. Cache Reflection Results: Store MethodInfo.GetCustomAttributes() results
2. Pool Parameter Arrays: Use ArrayPool<object> for hook parameter arrays
3. Lazy Instance Creation: Only create instances when hooks are actually executed
4. Assembly Metadata Caching: Cache assembly scanning results

🔒 Security Assessment

• Low Risk: Uses standard reflection APIs with appropriate attribute guards
• AOT Compatible: Properly annotated for trimming/AOT scenarios
• Assembly Filtering: Good protection against scanning unintended assemblies

🧪 Test Coverage

The VB.NET test file provides basic coverage but consider adding:

• Tests for all hook types and scopes
• Error scenarios (malformed hooks, disposal)
• Performance benchmarks vs source-generated mode
• Instance vs static hook behavior

📋 Behavioral Parity Check

✅ Confirmed: The implementation mirrors the source-generated hook registration patterns:

• Same hook method types (BeforeTestHookMethod, etc.)
• Same registration to Sources.* collections
• Same parameter handling logic
• Same DefaultHookExecutor behavior

🔧 Recommendations

1. High Priority: Fix the potential memory leak in instance creation
2. Medium Priority: Add debug logging to the catch block
3. Low Priority: Optimize reflection calls and parameter array allocations

📝 Minor Suggestions

• Consider extracting parameter resolution logic to reduce duplication
• Add XML documentation to public methods
• Use more descriptive variable names (e.g., attr → hookAttribute)

---

Overall Assessment: This is a solid implementation that successfully enables VB.NET hook support while maintaining architectural consistency. The critical memory leak issue should be addressed before merging.