Equals/GetHashCode override warning in derived class with no state

Question

I've created a strongly-typed, immutable wrapper class for various string IDs that flow through our system

The abstract BaseId class:

(some error-checking and formatting omitted for brevity...)

public abstract class BaseId
{
    // Gets the type name of the derived (concrete) class
    protected abstract string TypeName { get; }

    protected internal string Id { get; private set; }

    protected BaseId(string id) { Id = id; }

    // Called by T.Equals(T) where T is a derived type
    protected bool Equals(BaseId other)
    {
        if (ReferenceEquals(null, other))
            return false;
        if (ReferenceEquals(this, other))
            return true;
        return String.Equals(Id, other.Id);
    }

    // warning CS0660 (see comment #1 below)
    //public override bool Equals(object obj) { return base.Equals(obj); }

    public override int GetHashCode()
    {
        return TypeName.GetHashCode() * 17 + Id.GetHashCode();
    }

    public override string ToString()
    {
        return TypeName + ":" + Id;
    }

    // All T1 == T2 comparisons come here (where T1 and T2 are one
    // or more derived types)
    public static bool operator ==(BaseId left, BaseId right)
    {
        // Eventually calls left.Equals(object right), which is
        // overridden in the derived class
        return Equals(left, right);
    }

    public static bool operator !=(BaseId left, BaseId right)
    {
        // Eventually calls left.Equals(object right), which is
        // overridden in the derived class
        return !Equals(left, right);
    }
}

My goal was keep as much of the implementation in the base class so that the derived classes would be small, consisting mostly/entirely of boilerplate code.

Example concrete DerivedId class:

Note that this derived type defines no additional state of its own. Its purpose is solely to create a strong type.

public sealed class DerivedId : BaseId, IEquatable<DerivedId>
{
    protected override string TypeName { get { return "DerivedId"; } }

    public DerivedId(string id) : base(id) {}

    public bool Equals(DerivedId other)
    {
        // Method signature ensures same (or derived) types, so
        // defer to BaseId.Equals(object) override
        return base.Equals(other);
    }

    // Override this so that unrelated derived types (e.g. BarId)
    // NEVER match, regardless of underlying Id string value
    public override bool Equals(object obj)
    {
        // Pass obj or null for non-DerivedId types to our
        // Equals(DerivedId) override
        return Equals(obj as DerivedId);
    }

    // warning CS0659 (see comment #2 below)
    //public override int GetHashCode() { return base.GetHashCode(); }
}

Each class is generating a compiler warning:

1. Not overriding Object.Equals(object o) in BaseId generates a compile warning:
   
   warning CS0660: 'BaseId' defines operator == or operator != but does not override Object.Equals(object o)
   
   But if I implement BaseId.Equals(object o), it'll just be to call the base class implementation in Object.Equals(object o). I don't see how this will ever get called anyway; it's always overridden in the derived class, and the implementation there doesn't call up to this implementation.

2. Not overriding BaseId.GetHashCode() in DerivedId generates a compile warning:
   
   warning CS0659: 'DerivedId' overrides Object.Equals(object o) but does not override Object.GetHashCode()
   
   This derived class has no additional state, so there's nothing for me to do in an implementation of DerivedId.GetHashCode(), except to call the base class implementation in BaseId.GetHashCode().
   
   

I can suppress the compiler warnings or just implement the methods and have them call the base class implementations, but I want to make sure I'm not missing something.

Is there something odd about the way I did this, or is this just one of those things that you have to do to suppress warnings on otherwise correct code?

Answer 1

The reason those are warnings rather than errors is that the code will still work (probably), but it might do things that you don't expect. The warning is a big red flag that says, "Hey! You might be doing something bad here. You might want to take another look at it."

As it turns out, the warning is right on.

In this particular case, it's possible that some code can call Object.Equals(object) on one of your BaseId objects. For example, somebody could write:

bool CompareThings(BaseId thing, object other)
{
    return thing.Equals(other);
}

The compiler will generate a call to Object.Equals(object) because your BaseId type doesn't override it. That method will do the default comparison, which is the same as Object.ReferenceEquals(object). So you have two different meanings of Equals. You need to override Object.Equals(object) and have it call Equals(BaseId) after checking that the object being compared is indeed of type BaseId.

In the second case, you're right: there probably isn't a need to override GetHashCode, since the object doesn't define any new fields or do anything that changes the meaning of Equals. But the compiler doesn't know that. Sure, it knows that you didn't add any fields, but you did override Equals, meaning that you potentially changed the meaning of equality. And if you changed the meaning of equality, then you very likely changed (or should change) how hash codes are computed.

Not handling equality properly is a very common cause of mistakes when designing new types. It's a good thing that the compiler is overly cautious in this area.

Answer 2

It is generally not good for classes to have more than one overridable (virtual or abstract) Equals method. Either have derived classes override Equals(object) themselves, or else have a sealed base implementation of Equals(object) (and possibly GetHashCode()) chain to an abstract or virtual Equals(BaseId) (and possibly GetDerivedHashCode()). It's unclear what exactly your goal is, though I would suggest that if things always supposed to be equal if ID and type both match, and unequal if ID or type doesn't match, your base types shouldn't need to include any equality checking; simply have the base equality check test whether the types match (probably using GetType() rather than TypeName).

I should mention, btw, that I generally dislike classes which overload == and != unless they are supposed to fundamentally behave as values. In C#, the == operator can either call an overloaded equality-check operator or test reference equality; compare the effects of:

static bool IsEqual1<T>(T thing1, thing2) where T:class 
{
  return thing1 == thing2;
}

static bool IsEqual2<T>(T thing1, thing2) where T:BaseId
{
  return thing1 == thing2;
}

The first method above will perform a reference equality test even if T overloads the equality-check operator. In the second, it will use BaseId's overload. Visually, it's not exactly clear that the BaseId constraint should have such an effect, but it does. In vb.net, there would be no confusion since vb.net would not allow the overloadable equality-test operator in the IsEqual1; if a reference-equality test was desired in that method (or in the second, for that matter), code would have to use the Is operator. Since C# uses the same token as both a reference-equality test and an overloadable equality test, however, the binding of the == token isn't always obvious.

Answer 3

Addressing issue #2 in the question:

Not overriding BaseId.GetHashCode() in DerivedId generates a compile warning:

Run the following code with the GetHashCode() method commented out, then again without commenting it out, you'll see that when there is no implementation of GetHashCode the set contains two instances of Person, but when you add the implementation of GetHashCode the set contains only one instance demonstrating that some operations/classes use GetHashCode for comparison.

---

class Program
{
    static void Main(string[] args)
    {
        Person p1 = new Person() { FirstName="Joe", LastName = "Smith"};
        Person p2 = new Person() { FirstName="Joe", LastName ="Smith"};

        ISet<Person> set = new HashSet<Person>();
        set.Add(p1);
        set.Add(p2);
        foreach (var item in set)
        {
            Console.WriteLine(item.FirstName);
        }
    }

}
class Person
{
    public string FirstName { get; set; } 
    public string LastName { get; set; }

    public override bool Equals(object obj)
    {
        if (obj == null) return false;
        var that = obj as Person;
        if (that == null) return false;

        return 
               FirstName == that.FirstName &&
               LastName == that.LastName;
    }

    public override int GetHashCode() //run the code with and without this method
    {
        int hashCode = 1938039292;
        hashCode = hashCode * -1521134295 + EqualityComparer<string>.Default.GetHashCode(FirstName);
        hashCode = hashCode * -1521134295 + EqualityComparer<string>.Default.GetHashCode(LastName);
        return hashCode;
    }
}