2

Since R# 2017.1.1 I get warnings in the automatically generated GetHashCode() function. Let me explain how this function is created:

If you override the Equals function of a class, R# suggests you to create equality members. If you let R# generate those equality members, it overrides the GetHashCode() function as well. In there however it uses all the properties of my class. As those properties aren't all read-only R# tells me that I should make them readonly and displays a warning.

So my question is wether I should just leave the GetHashCode() function empty (or delete those parts with warnings) or wether I should try to make the properties readonly so that R# isn't warning me anymore.

Here's my code:

public override bool Equals(object obj) => obj is RamEntry && Equals((RamEntry) obj);

public override int GetHashCode()
{
    unchecked
    {
        var hashCode = (Value != null ? Value.GetHashCode() : 0);   //In those 5 lines the warnings are being displayed
        hashCode = (hashCode * 397) ^ Unimportant.GetHashCode();    //-------------------------------------------------
        hashCode = (hashCode * 397) ^ (Comment?.GetHashCode() ?? 0);//-------------------------------------------------
        hashCode = (hashCode * 397) ^ (Address?.GetHashCode() ?? 0);//-------------------------------------------------
        hashCode = (hashCode * 397) ^ LastUpdated.GetHashCode();    //-------------------------------------------------
        return hashCode;
    }
}

private bool Equals(RamEntry other)
    => string.Equals(Value, other.Value) && Unimportant == other.Unimportant &&
       string.Equals(Comment, other.Comment) && string.Equals(Address, other.Address);

This is the warning R# gives me if I delete the GetHashCode() function: The warning R# gives me

And this is the warning it gives me with the GetHashCode() function: The other warning from R#

MetaColon
  • 2,895
  • 3
  • 16
  • 38

2 Answers2

7

I think it's better explain on example why you should override GetHashCode when you override equality members, and also why you should not use mutable fields\properties when calculating hash code. Consider this simple class:

public class Test {
    public string First { get; set; }
    public string Second { get; set; }
}

Now you decide that two Test objects are equal if their First and Second properties are equal. You generate equality members but do not override GetHashCode:

public class Test {
    public string First { get; set; }
    public string Second { get; set; }
    public override bool Equals(object obj) {
        return obj is Test && Equals((Test)obj);
    }

    protected bool Equals(Test other) {
        return string.Equals(First, other.First) && string.Equals(Second, other.Second);
    }

    public static bool operator ==(Test left, Test right) {
        return Equals(left, right);
    }

    public static bool operator !=(Test left, Test right) {
        return !Equals(left, right);
    }
}

Then later you use your Test objects in something like Hashset.

var test1 = new Test();
test1.First = "a";
test1.Second = "b";
var set = new HashSet<Test>();
set.Add(test1);
var test2 = new Test();
test2.First = "a";
test2.Second = "b";
bool equal = test1 == test2; // true
bool contains = set.Contains(test2); // nope, false

Things went wrong - set clearly contains an item which is equals to test2, but we failed to find it, because we did not override GetHashCode (here is a recent example of how things went badly wrong when this is violated: https://stackoverflow.com/a/43356217/5311735). So we implement it:

public override int GetHashCode() {
    unchecked {
        return ((First != null ? First.GetHashCode() : 0) * 397) ^ (Second != null ? Second.GetHashCode() : 0);
    }
}

Now our problem is fixed and set.Contains return true. So far so good, but we are using mutable properties to calculate hash code. So consider this:

var test1 = new Test();
test1.First = "a";
test1.Second = "b";
var set = new HashSet<Test>();
set.Add(test1);
test1.First = "c";
var contains = set.Contains(test1); // nope, false

So we added item to hashset, modified one property and now set.Contains returns false for the exact same item we added couple of lines before. That is hash code has changed from the moment we placed item into a set and the moment we call Contains.

If you don't want such surprises - always override GetHashCode when you override equality members and do not use mutable fields\properties there.

Community
  • 1
  • 1
Evk
  • 98,527
  • 8
  • 141
  • 191
  • nice explanation, but in his case IMHO his implementation of `Equals()` doesn't decisively differ to the Base-Class's functionality, other than your example. So would you agree, that the OP would not necessarily implement `GetHashCode()` ? – LuckyLikey Apr 12 '17 at 13:35
  • @LuckyLikey base implementation of `Equals` will just compare if object instances are the same (that is - point to the same memory location). That's not the case here - two `RamEntry` are considered equal if those properties (like `Comment`, `Address` and so on) are equal. If they are of primitive types (int, string and so on) or they are structs, or they are classes which also correctly implement `Equals` - this is not the same as base implementation. So his case it basically the same as I described in my answer. If RamEntry1 == RamEntry2 then their hash codes should be equal too. – Evk Apr 12 '17 at 13:46
  • whoups, you are totally right. I have overseen the private part of the Equals Method, which really points out the necessity of this implementation. Thumbs up man. – LuckyLikey Apr 12 '17 at 14:09
  • 1
    @Evk so how to implement `GetHashCode` in this case if there are no immutable properties? – Konrad Aug 08 '18 at 08:09
  • 1
    @Konrad well it's better to not do that at all. If changing the design is not possible and you absolutely must store objects with mutable properties in hashset or similar - you have to be prepared to face subtle bugs as described in this answer. There is no "good" way to implement GetHashCode in case of mutable properties. – Evk Aug 08 '18 at 08:44
  • 1
    @Konrad if you must store such object in HashSet - implement GetHashCode as usual, using mutable properties. But then, after you place object in HashSet, you must take care to not modify properties any more. You can implement "freezable" functionality for such object - after you place it in HashSet, "freeze" this object in a way that attempt to modify any property will throw an exception. – Evk Aug 08 '18 at 08:50
2

You should either delete the whole GetHashCode() Stuff or implement it properly if you need its functionality.

If you only want to test for equality, you don't need it (Source):

Do not test for equality of hash codes to determine whether two objects are equal. (Unequal objects can have identical hash codes.) To test for equality, call the ReferenceEquals or Equals method

If you wonder when it should be implemented consider this link, which also contains basic implementation guidelines.

Why not-Readonly Members in GetHashCode() give warnings

Generally GetHashCode() is used in - oh what a miracle - in Hashtables or Dictionarys. That's why you should ensure, that the hashcode at least doesn't change as long as the object is inside a collection and stuff which might rely on that value. That's why you get a Warning on not readonly members.

You can override GetHashCode for immutable reference types. In general, for mutable reference types, you should override GetHashCode only if:
– You can compute the hash code from fields that are not mutable; or – You can ensure that the hash code of a mutable object does not change while the object is contained in a collection that relies on its hash code.

Why does not overriding GetHashCode() give a warning?

I mean that's easy, imagine if you really had implemented a whole new strategy of when an Object is equal to another, you propably had a special hashcode-creation.

Edit:

Actually the code part in your Equals Method really needs an implementation of GetHashCode(). Because equality is now no more depending on the Object-Instance on the Heap. So as @Evk points out, this is clearly a whole new Strategy of comparing objects.

Community
  • 1
  • 1
LuckyLikey
  • 3,504
  • 1
  • 31
  • 54
  • Actually I wouldn't implement it by myself, because I can't see any use of it for me, but if I delete it, R# gives me a warning ('TRDebugger.Types.RamEntry' overrides Object.Equals(object o) but does not override Object.GetHashCode()). I'll udpate my question to make this clear – MetaColon Apr 12 '17 at 11:02
  • @MetaColon I Updated my answer to more fit your Question regarding those warnings – LuckyLikey Apr 12 '17 at 11:29