IStructuralEquatable support #3646
Conversation
…omparer to delegate to enumerablecomparer
| @@ -48,6 +49,10 @@ internal EquatablesComparer(NUnitEqualityComparer equalityComparer) | |||
| Type xType = x.GetType(); | |||
| Type yType = y.GetType(); | |||
|
|
|||
| if (x is IEnumerable && DoesUseStructuralEquality(xType) | |||
| && y is IEnumerable && DoesUseStructuralEquality(yType)) | |||
| return null; | |||
There was a problem hiding this comment.
Skip using EquatablesComparer if each type fulfills these criteria:
- Implements IEnumerable
- Implements either IStructuralComparable or IStructuralEquatable
- Is a value type
I'm unsure if this is overkill. Would it be enough to just check for IEnumerable here, or would that be overly broad and break something? Alternately, would it be better to instead check if the types are valuetypes that implement IStructuralEquatable (though I suspect this might break the "defer to enumerables" idea)?
There was a problem hiding this comment.
I like the idea that we would give IStructural interfaces priority over IEnumerable. Not sure that being a value type should make a difference; is there are reason you can think of?
There was a problem hiding this comment.
Good points thanks @jnm2 . I think I'd been looking for a "quick and easy" fix here, but I see it's a little more nuanced. Especially with having IStructural take priority over IEnumerable (similar to how EquatablesComparer takes priority).
Re: value types, I had originally added that in error. I noticed MSDN primarily showed value types as implementing this, but of course both structs and classes can implement interfaces.
I've stepped back and given this a different approach now, with a new StructuralComparer type which, when invoked, will use NUnitEqualityComparer for all comparisons (including Tolerance).
src/NUnitFramework/framework/Constraints/Comparers/EquatablesComparer.cs
Outdated
Show resolved
Hide resolved
| @@ -32,6 +32,7 @@ | |||
|
|
|||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.7.0" /> | |||
| <PackageReference Include="NUnit3TestAdapter" Version="3.13.0" /> | |||
| <PackageReference Include="System.Collections.Immutable" Version="1.5.0" /> | |||
There was a problem hiding this comment.
I started to hit issues on the tests when building with Cake with version >= 1.6 on netcoreapp21 (similar to what was encountered here: Azure/azure-functions-vs-build-sdk#353). VS tests run fine with the latest version of this (1.7.1).
| if (!type.IsValueType) | ||
| return false; |
There was a problem hiding this comment.
Why would a reference type not use structural equality? For example, C# 9 records are emitted as ordinary classes, but they implement structural equality as indeed any class can if it wishes.
stevenaw
changed the title
stevenaw
changed the title
| @@ -11,6 +11,10 @@ | |||
| <PackageReference Include="jnm2.ReferenceAssemblies.net35" Version="1.0.1" /> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup Condition="'$(TargetFramework)' != 'net35' and '$(TargetFramework)' != 'net40'"> | |||
There was a problem hiding this comment.
IStructuralEquatable is not available to NET4 on NuGet via System.Runtime. Instead, it is just a part of the framework
Fixes #3447
It looks like the reason for the failures here was that, as an implementer of IEquatable<>, ImmutableArray was not deferring comparisons down to the enumerable-level. IEquatable<> in turn attempted reference equality on the boxed objects of ImmutableArray (a struct).
I'd welcome some feedback on the implementation here, as I'm not entirely sure the best solution.