Unexpected warning about dereferencing without a null check

[djeikyb]

I figured a quick example repo would be the easiest way to describe the problem:

https://github.com/djeikyb/borken_findbugs

In short, I don't expect findbugs to complain about the call to getFileName() on the second line not being sufficiently null checked.

if (p.getFileName() == null) return;
String s = p.getFileName().toString();

In case it's useful, I included the build artifacts after running mvn clean compile findbugs:findbugs (class files, findbugs report),

Used java 1.8 with findbugs 3.0.2, same results on linux and osx

[amaembo]

Surely FindBugs does not track whether method is pure (does not depend on external state and does not change it) or not. How we can know where two subsequent calls to getFileName() always return the same thing? Note that Path is actually an interface, so it can be implemented in various ways including the possible way when the results are always different. This is quite sofisticated analysis. We could implement it for simple getters, but it's complex and still would be fragile. Even JDK implementation of Path.getFileName() is not a simple getter: every time you call it it extracts the part of internally stored path to the newly-created String (using lastIndexOf, substring, etc.), then creates a new Path object, so if (p.getFileName() == null) actually creates at least 3 intermediate useless objects. The proper way to make this check is to use the local variable:

Path fileName = p.getFileName();
if(fileName == null) return;
String s = fileName.toString().

On the other hand probably we should not complain about getFileName() at all. It was added by @iloveeclipse in this commit. The method indeed may return null, but in many cases the programmer can be sure than null cannot be returned. It's somewhat similar to Map.get: while it may return null in many cases it's known in advance that the returned value cannot be null. Andrey, what do you think? Should we warn about unchecked use of Path.getFileName()? Do you have any positive experience with this bug report?

[iloveeclipse]

Tagir, if you think it is more abusing, we should remove this method. Since this is a new API, I guess this is not used often yet. This was a personal hate to the File API in Java which is often inconsistent.

[djeikyb]

Oh, I see. I simplified my original encounter down to the simple null check, but my original case was more like:

if (path.getFileCount() > 0) {
    String fn = path.getFileName();
}

which satisfies the conditions in the javadoc of Path::getFileName. I'm not sure it's reasonable to expect findbugs to know that skipping the null check of file name is okay when the file count is greater than 0.

[iloveeclipse]

This project continues development in a new home: https://github.com/spotbugs/spotbugs/

Please do not open new issues here anymore!