Skip to content

fix(assertions): skip ref-struct members in IsEquivalentTo (#5841)#5842

Open
thomhurst wants to merge 1 commit intomainfrom
fix/5841-isequivalentto-ref-struct
Open

fix(assertions): skip ref-struct members in IsEquivalentTo (#5841)#5842
thomhurst wants to merge 1 commit intomainfrom
fix/5841-isequivalentto-ref-struct

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • IsEquivalentTo blew up with NotSupportedException whenever the structural walk reached a property whose getter returned a ref struct (e.g. ReadOnlyMemory<T>.Span is a ReadOnlySpan<T>). PropertyInfo.GetValue/FieldInfo.GetValue cannot box ref structs.
  • Filter members whose type satisfies IsByRefLike in ReflectionHelper.BuildMembersToCompare. Single point of change covers StructuralEquivalencyAssertion, StructuralEqualityComparer, and StructuralDiffHelper — all route through the cached helper.
  • Type.IsByRefLike is .NET 5+; netstandard2.0 fallback scans GetCustomAttributesData() for IsByRefLikeAttribute (cheap because results are cached per-Type).

Fixes #5841.

Test plan

  • New regression tests Tests5841.IsEquivalentTo_with_ReadOnlyMemory_property_does_not_invoke_Span_getter and Tests5841.IsEquivalentTo_skips_ref_struct_returning_property pass on net10.0.
  • Existing IsEquivalentTo_TypeProperty_Tests (5/5), CollectionStructuralEquivalenceTests (20/20), IgnoringTypeEquivalentTests (12/12), full TUnit.Assertions.Tests.Bugs namespace (231/231), and EquivalentAssertionTests (29/29) all pass.
  • TUnit.Assertions builds clean across netstandard2.0;net8.0;net9.0;net10.0.

PropertyInfo.GetValue / FieldInfo.GetValue throw NotSupportedException for
ref-struct return types because they cannot be boxed. Filter members whose
type is ByRefLike in BuildMembersToCompare so the structural walk skips
e.g. ReadOnlyMemory<T>.Span. netstandard2.0 falls back to scanning for
IsByRefLikeAttribute since Type.IsByRefLike is .NET 5+.
@codacy-production
Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #5842: Skip ref-struct members in IsEquivalentTo

The fix is correct and well-scoped. The root cause is real (PropertyInfo.GetValue cannot box a ref struct), the single-point-of-change approach via ReflectionHelper is the right design choice, and the netstandard2.0 fallback is technically sound. A few items worth addressing before merge:


1. Missing test: inequality when a non-ref-struct member differs alongside a ref struct property

Both regression tests only assert the pass path (equal objects). There is no test verifying that IsEquivalentTo correctly fails when HasSpanProperty.Value differs while Span is silently skipped:

```csharp
[Test]
public async Task IsEquivalentTo_skips_ref_struct_returning_property_and_still_detects_mismatch()
{
var a = new HasSpanProperty { Value = 7 };
var b = new HasSpanProperty { Value = 99 };

await Assert.That(a).IsNotEquivalentTo(b);

}
```

Without this, the filter could accidentally wipe out all members and silently pass mismatched objects, and no existing test would catch it.


2. The field-side IsByRefLike guard is unreachable in practice for non-ref-struct types

A non-ref-struct class or struct cannot legally hold a ref-struct field — the C# compiler rejects it. So for any object that can be boxed and passed to IsEquivalentTo, its public instance fields will never have a ref-struct type. The field filter is harmless, but consider adding a comment explaining that it is defensive/future-proof rather than fixing a currently reachable crash, to prevent future maintainers from removing it as dead code:

```csharp
// Defensive: a class/non-ref-struct cannot legally declare a ref-struct field today,
// but guard anyway so structural comparison stays safe if language rules relax in future.
foreach (var field in type.GetFields(BindingFlags.Public | BindingFlags.Instance))
{
if (!IsByRefLike(field.FieldType))
members.Add(field);
}
```


3. netstandard2.0 fallback and the !type.IsValueType short-circuit

The early return if (!type.IsValueType) return false; is correct (ref structs are always value types). The GetCustomAttributesData() call works correctly for constructed generics like ReadOnlySpan<byte> — confirmed this was considered, no issue.

However, the #if NET5_0_OR_GREATER preprocessor guard prevents the GetCustomAttributesData() fallback from being hit at runtime on modern TFMs. It's worth confirming the build is clean with <TreatWarningsAsErrors>true</TreatWarningsAsErrors> across all TFMs — the PR description indicates it is, which is good.


4. No user-visible signal that ref-struct members are silently skipped

This is a design trade-off worth an explicit decision: when IsEquivalentTo silently skips a ref-struct property like Span, the user gets no diagnostic. For ReadOnlyMemory<T>.Span this is expected. But for a user-defined HasSpanProperty.Span, silently ignoring it could mask a real comparison gap.

Options:

  • Status quo (what this PR does): skip silently. Simple and avoids noise. Acceptable.
  • Log/warn in the assertion failure message or via a build-time analyzer.

If status quo is the chosen design, a comment in BuildMembersToCompare explaining the deliberate trade-off would prevent future maintainers from treating this as an oversight.


5. Minor: HasSpanProperty.Span always returns default

```csharp
public ReadOnlySpan Span => default; // always returns default regardless of state
```

This means the property being skipped doesn't actually contain meaningful data, making it a weaker regression test. A type whose ref-struct property is backed by real data (e.g., wrapping a Memory<byte>) would more faithfully replicate the real bug scenario. Not blocking.


Summary

Item Severity
Missing failure-case regression test Should fix
Field-side guard lacks explanatory comment Nit
Silent skip of ref-struct members (design decision) Worth documenting
Span test helper always returns default Nit

The core implementation in ReflectionHelper.IsByRefLike and BuildMembersToCompare is correct and the approach is the right one. Add the inequality regression test and this is good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IsEquivalentTo throws NotSupportedException when traversing properties whose getter returns a ref struct (e.g. ReadOnlyMemory<T>.Span)

1 participant