Skip to content

Inconsistent & unexpected NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE reported #3573

@iloveeclipse

Description

@iloveeclipse

After merge of #3559 to master we see now lot of NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE warnings because of this change for the code where the data read from the Map.get() is immediately dereferenced without NP check.

Trivial example is

package a;

import java.util.List;
import java.util.Map;

public class NpWarningOnMapGet {
    String key = "aa";
    public void main(Map<String, List<Object>> map) {
        if(map.get(key) != null) {
            List<Object> list = map.get(key);
            boolean b = list.contains(map); // warning here
            System.out.println(b);
        }
    }
}
Bug: Possible null pointer dereference in NpWarningOnMapGet.main(Map) due to return value of called method
 
 The return value from a method is dereferenced without a null check, and the return value of that method is one that should generally be checked for null.  This may lead to a 
NullPointerException when the code is executed.  
 
Rank: Troubling (13), 
confidence: Normal

Pattern: NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE 

Type: NP, 
Category: STYLE (Dodgy code)

Dereferenced at NpWarningOnMapGet.java:[line 11]
In method com.advantest.itee.measurement.ui.NpWarningOnMapGet.main(Map)
Value loaded from list
Known null at NpWarningOnMapGet.java:[line 10]

The problem with that is: it is only reported for some few cases and it is not clear why. We have ~251 warnings reported on 32856 classes with 2054585 code size (as reported by SpotBugs). I'm pretty sure there is a LOT of code that reads something from the maps without checking, but we only see small fraction reported.

I've put same class above in an empty project and it seem to not report any warnings, so it seems that other classes somehow affect the analysis result...

I personally dislike the style above, and for sure the result from the map.get() has to be checked for null differently (not by executing get(key) two times, because map content may change inbetween), but why I've marked this as "bug" is

  1. Because the developers should be able to understand why the problem is reported here but not there on almost identical code. It should be "deterministic" to the user.
  2. The code style is very common, but bug reported only for few cases. Either it should be always reported or it shouldn't be reported at all.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions