Potential null pointer access

Question

I encounter a strange situation that is currently not that clear to me:

When having the potential null pointer access warning enabled in Eclipse, I get warnings like in the following (the warnings are stick to the line preceding the corresponding comment):

protected Item findItemByName(String itemName, Items items) {
    boolean isItemNameMissing = null == itemName || itemName.isEmpty();
    boolean isItemsMissing = null == items || null == items.getItems() || items.getItems().isEmpty();

    if (isItemNameMissing || isItemsMissing) {
        return null;
    }
    // potential null pointer access: the variable items might be null at this location
    for (Item item : items.getItems()) {
        // potential null pointer access: the variable itemName might be null at this location
        if (itemName.equals(item.getName())) {
            return item;
        }
    }

    return null;
}

The same happens to me if I check for null using Guava's Preconditions like

Preconditions.checkArgument(argument != null, "argument must not be null");

Where I can understand that in the latter case a flow analysis for checking when the IllegalArgumentException will happen might be too difficult/expensive or even impossible I in turn do not understand why the compiler raises the warning at all (if I remove the checks they disappear).

Can one maybe explain how the potential null pointer access is accomplished and why it is raised in both of the cases? Or at least point me to the direction.

In the meanwhile I have a look and see whether I find it out myself...

Addendum

I've kind of broken it down to the bare core of the case. Given the following class, the warning only shows up in the method sample2 (as pointed out by the comment again). Please note that the method sample3 does not trigger the warning either.

public class PotentialNullPointerAccess {

    public void sample1(final String aString) {

        if (aString == null) {
            return;
        }

        System.out.println(aString.length());
    }

    public void sample2(final String aString) {

        boolean stringIsNull = null == aString;

        if (stringIsNull) {
            return;
        }

        // Potential null pointer access: The variable aString might be null at this location
        System.out.println(aString.length());

    }


    public void sample3(final String aString) {

        System.out.println(aString.length());
    }
}

Sperrylite

The first 2 lines of the code seems not reader friendly, IMO you should use braces to group the expressions – Naseby

What is the exact warning you're getting? And why do you think it's coming from the compiler, as opposed to from Eclipse? – Erg

@T.J.Crowder: I somehow subsumed compiler to include the Eclipse Java Compiler (ejc) as well. Further, the exact error message I am getting is stated in the comment, as I wrote (except for the first character being a capital letter). – Sperrylite

@sanbhat: Yes yes. Fwiw, it is code I discovered. If it comes to design flaws/code smell and such things, there would be a couple of things that are worth being mentioned within this tiny method... – Sperrylite

You should take this to an Eclipse forum, maybe even file a bug straight away. – Autocratic

@MarkoTopolnik: I am not sure about whether it is a bug or due to a certain constraint that would apply to the null checks. I am not seeing a big issue in this since I can just disable the warning. But I want to understand the why - if reasonable, admittedly. – Sperrylite

If commenting out the prologue removes the error, whereas in reality that is precisely the case where the error is appropriate, this seems like a bug regardless of further details. – Autocratic

@Andreas: "Further, the exact error message I am getting is stated in the comment, as I wrote..." If you're quoting something, showing it in quotes (ideally even a blockquote) makes that clear. – Erg

@MarkoTopolnik: The more I think about it the more it really seems to be a bug. – Sperrylite

Commenting after seeing your addendum, so it's not true that the warning disappears when you remove the prologue: you rewrite the prologue to an explicit null-check. That's a whole different story, and it boils down to the power of the inference algorithm used. This I would not call a bug. – Autocratic

I will just add a sample3 method that has no warning... – Sperrylite

Answer 1

I think it is somehow answered by Ed Merks within this forum post:

http://www.eclipse.org/forums/index.php/t/278687/

From what I understand, Eclipse raises the warning once you assume a variable to potentially be null in the preceding code. You can do so by just check against null (either equal or not equal) - but you do have to have it separated somewhere as a variable, not simply as an if's solely expression.

Sperrylite

Answer 2

This is probably not the cause of the warning but here you can have a null pointer.

for (Item item : items.getItems())
{
    // potential null pointer access: the variable itemName might be null at this location
    if (itemName.equals(item.getName()))
    {
        return item;
    }
}

You iterate over objects, but the object returned can be null. So item.getName() can cause a null pointer exception.

Exmaple

 List<String> l = new ArrayList<String>();
 l.add("test");
 l.add(null);
 l.add("another string");

 if(l == null)   // <-- this is similar to the above check
    return;

 for(String s : l)
 {
     s.charAt(0);   //  <-- null pointer access on second item.
 }

Heydon

You could just replace item.getName() by any constant and thus obvious non-null string, but the warning will stay. – Sperrylite

That's why I said that this is not the cause of the warning, but can still cause a null pointer. – Heydon

Ah, now I get your point. But fwiw, will the foreach loop really ever yield a null? – Sperrylite

That depends on what you pass in the iterator. I updated my posting with an exmaple to illustrate what I mean. Of course Eclipse doesn't show a null pointer warning on this, so you have to take care of it for yourself. – Heydon