Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 4, 2025

The AotConverterGenerator was attempting to generate AOT converters for generic implicit/explicit operators with unbound type parameters, which resulted in invalid C# code generation and compilation errors.

Problem

When using generic types with implicit operators like:

public record Foo<T>(T Value)
{
    public static implicit operator Foo<T>(T value) => new(value);
}

public class Tests
{
    [Test]
    [MethodDataSource(nameof(Data))]
    public void Test1(Foo<int> data) { }

    public static IEnumerable<Foo<int>> Data() => [new(1)];
}

The source generator would produce invalid code in AotConverters.g.cs:

public Type SourceType => typeof(T);  // CS0246: 'T' could not be found
public Type TargetType => typeof(global::Foo<>);  // CS7003: Unexpected use of unbound generic name
return (global::Foo<>)typedValue;  // CS7003: Unexpected use of unbound generic name

Solution

Added a check in the GetConversionInfo method to skip conversion operators where either the source type or target type contains unbound type parameters. Generic operators with type parameters cannot be properly represented in AOT converters at runtime since the concrete types are not known at compile time.

The fix uses the existing IsGenericDefinition() extension method to detect types with unbound type parameters:

// Skip conversion operators with unbound generic type parameters
// These cannot be properly represented in AOT converters at runtime
if (sourceType.IsGenericDefinition() || targetType.IsGenericDefinition())
{
    return null;
}

This preserves functionality for concrete type conversions while preventing the generation of invalid code for generic operators.

Fixes #2757.


💡 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.

Copilot AI changed the title [WIP] Generic data types with implicit operators don't compile in 0.50.0 Fix generic implicit operators compilation error in AotConverterGenerator Aug 4, 2025
Copilot AI requested a review from thomhurst August 4, 2025 20:16
Copilot finished work on behalf of thomhurst August 4, 2025 20:16
@thomhurst
Copy link
Owner

@copilot Will this still generate a runnable test?

@thomhurst thomhurst marked this pull request as ready for review August 4, 2025 20:48
@thomhurst
Copy link
Owner

@claude review this

@claude
Copy link
Contributor

claude bot commented Aug 4, 2025

Claude finished @thomhurst's task —— View job


Code Review

Tasks:

  • Read repository guidelines (CLAUDE.md)
  • Examine the AotConverterGenerator.cs changes
  • Analyze the fix for correctness and best practices
  • Answer whether tests will still be runnable
  • Provide comprehensive review feedback

Review Summary ✅

This is a well-implemented fix that correctly addresses the compilation error described in issue #2757.

Analysis

The Problem:
The AotConverterGenerator was attempting to generate AOT converters for generic implicit/explicit operators with unbound type parameters, producing invalid C# code like:

public Type SourceType => typeof(T);  // CS0246: 'T' could not be found  
return (global::Foo<>)typedValue;     // CS7003: Unexpected use of unbound generic name

The Solution:
The fix adds a targeted check in GetConversionInfo at AotConverterGenerator.cs:71-74:

// Skip conversion operators with unbound generic type parameters
// These cannot be properly represented in AOT converters at runtime
if (sourceType.IsGenericDefinition() || targetType.IsGenericDefinition())
{
    return null;
}

Code Quality Assessment

✅ Strengths:

  1. Correct Logic: Uses the existing IsGenericDefinition() extension method (TypeExtensions.cs:230-253) which properly detects:

    • Type parameters (ITypeParameterSymbol)
    • Unbound generic types
    • Generic types with type parameter arguments
  2. Appropriate Placement: Check is positioned early in GetConversionInfo() after basic validation but before creating the ConversionInfo object

  3. Good Documentation: Clear comments explain why generic operators are skipped

  4. Minimal Impact: Returns null to skip problematic operators without affecting the overall generation process

  5. Preserves Functionality: Concrete type conversions continue to work normally

✅ Security & Best Practices:

  • No security concerns - purely defensive filtering
  • Follows existing code patterns and conventions
  • Uses established extension methods from the codebase

Answering Your Question: "Will this still generate a runnable test?"

Yes, tests will still be runnable. Here's why:

  1. AOT converters are optional optimizations - they're used by the test framework for efficient type conversion in parameterized tests, but tests don't depend on them for basic functionality

  2. The fix only skips problematic generic operators - concrete type conversions and other valid operators still generate AOT converters normally

  3. Fallback mechanisms exist - When AOT converters aren't available, the test framework can fall back to reflection-based conversion or other mechanisms

  4. Test execution is preserved - For the example in the issue:

    [Test]
    [MethodDataSource(nameof(Data))]
    public void Test1(Foo<int> data) { }  // This will still run
    
    public static IEnumerable<Foo<int>> Data() => [new(1)];  // Concrete Foo<int> works fine

Minor Suggestions

Consider for future enhancement (not blocking for this PR):

  • Could add logging/diagnostics when skipping generic operators to help developers understand why certain conversions aren't AOT-optimized
  • The generated comment when no conversions are found could be more descriptive

Verdict: APPROVED

This fix correctly solves the compilation issue while maintaining test functionality. The implementation is clean, well-documented, and follows established patterns in the codebase.


@thomhurst thomhurst merged commit b7d960c into main Aug 4, 2025
2 checks passed
@thomhurst thomhurst deleted the copilot/fix-2757 branch August 4, 2025 20:53
Copy link
Contributor Author

Copilot AI commented Aug 4, 2025

@copilot Will this still generate a runnable test?

Yes, this will still generate a runnable test. The fix only prevents generation of invalid AOT converters for generic operators with unbound type parameters, but TUnit has robust fallback mechanisms in CastHelper.cs that handle type conversion at runtime.

When an AOT converter isn't available, the framework falls back to reflection-based conversion using GetConversionMethod(), which successfully finds and invokes the implicit operators. I verified this works by testing the exact scenario from the issue - tests with generic types and implicit operators compile and would run successfully.

The fix is surgical and preserves all functionality while preventing the compilation errors from invalid generated code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generic data types with implicit operators don't compile in 0.50.0

2 participants