ImmutableArray<T> struct implements IEquatable<T> interface but does not stick to the rules of properly implementing value semantics

[oliverzick]

Description

The ImmutableArray<T> struct implements the IEquatable<T> interface but does not stick to the rules of implementing value semantics acoording to the IEquatable interface.

When comparing two instance of a ImmutableArray<T> both instance are considered equal only when both instances refer to the same contained array instance. Instead both instances should be considered equal when the contents of the contained collections are equal.

Reproduction Steps

using System.Collections.Immutable;

var collection = Enumerable.Range(1, 5);

var instance1 = collection.ToImmutableArray();
var instance2 = collection.ToImmutableArray();

// Result = False ---> Wrong result for equality in terms of value semantics
var result = instance1.Equals(instance2); 

Console.WriteLine($"Result: {result}"); // Outputs "Result: False"

Expected behavior

using System.Collections.Immutable;

var collection = Enumerable.Range(1, 5);

var instance1 = collection.ToImmutableArray();
var instance2 = collection.ToImmutableArray();

// Result = True ---> Should be true because both instances represent a collection of integers from 1 to 5 which are equal in terms of value semantics defined by IEquatable<T> interface
var result = instance1.Equals(instance2);

Console.WriteLine($"Result: {result}"); // Outputs "Result: True"

Actual behavior

Already described in Description and Reproduction Steps sections.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

The IEquatable<T> interface should by properly implemented according to its specification to ensure value semantics of ImmutableArray<T>. This also includes desired behavior of Equals and GetHashCode methods according to interface specification.

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The ImmutableArray<T> struct implements the IEquatable<T> interface but does not stick to the rules of implementing value semantics acoording to the IEquatable interface.

When comparing two instance of a ImmutableArray<T> both instance are considered equal only when both instances refer to the same array instance. Instead both instances should be considered equal when the contents of the contained collections are equal.

Reproduction Steps

using System.Collections.Immutable;

var collection = Enumerable.Range(1, 5);

var instance1 = collection.ToImmutableArray();
var instance2 = collection.ToImmutableArray();

// Result = False ---> Wrong result for equality in terms of value semantics
var result = instance1.Equals(instance2); 

Console.WriteLine($"Result: {result}"); // Outputs "Result: False"

Expected behavior

using System.Collections.Immutable;

var collection = Enumerable.Range(1, 5);

var instance1 = collection.ToImmutableArray();
var instance2 = collection.ToImmutableArray();

// Result = True ---> Should be true because both instances represent a collection of integers from 1 to 5 which are equal in terms of value semantics defined by IEquatable<T> interface
var result = instance1.Equals(instance2);

Console.WriteLine($"Result: {result}"); // Outputs "Result: True"

Actual behavior

Already described in Description and Reproduction Steps sections.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

The IEquatable<T> interface should by properly implemented according to its specification to ensure value semantics of ImmutableArray<T>. This also includes desired behavior of Equals and GetHashCode methods according to interface specification.

Author:	oliverzick
Assignees:	-
Labels:	area-System.Collections
Milestone:	-

[Clockwork-Muse]

The ImmutableArray<T> struct implements the IEquatable<T> interface but does not stick to the rules of implementing value semantics acoording to the IEquatable interface.

IEquatable<T> doesn't require types implement deep value semantics. It implies no contract other than some loosely defined "equality" that's up for interpretation by types that implement it. It's not completely uncommon for either shallow equality or ignored properties to be common in implementations in C# or other languages.

[eiriktsarpalis]

And even if we wanted to implement structural equality at this point, it would most likely be a breaking change.

[RikkiGibson]

I think the typical guidance here is to use SequenceEqual, but the behavior can be a little troubling if you want to deeply compare records which contain collections, for example. (Records use EqualityComparer.Default to compare their fields.) SharpLab

using System;
using System.Collections.Generic;
using System.Linq;

var r1 = new R1(new List<R2> { new R2(new List<int> { 1 }) });
var r2 = new R1(new List<R2> { new R2(new List<int> { 1 }) });

Console.WriteLine(r1.Equals(r2)); // False
Console.WriteLine(Enumerable.SequenceEqual(r1.Items, r2.Items)); // False

Console.WriteLine(r1.Equals(r2)); // False
Console.WriteLine(Enumerable.SequenceEqual(r1.Items[0].NestedItems, r2.Items[0].NestedItems)); // True

record R1(List<R2> Items);
record R2(List<int> NestedItems);

That said, I'm not sure the problem is really "fixable", except that if you want these kinds of equality semantics, to take care to use SequenceEqual or equivalent methods when comparing collections, and perhaps do the same when you implement IEquatable.Equals.

[Sergio0694]

(Partially unrelated) Just wanted to say that while I agree that of course changing the behavior of IEquatable<T> here is definitely a breaking change and not doable, the current behavior is indeed extremely unfortunate when using lots of records like @RikkiGibson mentioned. This is extremely common when writing source generators (as incrementa generators require you to define models for your incremental steps), and the current situation is that the second you have even a single record parameter that's an immutable array, you just fall off a cliff and have to go all the way down to implementing equality manually for the whole record type, which is a pain. It would be nice if there was some way to improve this scenario, as it's arguably pretty common and unfortunately (1) extremely verbose and (2) extremely error prone, as if you don't know this behavior or just forget when refactoring, you'll silently break your entire incremental pipeline after that step 😅

[eiriktsarpalis]

@Sergio0694 Perhaps exposing an EqualityComparer combinator could help in that regard?

public static class EqualityComparer
{
     public static IEqualityComparer<TList> CreateListComparer<TList, TElement>(IEqualityComparer<TElement> elementComparer) 
        where TList : IList<TElement> { }
}

Related to #33873 and #19644

[oliverzick]

As @RikkiGibson and @Sergio0694 already mentioned the current equatable behavior of the ImmutableArray<T> struct - whether this was intended or not - is a pitfall, unfortunately.

At least I have experienced this on heavily utilizing records (e.g. when working with DDD or object-functional techniques that involve a lot of immutability and types that come with value semantics). So I was introducing new records representing composites that own their contained collections. Then I was looking for some out-of-the-box immutable collection and the ImmutableArray<T> was the only one I could find which is implementing the IEquatable<T> interface.
After replacing a custom immutable collection type that comes with value semantics the corresponding specs and tests were broken. So I digged a bit deeper and detected that the ImmutableArray<T> was actually not what I thought it is, according to its name and implementation of the IEquatable<T> interface. It looks like @Sergio0694 came across a similar situation. 😅

@eiriktsarpalis I know that changing this behavior afterwards would be a breaking change. In my opinion it would be great if there was an out-of-the-box immutable collection or bare enumerable implementation that comes with value semantics and can be used within records.

[Sergio0694]

"it would be great if there was an out-of-the-box immutable collection or bare enumerable implementation that comes with value semantics and can be used within records."

You know what, that's... Actually a very good idea. I reckon using something like that might very well make authoring records much much simpler, as there would be no more a need to handroll custom comparers every single time 🤔

I've hacked together really quick just to play around:

Old prototype (click to expand):

public readonly struct EquatableArray<T> : IEquatable<EquatableArray<T>>
    where T : IEquatable<T>
{
    private readonly T[] array;

    public bool Equals(EquatableArray<T> array)
    {
        return AsImmutableArray().SequenceEqual(array.AsImmutableArray());
    }

    public override bool Equals([NotNullWhen(true)] object? obj)
    {
        return obj is EquatableArray<T> array && Equals(this, array);
    }

    public override int GetHashCode()
    {
        if (this.array is not T[] array)
        {
            return 0;
        }

        HashCode hashCode = default;

        foreach (T item in array)
        {
            hashCode.Add(item);
        }

        return hashCode.ToHashCode();
    }

    public ImmutableArray<T> AsImmutableArray()
    {
        return Unsafe.As<T[], ImmutableArray<T>>(ref Unsafe.AsRef(in this.array));
    }

    public static EquatableArray<T> FromImmutableArray(ImmutableArray<T> array)
    {
        EquatableArray<T> array2 = default;

        Unsafe.AsRef(in array2.array) = Unsafe.As<ImmutableArray<T>, T[]>(ref array);

        return array2;
    }

    public static implicit operator EquatableArray<T>(ImmutableArray<T> array)
    {
        return FromImmutableArray(array);
    }

    public static implicit operator ImmutableArray<T>(EquatableArray<T> array)
    {
        return array.AsImmutableArray();
    }

    public static bool operator ==(EquatableArray<T> left, EquatableArray<T> right)
    {
        return left.Equals(right);
    }

    public static bool operator !=(EquatableArray<T> left, EquatableArray<T> right)
    {
        return !left.Equals(right);
    }
}

See updated code here.

And you'd use it like so:

record MyRecord(EquatableArray<string> Names, EquatableArray<int> Numbers);

ImmutableArray<string> names = GetNames();
ImmutableArray<int> numbers = GetNumbers();

MyRecord myRecord = new(names, numbers); // Implicit conversion

names = myRecord.Names;
numbers = myRecord.Numbers; // Also implicit conversion

I might actually try this out in a branch of the MVVM Toolkit and see how this goes 😄
It would seem like this would simplify a ton of code!

UPDATE: well this was an absolute success, this is amazing 🎉

[GSPP]

@oliverzick @Sergio0694 I had that problem as well. I wrote myself a struct wrapper that confers value semantics to any list. Something like:

struct ValueList<T> : IList<T>
{
    IList<T> inner;
    IEqualityComparer<T> elementComparer;

    //...
}

static class ValueList
{
    public static ValueList<T> Create<T>(IList<T> inner) => ...
}

var bytes = ValueList.Create(myByteArray);

That type can ease a lot of pain, especially in the context of LINQ. The main drawback is that the ValueList type leaks into the public surface area of APIs.

[Sergio0694]

Yup this is not something I'd recommend for public APIs. In my scenario though I was just referring to models in incremental generator steps, which are all internal anyway, so that's not an issue. @eiriktsarpalis that experiment was really a success both in the MVVM Toolkit and ComputeSharp, and I've also discovered yesterday that @jkoritzinsky has also been using an approach much like this in all the various COM interop generators as well. I wonder if this same idea could also help you simplify things in the System.Text.Json generators? Just really feels like a much simpler approach for incremental models 😄