Erroneous "Method Invocation may produce NPE"

I have a situation where I have some helper methods in a library.  One of them is like this:

  public static boolean isEmpty (String str) {
    return (str == null) || (str.trim().length() <= 0);
  } //isEmpty


This is a handy little bugger, but it seems to confuse the IntelliJ inspector, since when I have something like:

    if(!Misc.isEmpty(num) && !Misc.isEmpty(num = num.trim())) {
      setAmount(Fraction.toDouble(num));
    }


The "num.trim()" is flagged as "Method invocation 'num.trim()' may produce 'java.lang.NullPointerException'", which is clearly an error.  As we all know, Java's parser uses short circuit conditionals, so since !Misc.isEmpty(num) will ALWAYS return false if num is null, it is impossible for the dereference of num (in num.trim()) to cause a NullPointerException.

I looked into using the @NotNull annotation on the Misc.isEmpty() method, but since it returns a boolean, and the annotation only appears to affect an Object returned from a method, there doesn't seem to be a way to annotate the method to help out the inspector.

While writing up this report, I noticed that the second call to !Misc.isEmpty was really unnecessary, since isEmpty already tests the trimmed length of the string, so I changed the code to be more optimal.  But it still causes the inspector some heartburn.  The new code looks like this:

    if(!Misc.isEmpty(num)) {
      setAmount(Fraction.toDouble(num.trim()));
    }


And the call to num.trim() is still flagged as a possible NPE.

Am I missing something?  Is this a bug, or is there a way to inform the inspector of the side-effects of isEmpty?

  (*Chris*)

10 comments
Comment actions Permalink

It appears the inspector may be more broken than I thought.  Looking through one of my source files, I ran across this:

      File dir = new File(app.getRealPath("/WEB-INF/recipes"));
      if(dir.isDirectory()) {
        Document recipe;
        XPathFactory factory = XPathFactory.instance();
        XPathExpression<Element> expr = factory.compile("/recipeml/recipe/head/title",Filters.element());
        for(File file : dir.listFiles()) {
          recipe = new SAXBuilder().build(file);
          root.addContent(new Element("recipe").setAttribute("file",file.getName()).setText(expr.evaluateFirst(recipe).getText()));
        }
      }


And the inspector has marked the dir.listFiles() call as potentially causing a NPE.  But since the variable dir is initialzed to a new Object and is never modified, and would have thrown a NPE at the dir.isDirectory() call long before it got to the dir.listFiles() call, the inspector clearly seems confused.

Again, did I miss anything in my analysis?  Is this a bug?

  (*Chris*)

0
Comment actions Permalink

I used to have a similar isEmpty method. I had to give it up to use the null analysis. The flow analyzer doesn't understand your intent. You have to test for null directly or it won't work.

0
Comment actions Permalink

Hmmm, that doesn't sound like a viable solution.  Just abandoning perfectly valid code, especially when it's easier to read than the alternative seems like a mistake.  I wonder how IntelliJ/FindBugs handle things like Apache Commons Lang, that has a whole slew of these readability methods?

What we need would seem to be a simple tweak to the @NotNull/@Nullable annotations that allow them to be applied not just to the return type of a method, but also (conditionally) to a specified parameter (or set of parameters).  Something like:

@NotNull(parameter="str",conditional=true)
public static boolean isEmpty (String str) {
  return (str != null) && (str.trim().length() > 0);
}


That would apply non-nullness to the parameter, conditional on the return of the method.  And of course if extending the @NotNull/@Nullable doesn't make sense, it could easily be something like @ConditionallyNotNull/@ConditionallyNullable.

Any chance we could hear from the IntelliJ team on how feasable/likely this would be to make it in the product?
  (*Chris*)

0
Comment actions Permalink

Hi Chris,

The inspection is right here - 'dir' itself is guaranteed to be not null. However, 'dir.listFiles()' can return null. That means that your for-each loop might throw an exception.

Example: try to compile and run the following program:

import org.jetbrains.annotations.Nullable;

public class Test {

    public static void main(String[] args) throws Exception {
        for (Integer i : getData()) {
            System.out.println(i);
        }
    }

    @Nullable
    private static Integer[] getData() {
        return null;
    }
}


Regards, Denis

0
Comment actions Permalink

I marked your answer as helpful, but it only answered one of the three problems.  The bigger problem (with isEmpty) is still a completely valid concern.  But thanks for your help with the dir.getFiles() problem, you were correct.
  (*Chris*)

0
Comment actions Permalink

It's possible to make a nullability assumptions only in a situation when target variable is method-local (we can't do that for the field because it can be null-ed from another thread just after 'isEmpty()' processing).

You can create a feature request at the tracker and we'll see how popular the idea becomes at the community.

From our own experience inplace null-checks and marking method return value as @NotNull/@Nullable is enough.

Denis

0
Comment actions Permalink

How is that any different from:

if((num != null) && (num.trim().length() > 0)) {
  setAmount(Fraction.toDouble(num.trim()));
}


which doesn't display the error.  There is the exact same probability that another thread could interrupt the flow?
  (*Chris*)

0
Comment actions Permalink

I'd consider that to be incorrect inspection processing

Denis

0
Comment actions Permalink

Do I need to file a bug for that?  If so, how would I go about that?
  (*Chris*)

0
Comment actions Permalink

You can create a ticket at the IJ tracker.

Denis

0

Please sign in to leave a comment.