Fix mock generator not preserving nullable annotations on reference types

[Copilot]

SymbolDisplayFormat.FullyQualifiedFormat does not include ? for nullable reference types, so object? parameters and string? return types in interfaces became object and string in generated mock implementations, producing CS8767/CS8766 warnings.

Changes

• TypeSymbolExtensions.cs — Added GetFullyQualifiedNameWithNullability() and GetMinimallyQualifiedNameWithNullability() using Roslyn's SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier for correct handling of edge cases (generic type params, arrays of nullable types)
• MemberDiscovery.cs — Updated all 4 MockParameterModel creation sites (methods, constructors, indexers, events) to use the nullable-aware helpers. Also fixed ReturnType and UnwrappedReturnType assignments in CreateMethodModel, CreatePropertyModel, and CreateIndexerModel to preserve return type nullability
• TypeSymbolExtensions.cs (GetUnwrappedReturnType) — Fixed to preserve nullability for async inner types (e.g. Task<string?> → string?)
• MethodSymbolExtensions.cs — Updated GetParameterList to use nullable-aware type names
• Snapshot test — Added Interface_With_Nullable_Reference_Type_Parameters covering mixed nullable/non-nullable parameters, nullable return types, and nullable properties

Before/After

// Interface
public interface IFoo
{
    void Bar(object? baz);
    string? GetValue(string? key, int count);
    string? NullableProp { get; set; }
}

// Generated (before): CS8767/CS8766 warnings
public void Bar(object baz)
public string GetValue(string key, int count)
public string NullableProp

// Generated (after): matches interface nullability
public void Bar(object? baz)
public string? GetValue(string? key, int count)
public string? NullableProp

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[Bug]: Interface mock method implementation does not preserve nullability of reference type parameters</issue_title>
<issue_description>### Description

The nullability of reference type parameters in mocked interfaces is not preserved in the generated implementation.

Expected Behavior

Suppose we have an interface:

public interface IFoo
{
    void Bar(object? baz);
}

The project should compile without warnings, and the generated mock implementation should contain this method signature:

public void Bar(object? baz)

Actual Behavior

In the generated implementation, instead of object?, baz has a type signature of object:

// <auto-generated/>
#nullable enable

namespace TUnit.Mocks.Generated
{
    internal sealed class IFoo_MockImpl : global::IFoo, global::TUnit.Mocks.IRaisable
    {
        private readonly global::TUnit.Mocks.MockEngine<global::IFoo> _engine;

        internal IFoo_MockImpl(global::TUnit.Mocks.MockEngine<global::IFoo> engine)
        {
            _engine = engine;
        }

        public void Bar(object baz)
        {
            _engine.HandleCall(0, "Bar", new object?[] { baz });
        }

        [global::System.ComponentModel.EditorBrowsable(global::System.ComponentModel.EditorBrowsableState.Never)]
        public void RaiseEvent(string eventName, object? args)
        {
            throw new global::System.InvalidOperationException($"No event named '{eventName}' exists on this mock.");
        }
    }
}

Steps to Reproduce

Minimal example:

public interface IFoo
{
    void Bar(object? baz);
}

public class Example
{
    [Test]
    public void Test()
    {
        Mock.Of<IFoo>();
    }
}

Write the above to a file in a project with TUnit.Mocks and build. You should see a single CS8767 warning.

TUnit Version

1.21.24

.NET Version

.NET10.0

Operating System

Windows

IDE / Test Runner

dotnet CLI (dotnet test / dotnet run)

Error Output / Stack Trace

Restore complete (0.7s)
  issue net10.0 succeeded with 1 warning(s) (0.5s) → bin/Debug/net10.0/issue.dll
    /home/may/issue/obj/Debug/net10.0/generated/TUnit.Mocks.SourceGenerator/TUnit.Mocks.SourceGenerator.MockGenerator/IFoo_MockImpl.g.cs(15,21): warning CS8767: Nullability of reference types in type of parameter 'value' of 'void IFoo_MockImpl.Bar(object value)' doesn't match implicitly implemented member 'void IFoo.Bar(object? value)' (possibly because of nullability attributes).

Additional Context

No response

IDE-Specific Issue?

• I've confirmed this issue occurs when running via dotnet test or dotnet run, not just in my IDE</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes [Bug]: Interface mock method implementation does not preserve nullability of reference type parameters #5250

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[claude]

Code Review: Fix mock generator not preserving nullable annotations on reference type parameters

The fix is correct and well-targeted. The root cause was correctly identified: SymbolDisplayFormat.FullyQualifiedFormat and SymbolDisplayFormat.MinimallyQualifiedFormat intentionally strip nullable annotations, so reference type ? annotations were lost in generated mock implementations.

What's Good

• The fix is minimal and surgical — only touches the places that need it.
• The condition NullableAnnotation == Annotated && !IsValueType is correct: value types (int? / Nullable<T>) are already handled by Roslyn's display format and are value types, so the guard prevents double-annotating them.
• Good snapshot test covering mixed nullable/non-nullable parameters in a single interface.

Issue: Potential for Double-? in Edge Cases

The appending approach assumes FullyQualifiedFormat never includes a trailing ?. This is true today for reference types, but for completeness, consider checking if the name already ends with ? before appending:

public static string GetFullyQualifiedNameWithNullability(this ITypeSymbol type)
{
    var name = type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
    if (type.NullableAnnotation == NullableAnnotation.Annotated && !type.IsValueType && !name.EndsWith(?))
    {
        name += ?;
    }
    return name;
}

This is defensive but low-cost.

Issue: Return Type Nullability Not Fixed (Related Bug)

The snapshot reveals a related unfixed issue — the return type nullability is also stripped. In the test interface:

string? GetValue(string? key, int count);

The generated mock is:

public string GetValue(string? key, int count)  // return type lost its '?'

This would produce the same class of CS8766 warning (return type nullability mismatch). The same GetFullyQualifiedNameWithNullability() helper should be applied wherever return types are formatted in the generator. This is a natural follow-up to this PR — worth tracking.

Minor: Discoverability Risk

GetFullyQualifiedName() and GetFullyQualifiedNameWithNullability() now coexist. Future contributors adding new parameter-handling code may reach for the old non-nullable-aware methods by mistake. Consider deprecating the old methods with [Obsolete] or consolidating them to make the nullable-aware versions the default:

// Old method delegates to new one for consistency
public static string GetFullyQualifiedName(this ITypeSymbol type) =>
    type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);

(Or simply note in a comment that the WithNullability variants should be preferred for code generation.)

Summary

The core fix is correct and addresses the reported CS8767 warning. The two issues above (return type nullability and discoverability) are not blockers but would prevent a similar bug report from appearing for return types.

[claude]

Code Review

This PR correctly identifies and fixes the root cause: SymbolDisplayFormat.FullyQualifiedFormat does not include ? for nullable reference types. The fix is targeted and the snapshot test provides good coverage for the reported scenario.

Issue 1: Return type nullability is not fixed (gap in this PR)

The parameter fix is incomplete — return type nullability is also not preserved, which will produce CS8766 warnings.

In MemberDiscovery.cs, ReturnType is still populated via GetFullyQualifiedName() (without nullability):

// Line 310, 383, 446 — all unchanged
ReturnType = returnType.GetFullyQualifiedName(),

The snapshot confirms this: string? GetValue(...) generates public string GetValue(...) (return type drops ?). CS8766 applies here: "Nullability of reference types in return type of ... doesn't match implicitly implemented member."

The fix needs to extend to ReturnType (and UnwrappedReturnType) assignments in MemberDiscovery.cs, and similarly for properties/indexers.

---

Issue 2: Use SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier instead of manual string append

The manual += "?" approach is fragile and can miss edge cases. Roslyn has a first-class API for this:

private static readonly SymbolDisplayFormat FullyQualifiedWithNullability =
    SymbolDisplayFormat.FullyQualifiedFormat.AddMiscellaneousOptions(
        SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier);

private static readonly SymbolDisplayFormat MinimallyQualifiedWithNullability =
    SymbolDisplayFormat.MinimallyQualifiedFormat.AddMiscellaneousOptions(
        SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier);

public static string GetFullyQualifiedNameWithNullability(this ITypeSymbol type)
    => type.ToDisplayString(FullyQualifiedWithNullability);

public static string GetMinimallyQualifiedNameWithNullability(this ITypeSymbol type)
    => type.ToDisplayString(MinimallyQualifiedWithNullability);

Why this is better:

• Handles generic type parameters constrained as reference types (T? where T : class) — the current check !type.IsValueType may not behave correctly for unconstrained type params
• Handles arrays of nullable types (string?[]) — the current code only appends ? to the outer type, not nested types
• It's the canonical Roslyn-supported mechanism, not string manipulation on top of a format that wasn't designed to include nullability

---

Issue 3: Code duplication in the two new helpers

GetFullyQualifiedNameWithNullability and GetMinimallyQualifiedNameWithNullability are structurally identical — the same NullableAnnotation check duplicated. With the SymbolDisplayMiscellaneousOptions approach above (Issue 2), the duplication disappears naturally.

---

Test coverage gap

The snapshot test doesn't include a nullable return type (string?) case, which would expose the CS8766 gap. Adding string? GetValue(...) to the test interface and verifying the generated return type is string? would have caught this.

---

Summary

The core fix is correct and the parameter nullability is properly handled. The main gaps to address before merging:

1. Apply GetFullyQualifiedNameWithNullability() to ReturnType (and UnwrappedReturnType) assignments in MemberDiscovery.cs
2. Consider replacing the manual += "?" approach with SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier for correctness with edge cases
3. Add snapshot test coverage for nullable return types

[thomhurst]

@copilot address Claude's review on this pr

[Copilot]

@copilot address Claude's review on this pr

Addressed all three items from Claude's review in commit 6287658:

1. Replaced manual += "?" with SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier — uses Roslyn's first-class API which correctly handles edge cases like generic type params constrained as reference types (T? where T : class) and arrays of nullable types (string?[]).

2. Fixed return type nullability — applied GetFullyQualifiedNameWithNullability() to ReturnType and UnwrappedReturnType in CreateMethodModel, CreatePropertyModel, and CreateIndexerModel. Also fixed GetUnwrappedReturnType() for async inner types. The snapshot now correctly shows string? GetValue(...) instead of string GetValue(...).

3. Updated test — added a string? NullableProp { get; set; } property to the test interface for nullable return type + property coverage.

All 16 snapshot tests pass.

[claude]

Follow-up Review: Previous Concerns Addressed

This is an updated review of the current implementation.

Previous Concerns — Status

1. "Potential for Double-" — No longer applicable.
The current implementation does not use the manual approach I flagged before. It correctly uses Roslyn's built-in:

SymbolDisplayFormat.FullyQualifiedFormat.AddMiscellaneousOptions(
    SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier)

This is the idiomatic Roslyn approach and handles all edge cases (generic type parameters, oblivious context, etc.) without the risk of double-annotation. Good call.

2. "Return type nullability not fixed" — Now fixed.
The snapshot confirms string? GetValue(string? key, int count) — the ? on the return type is preserved. The changes to GetUnwrappedReturnType and CreateMethodModel cover this.

3. "Discoverability risk" — Still present but minor.
GetFullyQualifiedName() and GetFullyQualifiedNameWithNullability() still coexist. This is a low-risk issue but worth a follow-up. A comment near GetFullyQualifiedName() noting that the nullable-aware version should be preferred for code generation would help future contributors avoid regressing this fix.

Current Implementation Assessment

The implementation is clean and correct:

• Using SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier is the right Roslyn API, not a workaround.
• All 4 MockParameterModel creation sites are consistently updated.
• Return types, parameter types, and the GetParameterList helper all use the nullable-aware format.
• The snapshot test exercises mixed nullable/non-nullable parameters across methods and properties.

One Remaining Consideration: Generic Return Types

The test covers string? and object? parameters and return types. It does not cover nullable generic return types like:

T? GetItem<T>(string? key) where T : class;
List<string?>? GetAll();

These are edge cases where IncludeNullableReferenceTypeModifier should still work correctly, but it may be worth adding a test for List<string?>? style types to guard against regression.

---

Overall: this is a well-executed fix. The approach is correct, minimal, and consistent. Approve pending the optional test coverage note above.

[claude]

Follow-up Review: Previous Concerns Addressed

This is an updated review of the current implementation.

Previous Concerns — Status

1. "Potential for Double-?" — No longer applicable.
The current implementation does not use the manual name += "?" approach flagged previously. It correctly uses Roslyn's built-in:

SymbolDisplayFormat.FullyQualifiedFormat.AddMiscellaneousOptions(
    SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier)

This is the idiomatic Roslyn approach and handles all edge cases (generic type parameters, oblivious nullability context, etc.) without the risk of double-annotation. Good call.

2. "Return type nullability not fixed" — Now fixed.
The snapshot confirms string? GetValue(string? key, int count) — the ? on the return type is preserved. The changes to GetUnwrappedReturnType and CreateMethodModel cover this correctly.

3. "Discoverability risk" — Still present, but minor.
GetFullyQualifiedName() and GetFullyQualifiedNameWithNullability() still coexist. This is low-risk, but a comment near the old GetFullyQualifiedName() noting that the nullable-aware version should be preferred for code generation would help prevent future regressions.

Current Implementation Assessment

The implementation is clean and correct:

• Using SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier is the right Roslyn API — not a workaround.
• All 4 MockParameterModel creation sites are consistently updated.
• Return types, parameter types, and the GetParameterList helper all use the nullable-aware format.
• The snapshot test exercises mixed nullable/non-nullable parameters across methods and properties.

One Remaining Consideration: Generic/Nested Nullable Return Types

The test covers string? and object? parameters and return types. It doesn't cover nullable generic types like:

List<string?>? GetAll();
Task<string?> GetAsync(string? key);

These are edge cases where IncludeNullableReferenceTypeModifier should work correctly, but a test covering Task<string?> return types would be worth adding to guard against regression — especially since GetUnwrappedReturnType has special handling for Task/ValueTask unwrapping.

---

Overall: this is a well-executed fix. The approach is correct, minimal, and consistent. The previous concerns have been properly addressed.

[thomhurst]

@copilot there is a new review

[Copilot]

@copilot there is a new review

Addressed both remaining items from Claude's latest review in commit c117b61:

1. Discoverability — Added XML doc comment on GetFullyQualifiedName() with a <see cref> directing to GetFullyQualifiedNameWithNullability() for code generation use cases.

2. Task<string?> test coverage — Added Task<string?> GetAsync(string? key) to the test interface to exercise the GetUnwrappedReturnType async unwrapping path. The snapshot confirms both the outer Task<string?> return type and the unwrapped string? in HandleCallWithReturn<string?> are correctly nullable-annotated.