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

cushon · 2014-10-31T15:56:18Z

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:

map.containsKey(arg) --> check that either "T extends K" or "T super K"
map.containsValue(arg) --> check that either "T extends V" or "T super V"
map.get(arg) --> check that either "T extends K" or "T super K"
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:

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

Same for List.indexOf, List.lastIndexOf.

cushon · 2014-10-31T16:08:24Z

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 · 2014-10-31T16:10:07Z

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

Issue #127 has been merged into this issue.

cushon · 2014-10-31T16:10:19Z

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 · 2014-10-31T16:16:37Z

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 · 2014-10-31T16:16:39Z

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 · 2014-10-31T16:16:41Z

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 · 2014-10-31T16:16:44Z

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 · 2014-10-31T16:16:48Z

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 · 2014-10-31T16:16:50Z

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 · 2014-10-31T16:21:02Z

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 · 2014-10-31T16:22:51Z

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.

kevinb9n · 2015-10-01T17:54:39Z

What if, instead of hardcoding the giant list of Guava methods, we annotated Guava like so:

interface Multimap<K, V> {
boolean containsKey(@CompatibleWith("K") Object key);
}

Of course, you'd want to give a compile error for Multimap.java itself if the "K" doesn't match a type parameter name.

Some name ideas...

@CompatibleWith
@CompatibleWIthType
@ConvertibleTo
@CastableTo

The last one is a bit confusing. It may sound like you have to actually know that all possible instances are castable to that type; but if that were the case then you'd just accept K and users would actually make that cast. We only want to know that some instance could exist that is castable.

cgruber · 2015-10-02T14:53:57Z

@ComparableTo(...) ?

kevinb9n · 2015-10-02T16:04:59Z

Comparable, as well as being taken, doesn't seem to describe the concept we're talking about very well.

cushon · 2015-10-02T16:13:27Z

+1 for convertible or castable.

I think castable is the most accurate, in the sense that the language would let you to attempt the cast, not necessarily that it would succeed without CCE.

eaftan · 2015-10-02T17:03:31Z

I vote for @CompatibleWith. "Castable to" has a precise meaning in the language spec, and I'd rather not tie us to that or risk confusion. "Compatible with" is not well defined and gives us freedom on how to implement the check. It also matches the name of the check. :)

Question: what about methods like Iterables.removeAll(Iterable<?> removeFrom, Collection<?> elementsToRemove)? How would we annotate that to express the type constraint?

kevinb9n · 2015-10-02T17:11:06Z

We could just make it a generic method. Oh, but it's a little worse than that:

public static <T> boolean removeAll(
    Iterable<T> removeFrom,
    @CompatibleWith("T") Collection<?> elementsToRemove) {}

This is nowhere expressing that it's the type parameter of Collection that is what we want to be compatible with T.

One copout is just to say that methods like this still have to be hardcoded in the checker, at least until Java 8 gives us possible new possibilities for doing this with a type annotation maybe...

eaftan · 2015-10-02T23:39:00Z

Type annotation seems like the right approach for the case where we need to check the type parameter of the argument. Seems reasonable to hardcode until we support Java 8.

cushon · 2017-02-15T03:27:51Z

This happened: http://errorprone.info/bugpattern/CollectionIncompatibleType

cushon mentioned this issue Oct 31, 2014

Object param in Map.get() does not match or extend key type #127

Closed

cushon added Priority-Medium Type-NewCheck migrated Status-Accepted labels Oct 31, 2014

cushon added Priority-High and removed Priority-Medium labels Jan 23, 2015

cushon assigned eaftan Jan 23, 2015

cushon mentioned this issue Jan 23, 2015

Get collectionIncompatibleType working #63

Closed

cushon closed this as completed Feb 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

kevinb9n commented Oct 1, 2015

cgruber commented Oct 2, 2015

kevinb9n commented Oct 2, 2015

cushon commented Oct 2, 2015

eaftan commented Oct 2, 2015

kevinb9n commented Oct 2, 2015

eaftan commented Oct 2, 2015

cushon commented Feb 15, 2017

Navigation Menu

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

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

Comments

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

cushon commented Oct 31, 2014

kevinb9n commented Oct 1, 2015

cgruber commented Oct 2, 2015

kevinb9n commented Oct 2, 2015

cushon commented Oct 2, 2015

eaftan commented Oct 2, 2015

kevinb9n commented Oct 2, 2015

eaftan commented Oct 2, 2015

cushon commented Feb 15, 2017