[SOLVED] Struggling with @NonNullByDefault and required Nulls

Development
1

I have a class where the compiler tells that I HAVE to annotate it with @NonNullByDefault

[WARNING] org.openhab.binding.nanoleaf.internal.model.Ct.java:[20]
Classes/Interfaces should be annotated with @NonNullByDefault

So I do

@NonNullByDefault
public class Ct {

    private int value;
    private Integer max;

as it is a model that MUST have null values because it is used to generate JSON where the values MUST HAVE null values in certain situations I annotate the fields with @Nullable

@NonNullByDefault
public class Ct  {

    private int value;
    private @Nullable Integer max;
    private @Nullable Integer min;

which produces an even worse ERROR by the compiler

[ERROR] /Users/stefanhohn/Development/checkout/openhab2-addons-branch/bundles/org.openhab.binding.nanoleaf/src/main/java/org/openhab/binding/nanoleaf/internal/model/Ct.java:[41,983] Null type mismatch (type annotations): required '@NonNull Integer' but this expression has type '@Nullable Integer'
[ERROR] /Users/stefanhohn/Development/checkout/openhab2-addons-branch/bundles/org.openhab.binding.nanoleaf/src/main/java/org/openhab/binding/nanoleaf/internal/model/Ct.java:[49,1109] Null type mismatch (type annotations): required '@NonNull Integer' but this expression has type '@Nullable Integer'

which is weird because this documention https://www.openhab.org/v2.4/docs/developer/development/conventions.html
seems to say something different.

How can I get rid of the warning for NonNullByDefault while still intentionally allowing Nulls in my model?

I tried for hours but couldn’t find a solution. Any help is greatly appreciated!
Stefan

2

Did you annotate the methods also?

    public @Nullable Integer getMin() {
        return min;
    }

    public void setMin(@Nullable Integer min) {
        this.min = min;
    }
2 Likes
3

Thanks @hilbrand

I am bit cautious of saying that it might have helped (the weird thing is that in those cases when I try out different things on other places other warnings quickly pop up or come back again 
sigh
).

I marked the method as

    public @Nullable Integer getMin() {
        return min;
    }

However, this leads to the next problem when using the method:

Ct colorTemperature = ...

@NonNull Integer colorMin= (colorTemperature.getMin() == null) ? 0 : colorTemperature.getMin();

as it leads to a

Potential null pointer access: This expression of type Integer may be null but requires auto-unboxing

To find out what that means I disassembled into the following similar expression which makes clear what the issue is

        int colorMin = 0;
        if (colorTemperature.getMin() == null) colorMin = 0;
        else colorMin = colorTemperature.getMin();

So for some reason the checker doesn’t understand the fact that in this case colorTemperature.getMin() can never become null.

Any idea how to circumvent this now?

(did I ever say that I hate null handling? ;-))

4

Is there a reason not to make the fields primitive int in the first place? Because from your code snippets it doesn’t look like null is handled differently as 0. This doesn’t give an answer to the handling of nulls in general of course.

The null handling can certainly be difficult and the warnings are for worse case, meaning in practice it will never be a problem. I’m also not completely convinced the null annotations are the most elegant way to help with this. The underlying assumption for the warnings is, everything can be run a threaded environment. So in this specific case between the first call to getMin and the second call andother thread could have set the value to null, so the second call would be null. For variables that are really run threaded this makes sense (for example the scheduled task reference), but for most other objects it can be more of a burden to work around it than it really could cause a problem. To cover this first assign to a local variable and than use that variable. In general I prefer to use primitives as much as possible.

5

Thanks @hilbrand, the thread thing explains it.

So to wrap this up the most elegant way then is probably to use this implementation:

@Nullable Integer max = colorTemperature.getMax();
int colorMax = (min == null) ? 0 : min;

As of

Is there a reason not to make the fields primitive int in the first place?

Yes, there is. There are situations where the object is and needs to be completely “empty”, i.e. the values of the max and min are not set. The CT object will then be serialized to JSON and due to the API requirements on the nanoleaf must not be set and kept empty in the JSON.

I’m also not completely convinced the null annotations are the most elegant way to help with this.

cheers
Stefan

Thanks, that eases my pain a bit :wink: though still I like to provide as much clean code as possible before we go live, so I really strive to get rid of all warnings (otherwise by the way the broken window effect kicks in eventually).

6

Ah make sense for serialization.

If it’s a case that these fields should never be serialized when send to Nanoleaf, but do need to be read when read from Nanoleaf you can use the annotation @Expose.

You can also either let the get methods simply do the null check and return a primitive or add additional methods for returning primitive values. That way you don’t need to pollute your other code with all these local variables.

7

Ok, I will probably keep that in mind for the next release and put that on my list, @hilbrand , as I am pretty much through the whole code already


  1. Meanwhile I have two last issues which I do not have an answer for:
if (positionData!=null) {
             // do some stuff

            for (int index = 0; index < numPanels; index++) {
                if (positionData != null) {
                    @Nullable PositionDatum panel = positionData.get(index);
                     
                    if (panel != null) {

Now it complains on the second but last line (@Nullable PositionDatum panel
)

Potential null pointer access: this expression has a ‘@Nullable’ type

WHY? I am checking the “if positionData is null” one line before, so it can never be null at that line, can it?

  1. I have the following code
@Nullable Write response = gson.fromJson(...)
if (response!=null) {

and it complains about it “The variable response cannot be null at this location”

Why can it be so sure? It cannot know the code of gson.fromJson(), can it?
(Actually it is try because it returns “Primitives.wrap(classOfT).cast(object);”)

on the other hand if I do not declare the @Nullable Write it will complain by issuing

Unsafe interpretation of method return type as ‘@NonNull’ based on substitution ‘T=@NonNull Write’. Declaring type ‘Gson’ doesn’t seem to be designed with null type annotations in mind

cheers
Stefan

8

I absolutely hate the nullable annotation for too many reasons (and don’t want to hijack this thread).

Hard to tell what’s going on since you are only posting snippets. But if positionData is a non-final instance variable - then the compiler assumes that positionData could potentially change between your first reference and your next one [by some other thread] and thus can potentially be null (regardless if there is no possibility of that happening).

What I do (about 1000 times in the sony plugin) is something like:

Map<whatever,whatever> localPositionData = positionData;
if (localPositionData != null) {
   ...
   .. = localPositionData.get(index)..

That way the compiler knows it can’t be null after the if statement.

As for the gson one - weird, I use it all time time (without the @Nullable) and I don’t have issues. What I’m betting is that the variable you are passing into the fromJson is potentially null and it’s complaining about that (might be the same issue as the above).

EDIT: just did a full compile - the GSON one is just a INFO (or warn) message and can be ignored. Only the ERROR one’s really need addressing


9

Thanks @tmrobert8,

I will look at the warnings and your recommendations at the weekend. I appreciate your time for answering that and will revert as soon as I had a deeper look into it.

btw, the “do some stuff” in the snippet doesn’t change anything on positionData


cheers
Stefan

Cheers
Stefan

10

Doesn’t matter what you know and that’s one of the reasons I hate the nullable annotation. What’s potentially possible is what matters