added some null annotations to core APIs

[kaikreuzer]

I had started to put some null annotations to our Registry interface. I learned that adding such annotations quickly spreads throughout the code, so I ended up adding them at a lot of places.
I tried to limit it for the start and have a decent balance between the number of changed classes and the number of new warnings being added to the code at all unchecked places.

A few observations:

• In general, I think it is a good thing to have the annotations as one pays more attention to parameters that are passed to and from methods.
• Being able to express that one expects non-null params makes many of those throw new IllegalArgumentException() superfluous.
• It is indeed a pity that we cannot express that a collection does not contain any null values - when iterating over them (what we do very regularly), we always have to add an additional null check to be safe (and not to see any null check warning).
• Collections.emptyList() (and similar methods) are not recognised to be non-null, so we have to add a suppressWarnings when we use them (not ideal). The same applies to other 3rd party libs or JVM classes as we do not have any null information about them.
• I think we have introduced a bug when adding the Identifiable interface: I would claim that Identifiable.getUID() should return a non-null value. Adding such an annotation gets us into trouble with the Rule class, though as we allow the creation of rules without any UID (which is added only by the rule registry upon add). I think we should have a closer look at how this can best be resolved - for the moment, I didn't add the annotation on getUID().

Signed-off-by: Kai Kreuzer kai@openhab.org

[danchom]

Is this annotation is needed here? This method is not part of the public api and it always returns not null value. If it returns a null this is bug in implementation.

[kaikreuzer]

If it always returns a non-null value, why not state it here? It prevents warnings at places where this method is called (as you won't need to do any null checks there).

[danchom]

I'll not add any null checks because I know that it does not return null. The problem with the annotation is it adds unnecessary checks on each method call.

[kaikreuzer]

"because I know"

Now that is really helpful, is it? If somebody refactors the code so that it suddenly returns null, all your code will break without noticing. If somebody adds a functionality that uses your method, he will add null checks, because he does not know that it will always be non-null. For both situations, the annotation is very helpful.

[danchom]

Ok I agree.

[danchom]

Is it possible the AbstractRegistry to return a null value for already added element? If so it is a bug in the AbstractRegistry implementation for me.

[kaikreuzer]

No, it is a bug in your train of thoughts - which proves that the annotations are helpful :-)
We are requesting a rule of a certain UID here. You cannot assume that it exists here. Although you have added it previously, somebody else might have deleted it again or the initRuleId method might have decided to store it under a different UID - how could you tell?

[danchom]

The initRuleId won't change id because it is my method, but you are right that someone can meanwhile remove the added element. Ok this approach is better then synchronization for me.

[kaikreuzer]

because it is my method

There is no such thing as "my method" in an open source project. Make sure to implement robust code that does not do too many implicit assumptions that only exist in your mind.

[danchom]

"my method"

I mean that this is an implementation detail which is not exposed by the API. We have to find a limit where to stop with annotations. Do we have to set annotation on the private methods too?

[kaikreuzer]

If they are helpful as they remove a warning at another place as in this case, what speaks against it?

[danchom]

I don't have a worning in places where I use this method. Btw these warnings are related with missing null checks. Isn't be better to have handle these situations (as we saw that @nonnull does not defends us) and provider a suitable exception instead of just a NullPointerException at runtime.
Anyway I can live with this change.

[kaikreuzer]

as we saw that @nonnull does not defends us

It actually defends us especially on internal methods, because we have checks in place in our IDE and the Maven build. So we can be sure that all ESH code itself respects the annotations - imho definitely helpful for any kind of future refactorings.

[danchom]

The implementation of this method will never return a null element. I think the annotation is an overweight of the method. IMHO it is better to update java doc to said the method returns not null value instead of adding a runtime check on every method call.

[kaikreuzer]

No, that is the whole intention of the annotations (please read all the discussion on #3624): Nobody reads JavaDoc and if you can express the very same in a machine readable way, it is WAY BETTER.

[danchom]

OK I agree that it is more readable way, but it adds additional checks at runtime, which are not needed for me. Actually this is not public method used only by the RuleRegistryImpl and RuleEngine. Anyway I can live with it.

[maggu2810]

but it adds additional checks at runtime

I didn't get the point why the annotation itself will add checks at runtime.
Could you please explain.

[maggu2810]

About the JavaDoc and annotation: The JavaDoc is for the developer. If a developer does not read the documentation he does it wrong. But: The nullness annotations are for the tooling on the development and compilation time. Tools (e.g. IDE or something that is used at compile time) could check if your written code is safe.

[maggu2810]

but it adds additional checks at runtime

I didn't get the point why the annotation itself will add checks at runtime.
Could you please explain.

Because the @nonnull is annotated with @retention(RetentionPolicy.RUNTIME)

Can you please explain me about the relationship between the retention policy runtime and additional checks at runtime?

Annotations are to be recorded in the class file by the compiler and retained by the VM at run time, so they may be read reflectively.

So, a point could be "the file size of the classes increases by a few bytes".

[danchom]

Yes you are right, but are you sure that this annotation is not used at runtime by the VM to ensure that the passed parameter is not null? For example if we have this method:
public void foo(@nonnull String a)
the VM has to check on each call if the param a != null otherwise we can't remove the null check from the method. If so I'm not sure if there is a need from it at all.

[maggu2810]

The JVM does not care about the annotation at all.

[danchom]

Is than suitable to remove null pointer checks in all places in this commit? For example in the AbstractManagedProvider.add(E elemetn) was removed
if (element == null) {
throw new IllegalArgumentException("Cannot add null element");
}
which will lead to the NPE instead of exception with describable message

[kaikreuzer]

Right, see #3933 (comment).

[danchom]

I don't see what information gives this annotation here.

[kaikreuzer]

It says that the return value might be null (and by adding it, we state that this is deliberate and not that we merely didn't bother adding a @Nonnull annotation).

[danchom]

My concerns was that exactly this was said in the javadoc (but in case if it is not read ok) and it is obvious for me that it will return a null for any key without value (as a Map).

[kaikreuzer]

it is obvious for me that it will return a null for any key without value

Well, there are also other examples, so making it explicit is a good thing.

[danchom]

If I understand right the ItemRegistry has a duplicate get method named getItem() which throws and exception instead of returning null. Isn't be better to set @nonnull to ItemRegistry.getItem() instead of @nullable to all registries?

[maggu2810]

You can change the return value from @Nullable of a parent class to @Nonnull in a child but you can't change from @Nonnull to @Nullable.

It is the same logic used for more specialized returned classes used by the JVM.

[kaikreuzer]

Isn't be better to set @nonnull to ItemRegistry.getItem() instead of @nullable to all registries?

getItem() obviously only exists on ItemRegistry, so I do not understand the question.

[danchom]

@maggu2810 these are two different methods and the getItem() does not overwrite the get(), My proposal was: Registry.get() to be not annotated, because it is obvious that it returns null for missing elements (as any Map), and to annotated @nonnull ItemRegistry.getItem() because it never return a null.
But if you think current approach is better I'm ok with it.

[maggu2810]

There are some @SuppressWarnings("null") I would like to check. I can do it after this one is merged or before.
I know you already address that issue in your initial comment. But I would like to give it a try (some time).

All the other stuff looks good to me.

[maggu2810]

I would like to check this one (some time)

[kaikreuzer]

I'd be happy if you'd find a better solution - my issue is that the whole Java8 Stream API is pretty much incompatible with those annotations. Which might be a hint that for all new stream based APIs that we introduce, we might indeed want to consider the use of Optional.

[maggu2810]

See below...

[maggu2810]

• In general, I think it is a good thing to have the annotations as one pays more attention to parameters that are passed to and from methods.

Me, too.

• Being able to express that one expects non-null params makes many of those throw new IllegalArgumentException() superfluous.

Sometimes yes, sometimes no. It depend on the use case. The annotation itself is no safe-guard, it is equal to a comment in the JavaDoc. We cannot know if a binding developer is using the annotation checks etc. So, if the framework has to protect itself to prevent a NPE some time later, he still needs it.

• It is indeed a pity that we cannot express that a collection does not contain any null values - when iterating over them (what we do very regularly), we always have to add an additional null check to be safe (and not to see any null check warning).

Hm, IIRC this was my concern about using the chosen annotation and you told me it doesn't care.

• Collections.emptyList() (and similar methods) are not recognised to be non-null, so we have to add a suppressWarnings when we use them (not ideal). The same applies to other 3rd party libs or JVM classes as we do not have any null information about them.

The Checker Framework supports an "annotation stub" for the whole JDK classes.

• I think we have introduced a bug when adding the Identifiable interface: I would claim that Identifiable.getUID() should return a non-null value. Adding such an annotation gets us into trouble with the Rule class, though as we allow the creation of rules without any UID (which is added only by the rule registry upon add). I think we should have a closer look at how this can best be resolved - for the moment, I didn't add the annotation on getUID().

I agree that Identifiable.getUID() should return a non-null value. We should check if this could be fixed in the automation implementation (e.g. a flag that the UID is temporary generated -- but there are other ones that know the code better).

[kaikreuzer]

So, if the framework has to protect itself to prevent a NPE some time later, he still needs it.

Ok, agreed; but my suggestion would be to only have additional null checks at places where it could really have negative impact on the framework, if the NPE happens at a later stage in the code. If it "just" leads to an NPE being thrown, that's something in the responsibility of the caller then.

IIRC this was my concern about using the chosen annotation and you told me it doesn't care.

I didn't "not care", but I meant better have something than nothing. But I admit that I underestimated the widespread use of that case in our API, so point for you ;-)

The Checker Framework supports an "annotation stub" for the whole JDK classes.

There is also a similar feature in the Eclipse IDE (external annotations), maybe we should have a closer look at it in future.

I have rebased the branch, so imho it should be ready to be merged. Note that Travis is likely to fail for some other reason.

[maggu2810]

Ok, agreed; but my suggestion would be to only have additional null checks at places where it could really have negative impact on the framework, if the NPE happens at a later stage in the code. If it "just" leads to an NPE being thrown, that's something in the responsibility of the caller then.

Exactly what I was thinking about. 👍

[maggu2810]

I didn't "not care", but I meant better have something than nothing. But I admit that I underestimated the widespread use of that case in our API, so point for you ;-)

Thanks for the point, but be sure, its not about "I told you".
I don't understand your comment "better have something than nothing".
The related discussion was about "which" annotation is used as there are several ones.
The used annotation could be configured in the Eclipse IDE, so I don't think it was about "something or nothing" but "javax" namespace vs. another specific one.

There is also a similar feature in the Eclipse IDE (external annotations), maybe we should have a closer look at it in future.

Didn't see it yet. Will try to find it 😉
Then I would prefer (with respect to your comment above about the stream API etc.).

Adding @SupressWarnings("null") is IMHO contradictionar to introducing nullness annotations.

[maggu2810]

See below...

[kaikreuzer]

The related discussion was about "which" annotation is used as there are several ones.

Ok, you are right (again) ;-)

Didn't see it yet. Will try to find it

Here's something to read for the weekend: http://help.eclipse.org/oxygen/index.jsp?topic=/org.eclipse.jdt.doc.user/tasks/task-using_external_null_annotations.htm

[maggu2810]

Would it make sense to give the Eclipse JDT Nullness annotations a try?
If it works the same way as the javax annotations but allows to annotate the generic type argument types, I would prefer to use them.
As ESH is an Eclipse project itself, it is using the Eclipse IDE for it development, the annotations are optional at runtime, ... I assume using the Eclipse JDT Nullness annotations could be argued 😉

[kaikreuzer]

... can be argued, yes...
I'll leave the check if that is a simple switch that immediately gives us generic type annotations on top to @triller-telekom as he did the introduction of the annotations in the first place.

[triller-telekom]

I did a quick test with the eclipse annotations and this is the result:

my example

public void mainMethod() {
        List<String> sl = testMethod(null);
        for (String s : sl) {
            s.length();
        }
    }

    private @Nullable List<@Nullable String> testMethod(@NonNull String abc) {
        return new ArrayList<>();
    }

First observation: I can easily replace the package names of javax.annotation with the ones from eclipse.
Second observation: I can annotate the list elements: List<@Nullable String>
However, the s.length is not marked as a problem although the elements might be null.

The two examples from the Eclipse JDT documentation do work as stated in the comments:

int processWithNulls (@NonNull List<@Nullable Integer> ints) {
        int result = 0;
        for (int i = 0; i < ints.size(); i++) {
            Integer element = ints.get(i);
            result += element.intValue(); // NOT OK: list element can be null
            ints.set(i, null);            // OK: list accepts null value
        }
        return result;
    }
    
    int processWithoutNulls (@NonNull List<@NonNull Integer> ints) {
        int result = 0;
        for (int i = 0; i < ints.size(); i++) {
            Integer element = ints.get(i);
            result += element.intValue(); // OK: list element is known to be nonnull
            ints.set(i, null);            // NOT OK: list does not accept null value
        }
        return result;
    }

So I think the check works only intra-procedural?

@maggu2810 What are your observations in this regard?

[triller-telekom]

Ok, compared to the Findbugs annotations we can get rid of a warning on s.length() in this case (Findbugs does not know that the items inside the collection are non-null because we cannot put an annotation there):

    public void mainMethod() {
        List<String> sl = testMethod(null);
        for (String s : sl) {
            s.length();
        }
    }

    private @NonNull List<@NonNull String> testMethod(@NonNull String abc) {
        return new ArrayList<>();
    }

Though I am not sure if the warning disappears because there is a real check, OR there is no warning/check mechanism implemented for items within a collection. Because in the @Nullable example in my previous comment it also did not complain about the s.length(), although in that case there would be the danger of running into a NPE..

[kaikreuzer]

I have updated my PR to use the JDT annotations instead - a use of generic type annotations can be found here. This annotations prevents a warning in this line, since we have to make sure to pass a non-null value, which is now ensured through the new annotation that wasn't possible before.

It does not seem to solve ALL out collection issues though. For example, I wasn't able to get rid off this check, since iterator.next() does not seem to inherit the annotations of its own iterator for the entries... But hey, we cannot have everything.

So if this version is fine to everyone: @triller-telekom, could you then help to adapt the Maven build accordingly? I have already removed the javax.annotation from the target platform with this PR.

[triller-telekom]

If this is a step forward compared to the javax.annotation in terms of that we can now annotate collections which apparently we make use of quite frequently, I am fine with switching over to the jdt annotations.

@maggu2810 Since you are using annotations on your local IDE since quite a while. WDYT?

[maggu2810]

Sorry, very busy...

I am using nullness annotations and checks several month now (approximately about a year). I integrate the checks in the build system only. I didn't try to configure the IDE, yet.

IMHO we should use "something" that support the generic type annotations e.g. the JDT ones. If the tools that checks the sources (in our case the Eclipse tooling) does not detect all that stuff at the moment, this is another story (the tooling could be improved).

WDYT?

Let's give it a try.

[kaikreuzer]

Let's give it a try.

So feel free to merge!

[triller-telekom]

Since we are starting to use quite a lot of annotations now and more often run into situations where we would need annotations on the JDK or other external libraries, how shall we proceed with that?

Eclipse offers the "Eclipse External Annotations": @kaikreuzer mentioned them here: https://github.com/openhab/openhab2-addons/pull/2556#issuecomment-323672827.

I had a brief look at them and the repository doesn't look that full so far. But how about integrating these external annotations in our setup and in case they are lacking some annotation we can contribute them to the repository?