Fix abstract class constructor processing in hook metadata generation #2888

Copilot · 2025-08-13T18:46:34Z

The hook metadata generator was failing when processing abstract base classes that have parameterized constructors and hook methods. This resulted in a NullReferenceException during test discovery.

Problem

The issue occurred when:

An abstract base class has a parameterized constructor (which is automatically protected)
The abstract class contains hook methods like [Before(Test)]
A concrete class inherits from this abstract base class

The WriteClassMetadataGetOrAdd method in MetadataGenerationHelper.cs was calling typeSymbol.InstanceConstructors.FirstOrDefault() which returns any instance constructor, including protected ones. For abstract classes with parameterized constructors, this would select a protected constructor and later cause issues during metadata generation.

Example that previously failed

public abstract class BaseTestClass(IServiceProvider serviceProvider)
{
    [Before(Test)]
    public Task Hook() => Task.CompletedTask;
}

[ClassConstructor<DependencyInjectionClassConstructor>]
public sealed class ActualTestClass(IServiceProvider serviceProvider)
    : BaseTestClass(serviceProvider)
{
    [Test]
    public void Test1() { }
}

This would throw:

System.TypeInitializationException: The type initializer for 'TUnit.Generated.Hooks.GeneratedHookRegistry' threw an exception.
---> System.InvalidOperationException: Failed to initialize hook registry: Object reference not set to an instance of an object.
---> System.NullReferenceException: Object reference not set to an instance of an object.

Solution

Modified the constructor selection logic in MetadataGenerationHelper.cs to:

Skip constructor processing entirely for abstract classes - Since abstract classes cannot be instantiated directly, their constructor metadata is not needed for the testing framework
Only consider public constructors for concrete classes - This ensures we don't accidentally process protected or private constructors

The fix maintains backward compatibility while properly handling abstract inheritance patterns commonly used with dependency injection.

Testing

Added reproduction test case that matches the original issue report
Verified existing abstract class tests (like BaseClass<T>) continue to work
Confirmed PublicAPI tests pass without changes
Validated that the build succeeds without the previous null reference exception

Fixes #2887.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

aka.ms
- Triggering command: curl -I -sSL --retry 5 --retry-delay 2 --connect-timeout 15 REDACTED (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>

claude · 2025-08-13T19:45:01Z

Claude finished @Copilot's task —— View job

Code Review Complete ✅

Todo List:

Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
Analyze the core fix in MetadataGenerationHelper.cs
Review test coverage and reproduction cases
Examine code quality and best practices
Check for potential bugs or issues
Assess performance and security considerations
Provide comprehensive feedback

📋 Review Summary

This PR provides an excellent fix for issue #2887. The solution is well-designed, thoroughly tested, and maintains backward compatibility.

✅ Strengths

Code Quality

Surgical precision: The fix is minimal and targeted, modifying only the essential logic in MetadataGenerationHelper.cs:149-154
Clear intent: The conditional logic with descriptive comments makes the solution easy to understand
Consistent patterns: Uses existing codebase conventions and maintains the same approach in both method variants

Problem Resolution

Root cause addressed: Correctly identifies that abstract classes cannot be instantiated and skips their constructor processing
Defensive programming: The fix prevents future similar issues by explicitly handling abstract class scenarios
Logical correctness: Only processes public constructors for concrete classes, which aligns with testing framework requirements

Test Coverage

Comprehensive reproduction: ReproTest.cs exactly matches the failing scenario from the issue
Verification test: Issue2887Tests.cs ensures the fix works without generation errors
Generated output validation: The .verified.txt file shows correct metadata generation for the concrete class

💡 Minor Observations

Code Organization

The fix appears in two places due to method duplication:

WriteClassMetadataGetOrAdd() (lines 149-154) - newer method
GenerateClassMetadataGetOrAdd() (lines 217-222) - legacy method for backward compatibility

This is actually good practice as it maintains consistency across both code paths.

Performance Considerations

✅ Efficient early exit: Skipping abstract class processing reduces unnecessary work
✅ Minimal overhead: The IsAbstract check is a simple property access
✅ No new allocations: The fix doesn't introduce additional memory usage

🔒 Security Assessment

✅ No security concerns: The changes are limited to metadata generation logic
✅ Access control maintained: Constructor accessibility filtering remains proper
✅ No external dependencies: The fix uses existing framework APIs

🎯 Test Strategy Evaluation

The testing approach is solid:

Reproduction test: Validates the exact failing scenario
Source generation test: Ensures generated code compiles and runs
Regression prevention: The verified output serves as a snapshot test

📊 Overall Assessment

This is a high-quality fix that:

Solves the immediate problem effectively
Maintains backward compatibility
Includes comprehensive test coverage
Uses minimal, targeted changes
Follows existing code patterns

Recommendation: ✅ Approve

The fix is ready to merge. It addresses the root cause, includes proper testing, and maintains code quality standards.

Initial plan

c086c43

Copilot AI assigned Copilot and thomhurst Aug 13, 2025

Copilot started work on behalf of thomhurst August 13, 2025 18:46 View session

Fix abstract class constructor processing in hook metadata generation

c278514

Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Abstract Class [Before(Test)] breakage~~ Fix abstract class constructor processing in hook metadata generation Aug 13, 2025

Copilot AI requested a review from thomhurst August 13, 2025 19:05

Copilot finished work on behalf of thomhurst August 13, 2025 19:05

Copilot AI had a problem deploying to Pull Requests August 13, 2025 19:12 Failure

thomhurst marked this pull request as ready for review August 13, 2025 19:12

thomhurst enabled auto-merge (squash) August 13, 2025 19:19

test: Add test source for ActualTestClass in Issue 2887

0394303

thomhurst temporarily deployed to Pull Requests August 13, 2025 19:44 — with GitHub Actions Inactive

thomhurst had a problem deploying to Pull Requests August 13, 2025 19:44 — with GitHub Actions Failure

thomhurst disabled auto-merge August 13, 2025 20:29

thomhurst merged commit d506a98 into main Aug 13, 2025
7 of 9 checks passed

thomhurst deleted the copilot/fix-2887 branch August 13, 2025 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix abstract class constructor processing in hook metadata generation #2888

Fix abstract class constructor processing in hook metadata generation #2888

Uh oh!

Copilot AI commented Aug 13, 2025 •

edited

Loading

Uh oh!

claude bot commented Aug 13, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix abstract class constructor processing in hook metadata generation #2888

Fix abstract class constructor processing in hook metadata generation #2888

Uh oh!

Conversation

Copilot AI commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Example that previously failed

Solution

Testing

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

claude bot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

📋 Review Summary

✅ Strengths

Code Quality

Problem Resolution

Test Coverage

💡 Minor Observations

Code Organization

Performance Considerations

🔒 Security Assessment

🎯 Test Strategy Evaluation

📊 Overall Assessment

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Aug 13, 2025 •

edited

Loading

claude bot commented Aug 13, 2025 •

edited

Loading