While fixing a SonarQube issue for the rule (a “high” level rule, see SonarCloud ), I’ve realized that it would be good that we use @javax.annotation.Nullable for the case when a method returns a Boolean and the api contract allows to return null (as otherwise calling code may not always check for null).
I’m thus proposing to use @javax.annotation.Nullable for the case when a method returns a Boolean and the api contract allows to return null.
If we agree about it, we’ll need to progressively fix the places where we don’t use it, but that’s going to be helped by the fact that soanrqube will report these errors (and fail the build when we apply New SonarCloud quality rules ).
Let me know what you think and if you agree I’ll document it. If we don’t agree then we will need to add some exception to the sonarqube default set of rules used.
We need to decide if we want to place it just on the implementation method (where it’s expected by SonarQube) or also on the interface method (when there’s one).
I’m proposing to place it on both places so that it’s more clear when reading the interface if null is supposed to be allowed or not.
It does make sense since it’s a valid use case that we need to address now (again, it’s a high level sonarqube issue which will fail our build when we apply New SonarCloud quality rules).
My goal was to discuss this quickly for this specific use case only. It doesn’t mean that we shouldn’t discuss it more generally (but for that it would need more research and proposals as each annotation should be discussed individually and for a use case and a way to use it).
My point, and my worry, is that when you use annotations like this somewhere, users rightfully assume that you can rely on them in the whole API. For example, using only Nullable when its documentation states that “This annotation is useful mostly for overriding a {@link Nonnull} annotation.” can be pretty strange.
Sure but most (if not all) of the JSR305 annotations are independent of each other.
Re @Nullable and @NonNull, they’re indeed complementary but I don’t think we need to use @NonNull to use @Nullable.
BTW we also have the option to use @javax.annotation.CheckForNull instead of @javax.annotation.Nullable.
The full javadoc is:
The annotated element could be null under some circumstances.
In general, this means developers will have to read the documentation to determine when a null value is acceptable and whether it is necessary to check for a null value.
This annotation is useful mostly for overriding a Nonnull annotation. Static analysis tools should generally treat the annotated items as though they had no annotation, unless they are configured to minimize false negatives. Use CheckForNull to indicate that the element value should always be checked for a null value.
When this annotation is applied to a method it applies to the method return value.
How these annotations are interpreted vary slightly based on the tools. They are for static analysis. Our main tool here is SonarQube and they recommend using @javax.annotation.CheckForNull or @javax.annotation.Nullable.
Out of curiosity, why would a method return Boolean instead of boolean if not to be able to return also null? The fact that the return value is Boolean and not boolean implies that the return value is nullable no? Which is why I find the @Nullable redundant in this specific case.
Good question. I guess the question should be asked to SonarQube and why not only they implemented SonarCloud but also gave it the maximum severity level of “High” (that last part is to avoid NPE at runtime for sure).
All they say is Callers of a Boolean method may be expecting to receive true or false in response.. I think they’re taking a safe approach and it’s probably true that if we review all our code where we call a method returning a Boolean, we will likely find places where we don’t check for null.
Outside of the null use case, there are maybe cases where you need use Boolean (maybe if you need to pass the result to a method accepting an Object - although maybe autoboxing will be used in this case).
So indeed another option would be for us to give the rule “if we don’t return null then don’t use Boolean”. However the problem is that there’s no rule for this and to obey the SonarQube rule we would still need to add the annotation. And it’s a more semantic way than having to document it in the javadoc (which is also needed probably as it can and should explain why null can be returned), as in:
/**
* @param namespace the namespace for which to check if the extension is compatible
* @return true if the extension is compatible, false if incompatible and null if unknown
*/
Boolean isCompatible(String namespace);
Overall, I feel it’s an ok rule and it can force us to review that calling code check for null when it’s raised in SonarQube.
I understand that you’re making this proposal following the Sonar rule, but I find it quite limited to limit the usage to API returning Boolean type. IMO it would make sense to use the annotation for any API, especially for the APIs designed before we started using Optional. There’s plenty of APIs in oldcore (and not only) that might return null and it’s really not something you’d necessarily expect, so having this annotation might greatly help using those.
@javax.annotation.Nullable
public String foo(String param1, List<String> param2)
I’d instinctively think that foo might return null, I don’t think I’d imagine that it’s param2 that can be null, and it seems to me it could be error prone to authorize or even encourage using the annotation like that. Unless the annotation would be used inside the method in that case? Like this?
public String foo(String param1, @javax.annotation.Nullable List<String> param2)
Since sonar supports both @Nullable and @CheckForNull for this use case, I would prefer to recommend @CheckForNull in our rule since it matches better JSR305 specifications (in which you are supposed to use @CheckForNull for this use case, and @Nullable only to “cancel” a @Nonnull in an overwriting method).
We sometimes refer to them as “Weak Nullable” (javax.annotation.Nullable ), as opposed to “Strong nullable” (javax.annotation.CheckForNull ).
This case is not the same. From the citation above, there are cases where it is acceptable to not check the return value for null, because you determined that it was not necessary. We can not be sure statically that a NPE is possible, therefore we do not report an issue. If you want to enforce a null check, you should use CheckForNull , and an issue will appear!
I don’t know if this is true or not but for sure the annotation names suggest that (nullable means that it can be null, when checkfornull means you need to check for null).
Tooling support seems to vary for both depending on the tools.
In short these 2 annotations are not treated the same by SonarQube.
If you use @CheckForNull, SonarQube will raise an issue if calling code doesn’t explicitly check for null, while it’s not the case for @Nullable AFAIU.
Not sure this is still true given the age of the source you found.
Sure. Now, I’m pretty sure the cases where you should check the return value is much more common than cases like this so IMO we should recommend @CheckForNull for most use cases and explain when @Nullable is enough, instead of only using @Nullable for all cases.