Calling Map/Collection methods with arguments that are not compatible with type parameters

[cushon]

Original issue created by fixpoint@google.com on 2013-03-05 at 03:23 PM

---

There are some methods on Map<K,V> or Collection<V> that accept Object because of compatibility reasons, while in fact they should accept <V>. We can check that arguments passed to those methods are compatible with V.

Example: Map<MyProto, Integer> map = ...; MyProto.Builder proto = MyProto.newBuilder()...; if (map.get(proto)) { ... };

Following checks can be introduced for all "Map<K,V> map" and "T arg" variables:

1. map.containsKey(arg) --> check that either "T extends K" or "T super K"
2. map.containsValue(arg) --> check that either "T extends V" or "T super V"
3. map.get(arg) --> check that either "T extends K" or "T super K"
4. map.remove(arg) --> check that either "T extends K" or "T super K"

Following checks can be introduced for all "Collection<V> coll" and "T arg" variables:

1. coll.contains(arg) --> check that either "T extends V" or "T super V"
2. coll.remove(arg) --> check that either "T extends V" or "T super V"

Same for List.indexOf, List.lastIndexOf.

[cushon]

Original comment posted by wasserman.louis on 2013-03-05 at 08:38 PM

---

These are not this way for compatibility reasons, but because it's genuinely necessary. See e.g. http://smallwig.blogspot.com/2007/12/why-does-setcontains-take-object-not-e.html for details. Josh Bloch explains the original logic at http://youtu.be/wDN_EYUvUq0?t=6m41s.

[cushon]

Original comment posted by alexeagle@google.com on 2013-04-27 at 07:37 PM

---

Issue #127 has been merged into this issue.

[cushon]

Original comment posted by fixpoint@google.com on 2013-04-28 at 08:24 PM

---

wasserman.louis, I understand how this is a tradeoff between type discipline and convenience. Nevertheless, the check would help avoid some bugs (like the mentioned example of passing and Integer to Set<Long>.contains()).

[cushon]

Original comment posted by mdempsky@google.com on 2013-11-05 at 07:45 PM

---

Louis: I interpret Josh's argument as Set.contains() has to take an Object because of JLS limitations; i.e., there's no way to define that Set<? extends V>.contains() should allow any "V" argument, not just a "? extends V" argument.

Are there cases where you'd actually want to call Set<? extends V>.contains() with an object that isn't assignable to V? If not, I think it's reasonable for error-prone to provide special case generic handling logic for cases like these though (maybe just a warning initially).

[cushon]

Original comment posted by lowasser@google.com on 2013-11-05 at 07:48 PM

---

One example would be calling Map<ArrayList<Integer>, String>.get(ImmutableList<Integer>).

[cushon]

Original comment posted by mdempsky@google.com on 2013-11-05 at 07:52 PM

---

Ah, because you're relying on ArrayList and ImmutableList being comparable for equality? I suppose the check is only safe to make into an error when the container only uses reference equality, otherwise you need some extra logic (e.g., ArrayList and ImmutableList both implement List, so they should still be comparable with equals()).

[cushon]

Original comment posted by eaftan@google.com on 2013-11-05 at 07:53 PM

---

FYI the existing CollectionIncompatibleType does some of this but needs work.

[cushon]

Original comment posted by eaftan@google.com on 2013-11-08 at 07:52 PM

---

FindBugs detects this problem for the following methods. We should support as many of these as we can, as long as it doesn't increase our false positive rate.

java.util.Collection<E>: contains(java.lang.Object) remove(java.lang.Object) containsAll(java.util.Collection<?>) removeAll(java.util.Collection<?>) retainAll(java.util.Collection<?>)

java.util.Deque<E>: removeFirstOccurrence(java.lang.Object) removeLastOccurrence(java.lang.Object)

java.util.List<E>: indexOf(java.lang.Object) lastIndexOf(java.lang.Object)

java.util.Vector<E>: indexOf(java.lang.Object) lastIndexOf(java.lang.Object)

java.util.Map<K,V>: get(java.lang.Object) remove(java.lang.Object) containsKey(java.lang.Object) containsValue(java.lang.Object)

java.util.Hashtable<K,V>: contains(java.lang.Object)

java.util.concurrent.ConcurrentHashMap<K,V>: contains(java.lang.Object)

java.util.concurrent.ConcurrentMap<K,V>: remove(java.lang.Object,java.lang.Object) remove(java.lang.Object,java.lang.Object)

com.google.common.collect.Multimap<K,V>: containsEntry(java.lang.Object,java.lang.Object) containsEntry(java.lang.Object,java.lang.Object) containsKey(java.lang.Object) containsValue(java.lang.Object) remove(java.lang.Object,java.lang.Object) remove(java.lang.Object,java.lang.Object) removeAll(java.lang.Object)

com.google.common.cache.Cache<K,V>: invalidate(java.lang.Object)

com.google.common.collect.Multiset<E>: count(java.lang.Object) remove(java.lang.Object) remove(java.lang.Object,int) containsAll removeAll retainAll

com.google.common.collect.Table<R,C,V>: contains(java.lang.Object,java.lang.Object) contains(java.lang.Object,java.lang.Object) containsRow(java.lang.Object) containsColumn(java.lang.Object) containsValue(java.lang.Object) get(java.lang.Object,java.lang.Object) get(java.lang.Object,java.lang.Object) remove(java.lang.Object,java.lang.Object) remove(java.lang.Object,java.lang.Object)

Static methods

com.google.common.collect.Sets: intersection difference symmetricDifference

com.google.common.collect.Iterables: contains removeAll retainAll elementsEqual frequency

com.google.common.collect.Iterators: contains removeAll retainAll elementsEqual frequency

---

Owner: amshali@google.com

[cushon]

Original comment posted by amshali@google.com on 2013-11-08 at 08:23 PM

---

Alright. I will add these with caution.

---

Status: Accepted

[cushon]

Original comment posted by amshali@google.com on 2014-03-25 at 06:07 PM

---

I am gonna take a new look at this.

[cushon]

Original comment posted by kevinb@google.com on 2014-09-25 at 04:30 PM

---

FYI, I think this has to be one of the top 3 highest-value checks not yet enabled.