java:S1695 false positive for Comparable::compareTo

Rules and Languages Report False-positive / False-negative...
1
  • What language is this for? java
  • Which rule? “NullPointerException” should not be explicitly thrown [S1695]
  • Why do you believe it’s a false-positive/false-negative? The contract for Comparable requires a NullPointerException should be thrown if the input is null.
  • Are you using
    • SonarQube - v10.6 (92116)
  • How can we reproduce the problem?

class ComparableClass implements Comparable<ComparableClass> {
  @Override
  public int compareTo(@Nullable ComparableClass o) {
    if (o == null) {
      throw new NullPointerException("Cannot compare to null");
    }
    return 0;
  }
}

interface ComparableInterface extends Comparable<ComparableInterface> {
  @Override
  default int compareTo(@Nullable ComparableInterface o) {
    if (o == null) {
      throw new NullPointerException("Cannot compare to null");
    }
    return 0;
  }
}
4

Hi @throup , a contract requiring NPEs looks like a valid reason to throw them explicitly. However, the reason behind rule S1695 is that this:

if (o == null) throw new NullPointerException();
o.something();

is an antipattern which the rule tries to prevent, because the null check is redundant (o.something will already throw it for you in that case).

So before I create a ticket for that issue: is the code example you provide your real use-case, or did you just provide us with a minimal example that triggers the issue? I’m asking this because: Your example doesn’t fulfil the contract anyway, as compareTo always returns 0 (except for o == null). So in case your real (non-example) code features a pattern like I mentioned, it wouldn’t indeed be a false positive.

Cheers,
Marco

7

Hi, Marco.

Thanks for the reply. And yes, that code was just a minimal sample :slight_smile: Not the most usefui Comparable unless you just want to burn some CPU cycles!

A more realistic example would be something like this:

public interface OurInterface extends Comparable<OurInterface> {
  int value();

  @Override
  default int compareTo(@Nullable OurInterface o) {
    if (o == null) {
      throw new NullPointerException("Cannot compare to null");
    }
    return Comparator
        .comparingInt(OurInterface::value)
        .thenComparing(i -> i.getClass().getName())
        .compare(this, o);
  }
}

I completely understand the rule is trying to avoid an antipattern, throwing a NullPointerException, and there would usually be a better exception type to throw if we are explicitly checking for nulls.

I think it’s fair to say calling a method on a nullable object, without checking for null, is also an antipattern; or possibly even just “a bug”.

9

I checked various resources, including Effective Java by Joshua Bloch, and triggering NullPointerException by accessing the argument in compareTo appears to be a common practice.

That said, the rule is indeed somewhat opinionated and for this reason it is not enabled by default (not in the Sonar way profile), so we will not be changing it.