GetHashCode and Equals are implemented incorrectly in System.Attribute?

Question

Seeing from Artech's blog and then we had a discussion in the comments. Since that blog is written in Chinese only, I'm taking a brief explanation here. Code to reproduce:

[AttributeUsage(AttributeTargets.Class, Inherited = true, AllowMultiple = true)]
public abstract class BaseAttribute : Attribute
{
    public string Name { get; set; }
}

public class FooAttribute : BaseAttribute { }

[Foo(Name = "A")]
[Foo(Name = "B")]
[Foo(Name = "C")]
public class Bar { }

//Main method
var attributes = typeof(Bar).GetCustomAttributes(true).OfType<FooAttribute>().ToList<FooAttribute>();
var getC = attributes.First(item => item.Name == "C");
attributes.Remove(getC);
attributes.ForEach(a => Console.WriteLine(a.Name));

The code gets all FooAttribute and removes the one whose name is "C". Obviously the output is "A" and "B"? If everything was going smoothly you wouldn't see this question. In fact you will get "AC" "BC" or even correct "AB" theoretically (I got AC on my machine, and the blog author got BC). The problem results from the implementation of GetHashCode/Equals in System.Attribute. A snippet of the implementation:

  [SecuritySafeCritical]
  public override int GetHashCode()
  {
      Type type = base.GetType();

      //*****NOTICE*****
      FieldInfo[] fields = type.GetFields(BindingFlags.NonPublic 
            | BindingFlags.Public 
            | BindingFlags.Instance);

      object obj2 = null;
      for (int i = 0; i < fields.Length; i++)
      {
          object obj3 = ((RtFieldInfo) fields[i]).InternalGetValue(this, false, false);
          if ((obj3 != null) && !obj3.GetType().IsArray)
          {
              obj2 = obj3;
          }
          if (obj2 != null)
          {
              break;
          }
      }
      if (obj2 != null)
      {
          return obj2.GetHashCode();
      }
      return type.GetHashCode();
  }

It uses Type.GetFields so the properties inherited from base class are ignored, hence the equivalence of the three instances of FooAttribute (and then the Remove method takes one randomly). So the question is: is there any special reason for the implementation? Or it's just a bug?

Salinas

I'd say it's a bug, though I have difficulty imaging a real-world scenario where it would cause a problem. You could consider reporting it on connect.microsoft.com. – Asylum

The bug is in Equals(), it's okay for GetHashCode() to return identical values. Agreed, post this at connect. I actually doubt they'll fix it because that would be a breaking change. – Heinous

@HansPassant: You are correct. Here I post the code of GetHashCode just because the author post the code and I have no disassemblier in this pc. – Salinas

@Joe: Correct, that's why no one found this issue ever before. And in real world, we always put a Where expression between OfType<FooAttribute>() and ToList, avoiding this issue in accident. – Salinas

What's interesting is that Jeffrey Richter in his "CLR via C#" 3rd edition says: "... Equals uses reflection to compare the values of the two attribute objects' fields". Immediately after that he starts talking about overriding a virtual Match method "to provide richer semantics". This is interesting because in previous section describing attribute construction and "serialization" in metadata he explicitly mentions properties as well as fields. – Cologarithm

It's not a bug. You could argue it's a design flaw, or you could argue it isn't, but it's not a bug. – Unrequited

Answer 1

A clear bug, no. A good idea, perhaps or perhaps not.

What does it mean for one thing to be equal to another? We could get quite philosophical, if we really wanted to.

Being only slightly philosophical, there are a few things that must hold:

1. Equality is reflexive: Identity entails equality. x.Equals(x) must hold.
2. Equality is symmetric. If x.Equals(y) then y.Equals(x) and if !x.Equals(y) then !y.Equals(x).
3. Equality is transitive. If x.Equals(y) and y.Equals(z) then x.Equals(z).

There's a few others, though only these can directly be reflected by the code for Equals() alone.

If an implementation of an override of object.Equals(object), IEquatable<T>.Equals(T), IEqualityComparer.Equals(object, object), IEqualityComparer<T>.Equals(T, T), == or of != does not meet the above, it's a clear bug.

The other method that reflects equality in .NET are object.GetHashCode(), IEqualityComparer.GetHashCode(object) and IEqualityComparer<T>.GetHashCode(T). Here there's the simple rule:

If a.Equals(b) then it must hold that a.GetHashCode() == b.GetHashCode(). The equivalent holds for IEqualityComparer and IEqualityComparer<T>.

If that doesn't hold, then again we've got a bug.

Beyond that, there are no over-all rules on what equality must mean. It depends on the semantics of the class provided by its own Equals() overrides or by those imposed upon it by an equality comparer. Of course, those semantics should either be blatantly obvious or else documented in the class or the equality comparer.

In all, how does an Equals and/or a GetHashCode have a bug:

1. If it fails to provide the reflexive, symmetric and transitive properties detailed above.
2. If the relationship between GetHashCode and Equals is not as above.
3. If it doesn't match its documented semantics.
4. If it throws an inappropriate exception.
5. If it wanders off into an infinite loop.
6. In practice, if it takes so long to return as to cripple things, though one could argue there's a theory vs. practice thing here.

With the overrides on Attribute, the equals does have the reflexive, symmetric and transitive properties, it's GetHashCode does match it, and the documentation for it's Equals override is:

This API supports the .NET Framework infrastructure and is not intended to be used directly from your code.

You can't really say your example disproves that!

Since the code you complain about doesn't fail on any of these points, it's not a bug.

There's a bug though in this code:

var attributes = typeof(Bar).GetCustomAttributes(true).OfType<FooAttribute>().ToList<FooAttribute>();
var getC = attributes.First(item => item.Name == "C");
attributes.Remove(getC);

You first ask for an item that fulfills a criteria, and then ask for one that is equal to it to be removed. There's no reason without examining the semantics of equality for the type in question to expect that getC would be removed.

What you should do is:

bool calledAlready;
attributes.RemoveAll(item => {
  if(!calledAlready && item.Name == "C")
  {
    return calledAlready = true;
  }
});

That is to say, we use a predicate that matches the first attribute with Name == "C" and no other.

Unrequited

The expectation that == should have any relationship to Equals is IMHO unhelpful, given the extent to which built-in types' behavior with == doesn't even represent an equivalence relation (floating-point types can't even manage reflexivity, and the way implicit typecasts work the only way to make comparisons between long and double be transitive would be to define (long,double) and (double,long) overloads of ==. Perhaps the Framework should have made == be an equivalence relation in all cases where it compiles, but since it doesn't, I think it best... – Sectionalize

...to view == and Equals to be entirely independent concepts which may have some overlap but have no real relation. – Sectionalize

Answer 2

Yep, a bug as others have already mentioned in the comments. I can suggest a few possible fixes:

Option 1, Don't use inheritence in the Attribute class, this will allow the default implementation to function. The other option is use a custom comparer to ensure you are using reference equality when removing the item. You can implement a comparer easily enough. Just use Object.ReferenceEquals for comparison and for your use you could use the type's hash code or use System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode.

public sealed class ReferenceEqualityComparer<T> : IEqualityComparer<T>
{
    bool IEqualityComparer<T>.Equals(T x, T y)
    {
        return Object.ReferenceEquals(x, y);
    }
    int IEqualityComparer<T>.GetHashCode(T obj)
    {
        return System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(obj);
    }
}

Bacchus

What's buggy about it? In what way does it either not meet the general requirements for Equals and GetHashCode or else fail to match the documentation for Attribute.Equals? – Unrequited

A more questionable case is the fact that RuntimeHelpers.GetHashCode won't work as you intend if you run it on Windows Phone 7. Here the "is not intended to be used directly..." exists on MSs response to the bug report, and not in the actual docs. I'd recommend emitting IL like I do at github.com/hackcraft/Ariadne/blob/master/Collections/… rather than using it. – Unrequited

@Jon Hanna, A bug since the intent was a field-value comparison and does not function properly. As for WP7, you are correct the API is not supported; however, the OP makes no mention of a WP7 target. – Bacchus

How do you know the intent isn't to ignore certain fields on base attributes? All overrides of Equals ignore some way in which the two operands are not the same, after all. I've no target market of WP7 either, but one might as well go for a method that works on it rather than one that doesn't if you can't see a strong reason otherwise, which I can't. – Unrequited