Every other year I make the mistake of including mutable instance properties in the hashCode computation (in Java). If you modify one the properties are an object was used as a key in a HashMap (or HashSet) you usually don’t find the object any more since its hash code has changed. It would be nice if there were a rule that triggers if mutable instance properties are used for hashCode.
Hi Thorsten,
Thank you for your rule suggestion.
Let’s say we have two objects, myObject1 and myObject2 that have the mutable property someProp. We create them in a way, such that both objects’ hash sums are equal. We add myObject1 to a HashSet/HashMap. Now, we change myObject1.someProp to a different value, so that the hash codes are different if we include that property in the hash sum generation.
Are you suggesting that you would like to be able to find the modified myObject1 in the set/map using myObject2, despite the objects not being equal anymore?
The documentation for java.lang.Object defines part of the contract for hashCode as follows:
If two objects are equal according to the
equals(Object)method, then calling thehashCodemethod on each of the two objects must produce the same integer result.
So changing a mutable property of an object should also have an effect on the hash sum of that object, as long as you also include that property in the equals method.
Now of course you can choose to exclude a particular property from both equals and hashCode computations, if you like. However, whether or not you happen to want to exclude a mutable property depends heavily on the use case and implementing a rules that would simply trigger on any usages of mutable properties in hashCode would generate an immense amount of false positives.
If you happen to develop in a manner in which you exclusively want immutable properties to be used in your hashCode and equals methods, I suggest you have a look at implementing a custom rule for SonarJava. I hope this helps!
The HashMap case is probably of less importance. But the HashSet case is reall crucial because it leads to hard to find errors:
Set<A> set = new HashSet<A>();
A a = new A();
a.property = 41;
set.add(a);
a.property = 42;
assert set.contains(a); // this will fail in almost all cases if property is used for hashcode
And since you have no idea if and when an object is being used I strongly believe that a real rule would be quite useful. People can still decide whether they want to enable it or not.
Hello Thorsten,
Thanks for elaborating, I see what you mean now. I agree with you, this is a tricky case in Java and indeed has potential for a rule. I am not sure about the exact logic yet. I believe, if we would simply trigger whenever a non-final property was used in hashCode, we would get too many findings that could be considered false positives, as people may very well intend to do this and don’t want to modify these instances once they are in a map.
Implementations from the standard library exhibit this behavior themselves. Take an ArrayList, it is mutable and its hashCode will change with every changed element.
So to make this rule valuable and avoid too much noise, it would - at minimum - need to check two conditions and only trigger if both are met:
- The hashCode of a given class is non-constant (e.g. because it depends on mutable fields).
- The same class is used in collections such as a HashSet or as key in a HashMap.
It would be even better if the rule could detect that you are actually modifying objects while they are in one of those collection types mentioned above. However, now we are starting to get into pretty complex static analysis.
Perhaps we will see that this thread gains more traction, indicating that there is a broader need for this rule. In any case, I will note down this idea and maybe we can come up with something that is a good balance of finding these issues while not raising too many FPs in the future. If you don’t want to wait that long and instead want to try something yourself, you are of course more than welcome to open a PR on the Java analyzer.
IntelliJ IDEA has a Java code inspection which can detect such issues:
"Non-final field referenced in hashCode()"
and ReSharper has a similar inspection for C#: