fix(mocks): skip non-virtual 'new' methods when discovering mockable members

[thomhurst]

Summary

Fixes #5326

• When a derived class hides a base virtual method/property/event with new (e.g., Azure SDK's BlobClient.WithSnapshot hiding BlobBaseClient.WithSnapshot), the mock source generator was still collecting the base virtual and emitting override, causing CS0506 (override of non-virtual) and CS0111 (duplicate extension methods).
• Non-virtual members encountered during class hierarchy traversal are now recorded in the seen-sets to block their base virtuals from being collected later.
• Guards added to MergePropertyAccessors calls to handle the sentinel index used for hidden properties.

Test plan

• 26 new tests in PartialMockTests covering:
  • Basic new method hiding (configure non-hidden virtual, fallback, strict mode, verification)
  • new property hiding
  • Mixed new + override in the same class (methods and properties)
  • 3-level inheritance (grandchild hides middle override)
  • new virtual re-introducing virtual chain
  • Wrap mode with new methods
  • Complex class with overloaded methods, properties, and events where some overloads are hidden and others remain virtual
• All 729 mock tests pass
• All 22 source generator snapshot tests pass

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 minor

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

Results:
1 new issue

Category	Results
UnusedCode	1 minor

View in Codacy

🟢 Metrics 42 complexity

Metric	Results
Complexity	42

View in Codacy

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

[claude]

Code Review

This PR correctly identifies and fixes a real bug (CS0506/CS0111) caused by the mock source generator emitting override for members that are hidden by a non-virtual new in a derived class. The fix is sound and the test coverage is solid. A few observations follow.

---

Asymmetry Between Methods/Events vs Properties

The fix uses two different mechanisms for the three member kinds:

• Methods and events use a HashSet<string>. A non-virtual member just calls seenMethods.Add(key) (ignoring the return value). Because the hierarchy is walked derived-first, this naturally "claims" the key, and the base virtual with the same key is skipped at if (!seenMethods.Add(key)) continue;. Clear and correct.

• Properties use a Dictionary<string, int> mapping the key to an index in the properties list. Because you can't mark "absent" with a HashSet here, a sentinel value of -1 is used instead.

The sentinel works, but it overloads the meaning of the dictionary (index vs "hidden" marker). A small improvement would be a dedicated HashSet<string> hiddenProperties alongside the dictionary, keeping each data structure single-purpose. That said, the current approach is fully functional.

---

Potential Interaction: Static Abstract Members + Sentinel

CollectStaticAbstractMember at line 819 uses:

if (seenProperties.ContainsKey(key)) break;

This means if a class hierarchy introduces a non-virtual property named Foo (sentinel -1), and the class also implements an interface with a static abstract property named Foo, CollectStaticAbstractMember will silently skip the static abstract member. This is an extremely unlikely scenario (same property name in both a non-virtual instance position and a static abstract interface position), but the guard condition is broader than necessary. This is a pre-existing pattern issue rather than something introduced by this PR, but worth being aware of.

---

Missing Test: Event Hiding With Direct Raise/Subscribe

ComplexDerivedService declares:

public new event EventHandler? StatusChanged;  // hides base virtual
public virtual event EventHandler<string>? MessageReceived;  // remains virtual

The 26 new tests cover methods, properties, overloads, wrap mode, and the 3-level hierarchy well. However, there is no test that directly raises or subscribes to MessageReceived on a mock of ComplexDerivedService to verify the non-hidden event is still mockable. Given that the event path in ProcessClassMembers uses the same HashSet approach as methods, it should work correctly — but a test for mock.RaiseMessageReceived(...) or event subscription would make the coverage complete.

---

The Core Fix Is Correct

The walk order (derived → base) ensures non-virtual hiding members are processed first, claiming their key before the base virtual is seen. The conditions are:

// Non-virtual: just claim the key (derived-first walk ensures this blocks the base virtual)
seenMethods.Add(key);

// Virtual/Abstract/Override: only add if key not already claimed
if (!seenMethods.Add(key)) continue;
methods.Add(...);

This is logically sound. The property -1 sentinel achieves the same thing for the dictionary variant.

---

Minor: Comment in Non-Virtual Property Branch

// Non-virtual properties that hide a base virtual (via 'new') must
// still be recorded so the base virtual is not collected later.
if (!seenProperties.ContainsKey(key))
{
    seenProperties[key] = -1;
}

The guard if (!seenProperties.ContainsKey(key)) is intentionally conservative — if a more-derived virtual was already recorded (positive index), you don't want to overwrite it with -1. This is correct, though a brief note in the comment explaining the guard (// don't overwrite an already-collected virtual property) would help future readers.

---

Summary: The fix is correct and well-tested. The main suggestions are: (1) consider a dedicated HashSet<string> for hidden properties instead of the -1 sentinel to reduce dual-purpose use of the dictionary, and (2) add at least one test verifying event raise/subscribe on a type with mixed hidden/non-hidden events. Neither of these is a blocker.

[claude]

Code Review

Good fix for a subtle but impactful bug. The class hierarchy traversal correctly uses a "seen-first" strategy where derived members are processed before base members, so recording non-virtual new members in the seen-sets before reaching base virtuals is the right approach.

Architectural Concern: Sentinel Value in Typed Dictionary

The HiddenByNew = -1 sentinel stored in Dictionary<string, int> is a code smell. The type system doesn't enforce or communicate the dual meaning of the int (either a valid list index or a sentinel), which makes the API fragile and surprising to future readers:

// Current: int can mean "valid index" or "special hidden state"
private const int HiddenByNew = -1;
Dictionary<string, int> seenProperties;

A cleaner approach would be to use a nullable int, which makes the two states explicit in the type:

// Better: null explicitly means "hidden by new, no index"
Dictionary<string, int?> seenProperties;

// When hiding: seenProperties[key] = null;
// When collecting: seenProperties[key] = properties.Count;

// At use sites:
if (seenProperties.TryGetValue(key, out var existingIndex))
{
    if (existingIndex.HasValue)
    {
        MergePropertyAccessors(properties, existingIndex.Value, property, ref memberIdCounter);
    }
    // else: hidden by 'new', skip
}

This eliminates the magic constant and makes intent explicit via the type system. The XML doc comment on the constant is a sign that the design needs documentation to compensate for the non-obvious encoding.

Asymmetry: Methods/Events vs Properties

Methods and events use HashSet<string> (no sentinel needed) because there is no accessor-merging step. The HiddenByNew sentinel is only necessary because the dictionary conflates two concerns: "have we seen this key?" and "what is the index?". An alternative is separating them into a HashSet<string> hiddenByNewKeys plus Dictionary<string, int> propertyIndices, which eliminates the sentinel at the cost of two structures. The int? approach is probably the better middle ground.

Minor: XML Doc Covers Properties but Not Indexers

The HiddenByNew constant's XML comment only mentions "non-virtual property" but the sentinel is also stored in seenProperties for indexers (key = "I:[...]"). The comment should say "property or indexer".

Missing Test: Hidden Indexer

The PR tests hidden methods, properties, and events via new, but no test covers a class with a new hidden indexer. Since the indexer code path uses the same HiddenByNew guard (lines 99-105 and 204-210), a test with a new-hidden indexer would complete the coverage of the sentinel logic.

Everything Else Looks Good

• The traversal order (derived-first via ProcessClassMembers, then base classes, then interfaces) is correct — derived new members are recorded before base virtuals are encountered.
• new virtual re-introduced virtual chains work correctly because IsVirtual == true takes the collection path, and the base virtual is blocked by the already-seen key.
• The !seenProperties.ContainsKey(key) guard (line 312) correctly avoids overwriting an already-collected virtual with the sentinel when walking further up the hierarchy.
• 26 new tests covering complex overload mixing, 3-level hierarchies, wrap mode, and strict mode provide excellent regression coverage.

The fix is correct and the behaviour is well-validated. The main actionable suggestion is replacing the -1 sentinel with int? for type-safe clarity.

[claude]

Code Review

Good fix for a real-world pain point (Azure SDK patterns). The approach of walking the hierarchy most-derived-first and using seen-sets to block base virtuals is correct and the test coverage is thorough. A few observations:

Design Concern: Sentinel value in the properties dictionary

The HiddenByNew = -1 sentinel stored in seenProperties (a Dictionary<string, int> otherwise holding list indices) is the main design weakness. It creates two obligations:

1. Every call site that reads from seenProperties must now guard against -1.
2. If someone adds a new call site and forgets the guard, they get an ArgumentOutOfRangeException at runtime in the generator — a silent, hard-to-diagnose failure.

The asymmetry with methods/events (where a plain HashSet handles this naturally via Add returning false) makes this feel like an accidental difference rather than a deliberate one.

A simpler approach would be a dedicated HashSet<string> hiddenProperties alongside seenProperties:

// Instead of seenProperties[key] = HiddenByNew
hiddenProperties.Add(key);

// Instead of if (existingIndex != HiddenByNew)
if (!hiddenProperties.Contains(key) && seenProperties.TryGetValue(key, out var existingIndex))
    MergePropertyAccessors(...);

This eliminates the sentinel, removes the four != HiddenByNew guards, and makes the seenProperties dictionary a pure index-map again. The guard in CollectStaticAbstractMember (seenProperties.ContainsKey(key)) would also need to include the hidden-check, but that's explicit and clear.

Missing indexer coverage in ProcessClassMembers

The ProcessClassMembers switch only handles IPropertySymbol when !property.IsIndexer. There's no case for indexers, meaning a new indexer hiding a base virtual indexer won't be blocked. This is pre-existing, but since the PR touches this area, it would be worth either:

• Adding the indexer case with the same sentinel/hidden-set logic, or
• Adding a comment explicitly noting the limitation

else if (!seenProperties.ContainsKey(key)) — minor readability

// Current (line ~312):
else if (!seenProperties.ContainsKey(key))
{
    seenProperties[key] = HiddenByNew;
}

// Equivalent and more idiomatic:
else
{
    seenProperties.TryAdd(key, HiddenByNew);
}

The intent is the same but TryAdd communicates "only if absent" more directly.

What's good

• The hierarchy traversal order (most-derived first) is exactly right — the new member is encountered before its base virtual, so the block-by-key mechanism works correctly for all levels including 3-level chains.
• new virtual is handled correctly for free: IsVirtual=true means it gets collected and blocks the base via the seen-set, which is exactly what you want.
• Test coverage is excellent. The ComplexDerivedService test class exercising mixed overloads/properties/events in one type is particularly valuable.
• The sentinel-guard additions to the interface-merging loops (lines ~83, ~102, ~188, ~207) are correct — a class property hidden by new should also block merging from an interface that declares the same property name.

Overall this is a solid, well-reasoned fix. The sentinel-value design is worth revisiting before this pattern spreads to more call sites, but it doesn't block the fix from being correct today.

[claude]

Code Review — Follow-up

Previous reviews flagged two main concerns; checking whether they've been addressed in the latest commit ():

✅ Sentinel value replaced with int?

The HiddenByNew = -1 magic constant and Dictionary<string, int> have been replaced with Dictionary<string, int?> throughout. null now encodes "hidden by new" and HasValue guards at call sites are explicit and type-safe. This fully resolves the main design concern from the prior reviews.

✅ Indexer case added to ProcessClassMembers

A dedicated case IPropertySymbol indexer when indexer.IsIndexer: branch now handles the same null-sentinel logic for indexers. This closes the pre-existing gap that was noted.

Remaining minor item: else if (!seenProperties.ContainsKey(key))

This pattern is still used in three places to write null (the "hidden" sentinel) only when the key isn't already present:

It's functionally correct, but TryAdd communicates intent more directly and avoids a double-lookup:

Minor and non-blocking — the current code is readable.

Missing test: new-hidden indexer

The test suite covers hidden methods, properties, and events, but there's still no test for a class that hides a base virtual indexer with new this[...]. The indexer code path now has the same guard logic, so a test would complete the coverage. Not blocking.

Summary

The two structural issues from the prior reviews have been cleanly resolved. The fix is correct, the int? encoding is idiomatic, and the 26+ tests provide strong regression coverage. The remaining points above are low-priority polish items.

Approving.

[claude]

Code Review — Follow-up

Previous reviews flagged two main concerns; checking whether they've been addressed in the latest commit (f2091895f9af):

✅ Sentinel value replaced with int?

The HiddenByNew = -1 magic constant and Dictionary<string, int> have been replaced with Dictionary<string, int?> throughout. null now encodes "hidden by new" and HasValue guards at call sites are explicit and type-safe. This fully resolves the main design concern from the prior reviews.

✅ Indexer case added to ProcessClassMembers

A dedicated case IPropertySymbol indexer when indexer.IsIndexer: branch now handles the same null-sentinel logic for indexers. This closes the pre-existing gap that was noted.

Remaining minor item: else if (!seenProperties.ContainsKey(key))

This pattern is still used in three places to write null (the "hidden" sentinel) only when the key isn't already present:

else if (!seenProperties.ContainsKey(key))
{
    seenProperties[key] = null;
}

It's functionally correct, but TryAdd communicates intent more directly and avoids a double-lookup:

else
{
    seenProperties.TryAdd(key, null);
}

Minor and non-blocking — the current code is readable.

Missing test: new-hidden indexer

The test suite covers hidden methods, properties, and events, but there's still no test for a class that hides a base virtual indexer with new this[...]. The indexer code path now has the same guard logic, so a test would complete the coverage. Not blocking.

Summary

The two structural issues from the prior reviews have been cleanly resolved. The fix is correct, the int? encoding is idiomatic, and the 26+ tests provide strong regression coverage. The remaining points above are low-priority polish items.

Approving.