fix: enhance AOT compatibility in CastHelper with additional checks

[thomhurst]

Fixes #3312

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: AOT Compatibility Enhancement in CastHelper

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze main changes in CastHelper.cs
• Review API verification file changes
• Check test modifications
• Evaluate code quality and best practices
• Assess performance considerations
• Check for security concerns
• Verify test coverage adequacy
• Provide comprehensive review feedback

🎯 Overall Assessment: APPROVED with Minor Suggestions

This is a well-executed AOT compatibility improvement that follows TUnit's guidelines and modern .NET practices.

---

✅ Strengths

Code Quality & Best Practices:

• ✅ Follows TUnit's AOT/Trimming compatibility guidelines from CLAUDE.md
• ✅ Uses UnconditionalSuppressMessage appropriately for known-safe reflection
• ✅ Adds AggressiveInlining for performance-critical methods
• ✅ Proper error handling with runtime AOT detection
• ✅ Maintains dual-mode compatibility (source-gen and reflection)

AOT Compatibility:

• ✅ Strategic use of ThrowOnAot() prevents silent failures in AOT mode
• ✅ Runtime feature detection using RuntimeFeature.IsDynamicCodeSupported
• ✅ Graceful degradation when reflection isn't available
• ✅ Preserves DynamicallyAccessedMembers annotations for trimming

Performance:

• ✅ AggressiveInlining on hot path methods
• ✅ Early returns for common cases
• ✅ No unnecessary allocations added

---

💡 Minor Suggestions

Documentation:

// Consider updating the justification from "<Pending>" to be more descriptive:
[UnconditionalSuppressMessage("AOT", "IL3050...", 
    Justification = "Reflection usage is guarded by runtime AOT checks via ThrowOnAot()")]

Error Messaging:
The ThrowOnAot error message could be more helpful:

throw new InvalidOperationException(
    $"Cannot cast {value?.GetType()?.Name ?? "null"} to {targetType?.Name} in AOT mode. " +
    "Consider using AotConverterRegistry.Register() for custom type conversions.");

Test Coverage:

• The nuget tester adds a parameterized test, but consider adding specific AOT scenario tests
• Could benefit from tests that verify ThrowOnAot behavior when IsDynamicCodeSupported = false

---

🔍 Technical Review

Security: ✅ No security concerns

• Proper input validation maintained
• No new attack vectors introduced

Thread Safety: ✅ No issues

• Static methods remain thread-safe
• No shared mutable state added

Breaking Changes: ✅ None

• API surface changes are additive (removal of attributes)
• Behavioral changes are improvements (better error handling)

Performance Impact: ✅ Positive

• AggressiveInlining should improve performance
• AOT checks are minimal overhead
• Early path optimization maintained

---

📋 Verification Checklist (per CLAUDE.md)

• ✅ Works in both source-generated and reflection modes
• ✅ Snapshots updated correctly (API verification files)
• ✅ AOT/trimming compatible with proper annotations
• ✅ No performance regression (likely improvement)
• ✅ Thread-safe implementation

Recommendation: Merge after addressing the minor documentation suggestions if desired.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: AOT Compatibility Enhancement in CastHelper

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze main changes in CastHelper.cs
• Review API verification file changes
• Check test modifications
• Evaluate code quality and best practices
• Assess performance considerations
• Check for security concerns
• Verify test coverage adequacy
• Provide comprehensive review feedback

🎯 Overall Assessment: APPROVED with Minor Suggestions

This is a well-executed AOT compatibility improvement that follows TUnit's guidelines and modern .NET practices.

---

✅ Strengths

Code Quality & Best Practices:

• ✅ Follows TUnit's AOT/Trimming compatibility guidelines from CLAUDE.md
• ✅ Uses UnconditionalSuppressMessage appropriately for known-safe reflection
• ✅ Adds AggressiveInlining for performance-critical methods
• ✅ Proper error handling with runtime AOT detection
• ✅ Maintains dual-mode compatibility (source-gen and reflection)

AOT Compatibility:

• ✅ Strategic use of ThrowOnAot() prevents silent failures in AOT mode
• ✅ Runtime feature detection using RuntimeFeature.IsDynamicCodeSupported
• ✅ Graceful degradation when reflection isn't available
• ✅ Preserves DynamicallyAccessedMembers annotations for trimming

Performance:

• ✅ AggressiveInlining on hot path methods
• ✅ Early returns for common cases
• ✅ No unnecessary allocations added

---

💡 Minor Suggestions

Documentation:

// Consider updating the justification from "<Pending>" to be more descriptive:
[UnconditionalSuppressMessage("AOT", "IL3050...", 
    Justification = "Reflection usage is guarded by runtime AOT checks via ThrowOnAot()")]

Error Messaging:
The ThrowOnAot error message could be more helpful:

throw new InvalidOperationException(
    $"Cannot cast {value?.GetType()?.Name ?? "null"} to {targetType?.Name} in AOT mode. " +
    "Consider using AotConverterRegistry.Register() for custom type conversions.");

Test Coverage:

• The nuget tester adds a parameterized test, but consider adding specific AOT scenario tests
• Could benefit from tests that verify ThrowOnAot behavior when IsDynamicCodeSupported = false

---

🔍 Technical Review

Security: ✅ No security concerns

• Proper input validation maintained
• No new attack vectors introduced

Thread Safety: ✅ No issues

• Static methods remain thread-safe
• No shared mutable state added

Breaking Changes: ✅ None

• API surface changes are additive (removal of attributes)
• Behavioral changes are improvements (better error handling)

Performance Impact: ✅ Positive

• AggressiveInlining should improve performance
• AOT checks are minimal overhead
• Early path optimization maintained

---

📋 Verification Checklist (per CLAUDE.md)

• ✅ Works in both source-generated and reflection modes
• ✅ Snapshots updated correctly (API verification files)
• ✅ AOT/trimming compatible with proper annotations
• ✅ No performance regression (likely improvement)
• ✅ Thread-safe implementation

Recommendation: Merge after addressing the minor documentation suggestions if desired.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: AOT Compatibility Enhancement in CastHelper

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze main changes in CastHelper.cs
• Review API verification file changes
• Check test modifications
• Evaluate code quality and best practices
• Assess performance considerations
• Check for security concerns
• Verify test coverage adequacy
• Provide comprehensive review feedback

🎯 Overall Assessment: APPROVED with Minor Suggestions

This is a well-executed AOT compatibility improvement that follows TUnit's guidelines and modern .NET practices.

---

✅ Strengths

Code Quality & Best Practices:

• ✅ Follows TUnit's AOT/Trimming compatibility guidelines from CLAUDE.md
• ✅ Uses UnconditionalSuppressMessage appropriately for known-safe reflection
• ✅ Adds AggressiveInlining for performance-critical methods
• ✅ Proper error handling with runtime AOT detection
• ✅ Maintains dual-mode compatibility (source-gen and reflection)

AOT Compatibility:

• ✅ Strategic use of ThrowOnAot() prevents silent failures in AOT mode
• ✅ Runtime feature detection using RuntimeFeature.IsDynamicCodeSupported
• ✅ Graceful degradation when reflection isn't available
• ✅ Preserves DynamicallyAccessedMembers annotations for trimming

Performance:

• ✅ AggressiveInlining on hot path methods
• ✅ Early returns for common cases
• ✅ No unnecessary allocations added

---

💡 Minor Suggestions

Documentation:

// Consider updating the justification from "<Pending>" to be more descriptive:
[UnconditionalSuppressMessage("AOT", "IL3050...", 
    Justification = "Reflection usage is guarded by runtime AOT checks via ThrowOnAot()")]

Error Messaging:
The ThrowOnAot error message could be more helpful:

throw new InvalidOperationException(
    $"Cannot cast {value?.GetType()?.Name ?? "null"} to {targetType?.Name} in AOT mode. " +
    "Consider using AotConverterRegistry.Register() for custom type conversions.");

Test Coverage:

• The nuget tester adds a parameterized test, but consider adding specific AOT scenario tests
• Could benefit from tests that verify ThrowOnAot behavior when IsDynamicCodeSupported = false

---

🔍 Technical Review

Security: ✅ No security concerns

• Proper input validation maintained
• No new attack vectors introduced

Thread Safety: ✅ No issues

• Static methods remain thread-safe
• No shared mutable state added

Breaking Changes: ✅ None

• API surface changes are additive (removal of attributes)
• Behavioral changes are improvements (better error handling)

Performance Impact: ✅ Positive

• AggressiveInlining should improve performance
• AOT checks are minimal overhead
• Early path optimization maintained

---

📋 Verification Checklist (per CLAUDE.md)

• ✅ Works in both source-generated and reflection modes
• ✅ Snapshots updated correctly (API verification files)
• ✅ AOT/trimming compatible with proper annotations
• ✅ No performance regression (likely improvement)
• ✅ Thread-safe implementation

Recommendation: Merge after addressing the minor documentation suggestions if desired.

---