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

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 m开发者_运维技巧y 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.

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.

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.

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.

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);
}