Fix missing default constraint on explicit interface implementations with unconstrained generics

[thomhurst]

Summary

• Fixes CS0453/CS0539 compilation errors when TUnit.Mocks generates explicit interface implementations for methods with unconstrained generic type parameters using nullable annotations (T?)
• Detects unconstrained type parameters with nullable usage and emits where T : default on explicit interface implementations (MockWrapperTypeBuilder, MockBridgeBuilder)
• Extracts shared IsUnconstrained helper to avoid duplicating constraint checks between GetGenericConstraints and the new detection logic

Closes #5362

Test plan

• Added snapshot test Interface_With_Unconstrained_Nullable_Generic verifying where T : default is emitted for both Task<T?> and T? return types
• All 25 existing snapshot tests continue to pass (no regressions)

[codacy-production]

Not up to standards ⛔

🔴 Issues 5 minor

Alerts:
⚠ 5 issues (≤ 0 issues of at least minor severity)

Results:
5 new issues

Category	Results
CodeStyle	5 minor

View in Codacy

🟢 Metrics 16 complexity

Metric	Results
Complexity	16

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

This PR fixes a genuine C# compiler error (CS0453/CS0539) that occurs when a source-generated explicit interface implementation has an unconstrained generic type parameter used with nullable annotations (T?). The fix is correct and the approach is reasonable. Below are some observations and suggestions.

---

What the PR Does Well

• Correctly identifies the root cause: where T : default is required on explicit interface implementations when T? is used, because C# needs to know whether T? means nullable-value-type or nullable-reference-type.
• The IsUnconstrained() helper is a clean private extraction that avoids repeating the same six-condition check in both GetGenericConstraints and the new IsUnconstrainedWithNullableUsage.
• The recursive HasNullableTypeParameter correctly handles nested generics (Task<T?>) and arrays.
• Equality and hash code in MockTypeParameterModel are updated consistently.
• Snapshot test covers both the Task<T?> and T? cases.

---

Concerns and Suggestions

1. NeedsDefaultConstraint is context-sensitive and stored on a model meant to be context-free

MockTypeParameterModel is a serializable, equatable model snapshot of a type parameter. NeedsDefaultConstraint is not really a property of the type parameter itself — it is a derived consequence of how that type parameter is used in a specific method signature. Storing it on the model is workable but introduces a subtle conceptual mismatch.

A cleaner alternative would be to compute the where T : default clause lazily in FormatConstraintClauses by inspecting the method's full signature at the model level, or to encode the information as HasAnnotatedNullableUsage to be more explicit about what the flag actually means. The current name NeedsDefaultConstraint is already quite context-specific (it only "needs" it in explicit implementations), which is why the forExplicitImplementation flag in FormatConstraintClauses has to gate it.

Consider renaming to HasAnnotatedNullableUsage to better reflect what was detected vs. what action to take. The action of emitting where T : default is then a decision made by the code generator based on context.

2. IsUnconstrainedWithNullableUsage only checks the method symbol — properties are not covered

The NeedsDefaultConstraint flag is populated only from IMethodSymbol in MemberDiscovery.cs:

method.TypeParameters.Select(tp => new MockTypeParameterModel
{
    Name = tp.Name,
    Constraints = tp.GetGenericConstraints(),
    NeedsDefaultConstraint = tp.IsUnconstrainedWithNullableUsage(method)
})

C# interfaces can have generic indexers (though rare). More practically, the property discovery path at lines ~563 and ~628 in MemberDiscovery.cs creates MockTypeParameterModel entries but would call IsUnconstrainedWithNullableUsage with a method symbol that is a property accessor, not the original method. If this path does not go through IsUnconstrainedWithNullableUsage at all for properties, that's fine (properties can't have their own type parameters in C#), but it's worth confirming that the property discovery path never produces a non-empty TypeParameters array, so the new flag is unreachable from that code path and cannot produce incorrect output.

3. HasNullableTypeParameter does not recurse into tuple elements or pointer types

The recursive traversal handles INamedTypeSymbol (for generic type arguments) and IArrayTypeSymbol (for element types), but skips:

• ITupleTypeSymbol — tuples with nullable type parameter elements: (T?, string)
• IPointerTypeSymbol — unsafe pointer types

For IPointerTypeSymbol this is likely not a concern in practice (pointers can't be nullable reference types). For tuples, (T?, string) in an interface method would trigger the same CS0453 and is not currently detected. This is an edge case but worth documenting or adding a // TODO comment so future maintainers know the scope of coverage.

4. The forExplicitImplementation boolean flag could become an enum for clarity

Currently, GetConstraintClauses takes a bool forExplicitImplementation = false parameter, and the interpretation "emit where T : default when this is an explicit implementation" is implicit. As the generator gains more call sites or more nuanced constraint requirements, a boolean flag tends to accumulate more flags alongside it. An enum like ConstraintContext { RegularImpl, ExplicitImpl } would make the intent clear at each call site without adding boolean proliferation.

This is a minor readability point, not a blocking concern.

5. Early-exit refactoring in GetGenericConstraints slightly changes observable behavior (benign)

Before this PR, GetGenericConstraints would return "" for an unconstrained type parameter via the final constraints.Count > 0 ? ... : "" guard. After the PR, it returns early via if (typeParam.IsUnconstrained()) return "";. The end result is the same, but the refactor also silently dropped the constraints.Count > 0 check from the final return statement (changed to return string.Join(", ", constraints);). This is still correct because the early-exit handles the empty case, but it is worth noting as a subtle structural change.

---

Summary

The fix is correct and solves a real compiler error. The main thing to consider is whether NeedsDefaultConstraint on MockTypeParameterModel is the right abstraction boundary (renaming it to HasAnnotatedNullableUsage would make it clearer), and whether the tuple recursion gap in HasNullableTypeParameter is worth a comment. Neither concern is blocking for the stated goal.

[thomhurst]

Addressed review feedback:

Item 1 (Rename): Renamed NeedsDefaultConstraint → HasAnnotatedNullableUsage across model, discovery, and builder. The model now describes what was detected; the builder decides whether to emit where T : default based on context.

Item 2 (Properties): No action needed — C# properties cannot have their own type parameters, so TypeParameters is always empty on property models.

Item 3 (Tuples): No action needed — Roslyn represents tuples as INamedTypeSymbol (with IsTupleType == true). There is no ITupleTypeSymbol interface. The existing INamedTypeSymbol branch already recurses into TypeArguments, so (T?, string) is handled.

Item 4 (Bool → enum): Skipping — single boolean with one call-site distinction doesn't warrant an enum yet.

Item 5 (Early-exit): Acknowledged as benign. The IsUnconstrained() early-exit handles the empty case, making the final constraints.Count > 0 ternary unnecessary.

[claude]

Follow-up Review

The author has already incorporated the NeedsDefaultConstraint → HasAnnotatedNullableUsage rename from the previous review round — thank you, that is a clear improvement. The remaining observations from the first pass are still applicable:

---

Still open: tuple elements not covered by HasNullableTypeParameter

HasNullableTypeParameter recurses into INamedTypeSymbol type arguments and IArrayTypeSymbol element types, but not into ITupleTypeSymbol elements. A method like:

(T?, string) GetPair<T>();

would not trigger the where T : default emission, which would still cause CS0453 when the explicit interface implementation is generated. This is a genuine gap. Even if tuples with unconstrained nullable generic elements are rare in practice, it is worth either:

1. Adding a recursive branch for ITupleTypeSymbol in HasNullableTypeParameter, or
2. Adding a // Note: tuple element types are not inspected comment so future maintainers understand the scope.

---

Minor: forExplicitImplementation boolean could become an enum over time

The bool forExplicitImplementation = false parameter on GetConstraintClauses / FormatConstraintClauses is fine for now, but if additional constraint contexts emerge (e.g., override forwarding methods that need special treatment), a named enum like ConstraintContext { RegularImpl, ExplicitImpl } would scale better. Not blocking, just something to keep in mind.

---

Confirmed correct behaviour

• where T : default is correctly emitted on explicit interface implementations in both MockWrapperTypeBuilder and MockBridgeBuilder.
• The recursive HasNullableTypeParameter correctly handles Task<T?> and other nested generic cases.
• MockTypeParameterModel equality/hash-code are updated consistently.
• The snapshot test covers the two primary cases (Task<T?> return and T? return).
• The early-return refactor in GetGenericConstraints (dropping the trailing constraints.Count > 0 guard) is safe because the unconstrained early-exit handles that case.

The fix is correct and solves a real compiler error. The tuple gap is the only remaining functional concern worth addressing before merge.

[thomhurst]

Re: tuple elements — ITupleTypeSymbol does not exist in the Roslyn API. C# tuples are represented as INamedTypeSymbol (specifically System.ValueTuple<...>), so the existing INamedTypeSymbol branch already recurses into their TypeArguments.

Added a (T?, string) GetPair<T>() case to the snapshot test to prove it. The generated output correctly emits where T : default:

(T?, string) global::IFoo.GetPair<T>() where T : default => Object.GetPair<T>();

No additional handling needed.

[claude]

Follow-up Review (third pass)

This pass covers the three commits since the last review round.

---

Commit 075c64ed: Rename NeedsDefaultConstraint → HasAnnotatedNullableUsage

This is the rename suggested in the first review. It is a clear improvement — the name now describes what was detected (an annotated nullable usage) rather than prescribing a downstream action. No concerns here.

---

Commit d8ceeae3: Add tuple case to test + confirm Roslyn behaviour

The prior review flagged a potential gap: (T?, string) return types might not trigger where T : default because HasNullableTypeParameter had no branch for ITupleTypeSymbol. The commit message explains (correctly) that this gap does not exist:

Roslyn represents tuples as ValueTuple<T?, string> which is an INamedTypeSymbol with TypeArguments. There is no ITupleTypeSymbol interface in Roslyn.

The snapshot confirms where T : default is emitted for (T?, string) GetPair<T>(). The concern raised in the earlier reviews was therefore a false alarm — the INamedTypeSymbol branch already covers tuple element types.

The snapshot also confirms all three explicit interface implementations receive where T : default:

global::System.Threading.Tasks.Task<T?> global::IFoo.DoSomethingAsync<T>() where T : default
T? global::IFoo.GetValue<T>() where T : default
(T?, string) global::IFoo.GetPair<T>() where T : default

---

Remaining (now minor) observations

1. IPointerTypeSymbol not recursed — still not a concern

Pointer types cannot be nullable reference types, so T? in a pointer context is not valid C#. The omission is correct by language rules, but a one-line comment would help future readers:

// Pointer types cannot carry nullable annotations on type parameters, so no branch needed.

2. forExplicitImplementation boolean — still a readability nit, not blocking

The bool forExplicitImplementation = false parameter is now used in two call sites (MockWrapperTypeBuilder and MockBridgeBuilder). If a third constraint context ever arises, consider an enum. Not blocking.

---

Overall assessment

All substantive concerns from the previous review rounds have been addressed:

• HasAnnotatedNullableUsage is a better name than NeedsDefaultConstraint.
• The tuple recursion gap was a false alarm; Roslyn uses INamedTypeSymbol for tuples, and the test now proves it.
• The IsUnconstrained() helper deduplication is clean.
• Equality and hash-code on MockTypeParameterModel are consistent.
• The early-return refactor in GetGenericConstraints is safe.

This PR is correct, fixes a real compiler error (CS0453/CS0539), and has adequate test coverage. Approving.

[claude]

All substantive concerns from previous reviews have been addressed. The tuple recursion gap was a false alarm (Roslyn represents tuples as ValueTuple INamedTypeSymbol), the rename to HasAnnotatedNullableUsage is a clear improvement, and the snapshot now proves correct where T : default emission for Task<T?>, T?, and (T?, string) return types. Approving.