SonarJava 4.12 complains about runtime checks for @Nonnull parameters

[andreas...@coremedia.com]

Hi,

after updating from SonarJava 4.10 to 4.12 I see additional violations for rules S2583 and S2589, which now complain about null checks for @Nonnull method parameters.

For example:

import javax.annotation.Nonnull;
import java.util.Objects;

public class Fp2583and2589 {

  private String a;
  private String b;

  public void setA(@Nonnull String a) {
    if (a == null) { // <------- S2583 reports violation: Change this condition so that it does not always evaluate to false
      throw new NullPointerException();
    }
    this.a = a;
  }

  public void setB(@Nonnull String b) {
    this.b = Objects.requireNonNull(b); // no violation reported
  }

  public void m(@Nonnull String a, @Nonnull String b) {
    if (a == null || b == null) { // <----- S2589 reports: Remove this expression which always evaluates to false
      throw new NullPointerException();
    }    
  }
}

One could argue that the null checks in these methods are superfluous because callers should never use null values here and Sonar would find callers that violate the contract.

However, in my opinion these null checks are still very useful and Sonar should not complain here. They are especially very useful when a) you're building an API or framework that is used by code that is not under your control or b) when methods are called reflectively. Null checks for setter or constructor parameters allow code to fail early. Otherwise NPEs might occur somewhere in the middle of the code and framework users cannot easily see their error.

Interestingly the Sonar rules do not complain about Objects.requireNonNull in method #setB in the above example, although it's semantically the same as method #setA.

It would be great, if you could change the rules to not create issues for these cases. Or, alternatively extract these cases into a separate rule that can be easily disabled. The rules S2583 and S2589 are really helpful and find a lot of issues and I don't want to disable them generally.

Thank you,

Andreas

ps: Please tell me if you have a JIRA issue for this request.

[jeanchrist...@sonarsource.com]

Hello,

The very object of the @NonNull annotation is to inform tools, and static analysers in particular, that this particular parameter can not, and should not, be null.

So to me this is absolutely redundant and SonarJava should be reporting it.

By the way, you don't have to disable the rule if you disagree with this particular case, you can just mark these issues as "Won't fix" in SonarQube.

Best Regards,

[andreas...@coremedia.com]

Hi,

sorry, but I strongly disagree. Let me explain.

There's no way to statically enforce that a @Nonnull parameter cannot be null, especially not in public methods of framework libraries. While a value should not be null, the documented API contract can still be violated at runtime. Additional safeguards are a good thing for non-private methods.

This is not only my personal opinion, see for example this Oracle blog post, which says "Optional Type Annotations are not a substitute for runtime validation"
https://blogs.oracle.com/java-platform-group/java-8s-new-type-annotations

While it's not necessary to have null checks in all methods that use the values right away, they're important in public API setters and constructors which just store the values. If you don't check input values here, nullability problem can propagate badly. Read "Item 38: Check parameters for validity" of J. Bloch's book Effective Java, 2nd ed.: "It is particularly important to check the validity of parameters that are not used by a method but are stored away for later use."

Nullable annotations are not only for tools, they also serve as API documentation for humans. They're a great way to improve API documentation. With your decision to forbid runtime-checks, you're basically saying that these annotations cannot be used in public framework methods.

Please, please, revisit your decision. Or would you even forbid the semantically equivalent call to "requireNonNull" in a future version? I hope not.

If you still disagree, let's at least treat this as a "controversial rule" and move it out of the useful rules S2583 and S2589. According to this post you don't want to enable controversial rules by default: https://groups.google.com/d/msg/sonarqube/DFPXz2PrToE/ABn_8_viAwAJ

Using "Won't Fix" (or @SuppressWarnings in code) at every such issue is not an option in a large existing code base. And it's problematic for large development teams, because an active rule would misguide inexperienced developers. It's important to provide sane defaults.

Thank you,
Andreas

[andreas...@coremedia.com]

Hi,

have you seen my reply?

Best regards,
Andreas

[Jean-Christophe Collet]

Hello,

I think I respectfully disagree on one thing: Annotations are not a substitute for proper documentation via javadoc. If the parameter should not be null it should be documented in the javadoc regardless of whether there is an annotation or not. Annotations are meant primarily for compilers, analysers and IDEs.

I fully agree that Input validation and defensive programming are fundamental to code quality and I would definitely require checking for null in all public APIs. However, this means that you expect that this can happen therefore adding such an annotation is misleading.

Adding a switch to you the rule is, unfortunately not doable as that annotation is treated at the data-flow analysis level and is not specific to one particular rule.

So I'm afraid you're best course of action is still to mark these cases as "Won't fix".

Best regards,

--

Jean-Christophe COLLET | SonarSource

Product Manager

http://sonarsource.com

[andreas...@coremedia.com]

Okay, so you're basically saying: You must not use @Nonnull annotations in public API.

Because we cannot ensure that callers really always use non-values at runtime and we want to have defensive null-checks, which however is forbidden by your data-flow analysis.

Parts of our customers use SonarQube to check that they're using our API correctly. If we remove the @Nonnull annotations from our API, as you recommend, our customers cannot use SonarQube anymore to check there code for null issues.

That's very disappointing and in my opinion a bad decision for SonarQube to take. From my good and long experience with SonarQube, this is also very surprising.

I still think this quote is correct: "Optional Type Annotations are not a substitute for runtime validation" (https://blogs.oracle.com/java-platform-group/java-8s-new-type-annotations)

Cheers,
Andreas