fix: IsEquivalentTo falls back to Equals() for types with no public members

[thomhurst]

Summary

Fixes #5040.

• IsEquivalentTo now falls back to Equals() when a type has no public properties or fields to compare structurally. This fixes false positives (and NotSupportedException crashes) for types like sealed class FilePath that implement IEquatable<T> with only private state.
• Applied to both StructuralEqualityComparer (collection equivalence) and StructuralEquivalencyAssertion (direct object equivalence).
• Added NotSupportedException safety net around reflection-based member access in StructuralEqualityComparer.
• Does not regress [Bug]: IsEquivalentTo broken for value types #4358 — types with public members (records, structs, tuples) still use structural comparison.

Test plan

• New tests in Tests5040: equal values pass, different values fail, unordered collection matching — all 3 pass
• Regression tests for [Bug]: IsEquivalentTo broken for value types #4358: all 46 pass
• CollectionStructuralEquivalenceTests: all 20 pass

[claude]

Code Review: fix: IsEquivalentTo falls back to Equals() for types with no public members

The core fix is valid and addresses a real UX problem — IsEquivalentTo silently "passing" types with no public members (because 0 members = nothing failed = pass) is a confusing false positive. The fallback to Equals() is the right semantic intent. A few architectural concerns worth discussing below.

---

Issue 1: try/catch (NotSupportedException) is Too Broad ⚠️

Location: StructuralEqualityComparer.cs, lines 119–129

try
{
    xValue = ReflectionHelper.GetMemberValue(x, member);
    yValue = ReflectionHelper.GetMemberValue(y, member);
}
catch (NotSupportedException)
{
    // Fall back to Equals() for the entire object comparison.
    return Equals(x, y);
}

Why this is problematic:

1. Silences legitimate exceptions: A property getter that throws NotSupportedException for intentional business reasons (e.g., throw new NotSupportedException("Use X instead")) will silently fall back to Equals() instead of surfacing the bug.
2. Inconsistent mid-stream semantics: If member A is compared structurally and then member B throws, the result is Equals(x, y) for the whole object. You've already validated some members structurally, then validated the whole object by equality — this is internally inconsistent and hard to reason about.
3. The original NotSupportedException concern should be addressed at the source: Check PropertyInfo.CanRead before accessing the getter, or handle this in ReflectionHelper.GetMemberValue more precisely.

Suggested approach:

// In ReflectionHelper.GetMembersToCompare, filter out non-readable properties:
var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
foreach (var prop in properties)
{
    if (prop.GetIndexParameters().Length == 0 && prop.CanRead && prop.GetMethod?.IsPublic == true)
    {
        members.Add(prop);
    }
}

This prevents the problem at the data source rather than catching exceptions after the fact.

---

Issue 2: Unnecessary Double Reflection Call + Overly Complex Check ⚠️

Location: StructuralEquivalencyAssertion.cs, lines 199–209

if (expectedMembers.Count == 0)
{
    var actualMembersCheck = ReflectionHelper.GetMembersToCompare(actualType);
    if (actualMembersCheck.Count == 0)
    {
        if (\!Equals(actual, expected))
        {
            return AssertionResult.Failed(...);
        }
        return AssertionResult.Passed;
    }
}

Two problems here:

a) The inner actualMembersCheck is redundant. When expectedMembers.Count == 0 but actual does have members, the code falls through to the existing "extra properties" check (lines 267–301), which will correctly fail with a descriptive message. The nested guard adds complexity without changing correctness in most practical cases.

b) ReflectionHelper.GetMembersToCompare(actualType) gets called up to 3 times in the failing path: once in the new check (line 201), again at line 269 in the extra-properties block. Reflection isn't free, especially for large type hierarchies.

Simpler equivalent:

if (expectedMembers.Count == 0)
{
    // No public members to compare structurally — respect IEquatable<T>/Equals()
    if (\!Equals(actual, expected))
    {
        return AssertionResult.Failed($"Objects at '{path}' are not equal{Environment.NewLine}Expected: {FormatValue(expected)}{Environment.NewLine}Received: {FormatValue(actual)}");
    }
    return AssertionResult.Passed;
}

If actual has extra members but expected doesn't, Equals() (which delegates to the type's equality semantics) is the correct arbiter here anyway.

---

Issue 3: Misleading Error Message for the No-Member Case (Minor) 💬

When path is empty (top-level comparison) and the fallback triggers, the error reads:

"Property did not match" (note double space) is odd when there are no properties. Suggest using a different message format for the equality-fallback path, e.g. "Objects are not equal" or "Values at root did not match".

---

Issue 4: Test Coverage Gap 🔍

The test covers the happy paths well, but misses:

• Mismatched types: expected has no public members but actual does (or vice versa). What happens when comparing PrivateStatePath to a plain string via IsEquivalentTo?
• Property throws NotSupportedException for non-reflection reasons: Verifying the catch doesn't suppress real errors.
• Partial equivalency mode (.WithPartialEquivalency()) with no-public-member types.

---

Positive Notes ✅

• The root cause analysis is correct: 0 members → no failures → false positive pass is the real bug.
• Applying the fix to both StructuralEqualityComparer (used for collection equivalence) and StructuralEquivalencyAssertion (direct object comparison) is the right scope.
• The members.Count == 0 check in StructuralEqualityComparer is clean and symmetric (since that comparer only works on one type parameter).
• Regression test mention for #4358 is reassuring — records/structs/tuples with public members are unaffected.
• New test class is well-structured with a realistic reproducer matching the issue scenario.

---

Summary

The fix addresses the reported issue correctly. The main architectural concern is the try/catch (NotSupportedException) — it trades one class of confusing behavior (silent false-positive) for another (silently swallowed exceptions with inconsistent mid-comparison fallback). The cleanest fix is to filter non-readable properties in ReflectionHelper.GetMembersToCompare rather than catching exceptions later. The double-member-check in StructuralEquivalencyAssertion can also be simplified for clarity.

[claude]

Code Review: fix: IsEquivalentTo falls back to Equals() for types with no public members

The core fix is valid and addresses a real UX problem — IsEquivalentTo silently "passing" types with no public members (because 0 members = nothing failed = pass) is a confusing false positive. The fallback to Equals() is the right semantic intent. A few architectural concerns worth discussing below.

---

Issue 1: try/catch (NotSupportedException) is Too Broad ⚠️

Location: StructuralEqualityComparer.cs, lines 119–129

The catch (NotSupportedException) block has two problems:

1. Silences legitimate exceptions: A property getter that throws NotSupportedException for intentional business reasons will silently fall back to Equals() instead of surfacing the issue.
2. Inconsistent mid-stream semantics: If member A is compared structurally and then member B throws, the result is Equals(x, y) for the whole object. You have already validated some members structurally, then validated the whole object by equality — this is internally inconsistent and hard to reason about.

Suggested approach: Address this at the source in ReflectionHelper.GetMembersToCompare by filtering out non-readable properties:

var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
foreach (var prop in properties)
{
    if (prop.GetIndexParameters().Length == 0 && prop.CanRead && prop.GetMethod?.IsPublic == true)
    {
        members.Add(prop);
    }
}

This prevents the problem at the data source rather than catching exceptions after the fact, and does not swallow legitimate errors.

---

Issue 2: Unnecessary Double Reflection Call + Overly Complex Check ⚠️

Location: StructuralEquivalencyAssertion.cs, lines 199–209

The nested actualMembersCheck.Count == 0 guard is redundant. When expectedMembers.Count == 0 but actual does have members, the code falls through to the existing "extra properties" check (lines 267–301), which correctly fails with a descriptive message. The nested guard adds complexity without changing correctness.

It also results in ReflectionHelper.GetMembersToCompare(actualType) being called up to 3 times in the failing path (lines 201, 269).

Simpler equivalent:

if (expectedMembers.Count == 0)
{
    // No public members to compare structurally — respect IEquatable<T>/Equals()
    if (!Equals(actual, expected))
    {
        var label = string.IsNullOrEmpty(path) ? "Objects" : $"Property {path}";
        return AssertionResult.Failed($"{label} did not match{Environment.NewLine}Expected: {FormatValue(expected)}{Environment.NewLine}Received: {FormatValue(actual)}");
    }
    return AssertionResult.Passed;
}

If actual has extra members but expected doesn't, Equals() is the correct arbiter anyway.

---

Issue 3: Misleading Error Message for the No-Member Case (Minor) 💬

When path is an empty string (top-level comparison) and the fallback triggers, the current message reads "Property did not match" (double space). Since there are no properties involved, the message should be adapted. See the snippet above for a suggested fix using a label variable.

---

Issue 4: Test Coverage Gap 🔍

The tests cover the main happy paths well, but miss:

• Mismatched types: what happens when expected has no public members but actual does?
• Partial equivalency mode (.WithPartialEquivalency()) combined with no-public-member types.
• A test confirming that a property intentionally throwing NotSupportedException is not silently swallowed (relevant if the catch block stays).

---

Positive Notes ✅

• The root cause diagnosis is correct: 0 members → no failures → false-positive pass, and Equals() fallback is the right semantic fix.
• Fixing both StructuralEqualityComparer (collection equivalence) and StructuralEquivalencyAssertion (direct comparison) covers the full scope.
• The members.Count == 0 check in StructuralEqualityComparer is clean and symmetric.
• Regression test coverage for #4358 is reassuring — records/structs/tuples with public members remain unaffected.
• New test class is well-structured with a realistic reproducer matching the reported scenario.

---

Summary

The fix correctly addresses the reported issue. The main architectural concern is the try/catch (NotSupportedException) — it trades one class of confusing behavior (silent false-positive) for another (silently swallowed exceptions + inconsistent mid-comparison fallback). The cleanest fix is to filter non-readable properties in ReflectionHelper.GetMembersToCompare rather than catching exceptions later. The double-member-check in StructuralEquivalencyAssertion can also be simplified for clarity and performance.

[claude]

Follow-up Review: All Previous Issues Addressed ✅

The previous review raised four concerns. All four have been resolved in the current state of the PR.

---

Previous Issue 1: try/catch (NotSupportedException) was too broad → Fixed

The catch block has been removed entirely. Instead, ReflectionHelper.GetMembersToCompare now guards at the source:

if (prop.GetIndexParameters().Length == 0 && prop.CanRead && prop.GetMethod?.IsPublic == true)

This is the correct approach — preventive filtering beats exception-based fallback. The prop.GetMethod?.IsPublic == true guard is also strictly necessary: GetProperties(BindingFlags.Public | BindingFlags.Instance) includes properties where any accessor is public (e.g., private get; public set;), so the getter must be checked separately.

---

Previous Issue 2: Unnecessary double reflection call + overly complex check → Fixed

The nested actualMembersCheck.Count == 0 guard has been removed. The expectedMembers.Count == 0 block in StructuralEquivalencyAssertion is now a clean early-return without redundant reflection calls.

---

Previous Issue 3: Misleading error message (double space) → Fixed

The label variable correctly handles the top-level vs. nested path cases:

var label = string.IsNullOrEmpty(path) ? "Objects" : $"Property {path}";

---

Previous Issue 4: Test coverage gaps → Fixed

Three new tests cover the previously missing scenarios:

• No_Public_Members_Vs_Public_Members_Fails — mismatched types
• Partial_Equivalency_With_No_Public_Members_Equal_Values_Passes
• Partial_Equivalency_With_No_Public_Members_Different_Values_Fails

---

Remaining Observations (Non-blocking)

Behavior note worth documenting: For types with no public members that also do not override Equals() or implement IEquatable<T>, the fallback uses reference equality. This is correct by definition — there's no other equality contract to honor — but it's worth noting in the PR description or a code comment for future readers who might wonder why two new BlackBoxType(42) instances fail equivalency.

StructuralEqualityComparer vs StructuralEquivalencyAssertion asymmetry: The comparer checks xType (runtime type of x) while the assertion checks expectedType. This asymmetry is intentional and correct given their different comparison models.

---

Summary

Clean fix, properly scoped, well-tested, and all previous feedback has been incorporated. The implementation is correct and the approach is sound. Approved.