fix(mocks): unique __argArray locals per event in RaiseEvent dispatch (#5423)

[thomhurst]

Summary

• Fixes [Bug]: CS0128 when interface has more than one event #5423: generated RaiseEvent switch declared __argArray once per case in the same scope, producing CS0128/CS0165 whenever an interface exposed more than one multi-parameter event (e.g. an interface inheriting both INotifyPropertyChanging and INotifyPropertyChanged).
• Suffix the pattern variable with the event name (__argArray_<EventName>) so each case branch has its own local.

Test plan

• Added snapshot test MockGeneratorTests.Interface_With_Multiple_Multi_Parameter_Events covering two multi-parameter delegate events on a single interface.
• Added runtime regression EventTests.Mock_With_Multiple_Multi_Parameter_Events_Compiles_And_Raises using the exact INotifyPropertyChanging + INotifyPropertyChanged repro from the issue.
• Both tests pass locally on net10.0.

[claude]

Code Review

Summary

This is a clean, minimal fix for a real compiler error (CS0128/CS0165) that occurred when a mocked interface had more than one multi-parameter event. The root cause is that C# switch branches share scope unless explicitly wrapped in braces, so declaring if (args is object?[] __argArray) in two separate cases produced a duplicate variable error.

---

The Fix — Correct, But There's a More Idiomatic Alternative

Current approach (naming the variable per event):

case "First":
    if (args is object?[] __argArray_First) { ... }
    break;
case "Second":
    if (args is object?[] __argArray_Second) { ... }
    break;

This works. However, a more idiomatic C# solution is to give each case its own braced block {}, which creates a new scope and lets all cases reuse the same clean name:

case "First":
{
    if (args is object?[] __argArray) { ... }
    break;
}
case "Second":
{
    if (args is object?[] __argArray) { ... }
    break;
}

Why this is preferable:

• It's the standard C# idiom for scoping locals inside switch cases — readers immediately recognise the intent.
• Variable names stay short and uniform (__argArray vs __argArray_PropertyChanging, __argArray_PropertyChanged, etc. for deeply inherited interfaces).
• Eliminates any theoretical risk of the event name containing characters that are valid in an event identifier but awkward inside a generated local variable name (not a current problem since C# event names are also valid identifiers, but defensive).
• The braced-block pattern generalises better if more locals are ever added inside a case.

The current fix is not wrong — it just encodes naming information in the variable rather than using scoping. Either approach is acceptable; the braced-block version is worth considering for a follow-up.

---

Test Coverage — Well Done

• Snapshot test (): correctly exercises two distinct multi-parameter delegate types on a single interface, and the file is committed (not ). The snapshot output clearly shows / in the generated switch.
• Runtime regression (): uses the exact repro from the issue ( + ), verifies both events dispatch independently, and subscribes/raises/asserts in one readable test. This is exactly the right level of coverage for a regression fix.

---

Minor Observation

The interface definition placed in :

public interface IMultiEventNotifier : INotifyPropertyChanging, INotifyPropertyChanged { }

is a top-level public type in a test file. Consider making it internal or nesting it inside the test class — it's not part of the public API and the public visibility may leak into the assembly's public surface unnecessarily. Minor point, not a blocker.

---

Verdict

The bug is real, the fix is correct, and the test coverage is solid. The braced-block approach is worth considering as a follow-up refactor for cleaner generated code, but this PR is good to merge as-is.

[claude]

Code Review

Summary

This is a clean, minimal fix for a real compiler error (CS0128/CS0165) that occurred when a mocked interface had more than one multi-parameter event. The root cause is that C# switch case branches share scope unless explicitly wrapped in braces, so declaring if (args is object?[] __argArray) in two separate cases produced a duplicate variable error.

---

The Fix — Correct, But There Is a More Idiomatic Alternative

The current approach names the pattern variable per event (__argArray_First, __argArray_Second). This works, but a more idiomatic C# solution is to give each case its own braced block {}, which creates a new scope and allows all cases to reuse the same clean name __argArray.

Why the braced-block approach is worth considering:

• It is the standard C# idiom for scoping locals inside switch cases.
• Variable names stay short and uniform — for deeply inherited interfaces the suffixed names can get verbose (e.g. __argArray_PropertyChanging, __argArray_PropertyChanged).
• Generalises better if more locals are added inside a case in the future.

The current fix is not wrong — it is a valid minimal change. The braced-block alternative is worth considering as a follow-up refactor in MockImplBuilder.cs.

---

Test Coverage — Well Done

• Snapshot test (Interface_With_Multiple_Multi_Parameter_Events): exercises two distinct multi-parameter delegate types on a single interface. The .verified.txt file is correctly committed (not .received.txt), and the snapshot clearly shows the fixed variable names in the generated switch.
• Runtime regression (Mock_With_Multiple_Multi_Parameter_Events_Compiles_And_Raises): uses the exact repro from the issue (INotifyPropertyChanging + INotifyPropertyChanged), verifies both events dispatch independently, and is written in a clear subscribe/raise/assert style. Exactly the right coverage for a regression fix.

---

Minor Observation

The interface definition added in EventTests.cs:

public interface IMultiEventNotifier : INotifyPropertyChanging, INotifyPropertyChanged { }

is a top-level public type inside a test file. Consider making it internal — it is not part of the library's public API and the public modifier leaks it into the assembly's public surface unnecessarily. Minor point, not a blocker.

---

Verdict

The bug is real, the fix is correct, and the test coverage is solid. The braced-block approach is worth a follow-up for cleaner generated code, but this PR is good to merge as-is.

[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

Previous Feedback Addressed

The main suggestion from the prior reviews — using braced blocks per case instead of event-name-suffixed variable names — has been implemented in commit 6e9eb115. The generated RaiseEvent switch now looks like:

case "First":
{
    if (args is object?[] __argArray) { ... }
    break;
}
case "Second":
{
    if (args is object?[] __argArray) { ... }
    break;
}

This is the idiomatic C# approach and is clearly the better outcome. Good follow-through on the review feedback.

---

One Item Still Open

The IMultiEventNotifier interface in EventTests.cs remains public:

public interface IMultiEventNotifier : INotifyPropertyChanging, INotifyPropertyChanged { }

The prior review flagged this as a minor point — it is a test-only type leaking into the assembly's public surface. Changing it to internal would be cleaner, though the mock generator requires the type to be visible enough that it can be referenced by the generated extension types. Worth confirming whether internal is sufficient here (it likely is, since the generated code lives in the same assembly in tests), but this is not a blocker.

---

Stale PR Description

The PR description still says "Suffix the pattern variable with the event name" but the implementation now uses braced scopes instead. Minor, but worth updating the description to reflect the actual implementation.

---

Verdict

Both commits together produce a correct, idiomatic fix with solid test coverage. Ready to merge.