views:

320

answers:

2

Anyone have any opinions on whether or not IEquatable or IComparable 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. 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 and IComparable 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 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, since the field could potentially store subtypes of T and I doubt most implementations of IEquatable are designed appropriately for inheritance.

+4  A: 

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.

JaredPar
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.
Marc Gravell
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.
Phil
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
Phil
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.
Phil
A: 

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.

ninj
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!
Phil
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>?
Phil
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...
ninj