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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.43.11 to 1.44.0. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.44.0 <!-- Release notes generated using configuration in .github/release.yml at v1.44.0 --> ## What's Changed ### Other Changes * Generated mocks live in the same namespace as the mocked type by @thomhurst in thomhurst/TUnit#5870 * Show multi-step test spans in class timeline, align report ordering with execution, and correlate linked OTel activities by @Copilot in thomhurst/TUnit#5847 * fix: don't leak RUC onto Should-style comparer overloads (#5857) by @thomhurst in thomhurst/TUnit#5873 * Fix culture-dependent timestamp in HTML test report (#5868) by @thomhurst in thomhurst/TUnit#5872 * fix(mocks-http): auto-prepend `/` to partial endpoint paths (#5838) by @thomhurst in thomhurst/TUnit#5874 * Replace Report.ExpandClassTimeline with [ClassTimeline] attribute by @thomhurst in thomhurst/TUnit#5875 * feat(assertions): make ShouldAssertion<T> implement IAssertion (#5824) by @thomhurst in thomhurst/TUnit#5876 * feat(mocks): support non-span ref struct out/ref params by @thomhurst in thomhurst/TUnit#5878 * fix(core): fill optional params when invoking MethodDataSource via reflection by @thomhurst in thomhurst/TUnit#5880 * Mocks: structural fix for Mock<T> / mocked-member name collisions by @thomhurst in thomhurst/TUnit#5881 * chore(mocks): promote TUnit.Mocks packages to stable by @thomhurst in thomhurst/TUnit#5877 ### Dependencies * chore(deps): update tunit to 1.43.41 by @thomhurst in thomhurst/TUnit#5863 * chore(deps): update dependency tunit.assertions.fsharp to 1.43.41 by @thomhurst in thomhurst/TUnit#5865 * chore(deps): bump @babel/plugin-transform-modules-systemjs from 7.28.5 to 7.29.4 in /docs by @dependabot[bot] in thomhurst/TUnit#5867 * chore(deps): bump fast-uri from 3.1.0 to 3.1.2 in /docs by @dependabot[bot] in thomhurst/TUnit#5862 **Full Changelog**: thomhurst/TUnit@v1.43.41...v1.44.0 ## 1.43.41 <!-- Release notes generated using configuration in .github/release.yml at v1.43.41 --> ## What's Changed ### Other Changes * feat(playwright): expose default Context/Launch options on settings by @thomhurst in thomhurst/TUnit#5861 * fix(hooks): resolve TestDiscovery hook context type by attribute kind, not method name by @thomhurst in thomhurst/TUnit#5860 ### Dependencies * chore(deps): update tunit to 1.43.38 by @thomhurst in thomhurst/TUnit#5858 **Full Changelog**: thomhurst/TUnit@v1.43.38...v1.43.41 ## 1.43.38 <!-- Release notes generated using configuration in .github/release.yml at v1.43.38 --> ## What's Changed ### Other Changes * feat(playwright): add TUnitPlaywrightSettings defaults by @thomhurst in thomhurst/TUnit#5859 **Full Changelog**: thomhurst/TUnit@v1.43.37...v1.43.38 ## 1.43.37 <!-- Release notes generated using configuration in .github/release.yml at v1.43.37 --> ## What's Changed ### Other Changes * docs: clarify MethodDataSourceAttribute.Factory is source-generator-managed by @Copilot in thomhurst/TUnit#5835 * fix(assertions): skip ref-struct members in IsEquivalentTo (#5841) by @thomhurst in thomhurst/TUnit#5842 * feat(playwright): add composition-based fixtures by @thomhurst in thomhurst/TUnit#5840 ### Dependencies * chore(deps): update tunit to 1.43.11 by @thomhurst in thomhurst/TUnit#5821 * chore(deps): update dependency polyfill to 10.4.0 by @thomhurst in thomhurst/TUnit#5830 * chore(deps): update dependency polyfill to 10.4.0 by @thomhurst in thomhurst/TUnit#5829 * chore(deps): update react to ^19.2.6 by @thomhurst in thomhurst/TUnit#5839 * chore(deps): update dependency polyfill to 10.5.0 by @thomhurst in thomhurst/TUnit#5848 * chore(deps): update dependency polyfill to 10.5.0 by @thomhurst in thomhurst/TUnit#5849 * chore(deps): update aspire to 13.3.0 by @thomhurst in thomhurst/TUnit#5851 * chore(deps): update dependency brace-expansion to v5.0.6 by @thomhurst in thomhurst/TUnit#5853 * chore(deps): update dependency polyfill to 10.5.1 by @thomhurst in thomhurst/TUnit#5854 * chore(deps): update dependency polyfill to 10.5.1 by @thomhurst in thomhurst/TUnit#5855 * chore(deps): update verify to 31.16.3 by @thomhurst in thomhurst/TUnit#5856 **Full Changelog**: thomhurst/TUnit@v1.43.11...v1.43.37 Commits viewable in [compare view](thomhurst/TUnit@v1.43.11...v1.44.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
1 participant
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.