Introduce possibility to use @NonNull and @Nullable

[kaikreuzer]

Introduce possibility to use @nonnull and @nullable annotations in IDE and Maven build.

[maggu2810]

If we add

 org.eclipse.jdt.annotation;resolution:=optional

to the Import-Package section of a manifest file we can already use the Eclipse JDT nullness annotations on that bundle.

---

My suggestion for the nullable annotations and checks:

• add @Nullable annotations for existing code only if needed or changes are done at the code. So, no need to start reading all interfaces and classes and add it just to use it on every place
• use the code as it has been annotated by @NonNullByDefault. That is the suggested way of using the annotations and some checkers use this default
• WRT above, no need to use @NonNull annotations -- only if you want to state that a "normal" nullable type is not null (e.g. a value of a map could be null, but you can use Map<String, @NonNull Object>).
• Do not enable the null check support in the Eclipse IDE using the setup file. This feature could be enabled by some developers but a global configuration could be very very noisy while it is not used in the whole framework.

For a first step, have a look at:
https://github.com/eclipse/smarthome/compare/master...maggu2810:nullable?expand=1

Question:
Should we consider to use (for new code -- so we don't need to break API) Optional<T> if a return value or argument is allowed to be null? As Optionals are now a language feature, we do not need to use annotations for new code but the new type.

[triller-telekom]

The Eclipse nellness annotations sound like a good way to start, however if I see lines like this I am not sure if we really want @Nullable "everywhere".

And I agree that we should not start flooding the old code right away with these annotations but introduce them step by step.

Does having a default @NonNullByDefault mean that if one wants to pass null to a variable that this variable has to be annotated with @Nullable? Seems strange at first but I think one can get used to it. But the normal use case is certainly that a variable should not be null, so this default is probably the best option rather than the other way around.

If we use those annotations I clearly vote for having them turned on for everyone since they should support other developers that they can see right away in Eclipse if a value can be null or not.

We had quite some discussions about the use of Optional<T> and it turned out that in most cases one has to call isPresent() instead of != null which is not a real benefit and it "uglyfies" the code in my opinion.

[maggu2810]

We had quite some discussions about the use of Optional and it turned out that in most cases one has to call isPresent() instead of != null which is not a real benefit and it "uglyfies" the code in my opinion.

My suggestion has been to use Optional<T> at that places where null has been a valid option.
Sure, you have to call isPresent instead of null, but that is only one point.
It could be used by other lambda expressions as well.
But my point has been, instead of using the @Nullable annotation AND check for null, we could use a official Java type to state that a value could be non present.
So, instead of using @NonNull and @Nullable for new code (only annotations) we could use Optional<T> instead of @Nullable and T instead of @NonNull.
So, it is IMHO the most readable way to state (using the type system) that a value is optional / perhaps non present / nullable.

[amitjoy]

@maggu2810 I agree that the use of Optional<T> would be beneficial for this project and it has already been moved as a first-class citizen in Java. I used to believe that Optional#isPresent is beneficial but later on, after delving deep into it, found out that it pollutes the source base as well. We just need to make sure that we should only use Optional<T> when we need to write concise and user-friendly code using Java's functional approach. We just need to make a considerable trade between the usage of Optional<T> and null. So, I believe using null cannot be completely forsaken or we just cannot adopt Optional<T> everywhere. We have to use them interchangeably. I have also written a blogpost [1] at DZone pointing out my endeavors with Optional<T> and how to use it.

[1] - https://dzone.com/articles/optionals-npes-and-design-practices

[kaikreuzer]

I think we all agree that Optionals are no option for the existing ESH APIs, so I would suggest to open a separate issue/discussion about that as it might potentially get long and distracts from the issue we are trying to address here.

So just as a recap: The wish was to use annotations on our existing APIs to express whether null values are allowed in or out, so that we do not have to mention this in the JavaDoc, but can actually rely on IDE features to highlight if a user tries to misuse our APIs. Note that my focus/priority is definitely on our APIs / public interfaces, imho we do not need to cover the "full code base" for now.

If we agree on that approach, my fist question would be: WHICH ANNOTATIONS?
There are afaik mainly 3 sources:

• Eclipse JDT
• Checker Framework
• FindBugs

FindBugs seem to be the reference implementation for (the dormant) JSR-305, which uses javax.annotation namespace - this would be my clear preference, as it is IDE independent and probably also best goes hand in hand with Findbugs itself.

I am not so sure about using @NonNullByDefault (which has to happen rather hidden in a package-info.java, if I get it right). If we only focus on framework APIs, we should imho explicitly express which passed parameters must NOT be null (as the default in Java is that it can be null) and whether or not we potentially return a null value. So I would vote for @NonNull annotations on parameters and @Nullable on return values - this would be exactly the same information that we so far also add in the JavaDoc.

Do not enable the null check support in the Eclipse IDE using the setup file. This feature could be enabled by some developers but a global configuration could be very very noisy while it is not used in the whole framework.

Currently it actually IS activated in the IDE. Afaik, it will only show you warnings/errors if there are annotations in place - so every message should be helpful and not be noise. Or am I missing something?

[maggu2810]

This one does not work with the FindBugs one:

final Map<String, @javax.annotation.Nullable Object> map = new HashMap<>();

I am using the Checker Framework for a while now:
https://blogs.oracle.com/java-platform-group/java-8s-new-type-annotations

I am not so sure about using @NonNullByDefault (which has to happen rather hidden in a package-info.java, if I get it right).

This is only the interpretation of the Eclipse IDE.
E.g. the Checker Framework already uses non-null on default.

IMHO arguments are more often non-null, then allowed to be null. So I would prefer to annotate @nullable and not the other one.

So I would vote for @nonnull annotations on parameters and @nullable on return values - this would be exactly the same information that we so far also add in the JavaDoc.

This could be done, but I don't see the tooling that could be configured that way to check for potential null pointer access violations.
AFAIK you can set one default for every type and the other one needs to be annotated. Which one differs between return values and arguments?

[kaikreuzer]

E.g. the Checker Framework already uses non-null on default.

My "benchmark" is actually what we have if we do not do anything - so the status quo of Java development. And this is that we have nullable as default everywhere, right? Likewise, this is what every Java developer expects, if he has never heard about such annotations.

AFAIK you can set one default for every type and the other one needs to be annotated.

I'll have to test this. But my assumption was that it will work nicely. If I add some constraint for any specific value (parameter or return value), this should suffice for the IDE to check that this is satisfied. I don't see why there should be the need to declare all other values in the same class at the same time (and be it through a package default).

[maggu2810]

What is the status quo of Java development at all?
Tools like FindBugs, PMD, Checkstyle and so on are not an language feature, but tooling could be used to improve the code quality.
Sure, we could use annotations only for the reader that have a look at the annotation. But then I assume the JavaDoc is read by more people then the annotation.
Or we would like to satisfy the checks of the Eclipse IDE only.
But IMHO the code and the framework is much better if code quality tools could check it on build time, too (e.g. quality gates for CI and so on). As this would be a big work to fulfill all that checks for the framework build, my intention was to improve that stuff, that is used by third party developers, so that ones could use that tooling.

The Checker Framework itself supports stubs (it also provides stubs for the whole JRE), where you can provide missing annotations. I use it at the moment, but you need to maintain it yourself. So, I can continue using my null checks regardless of the decision taken here, but for me I don't want to keep a status quo, but I want to make it as good as possible.

[kaikreuzer]

What is the status quo of Java development at all?

That you can pass null as a parameter to any method and that you should expect null as a return value of any method - the Java compiler won't complain.

much better if code quality tools could check it on build time

Absolutely! This is EXACTLY what I want to achieve with this feature as mentioned above: "so that we do not have to mention this in the JavaDoc, but can actually rely on IDE features to highlight if a user tries to misuse our APIs" - "IDE highlighting" should be translated to "showing compiler errors" here, and yes, that should/could also include the headless build (that's why I referred to the Findbugs integration).

[maggu2810]

I agree that using an annotation package like javax.annotation would make the most sense. But as written above, you cannot annotate a type of a collection. So they don't support the features of Java 8.

I assume that FindBugs could handle the Eclipse JDT annotations reading this file: https://github.com/findbugsproject/findbugs/blob/master/findbugs/src/java/edu/umd/cs/findbugs/ba/NullnessAnnotation.java

That you can pass null as a parameter to any method and that you should expect null as a return value of any method - the Java compiler won't complain.

So, if we would like to use that status quo, we need to add @nonnull for every return type and argument type. Didn't we?

So I would vote for @nonnull annotations on parameters and @nullable on return values - this would be exactly the same information that we so far also add in the JavaDoc.

As you already written above, the tooling need to be tested.
I tried multiple ones and use the one that works the best way for me.
I am interested in your observations.

[maggu2810]

I checked the Eclipse IDE again...

import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.Nullable;

public class Test {

    public static class TestWithoutAnnotation {
        public static Object testReturnNull() {
            return null;
        }

        public static void testArgumentDereference(Object obj) {
            if (obj.hashCode() == 1) {
                throw new IllegalArgumentException();
            }
        }

        public static void testCheckArgForNull(Object obj) {
            if (obj == null) {
                throw new IllegalArgumentException();
            }
        }
    }

    public static class TestWithAnnotationNullable {
        public static @Nullable Object testReturnNull() {
            return null;
        }

        public static void testArgumentDereference(@Nullable Object obj) {
            if (obj.hashCode() == 1) {
                throw new IllegalArgumentException();
            }
        }

        public static void testCheckArgForNull(@Nullable Object obj) {
            if (obj == null) {
                throw new IllegalArgumentException();
            }
        }
    }

    public static class TestWithAnnotationNonNull {
        public static @NonNull Object testReturnNull() {
            return null;
        }

        public static void testArgumentDereference(@NonNull Object obj) {
            if (obj.hashCode() == 1) {
                throw new IllegalArgumentException();
            }
        }

        public static void testCheckArgForNull(@NonNull Object obj) {
            if (obj == null) {
                throw new IllegalArgumentException();
            }
        }
    }

    private static void nop() {
    }

    public static void main() {
        if (TestWithoutAnnotation.testReturnNull() == null) {
            nop();
        } else {
            nop();
        }
        if (TestWithAnnotationNonNull.testReturnNull() == null) {
            nop();
        } else {
            nop();
        }
        if (TestWithAnnotationNullable.testReturnNull() == null) {
            nop();
        } else {
            nop();
        }

        TestWithoutAnnotation.testArgumentDereference(new Object());
        TestWithAnnotationNonNull.testArgumentDereference(new Object());
        TestWithAnnotationNullable.testArgumentDereference(new Object());

        TestWithoutAnnotation.testArgumentDereference(null);
        TestWithAnnotationNonNull.testArgumentDereference(null);
        TestWithAnnotationNullable.testArgumentDereference(null);
    }

}

Eclipse complains about (changed nullable settings in that way that nothing is ignored):

• TestWithAnnotationNullable: testArgumentDereference: obj.hashCode(): Potential null pointer access
• TestWithAnnotationNonNull: testReturnNull: return null: Null type mismatch
• TestWithAnnotationNonNull: testCheckArgForNull: obj == null: Null comparison always yields false
• main: TestWithAnnotationNonNull.testReturnNull() == null: : Null comparison always yields false + dead code for first branch
• main: TestWithAnnotationNonNull.testArgumentDereference(null): Null type mismatch

So, IMHO non-annotated stuff is not checked at all.
If everything could be null, "obj.hashCode()" in TestWithoutAnnotation.testArgumentDereference should result in a warning.
If everthing is default non null, TestWithoutAnnotation.testReturnNull should result in a warning.

[kaikreuzer]

Ok, here are my tests and it is actually exactly what I expected and what I planned to achieve.
Check this code:

package test;

import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.Nullable;

public class Test {

    public @Nullable String myFrameworkMethod(@NonNull String paramA, String paramB) {
        return null;
    }

    public void myClientCodeThatUsesTheFramework() {
        String valueA = null;
        String valueB = null;

        String retValue = myFrameworkMethod(valueA, valueB);
        System.out.print(retValue.toUpperCase());
    }
}

I added two annotations that provide information about our framework method: paramA is expected to not be null, so we do not need to mention this in the JavaDoc nor do we need to throw IAEs if null is passed. The second information is that we might return a null value.
Without these two annotations, the code compiles just fine without any errors or warnings:

With these annotations, we have an error and a warning if we misuse the method:

This is all I wanted. Any unannotated code is just still the vary same as before, so this feature is minimal invasive and really only brings new behavior at the places where we desire it.

[triller-telekom]

I just did a couple of more tests and enabled all nullanalysis checks from eclipse as Error except for the "Missing @NonNullByDefault annotation on package."

An interesting finding is this one in the generated ItemsValidator from xtext:

if ((item == null)) {
      _or = true;
    } else {
      String _name = item.getName();
      boolean _tripleEquals = (_name == null);
      _or = _tripleEquals;
    }
    if (_or) {
      return;
    }
    String _name_1 = item.getName();

In the last line I get an error saying "Potential null pointer access: The variable item may be null at this location". The problem is that the basic static code analysis does not catch the fact that or is always set to true which makes the method return in case that item is null.

I have taken this as one basic example showing that we cannot catch all of our null problems with enabling static analysis. If you want to analyze all cases correctly the static analysis will take ages to complete which is why it has its limitations, the fore-mentioned example is one of the eclipse`s implementation. Due to these limitations there will always be false positives.

With such false positives we are also tight to use only "warning" level instead of "error". That is unless we fix the code to be easier to parse by the analyzer. The only problem in my example is that the code was auto generated, though maybe we could exclude this code from our checks? I did not (yet) find that option to exclude certain directories from the check...

Given that the static analysis is limited anyway, I think that the problem of missing

final Map<String, @javax.annotation.Nullable Object> map = new HashMap<>();

can be neglected for now.

Since we want to be independent of an IDE while compiling the code I would also prefer to use other annotations than the one from eclipse JDT. This would leave us to the decision between the checker framework and the reference implementation of JSR305 which is FindBugs, where I think the reference implementation should be used if it is sufficient for the problem we want to solve.

Citing @kaikreuzer on our goal that we want to achieve with these annotations

So just as a recap: The wish was to use annotations on our existing APIs to express whether null values are allowed in or out, so that we do not have to mention this in the JavaDoc, but can actually rely on IDE features to highlight if a user tries to misuse our APIs. Note that my focus/priority is definitely on our APIs / public interfaces, imho we do not need to cover the "full code base" for now.

I think that it is enough to put them only on our public API and not flood internal code with them. Also regarding the types of annotations to use I agree with @kaikreuzer on

So I would vote for @nonnull annotations on parameters and @nullable on return values - this would be exactly the same information that we so far also add in the JavaDoc.

One question remains for me: Which null analysis check do we set to which level, i.e. where do we want to use "warning" and where "error". Because of the false positives I tend to use "warning" though the problem might be that people do not read them...

Summarizing my post: I would vote for Findbugs JSR303 together with @nonnull and @nullable.

[kaikreuzer]

I would vote for Findbugs JSR303 together with @nonnull and @nullable.

How about making it available in the IDE and testing it on some first classes to make a start? If anybody then feels this isn't good, we can still decide to remove it again.

[sjsf]

Sounds like a plan.
I created CQ 13833 (PB CQ 11244) for it as a works-with dependency.
@triller-telekom will you create a PR for adding it to the target platform?

[triller-telekom]

First step for integration in target plattform is started, see: #3830