Discussion: equality of records

[orthoxerox]

There are multiple open questions regarding equality in https://github.com/dotnet/csharplang/blob/master/proposals/records.md (#39) that I think are worth discussing by the community.

Open issue: should the implementation of IEquatable<RecordType>.Equals(RecordType other) be a public member of RecordType?

I think it won't hurt. The difference between this method and Equals(object other) is basically a single cast, but most existing types expose both.

Open issue: This implementation of Equals is not symmetric in the face of inheritance.

The alternative implementation in the proposal doesn't really answer the big question:

Should base types care about fields defined in derived types when testing for equality?

public abstract class Person(string Name);
public sealed class Student(string Name, double Gpa) : Person(Name);

var people = new HashSet<Person>();
people.Add(new Student("Bob", 4.0));
people.Add(new Student("Bob", 3.0));

How many elements are there in the set, one or two? I think the answer must be two, and Student should explicitly implement IEquatable<Person>.Equals(Person other):

public bool IEquatable<Person>.Equals(Person other)
{
	var otherStudent = other as Student;
	
	if(otherStudent == null)
		return false;
	
	if (object.ReferenceEquals(this, otherStudent))
		return true;
	
	return EqualsHelper(this, otherStudent);
}

//and the same for object.Equals(object other)
public override bool Equals(object other)
{
	var otherStudent = other as Student;
	
	if(otherStudent == null)
		return false;
	
	if (object.ReferenceEquals(this, otherStudent))
		return true;
	
	return EqualsHelper(this, otherStudent);
}

private bool EqualsHelper(Student left, Student right)
{
	return left.Name.Equals(right.Name)
		&& left.Gpa.Equals(right.Gpa);
}

I don't see how this implementation will break down the symmetry of Equals.

Open issue: Should a record declare operator == and operator !=?

My answer is a big emphatic YES. Reference type records would use reference equality otherwise, and this would be the wrong behavior in all of the cases, since records should have value type semantics.

An important caveat: operator == should not match the behavior of Equals for all records. If I define public struct Velocity(double Value);, I want operator == on it to have IEEE 754:2008 semantics.

This also means that there should be a private virtual helper method for operator == that allows you to correctly compare two objects of a derived type.

Does this mean that having value type members that do not have operator == defined should be an error or that the compiler should fall back to Equals?

I think preferring == to Equals, but allowing both is fine. This might be a bit surprising, though.

Records members of a generic type should not be compared using Equals

EqualityComparer<T>.Default.Equals is a much better choice for performance reasons. It's a tiny, tiny gain, but why not take it for free?

[gafter]

@orthoxerox You haven't shown your implementation of object.Equals in the type Person, but I suspect the implementation you have in mind isn't symmetric. If you show me your implementation I can demonstrate any asymmetry.

This is how I would expect object.Equals to be implemented so as to be symmetric:

class Person
{
    public string Name { get; }

    public Person(string name) { Name = name; }

    protected virtual Type EqualityContractOrigin => typeof(Person);

    public override bool Equals(object obj)
    {
        var that = obj as Person;
        return that != null &&
            that.EqualityContractOrigin == this.EqualityContractOrigin &&
            that.Name.Equals(this.Name);
    }
}

class Student : Person
{
    public decimal Gpa { get; }

    public Student(string name, decimal gpa) : base(name) { Gpa = gpa; }

    protected override Type EqualityContractOrigin => typeof(Student);

    public override bool Equals(object obj)
    {
        var that = obj as Student;
        return that != null &&
            that.EqualityContractOrigin == this.EqualityContractOrigin &&
            that.Name.Equals(this.Name) &&
            that.Gpa.Equals(this.Gpa);
    }
}

[YaakovDavis]

On a related subject, what about comparability (ie. implementing IComparable<T>)?
F# implements this on records, would be nice if C# did the same, either implicitly or explicitly.

[orthoxerox]

@gafter I've refined my original idea a little bit, so the equality would look like this:

public abstract class Base : IEquatable<Base>
{
    public string Name { get; }

    protected Base(string name)
    {
        Name = name;
   }

    public override bool Equals(object other)
    {
        var otherBase = other as Base;

        return Equals(otherBase);
    }

    public abstract bool Equals(Base other);
}

public abstract class Intermediate : Base, IEquatable<Intermediate>
{
    public double Value { get; }

    protected Intermediate(string name, double value)
        : base(name)
    {
        Value = value;
    }

    public abstract bool Equals(Intermediate other);
}

public sealed class Derived1 : Intermediate, IEquatable<Derived1>
{
    public Base Parent { get; }

    public Derived1(string name, double value, Base parent)
        : base(name, value)
    {
        Parent = parent;
    }

    private bool EqualsHelper(Derived1 left, Derived1 right)
    {
        return left.Name.Equals(right.Name)
            && left.Value.Equals(right.Value)
            && left.Parent.Equals(right.Parent);
    }

    public bool Equals(Derived1 other)
    {
        if (other == null)
            return false;

        if (ReferenceEquals(this, other))
            return true;

        return EqualsHelper(this, other);
    }

    public override bool Equals(Intermediate other)
    {
        var otherDerived1 = other as Derived1;

        return Equals(otherDerived1);
    }

    public override bool Equals(Base other)
    {
        var otherDerived1 = other as Derived1;

        return Equals(otherDerived1);
    }
}

public sealed class Derived2<T> : Intermediate, IEquatable<Derived2<T>>
{
    public T Payload { get; }

    public Derived2(string name, double value, T payload)
        : base(name, value)
    {
        Payload = payload;
    }

    private bool EqualsHelper(Derived2<T> left, Derived2<T> right)
    {
        return left.Name.Equals(right.Name)
            && left.Value.Equals(right.Value)
            && EqualityComparer<T>.Default.Equals(
                left.Payload, 
                right.Payload);
    }

    public bool Equals(Derived2<T> other)
    {
        if (other == null)
            return false;

        if (ReferenceEquals(this, other))
            return true;

        return EqualsHelper(this, other);
    }

    public override bool Equals(Intermediate other)
    {
        var otherDerived2 = other as Derived2<T>;

        return Equals(otherDerived2);
    }

    public override bool Equals(Base other)
    {
        var otherDerived2 = other as Derived2<T>;

        return Equals(otherDerived2);
    }
}

I don't see where it is asymmetrical. Instances of any two different sealed classes derived from Base will not compare as equal, instances of the same sealed class will be compared using the virtual method defined in the same class.

Or are parent record types no longer required to be abstract?

[orthoxerox]

For non-abstract parent types the following methods should be symmetrical:

public class Base : IEquatable<Base>
{
    public string Name { get; }

    public Base(string name)
    {
        Name = name;
    }

    private bool EqualsHelper(Base left, Base right)
    {
        return left.Name == null
            ? left.Name.Equals(right.Name)
            : right.Name == null;
    }

    public override bool Equals(object other)
    {
        var otherBase = other as Base;

        return Equals(otherBase);
    }

    public virtual bool Equals(Base other)
    {
        //We only have to cover the case where
        //this is Base and the other is Derived
        //since if this is not Base the appropriate
        //virtual method will be called
    
        if (other == null)
            return false;

        if (ReferenceEquals(this, other))
            return true;

        if (other.GetType() != typeof(Base))
            return false;

        return EqualsHelper(this, other);
    }
}

public class Derived1 : Base, IEquatable<Derived1>
{
    public Base Parent { get; }

    public Derived1(string name, Base parent)
        : base(name)
    {
        Parent = parent;
    }

    private bool EqualsHelper(Derived1 left, Derived1 right)
    {
        return left.Name.Equals(right.Name)
            && left.Parent != null
            ? left.Parent.Equals(right.Parent)
            : right.Parent == null;
    }

    public bool Equals(Derived1 other)
    {
        if (other == null)
            return false;

        if (ReferenceEquals(this, other))
            return true;

    if (other.GetType() != typeof(Derived1))
        return false;

        return EqualsHelper(this, other);
    }

    public override bool Equals(Base other)
    {
        var otherDerived1 = other as Derived1;

        return Equals(otherDerived1);
    }
}

public sealed class Derived2<T> : Base, IEquatable<Derived2<T>>
{
    public T Payload { get; }

    public Derived2(string name, T payload)
        : base(name)
    {
        Payload = payload;
    }

    private bool EqualsHelper(Derived2<T> left, Derived2<T> right)
    {
        return left.Name.Equals(right.Name)
            && EqualityComparer<T>.Default.Equals(
                left.Payload, 
                right.Payload);
    }

    public bool Equals(Derived2<T> other)
    {
        if (other == null)
            return false;

        if (ReferenceEquals(this, other))
            return true;

    if (other.GetType() != typeof(Derived2<T>))
        return false;

        return EqualsHelper(this, other);
    }

    public override bool Equals(Base other)
    {
        var otherDerived2 = other as Derived2<T>;

        return Equals(otherDerived2);
    }
}

[DavidArno]

Regarding the following two points:

Open issue: Should a record declare operator == and operator !=?
Does this mean that having value type members that do not have operator == defined should be an error or that the compiler should fall back to Equals?

Could this be controlled through attributes? Not happy with the attribute names here, as I suspect there are more precise terms that could be used. But lets say we have the following attributes available:

[EnforceDoubleEquals]   // This ought to be the default and so optional
[AllowEqualsAsFallback]
[ConsistentEquality]

Then for struct S {} and public sealed class R(S x, double y), the following would occur when these attributes were used:

// compilation error as S doesn't support ==
[EnforceDoubleEquals] public sealed class R(S x, double y); 

// compilation error as S doesn't support ==
public sealed class R(S x, double y); 

// == implements .Equals for x & == for y
[AllowEqualsAsFallback] public sealed class R(S x, double y); 

// == just calls .Equals
[ConsistentEquality] public sealed class R(S x, double y); 

[DavidArno]

Hmm, a few minutes after writing the above, I can now answer my own question: no, we shouldn't do that. I'd opt for implementing == and having it invoke == on the fields if possible, falling back to . Equals if the field's type doesn't itself support ==.

(Which just goes to show how argumentative I am. If no one turns up to argue with my position, I just argue with myself instead 😁)

[HaloFour]

Good opportunity for source generators too, if the compiler only auto-implements those operators/members if they aren't explicitly provided.

[gafter]

We want to remove the restriction that record types be either abstract or sealed.

[DavidArno]

@gafter,

Presumably they can still be abstract or sealed if wanted, you will just be removing the need for them to be one of those?

[gafter]

Yes, I should have said "requirement" rather than "restriction".