fix(assertions): skip ref-struct members in IsEquivalentTo (#5841)#5842
fix(assertions): skip ref-struct members in IsEquivalentTo (#5841)#5842
Conversation
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+.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
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.
There was a problem hiding this comment.
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.
Summary
IsEquivalentToblew up withNotSupportedExceptionwhenever the structural walk reached a property whose getter returned aref struct(e.g.ReadOnlyMemory<T>.Spanis aReadOnlySpan<T>).PropertyInfo.GetValue/FieldInfo.GetValuecannot box ref structs.IsByRefLikeinReflectionHelper.BuildMembersToCompare. Single point of change coversStructuralEquivalencyAssertion,StructuralEqualityComparer, andStructuralDiffHelper— all route through the cached helper.Type.IsByRefLikeis .NET 5+; netstandard2.0 fallback scansGetCustomAttributesData()forIsByRefLikeAttribute(cheap because results are cached per-Type).Fixes #5841.
Test plan
Tests5841.IsEquivalentTo_with_ReadOnlyMemory_property_does_not_invoke_Span_getterandTests5841.IsEquivalentTo_skips_ref_struct_returning_propertypass on net10.0.IsEquivalentTo_TypeProperty_Tests(5/5),CollectionStructuralEquivalenceTests(20/20),IgnoringTypeEquivalentTests(12/12), fullTUnit.Assertions.Tests.Bugsnamespace (231/231), andEquivalentAssertionTests(29/29) all pass.TUnit.Assertionsbuilds clean acrossnetstandard2.0;net8.0;net9.0;net10.0.