String test = null;
checkNotNull(test, "This imported method will throw an exception because string `test` is null, hence the next line will not evaluate.");
char unused = test.charAt(0);
We have noted that the issue does not arise if the imported method’s definition is used directly; the issue is due to SonarCloud not properly handling the outcome using the imported method definition.
String test2 = null;
if (test2 == null)
{
throw new PermissibleArgumentException(String.valueOf("This will throw an exception because string `test` is null"));
}
char unused2 = test2.charAt(0);
The imported method is a helper/convenience function and does what it is intended to do.
Should SonarCloud be able to determine that there is no NullPointerException risk in the first reproducible code sample (without the addition of a comment to tell SonarCloud to avoid adding a bug for this line of code)?
Note that the checkNotNull method definition is in the 2nd code segment, also here explicitly:
public static <T> T checkNotNull(@Nullable T reference, @Nullable String errorMessage)
{
if (reference == null)
{
throw new PermissibleArgumentException(String.valueOf(errorMessage));
}
return reference;
}
Answer greatly inspired by this other post, with up-to-date information.
Just a few words about the rule now. Rule S2259 is based on a Symbolic Execution (SE) engine. This engine is validating execution paths and checking for non-null values or null-check along the way, exploring all the possibilities. Unfortunately, its actual state also has some limitations, like the one you are hitting here.
As of today, the SE engine is able to explore non-overridable methods (static, for instance), when declared in the same file being analyzed. When exploring such methods, the engine then deduces behaviors regarding null-checking (among other things). The SonarJava SE engine is, however, by default, not configured to explore methods declared in other files (in your case, I suspect that checkNotNull is defined in another file).
In addition to methods declared in the same file, we also support “behavior” of well-known methods. As of today, we support the methods from the classes listed here. For example, the following code is not raising an issue:
if (Objects.nonNull(arg)) {
arg.doSomething(); // No issue here, arg is considered as not null
}
If you are using your own helper method, it is unfortunately not possible to define “custom method behavior” or anything like that. For the time being, I would recommend to mark as False Positive the issue.
The second example is not exactly related to the problem described in my previous post, but is also due to a limitation of the engine: it is not able to detect that a call on an object has consequences on the result of another call.
To go a bit further into the implementation details, getBody() is annotated with org.springframework.lang.Nullable, annotation that we consider as Strong Nullable (An element which nullability can not be statically determined, it should always be checked for null). It is therefore fair to report an issue for this code… A Weak Nullable annotation would make more sense here. It seems to be another consequence of the inconsistent usage of nullability annotations in Java ecoystem.
All in all, the outcome is the same: I would recommend to mark as False Positive the issue.
Thank you for your help here. We face exactly the same problem in my team : we use a method from a third party library that checks for NULL just like the Christina’s example.
It’s a mess for us because we have a lot of false-positive because of that.
Can we make something on our side to reduce those false-positive ?
Maybe extend the “behavior” of well-known methods ?
Note that JetBrains Idea uses a @Contract annotation to be able to “extend the behavior of well-known methods”. You might e.g. annotate the checkNotNull method with @Contract(value = "null, _ -> fail") meaning “whenever the first argument is null, this method will throw an exception. And Idea understands that, so will not warn on the possibility of null reference in that case. (And you might even add “external annotations” for third-party code.)