Regarding java:S2259, NullPointerException not being reported

Clean Code Report False-positive / False-negative...
,
1

Hi,

I’m currently working with the rule java:S2259 for detecting potential NullPointerExceptions (NPEs) in Java code. However, I’ve noticed some inconsistencies in its behavior. Specifically:

In the following examples, the rule fails to detect an NPE:

public Integer process(List<Integer> a){
        if(CollectionUtils.isEmpty(a)){
            return a.get(0) + a.get(1);
        }
        return 0;
    }

or

public class NpeCheckerTestFile {
  public Integer process(List<Integer> a) {
    if(CollectionUtils.isEmpty(a)){
      int x = a.get(0) + a.get(1); // Noncompliant
    }
    return 0;// compliant :  unreachable.
  }
}

// 2nd File, synonymous to My personal-Util Functions in my repo.
public class NPECheckerTestFile2 {
  public static boolean isEmpCollection(List<?> collection) {
    return collection == null || collection.isEmpty();
  }
}

Observation

  • When the code is split across multiple classes or files, the rule does not flag the NPE as expected.
  • However, if the utility functions (like isEmpCollection) or both classes are in the same file, the NPE is detected correctly.

This discrepancy makes it challenging to rely on java:S2259, since projects often use utility functions and class separation.

Is there a known workaround or configuration for SonarQube or this specific rule to ensure accurate detection in such cases? If not, are there any plans to enhance java:S2259 to account for cross-class and cross-file NPE scenarios?

Thanks,
Akshaj.

4

Hello,

in order to be sure I would need to see what the CollectionUtils.isEmpty method does, but assuming that it only checks the .size() == 0 of a collection, I would say that the exception that could be raised is something like IndexOutOfBoundsException and not an NPE.

Can you clarify why you would expect a NullPointerException?

Also no, there are no plans to improve this rule, as it will be replaced in the future by a different one.

6

CollectionUtils.isEmpty actually also checks for null as well, here is the code for reference:

    public static boolean isEmpty(Collection coll) {
        return coll == null || coll.isEmpty();
    }
7

If this method is in another file it will not be picked up by the analysis, so the engine is not aware of the null-check that was already performed.

8

IT’S A Spring-Java Lib class, please check here,Collection. It’s pretty standard and used everywhere.

9

Hi, So I thought I should clarify few things here :

package org.example;
import org.apache.commons.collections4.CollectionUtils;

import java.util.List;

public class dummy {
    public Integer process(List<Integer> a) {
        if (CollectionUtils.isEmpty(a)) {
            return a.get(0) + a.get(1);
        }
        return 0;
    }
}

Whereas the below code reports an NPE.

package org.example;

import java.util.Collection;
import java.util.List;

public class dummy {
    public Integer process(List<Integer> a) {
        if (isEmpty(a)) {
            return a.get(0) + a.get(1);
        }
        return 0;
    }

    public static boolean isEmpty(Collection<?> coll) {
        return coll == null || coll.isEmpty();
    }
}

The underlying code for
org.apache.commons.collections4.CollectionUtils.isEmpty() = dummy.isEmpty()

My only question is, even though config for this method is explicitly defined in the json file, why is it unable to track NPE ?

10

Hey there, you have correctly identified where the engine is pulling his knowledge from, we try to track the most popular library methods with this behavior cache.
This particular method was tracked this way to avoid potential FPs. If you mark the collection as nullable on the true path of the utility method, it will propagate further, possibly creating noise and FPs. When you enter the true path of that method, it means that the collection is either null, or not null but empty, so the engine avoids marking it as nullable

11

I don’t quite get your response, let’s talk specific to the above example I provided, how is it unable to track NPE above ?