Check for arrays passed to Objects.hashCode()

[cushon]

Original issue created by geb@google.com on 2013-11-22 at 10:18 PM

---

The javadoc for Objects.hashCode() contains the following:

"Note that array arguments to this method, with the exception of a single Object array, do not get any special handling; their hash codes are based on identity and not contents."

In other words, since an array's hashCode() method derives from Object.hashCode(), calling Object.hashCode(array) will yield the hash derived from the array's object reference. This is almost certainly not what the author intended: what they actually wanted was a hash derived from each of the array's elements. This can lead to subtle bugs because the hash code derived from the array is not stable; for example it can change if the array is recomputed.

The fix is to call Arrays.hashCode(array) instead. Hence I propose that whenever the analyzer detects Objects.hashCode() being called with a single array argument, it should suggest calling Arrays.hashCode() instead.

For motivation, see b/11825155 for an example bug which could have easily been avoided with this check. For the CL that implements the fix, see cl/57133395.

[cushon]

Original comment posted by eaftan@google.com on 2014-01-30 at 12:22 AM

---

This is very similar to the ArrayEquals check. Would be easy to extend that check or fork it for this case.

---

Labels: -Priority-Medium, Priority-High

[cushon]

Original comment posted by kevinb@google.com on 2014-01-30 at 12:25 AM

---

Note that this must apply to both java.util.Objects and com.google.common.base.Objects (they're identical).

[cushon]

Original comment posted by eaftan@google.com on 2014-01-30 at 01:44 AM

---

(No comment entered for this change.)

---

Status: Started Owner: eaftan@google.com

[cushon]

Original comment posted by eaftan@google.com on 2014-01-30 at 01:45 AM

---

Issue #160 has been merged into this issue.

[cushon]

Original comment posted by kevinb@google.com on 2014-01-30 at 08:26 PM

---

To amend my previous comment I am referring to java.util.Objects.hash, and c.g.c.g.Objects.hashCode.

[cushon]

Original comment posted by eaftan@google.com on 2014-01-30 at 10:21 PM

---

As usual, this turns out to be more complicated than you'd think.

1) java.util.Objects has two hashcode methods: java.util.Objects#hashCode(Object o) java.util.Objects#hash(Object... o)

The first one is always incorrect when called on any array (Object or otherwise). I'll talk about the second one below.

2) Both java.util.Objects#hash and com.google.common.base.Objects#hashCode take a varargs array of Object. Because of how varargs work, if you pass in a single argument of a certain type, here's what you get: Object[] --> correct String[] --> correct (the String[] array is implicitly cast to Object[]) int[] --> incorrect (the int[] array is boxed into a single element Object[] array, and the identity hashcode of that array is returned)

The question is whether we want to allow the Object[] case, since it's equivalent to Arrays.hashCode() and is less clear. I'm inclined to just let it be and detect the clearly-wrong case. It's easier to justify the check to the user if it detects only incorrect code.

[cushon]

Original comment posted by kevinb@google.com on 2014-01-30 at 10:24 PM

---

Spot-on analysis. I'd love to urge people to fix Objects.hash(anObjectArray) to Arrays.hashCode, but it's not on the same level at all -- it can be part of our suite of optional suggestions.