How to fix the Findbugs issue "Null value is guaranteed to be dereferenced" NP_GUARANTEED_DEREF

Question

Hi I have got some code that is reported as having the NP_GUARANTEED_DEREF issue by Findbugs. Now looking at my code I don't quite understand what is wrong with it, can anyone suggest what the problem is.

public void test() {
  String var = "";
  int index = 2;
  if (index == -1) {
    var = String.class.getName();
    if (var.length() == 0) {
      var = null;
    }
  } else {
    var = Integer.class.getName();
    if (var.length() == 0) {
      var = null;
    }
  }
  if (var == null) {// FINBUGS reports on this line NP_GUARANTEED_DEREF
    /*
     * There is a statement or branch that if executed guarantees that a value
     * is null at this point, and that value that is guaranteed to be
     * dereferenced (except on forward paths involving runtime exceptions).
     */
    throw new NullPointerException("NULL");
  }
}

Now drilling into the Error in Findbugs it highlights the two assignments to var = null; as cause for the bug but I don't quite understand why. It is not like I am actually doing anything with the var object I am just doing a Null check. The example is taken from real production code but stripped of anything that wasn't needed to reproduce the error. What I am wondering if this is a false positive or not. And if not what would be an appropriate fix.

Here is the link to the Findbugs Bug Detail: http://findbugs.sourceforge.net/bugDescriptions.html#NP_GUARANTEED_DEREF

[UPDATE] After recieving some feedback on this issue I have now logged this as a False Positive in the Findbugs Bugtracker on Sourceforge the link is https://sourceforge.net/tracker/?func=detail&aid=3277814&group_id=96405&atid=614693

Conversation about the problem will continue there.

Haemin

Did you by any chance have var.equals(null) before? Are you sure you have re-run Findbugs on that file (what I usually do is I call the "clean bug markers"). – Sparse

Yes I am sure that I have rerun Findbugs, and no it never was var.equals(null) if it was that it would be easily understandable. As jzd said it does not look wrong. If I get more confirmation that this does not look wrong I will probably post a Bug for (False positive) with Findbugs – Haemin

I see. I can confirm the same FB behavior on my computer. Looks strange indeed. What's funny, that if you replaced throw new NullPointerException with throw new RuntimeException the bug marker would disappear. – Sparse

Now I think I understand what they've meant. The wording of the message is not exact, but they are warning you against a NPE. I guess they consider explicitly throwing NPE a bad practice. – Sparse

ha interesting find Grzegorz, I can confirm your find. So it is the NPE that is the issue here. Thanks. If you put that into an actual answer I can mark it as the answer I was looking for. – Haemin

@AGrunewald - copied my comments to an answer – Sparse

Answer 1

I see. I can confirm the same FB behavior on my computer. Looks strange indeed. What's funny, that if you replaced throw new NullPointerException with throw new RuntimeException the bug marker would disappear.

Now I think I understand what they've meant. The wording of the message is not exact, but they are warning you against a NPE. I guess they consider explicitly throwing NPE a bad practice.

Sparse

Answer 2

It is a bug in FindBugs, post this issue on their issue tracker page. findbugs.sf.net

Ampliate

thanks yes indeed it is a bug, the mailinglist gave the same reply. I will file it as a bug and update my question accordingly. – Haemin

Answer 3

OK, what FindBugs is looking for is a statement or branch that is guaranteed to lead to a null pointer exception. Originally, we only looked for dereferences of null values. We later augmented the analysis to treat

if (x == null) throw new NullPointerException()

the same as an explicit dereference of x. This was primarily to help interprocedural analysis, so that methods that had explicit null checks for their parameters would treated the same as methods that dereference their parameters without explicit null checks, and report errors when null values are passed for such parameters.

So some of the text in our error msgs might need to be updated, but we really haven't found many realistic cases where it causes confusion.

I'm not quite sure what the purpose of the above code is. At the points where are you assigning null to var, you are creating a situation that will lead to an explicit throw of a null pointer exception further down. Is that really the behavior you want?

Terminology

Conversation continues in the SourceForge bugtracker see sourceforge.net/tracker/… – Haemin

Answer 4

Looking closer into the definition of the error message here, it says:

There is a statement or branch that if executed guarantees that a value is null at this point, and that value that is guaranteed to be dereferenced (except on forward paths involving runtime exceptions)

Which makes me think it is either just letting you know var is going to be null or something actually is making findbugs think that var is referenced inside the if statement.

The code you posted looks fine, I would double check that var is not accessed in the true code.

The only thing I might change is to write the comparision backwards like so:

if (null == var)

That way it is obvious if you leave out one of the ='s/

Knecht

Thanks for the answer jzd, while I agree with you on the swapping of the comparison it unfortunately does not change what Findbugs is reporting. – Haemin

@Agrune, I looked up the definition and I feel better about my answer now. I have updated it to include the definition. – Knecht