[#IDEADEV-16164] "Suspicious collections method calls" inspection is a bit over-zealous

IDEA: Development

"Suspicious collections method calls" inspection is a bit over-zealous

Created: 20 Feb 07 18:03 Updated: 11 May 07 17:48

Component/s:

Code Analysis. Inspection

Fix Version/s:

Selena 6913

Original Estimate:

Unknown

Remaining Estimate:

Unknown

Time Spent:

Unknown

Issue Links:

Duplicate

This issue duplicates:
	~~IDEA-12276~~ Regression: "Suspicious collections m...

Build:	6,700
Fixed in build:	6,821
Severity:	Medium

Description

« Hide

It shouldn't (at least in option, if not by default) warn about code like this:

Map<Integer,String> map = getMap();
    Number n = new Integer(3);
    if (map.containsKey(n)) {
        ...
    }

That is to say, no warning should be emitted if the type given is a supertype of the desired type. I suggest that this be the default behaviour, perhaps with a checkbox to give the current behaviour if desired.

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Eugene Vigdorchik - 20 Feb 07 18:42

That would make the whole inspection useless for e.g. retrieving Objects.

[ Permlink | « Hide ]

Gibson - 20 Feb 07 19:10

That's why I suggest it maybe should be an option. Note that I'm suggesting that you tighten the rules a little to only warn if it's impossible for the call to ever succeed: if there is a possibility it might succeed, no warning should be emitted (or, as an option, also emit a warning in this case).

We have lots of code like this:

public boolean accept(Map<Integer,String> map, Object key) {
        return map.containsKey(key);
    }

Here I don't want Idea to warn me that the runtime type of "key" might not give any results: because the runtime type might be Integer. And I don't want to cast to Integer because I want my code to return false if a Double is passed in.

On the other hand, I do want to see a big warning in this case:

public boolean accept(Map<Integer,String> map, String key) {
        return map.containsKey(key);
    }

[ Permlink | « Hide ]

Gibson - 21 Feb 07 15:17

Just one quick last point: consider the consistency of this code:

Integer i = Integer.valueOf (0); 
Object o = i;
String s = "Test";
List<Integer> l = new ArrayList<Integer> ();
// No warning - good
if (o.equals (i)) { }
if (i.equals (o)) { }
// Warning "Suspicious call to List.contains()" - not good
if (l.contains (o)) { }
// Warning "equals between objects of inconvertible types" - good
if (s.equals (i)) { }
if (i.equals (s)) { }
// Warning "Suspicious call to List.contains()" - good
if (l.contains (s)) { }

Doesn't that feel a little bit wrong to you? Surely the various inspections of this type (that is, "Suspicious 'Collections.toArray()' call", "Suspicious 'System.arraycopy()' call", "Suspicious collections method calls" and "'equals()' between objects of inconvertible types" should be internally coherent (and not just the ones that are "Powered by InspectionGadgets")

[ Permlink | « Hide ]

Gibson - 21 Feb 07 16:09

Also see IDEA-11410

[ Permlink | « Hide ]

AlexL - 11 Mar 07 05:44

At least for your original example, it looks suspicious to me, unless IDEA can use
dataflow analysis to make sure that the variable n is only ever assigned to an Integer.

Why do you not have Map<Number,String> or else if you want Map<Integer,String>
then why don't you have

Map<Integer,String> map = getMap();
    Integer n = new Integer(3);
    if (map.containsKey(n)) {
        ...
    }

[ Permlink | « Hide ]

Gibson - 12 Mar 07 11:34

I don't want to be warned when the code is "suspicious", just "wrong". Using an Object to retrieve from a map that takes Integer keys could be considered "suspicious" but for me it gives too many false positives to be useful. On the other hand, using a String to retrieve from a map that takes Integer keys is valid Java but is always going to be a bug.

[ Permlink | « Hide ]

Eugene Vigdorchik - 11 Apr 07 23:26

This would render the inspection useless for the cases where either type parameter or argument of the method have interface type. I would still suggest you introduce the variable of concrete type, and the inspection for single-used local variable be patched to account for this case.

[ Permlink | « Hide ]

Gibson - 13 Apr 07 12:22

Eugene - good point about interface types, although I don't have that specific use case in my code. Maybe the inspection should always warn about this case

I can't really see a nice general solution to this. For example, you suggest transforming this

public boolean accept(Map<Integer,String> map, Object key) {
    return map.containsKey(key);
}

into this

public boolean accept(Map<Integer,String> map, Object key) {
    if (key instanceof Integer)
        // Whoops, I get an unnecessary cast warning here :-/
        return map.containsKey((Integer) key);
    else
        return false;
}

or maybe this

public boolean accept (Map<Integer, String> map, Object key)
{
	if (key instanceof Integer)
	{
		Integer ikey = (Integer) key;
		return map.containsKey (ikey);
	} else
	{
		return false;
	}
}

which are both kind of ugly and much more verbose than the original; and it doesn't work with generics, for example, in this contrived example

public boolean accept(Map<Class<Integer>,String> map, Class<?> key) {
    if (key instanceof Class<Integer>)
        return map.containsKey((Integer) key);
    else
        return false;
}

[ Permlink | « Hide ]

Alexey Kudravtsev - 11 May 07 17:47

~~IDEADEV-16164~~