#1332 CheckForNull/NonNull annotations on HashMap/Properties

[rt15]

Hello,

It is sad that the jre doesn't use @CheckForNull/@NonNull annotations and others.

As a result, FindBugs is missing a lot of issues.

For example, FindBugs should detect a NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE in following code:

@Test
public void test()
{
    Map<String, String> foo = new HashMap<>();
    System.out.println(foo.get("bar").length());
}

But it doesn't.

The Map.get method seems to be a critical method linked to this issue as there are many threads on that subject.

how-to-use-findbugs-nonnull-with-external-libraries
is-there-a-sonar-findbugs-or-pmd-rule-that-detects-this-possible-npe-that-code
nonnull-annotations-and-standard-java-packages

Luckily, it seems that you have tried to mitigate this kind of issues in class edu.umd.cs.findbugs.ba.DefaultNullnessAnnotations.

Would it be possible to add NullnessAnnotation.CHECK_FOR_NULL to java.util.Map.get in DefaultNullnessAnnotations?

[Andrey Loskutov]

Same for Properties...
It's easy to add, but I think there will be MANY false positives...

@Tagir, as you seem to have good OS projects coverage, could you check how many false positives we will get?

CU
Andrey

[Tagir Valeev]

I'll check this, but I don't think that map.get should be marked as @CheckForNull. First many people use the following code pattern:

if(map.containsKey("blahblah")) {
    int valueLength = map.get("blahblah").length();
}

That's probably not the best code from the point of performance (as it queries the map key twice), but it's still used here and there. If map is guaranteed to have no null values inside and is not modified from another threads, then this code is totally safe.

Another case is some collection based algorithms where you know in advance that everything you pass into get is a valid key (and also map doesn't contain null values). For example, something like this:

int getTotalLength(Map<String, String> map, Set<String> exclude) {
  Set<String> keys = new HashSet<>(map.keySet());
  keys.removeAll(exclude);
  int count = 0;
  for(String key : keys) {
    count += map.get(key).length();
  }
  return count;
}

Similar algorithms are quite common and they are safe if you know that you did not put null values into the map. Additional checks will make code more complex and may harm the performance.

I have some testing codebase of open source projects (around 70000 classes). I will check a little bit later how many true-positives and false-positives this change will produce.

[Tagir Valeev]

For now I've checked the FindBugs itself (1774 classes). This change reports 41 new warnings. For around half of them I'm pretty sure NPE is impossible. For the other half additional research is necessary, but I also think that NPE is impossible.

[Tagir Valeev]

I analyzed Eclipse SDK (~20000 classes) and got 406 new warnings. It would be quite time-consuming to check all of them, but I'm pretty sure that in most cases there are false-positives.

Common patterns include:

• get after containsKey. Example: MarkerSupportRegistry#486

• get where argument derived from keySet. Example: PropertySheetEntry#199. Some of these cases are already reported with WrongMapIterator detector.

• map is unmodifiable and filled with predefined set of keys, so it's known that the mapping exists. Example: TemplatePreferencePage#567 (map filling is right below)

• map is filled once and all its keys are known (for example, stored as values in another map). Example: ConfigurationElementSorter#117. Here fPrereqsMapping and fDescriptorMapping are filled only in initialize() method (line 127) and values of fDescriptorMapping are always keys of fPrereqsMapping (bundle symbolic names).

I did not find any obvious true positives. Probably there are actually some questionable cases, but they are buried under hundreds of false-positives. I'd vote for close-wontfix for this bug.

[Tagir Valeev]

In general I think that @CheckForNull annotation should be added for method return value only if there's no reliable other way to check that value is non-null. For example, File.listFiles returns null if files listing cannot be created. Even if you checked right before this call that file exists, it's a directory and you have read access to it, it's still possible that during listFiles execution the directory will become inaccessible (e.g., due to network failure). So you never can be sure that listFiles will not return null and you have to check its return value. However for maps there are many cases when you can be 100% sure, because you fully control the map and don't depend on something unpredictable like network conditions.

[rt15]

In a perfect world, I think that @CheckForNull would have to be added for any method that can return null. To make developers think about what they are writing. To have rigorous bugs findings. Fixing false positives using assertions appears a clean solution to me:

int getTotalLength(Map<String, String> map, Set<String> exclude) {
  Set<String> keys = new HashSet<>(map.keySet());
  keys.removeAll(exclude);
  int count = 0;
  for(String key : keys) {
    String value = map.get(key);
    assert value != null; // We are using the keys to access the map.
    count += value.length();
  }
  return count;
}

There are many false positives, but there is many more cases in which there is already a correct check for nullity of the result of Map.get().
Using map.containsKey before map.get might be a bad practice.

Another bad practice generating false positive, very often used by beginners and not only beginners:

Map<String, String> map = new HashMap<>();
for (String key : map.keySet())
{
    String value = map.get(key);
    System.out.println(key + " = " + value);
}

Map.entrySet should obviously be used instead.
Found for example in FindBugs code, in FindInconsistentSync2.report().
But as you said, all that stuff should not exist because of WrongMapIterator.

Here is another false positive code from findbugs DockLayout.java, that might be questionable:

public JMenu createWindowMenu() {

    viewMenuItems = new HashMap<View, ViewMenuItem>();
    viewMenuItems.put(summaryView, new ViewMenuItem(summaryView, "Bug summary"));
    viewMenuItems.put(commentsView, new ViewMenuItem(commentsView, "Comments"));
    viewMenuItems.put(sourceView, new ViewMenuItem(sourceView, "Source code"));

    JMenu windowMenu = new JMenu("Window");
    windowMenu.setMnemonic(KeyEvent.VK_W);
    windowMenu.add(viewMenuItems.get(summaryView));
    windowMenu.add(viewMenuItems.get(commentsView));
    windowMenu.add(viewMenuItems.get(sourceView));
    return windowMenu;
}

It would be more efficient to use local variables for ViewMenuItem instances, or an array like this:

public JMenu createWindowMenu() {

    ViewMenuItem[] viewMenuItems = { new ViewMenuItem(summaryView, "Bug summary"),
                                     new ViewMenuItem(commentsView, "Comments"),
                                     new ViewMenuItem(sourceView, "Source code")
                                   };

    viewMenuItemByView = new HashMap<View, ViewMenuItem>();
    viewMenuItemByView.put(summaryView, viewMenuItems[0]);
    viewMenuItemByView.put(commentsView, viewMenuItems[1]);
    viewMenuItemByView.put(sourceView, viewMenuItems[2]);

    JMenu windowMenu = new JMenu("Window");
    windowMenu.setMnemonic(KeyEvent.VK_W);
    for (ViewMenuItem viewMenuItem : viewMenuItems) {
        windowMenu.add(viewMenuItem);
    }

    return windowMenu;
}

Anyway I understand your points that there are too much false positives and too few real issues. But I believe that hiding these issues is not a perfect solution either. A better solution might be to modify the jre (For example to have a Map.search() with @CheckForNull and a Map.get() that throws an exception if the key is not known).

[Tagir Valeev]

I fixed FindInconsistentSync2.report:
https://code.google.com/p/findbugs/source/detail?r=c398e4a87e7be48d26b2d0d6495fff50013bb6f8
WrongMapIterator is not issued here in FB 3.0.0. I improved this detector to support fields only recently (see [bugs:#1308]). Thus it was unnoticed before.

Using containsKey before get may save you from creating a new variable, so I wouldn't definitely say it's a bad practice. Additional variables and checks may enlarge method body making it harder to read. In particular your proposed assert in getTotalLength adds nothing: code is short and clear enough, so explanation is not necessary and in case of error you just replace NPE with AssertionError. If something should never ever happen, then NPE is also nice for this case. Additional assertion might be good if it adds more information helpful for debugging.

I agree that having strict get in Map which throws an exception containing String.valueOf(key) in its message would be nice. But we are living in real world. You may always create a wrapper class in your project for this.

[rt15]

It appears that you will not convince me and I will not convince you.

Anyway great thanks for the investigations and the quick treatment of this ticket. :)

[Tagir Valeev]

Anyways final decision about this bug should be taken by Andrey or Bill as I'm a newbie in FindBugs team :-) Andrey, what do you think?

[Andrey Loskutov]

Tagir,
thanks for investigation, I'm really happy to have you on board!

I think one of the FB main goals is to avoid reporting false positives as much as we can do.

I think we both agree that the proposed change (even if it easy to add) would introduce lot of "noise".

Closing this one as wont fix (please notice that I've changed the title to reflect the fact that we are not against adding the CheckForNull for jre classes in general - but this cases must be carefully chosen).

Regards,
Andrey

[saidin]

For those of us that would like the default behavior to change (e.g. ConcurrentHashMap#get to me marked CHECK_FOR_NULL) or add our own behavior (e.g. HashMap doesn't feature in the list), how can we customize this?

I did this with a detector that calls into AnalysisContext.currentAnalysisContext().getNullnessAnnotationDatabase().addMethodAnnotation(), but it's not great.