Should IEquatable<T>, IComparable<T> be implemented on non-sealed classes?

Question

Anyone have any opinions on whether or not IEquatable<T> or IComparable<T> should generally require that T is sealed (if it's a class)?

This question occurred to me since I'm writing a set of base classes intended to aid in the implementation of immutable classes. Part of the functionality which the base class is intended to provide is automatic implementation of equality comparisons (using the class's fields together with attributes which can be applied to fields to control equality comparisons). It should be pretty nice when I'm finished - I'm using expression trees to dynamically create a compiled comparison function for each T, so the comparison function should be very close to the performance of a regular equality comparison function. (I'm using an immutable dictionary keyed on System.Type and double check locking to store the generated comparison functions in a manner that's reasonably performant)

One thing that has cropped up though, is what functions to use to check equality of the member fields. My initial intention was to check if each member field's type (which I'll call X) implements IEquatable<X>. However, after some thought, I don't think this is safe to use unless X is sealed. The reason being that if X is not sealed, I can't know for sure if X is appropriately delegating equality checks to a virtual method on X, thereby allowing a subtype to override the equality comparison.

This then brings up a more general question - if a type is not sealed, should it really implement these interfaces AT ALL?? I would think not, since I would argue that the interfaces contract is to compare between two X types, not two types which may or may not be X (though they must of course be X or a subtype).

What do you guys think? Should IEquatable<T> and IComparable<T> be avoided for unsealed classes? (Also makes me wonder if there is an fxcop rule for this)

My current thought is to have my generated comparison function only use IEquatable<T> on member fields whose T is sealed, and instead to use the virtual Object.Equals(Object obj) if T is unsealed even if T implements IEquatable<T>, since the field could potentially store subtypes of T and I doubt most implementations of IEquatable<T> are designed appropriately for inheritance.

Cephalization

Answer 1

I've been thinking about this question for a bit and after a bit of consideration I agree that implementing IEquatable<T> and IComparable<T> should only be done on sealed types.

I went back and forth for a bit but then I thought of the following test. Under what circumstances should the following ever return false? IMHO, 2 objects are either equal or they are not.

public void EqualitySanityCheck<T>(T left, T right) where T : IEquatable<T> {
  var equals1 = left.Equals(right);
  var equals2 = ((IEqutable<T>)left).Equals(right);
  Assert.AreEqual(equals1,equals2);
}

The result of IEquatable<T> on a given object should have the same behavior as Object.Equals assuming the comparer is of the equivalent type. Implementing IEquatable<T> twice in an object hierarchy allows for, and implies, there are 2 different ways of expressing equality in your system. It's easy to contrive any number of scenarios where IEquatable<T> and Object.Equals would differ since there are multiple IEquatable<T> implementations but only a single Object.Equals. Hence the above would fail and create a bit of confusion in your code.

Some people may argue that implementing IEquatable<T> at a higher point in the object hierarchy is valid because you want to compare a subset of the objects properties. In that case you should favor an IEqualityComparer<T> which is specifically designed to compare those properties.

Bareheaded

Indeed - the main time I've had to do this is on structs, or immutable classes that would be structs if they weren't overweight. – Capybara

Exactly Jared. I'd add to what you've said that if you were to attempt to implement IEquatable<T> in an inheritance safe way, you would have to provide a virtual bool IsEqual(T obj) method, and require that each time overriden by a subtype, that the subtype's override first calls base.IsEqual(T obj) and returns false if the base method returns false. Of course then you look at it, and ask yourself given the existence of virtual object.Equals(object obj) maybe you could just use this instead. You take an additional cast hit on the first subtype but additional descendents have to cast anyway. – Cephalization

The more I look at it, the less sense implementing IEquatable<T> over object.Equals on non-sealed classes makes. For value types, which of course are automatically sealed anyway, you avoid boxing and casting, so a nice perf improvement there. For classes the only perf improvement I can see is avoiding a cast, but if you haven't sealed your class it should be designed to be inheritance safe, and you'll end up potentially casting on subtypes anyway. The biggest argument against implementing on non-sealed classes though is (I think) that the implicit contract is comparison of T, not subtypes – Cephalization

One more thing... it makes me wonder if Microsoft should add the ability to state a 'sealed' constraint to generics. Then when specifying an interface such as IEquatable<T> you could constraint T to be a sealed type. – Cephalization

@Phil: If IEquatable<T> included GetHashCode, it might be useful to define to compare things as type T, rather than as their own type. Since it doesn't, however, it's not possible for IEquatable<T> to differ much from Object.Equals. BTW, I wish Microsoft had included an IIgnoreEquatable interface (no members) and made EqualityComparer.Default ignore any IEquatable interface of a class which implemented IIgnoreEquatable. Such a thing would allow the design of safe inheritable immutable classes. – Tallahassee

But if you implement IEquatable<T> on a base class but not on the derived class, then derived1.Equals(derived2) and Object.Equals(derived1, derived2) can return different values. – Helman

Isn't it OK to implement IEquatable<> on a non-sealed class if you remember to check for GetType() != other.GetType() in the implementation of bool Equals(MyClass other)? I think that is the recommendation. – Declaration

@JeppeStigNielsen: That's not sufficient. If BaseObject:IEquatable<BaseObject>, and DerivedObject:BaseObject has some additional property, the only way to ensure that two instances of DerivedObject which differ only in that extra property will be recognized as different for each other is for IEquatable<BaseObject> to chain to a virtual method (typically Equals) which then has to cast the objects to the derived type. – Tallahassee

@Tallahassee You are right. It will still be the responsibility of DerivedObject to ovverride that method you mention. If they do not override it, it means DerivedObject decided that their new property shall not participare in Equals. With my "design", again if the authors of DerivedObject do nothing, their new property is not considered with Equals. However, with my "design" the authors of DerivedObject do not have much chance to change the behavior. Implementing IEquatable<DerivedObject> is not enough. – Declaration

@JeppeStigNielsen: One could have sealed Equals methods which check base-class properties before calling a virtual property to test other properties; such a design would be safe with IEquatable<T>, but if even IEquatable<T>.Equals is going to have to check objects' types, there's not really much advantage to using it rather than Equals(Object). – Tallahassee

Answer 2

I would generally recommend against implementing IEquatable<T> on any non-sealed class, or implementing non-generic IComparable on most, but the same cannot be said for IComparable<T>. Two reasons:

1. There already exists a means of comparing objects which may or may not be the same type: Object.Equals. Since IEquatable<T> does not include GetHashCode, its behavior essentially has to match that of Object.Equals. The only reason for implementing IEquatable<T> in addition to Object.Equals is performance. IEquatable<T> offers a small performance improvement versus Object.Equals when applied to sealed class types, and a big improvement when applied to structure types. The only way an unsealed type's implementation of IEquatable<T>.Equals can ensure that its behavior matches that of a possibly-overridden Object.Equals, however, is to call Object.Equals. If IEquatable<T>.Equals has to call Object.Equals, any possible performance advantage vanishes.
2. It is sometimes possible, meaningful, and useful, for a base class to have a defined natural ordering involving only base-class properties, which will be consistent through all subclasses. When checking two objects for equality, the result shouldn't depend upon whether one regards the objects as being a base type or a derived type. When ranking objects, however, the result should often depend upon the type being used as the basis for comparison. Derived-class objects should implement IComparable<TheirOwnType> but should not override the base type's comparison method. It is entirely reasonable for two derived-class objects to compare as "unranked" when compared as the parent type, but for one to compare above the other when compared as the derived type.

Implementation of non-generic IComparable in inheritable classes is perhaps more questionable than implementation of IComparable<T>. Probably the best thing to do is allow a base-class to implement it if it's not expected that any child class will need some other ordering, but for child classes not to reimplement or override parent-class implementations.

Tallahassee

I know this is an old topic, but still: I don't think your first point is true at all. object.Equals checks for reference equality, so the main reason for implementing IEquatable<T> in my opinion is to change that behavior and check for value equality instead (or however equality is defined for a certain type). As to performance: I have no proof of this, but I'm pretty sure that object.Equals beats most if not all self-made implementations since it only checks for reference equality (only on reference types of course). – Hayrick

@enzi: The static Object.Equals method performs a reference-equality check only if at least one argument is null. Otherwise it calls the first object's override of Object.Equals. Both that override of Object.Equals and the implementation of IEquatable<T>.Equals, if any, are expected to be consistent with GetHashCode(), which implies that unless GetHashCode() returns a constant, the two Equals methods must return somewhat-consistent results. If either Equals method test reference equality, both must do so. – Tallahassee

@enzi: Are you perhaps confused by the use of the term Object.Equals to refer to the overridable method, as opposed to the default implementation thereof? Would some other notation be clearer? Given Sillier:Silly:Foo, where Foo and Sillier declare public override bool Equals(Object Other) and Silly declares public new virtual bool Equals(Object Other). Then for a Sillier instance, I would say that Foo overrides Object.Equals and Sillier overrides Silly.Equals. There shouldn't be any Equals(Object) other than Object.Equals, but I use the term Object.Equals... – Tallahassee

...to make it clear that code should expect to be able to use that override for comparing things of arbitrary type, rather than looking for any other Equals method. – Tallahassee

I was referring to the default implementation of Object.Equals. I agree that if you override it and implement IEquatable<T>.Equals then both should return the same result and be consistent with GetHashCode. I was merely pointing out that the reason why you would override Object.Equals in the first place is not performance, but to change the default behavior of checking for reference equality. To me it isn't clear in your answer if you are referring to an overriden Object.Equals or not when you talk about the performance benefit of IEquatable.Equals. – Hayrick

… either way I believe that there's more to IEquatable<T> than a negligable performance gain (on reference types). By using this interface, e.g. in collections, I can guarantee that there can be no runtime cast exceptions because someone tried to compare my Apple with an Orange. Just a remark to the interface's general purpose, not related to whether or not to implement it in non-sealed classes (which I wouldn't). – Hayrick

@enzi: The IEquatable<T> interface exists primarily for performance reasons. The performance gains with class types are slight, but the performance gains on structure types can be significant. As for runtime cast exceptions, a properly written Object.Equals method should not throw an exception if used to compare an object to one of another type; it should simply return false. – Tallahassee

Answer 3

Most Equals implementations I've seen check the types of the objects being compared, if they aren't the same then the method returns false.

This neatly avoids the problem of a sub-type being compared against it's parent type, thereby negating the need for sealing a class.

An obvious example of this would be trying to compare a 2D point (A) with a 3D point (B): for a 2D the x and y values of a 3D point might be equal, but for a 3D point, the z value will most likely be different.

This means that A == B would be true, but B == A would be false. Most people like the Equals operators to be commutative, to checking types is clearly a good idea in this case.

But what if you subclass and you don't add any new properties? Well, that's a bit harder to answer, and possibly depends on your situation.

Cultivable

ninj, I'm not sure I get you, checking that the other object's type isn't sufficient - what if Equals(A obj) is called on type B? - perfectly possible when for example you have a List<A> since the collection can contain both A and subtype B. And, since you haven't sealed A you can't know what would make sense for Equals(A obj) when called on any subtype (incl. B). You're going to be forced to provide a virtual member so that subtypes can override behavior as appropriate. An alternative might be for Equals(A object) to check this.GetType() == typeof(A) and throw an exception if false. YUCK! – Cephalization

IMO, It keeps coming back to the contract for IEquatable<T> is to compare objects of type T, not subtypes. To guarantee this, T needs to be sealed. If alternatively, you don't agree with that, and want T to remain unsealed, you need to design the IEquatable<T> implementation to be overridable by a subtype, so you have to introduce an additional virtual member - but given system.object already has virtual object.Equals(object obj) would be there really be any point in implementing IEquatable<T>? – Cephalization

Oh, whoops. Good point, IEquatable<T> shouldn't really have to handle checking types (yes, it's icky to have to do so.) I was implementing a helper class for Equals and IEquatable a while ago, and eventually I took the chicken way out - I narrowed the purpose of IEquatable<T> from "type safe equals" to "what makes things like generic dictionaries work". No need to seal the types if you take this way out... – Cultivable

@Cephalization The correct thing to do is to check this.GetType() == other.GetType(), not this.GetType() == typeof(A). If the first requirement is not met, return false, don't throw! Never allow two instances to be considered equal if one is more derived than the other (and you won't know if this or other is the more derived one). As you (Phil) said yourself, "you can't know what would make sense for ... any subtype" of A. – Declaration

Answer 4

I have stumbled over this topic today when reading
https://blog.mischel.com/2013/01/05/inheritance-and-iequatable-do-not-mix/
and I agree, that there are reasons not to implement IEquatable<T>, because chances exist that it will be done in a wrong way.

However, after reading the linked article I tested my own implementation which I use on various non-sealed, inherited classes, and I found that it's working correctly.
When implementing IEquatable<T>, I referred to this article:
http://www.loganfranken.com/blog/687/overriding-equals-in-c-part-1/
It gives a pretty good explanation what code to use in Equals(). It does not address inheritance though, so I tuned it myself. Here's the result.

And to answer the original question:
I don't say that it should be implemented on non-sealed classes, but I say that it definitely could be implemented without problems.

//============================================================================
class CBase : IEquatable<CBase>
{
  private int m_iBaseValue = 0;

  //--------------------------------------------------------------------------
  public CBase (int i_iBaseValue)
  {
    m_iBaseValue = i_iBaseValue;
  }

  //--------------------------------------------------------------------------
  public sealed override bool Equals (object i_value)
  {
    if (ReferenceEquals (null, i_value))
      return false;
    if (ReferenceEquals (this, i_value))
      return true;

    if (i_value.GetType () != GetType ())
      return false;

    return Equals_EXEC ((CBase)i_value);
  }

  //--------------------------------------------------------------------------
  public bool Equals (CBase i_value)
  {
    if (ReferenceEquals (null, i_value))
      return false;
    if (ReferenceEquals (this, i_value))
      return true;

    if (i_value.GetType () != GetType ())
      return false;

    return Equals_EXEC (i_value);
  }

  //--------------------------------------------------------------------------
  protected virtual bool Equals_EXEC (CBase i_oValue)
  {
    return i_oValue.m_iBaseValue == m_iBaseValue;
  }
}

//============================================================================
class CDerived : CBase, IEquatable<CDerived>
{
  public int m_iDerivedValue = 0;

  //--------------------------------------------------------------------------
  public CDerived (int i_iBaseValue,
                  int i_iDerivedValue)
  : base (i_iBaseValue)
  {
    m_iDerivedValue = i_iDerivedValue;
  }

  //--------------------------------------------------------------------------
  public bool Equals (CDerived i_value)
  {
    if (ReferenceEquals (null, i_value))
      return false;
    if (ReferenceEquals (this, i_value))
      return true;

    if (i_value.GetType () != GetType ())
      return false;

    return Equals_EXEC (i_value);
  }

  //--------------------------------------------------------------------------
  protected override bool Equals_EXEC (CBase i_oValue)
  {
    CDerived oValue = i_oValue as CDerived;
    return base.Equals_EXEC (i_oValue)
        && oValue.m_iDerivedValue == m_iDerivedValue;
  }
}

Test:

private static void Main (string[] args)
{
// Test with Foo and Fooby for verification of the problem.
  // definition of Foo and Fooby copied from
  // https://blog.mischel.com/2013/01/05/inheritance-and-iequatable-do-not-mix/
  // and not added in this post
  var fooby1 = new Fooby (0, "hello");
  var fooby2 = new Fooby (0, "goodbye");
  Foo foo1 = fooby1;
  Foo foo2 = fooby2;

// all false, as expected
  bool bEqualFooby12a = fooby1.Equals (fooby2);
  bool bEqualFooby12b = fooby2.Equals (fooby1);
  bool bEqualFooby12c = object.Equals (fooby1, fooby2);
  bool bEqualFooby12d = object.Equals (fooby2, fooby1);

// 2 true (wrong), 2 false
  bool bEqualFoo12a = foo1.Equals (foo2);  // unexpectedly "true": wrong result, because "wrong" overload is called!
  bool bEqualFoo12b = foo2.Equals (foo1);  // unexpectedly "true": wrong result, because "wrong" overload is called!
  bool bEqualFoo12c = object.Equals (foo1, foo2);
  bool bEqualFoo12d = object.Equals (foo2, foo1);

// own test
  CBase oB = new CBase (1);
  CDerived oD1 = new CDerived (1, 2);
  CDerived oD2 = new CDerived (1, 2);
  CDerived oD3 = new CDerived (1, 3);
  CDerived oD4 = new CDerived (2, 2);

  CBase oB1 = oD1;
  CBase oB2 = oD2;
  CBase oB3 = oD3;
  CBase oB4 = oD4;

// all false, as expected
  bool bEqualBD1a = object.Equals (oB, oD1);
  bool bEqualBD1b = object.Equals (oD1, oB);
  bool bEqualBD1c = oB.Equals (oD1);
  bool bEqualBD1d = oD1.Equals (oB);

// all true, as expected
  bool bEqualD12a = object.Equals (oD1, oD2);
  bool bEqualD12b = object.Equals (oD2, oD1);
  bool bEqualD12c = oD1.Equals (oD2);
  bool bEqualD12d = oD2.Equals (oD1);
  bool bEqualB12a = object.Equals (oB1, oB2);
  bool bEqualB12b = object.Equals (oB2, oB1);
  bool bEqualB12c = oB1.Equals (oB2);
  bool bEqualB12d = oB2.Equals (oB1);

// all false, as expected
  bool bEqualD13a = object.Equals (oD1, oD3);
  bool bEqualD13b = object.Equals (oD3, oD1);
  bool bEqualD13c = oD1.Equals (oD3);
  bool bEqualD13d = oD3.Equals (oD1);
  bool bEqualB13a = object.Equals (oB1, oB3);
  bool bEqualB13b = object.Equals (oB3, oB1);
  bool bEqualB13c = oB1.Equals (oB3);
  bool bEqualB13d = oB3.Equals (oB1);

// all false, as expected
  bool bEqualD14a = object.Equals (oD1, oD4);
  bool bEqualD14b = object.Equals (oD4, oD1);
  bool bEqualD14c = oD1.Equals (oD4);
  bool bEqualD14d = oD4.Equals (oD1);
  bool bEqualB14a = object.Equals (oB1, oB4);
  bool bEqualB14b = object.Equals (oB4, oB1);
  bool bEqualB14c = oB1.Equals (oB4);
  bool bEqualB14d = oB4.Equals (oB1);
}

Windsucking