fix(mocks): preserve nullability on event handler types (#5425)

[thomhurst]

Summary

• Fixes [Bug]: Nullability of event args is ignored in generated mocks #5425. CreateEventModel was using GetFullyQualifiedName() for the event delegate type, which strips nullable reference annotations. This caused generated mocks to emit EventHandler<string> for an EventHandler<string?> source event, producing CS8604 because the Raise_* method (built from a separate, nullability-aware path) correctly used string?.
• Switched to GetFullyQualifiedNameWithNullability() so the wrapper, impl, and bridge builders all see the annotated type.

Test plan

• Added Interface_With_Nullable_Event_Args snapshot test reproducing the issue scenario
• All *Event* mock generator tests pass

[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]

Code Review

This PR fixes a real nullability bug (#5425) where CreateEventModel was using GetFullyQualifiedName() (which strips nullable annotations) for the event handler type, causing the generated wrapper class to emit EventHandler<string> for a source EventHandler<string?> event, producing CS8604. The fix switches to GetFullyQualifiedNameWithNullability() for the top-level event type, which is the correct call.

What the fix does

CreateEventModel in MemberDiscovery.cs now calls evt.Type.GetFullyQualifiedNameWithNullability() to populate MockEventModel.EventHandlerType. This value flows into:

• MockWrapperTypeBuilder — explicit interface event forwarding
• MockImplBuilder — backing field and event declaration (EventHandlerTypeNonNullable + trailing ?)

The RaiseParameterList path already used GetFullyQualifiedNameWithNullability() on parameter types, so raise-method signatures were already correct. Only the top-level event handler type was broken.

Observations and minor issues

EventArgsType field appears to be dead code. MockEventModel has a property EventArgsType (populated from eventArgsType in CreateEventModel), and the builder comment in the model file says "use this instead of parsing RaiseParameters". However, after searching all eight builder files, EventArgsType is not consumed anywhere. The legacy raiseParameters string local and eventArgsType variable inside CreateEventModel still call GetFullyQualifiedName() (without nullability), but since nothing reads EventArgsType from the builders, this is harmless in practice. It would be cleaner to either remove the field or migrate those internal GetFullyQualifiedName() calls to GetFullyQualifiedNameWithNullability() for consistency and future safety.

#pragma warning error in Issue5425Tests.cs is valid (introduced in C# 9), and using it as a compile-time regression guard is a good technique. The file explicitly explains this intent in comments.

Snapshot test for Interface_With_Nullable_Event_Args: The backing field in the impl is emitted as private global::System.EventHandler<string?>? _backing_Something, with the trailing ? appended by EventHandlerTypeNonNullable + ?. For a non-nullable delegate like EventHandler<string?>, this results in a nullable backing field (always ?), which is the correct and intentional pattern for internal backing fields.

The two new snapshot files (Interface_With_Nullable_Event_Args and Interface_With_Nullable_Event_And_Nullable_Args) are identical except for the wrapper type's explicit interface event declaration (one emits EventHandler<string?>, the other EventHandler<string?>?). This is the exact difference expected and correctly verified.

Suggestion

The eventArgsType local variable in CreateEventModel (around line 679) still uses GetFullyQualifiedName():

eventArgsType = argsParams.Length == 1
    ? argsParams[0].Type.GetFullyQualifiedName()  // <-- strips nullable annotation
    : raiseParameters;

Likewise the raiseParameters local uses GetFullyQualifiedName() for parameter types. Since EventArgsType is currently dead in the builders, this is not a regression risk today. But if EventArgsType is ever wired up it could reintroduce the same category of bug. It is worth either removing EventArgsType from the model or switching those calls to GetFullyQualifiedNameWithNullability() proactively.

Summary

The core fix is correct and minimal. Tests are well-structured with both snapshot regression coverage and compile-time enforcement via #pragma warning error. The PR addresses the root cause cleanly. The only housekeeping item worth noting is the apparently unused EventArgsType field in MockEventModel, which could be removed or cleaned up in a follow-up.