fix(mocks): preserve nullable annotations on generated event implementations

[thomhurst]

Summary

• Fixes [Bug]: CS8615 for generated nullable event mocks #5424: generated mocks emitted CS8615 when an interface declared an event as nullable (e.g. event EventHandler<string>? Something), because the explicit interface event implementation dropped the ? annotation.
• Added EventHandlerTypeWithNullability to MockEventModel and use it from MockWrapperTypeBuilder and MockBridgeBuilder when emitting explicit interface event implementations. The existing EventHandlerType field is retained because MockImplBuilder uses it as the base for the always-nullable backing delegate field (appending ?) — using the nullability-preserving form there would produce ??.

Test plan

• New regression test Interface_With_Nullable_Event in TUnit.Mocks.SourceGenerator.Tests with verified snapshot
• Full TUnit.Mocks.SourceGenerator.Tests suite passes (33/33)

[claude]

Code Review

This is a clean, targeted fix for CS8615 (nullability mismatch on explicit interface event implementations). The approach is sound and the regression test + snapshot coverage is good. A few observations below.

---

Design observation: two fields for the same type with different purposes

The PR introduces EventHandlerTypeWithNullability alongside the existing EventHandlerType, with the distinction being:

• EventHandlerType = no nullable annotation (used in MockImplBuilder because it appends ? itself)
• EventHandlerTypeWithNullability = nullable annotation preserved (used in MockBridgeBuilder and MockWrapperTypeBuilder for explicit interface implementations)

The PR description explains the reason: MockImplBuilder appends ? explicitly for the backing field/event declaration, and using EventHandlerTypeWithNullability directly there would produce ?? for nullable events.

This is a pragmatic split that works, but it introduces a subtle semantic trap: two callers that look nearly identical but must use different fields. If someone adds a new builder in the future that emits an explicit interface event, they might naturally reach for EventHandlerType (the first-declared, "primary" field) and silently reintroduce CS8615.

Suggestion: Make the intent encode-safe by inverting which field is the "primary" one, or by renaming to make the distinction obvious at the call site:

// Option A – rename to be explicit about context
public string EventHandlerTypeForBackingField { get; init; } = "";   // always without `?` (caller appends it)
public string EventHandlerTypeForInterfaceImpl { get; init; } = "";  // preserves source nullability

// Option B – keep a single WithNullability field and strip in MockImplBuilder
// Since GetFullyQualifiedName() strips the `?`, and GetFullyQualifiedNameWithNullability()
// keeps it, you could expose only the nullability-preserving form and have MockImplBuilder
// call TrimEnd('?') when it needs the non-annotated base:
writer.AppendLine($"private {evt.EventHandlerType.TrimEnd('?')}? _backing_{evt.Name};");

Option B would let you remove the EventHandlerType field entirely — one source of truth instead of two — and the intent becomes self-documenting. The TrimEnd('?') is safe because GetFullyQualifiedName() currently produces the identical result (without ?), so it is strictly equivalent.

---

Minor: Equals / GetHashCode redundancy now that both fields usually differ only by ?

For a non-nullable event (e.g. event Action Foo), EventHandlerType and EventHandlerTypeWithNullability will be identical strings. Including both in GetHashCode adds unnecessary hash churn without any real collision-avoidance benefit. If Option B above is adopted (single field), this goes away naturally.

---

Snapshot file: trailing newline

Interface_With_Nullable_Event.verified.txt ends without a trailing newline (\ No newline at end of file). That's consistent with the diff output, but worth confirming this matches what the snapshot test framework expects — some snapshot frameworks are sensitive to trailing newlines across platforms.

---

Overall

The fix is correct and well-motivated. The regression test exercising the exact CS8615 scenario is exactly the right approach. The main actionable improvement is simplifying the model to a single field (Option B above), which would make it harder to introduce the same class of bug in future builders.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

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

[claude]

Follow-up Review

The previous review raised three concerns. Here's how each was addressed:

✅ Previous concern: two stored fields for the same type with different nullability semantics

The suggestion was to adopt Option B — make EventHandlerType the single source of truth (with nullability preserved) and have MockImplBuilder derive the non-nullable form. This has been implemented cleanly:

• MemberDiscovery.cs now calls GetFullyQualifiedNameWithNullability(), so EventHandlerType always carries the source nullability annotation.
• EventHandlerTypeNonNullable is a computed property (EventHandlerType.TrimEnd('?')), not a stored field — single source of truth, derived form only where needed.
• MockImplBuilder now uses EventHandlerTypeNonNullable when appending ? for the backing field and event declaration.

This is exactly what was asked for and is better than the earlier approach: any future builder that emits an explicit interface event will naturally reach for EventHandlerType and get correct nullability for free.

One small note: the PR description mentions an EventHandlerTypeWithNullability property that doesn't appear in the actual diff — looks like the description reflects an earlier iteration. The final implementation (changing the semantics of the existing EventHandlerType) is simpler and preferred.

✅ Previous concern: Equals/GetHashCode churn from two near-identical fields

Resolved naturally — EventHandlerTypeNonNullable is a computed property and not part of the record's auto-generated equality, so there is no redundancy.

⚠️ Previous concern: snapshot file trailing newline — unchanged

Interface_With_Nullable_Event.verified.txt still ends without a trailing newline. This is a low-priority observation; if the existing snapshot tests pass consistently on CI it's fine to leave as-is.

---

Overall

All substantive concerns from the previous review have been addressed. The fix is correct, the model is now cleanly structured with a single stored field, and the regression test + snapshot provide good coverage of the exact failure mode. No blocking issues.