add some nullness annotations

[maggu2810]

Also fixed a potential NPE.

[triller-telekom]

One main comment from me: Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

And I found 2 other lines in the diff on sections that you touched where you could add an annotation.

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[triller-telekom]

How about a @NonNull on the return value here?

[triller-telekom]

How about a @NonNull on the return value?

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[triller-telekom]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

[maggu2810]

Didn't we agree on that we only want to state if a parameter is NonNull and if if can be null we omit the @Nullable annotation?

No, we didn't. @kaikreuzer has been allowed to use @Nullable, too. See: #3933

[maggu2810]

And I found 2 other lines in the diff on sections that you touched where you could add an annotation.

I assume there are thousands of other places, too 😉

[maggu2810]

And to be fair... The Checker Framework I am using to check my code assumes that a non @Nullable marked parameter must not be null.
So why do we care about adding @Nullable to parameters?

[triller-telekom]

Hmm you are right, @kaikreuzer used it on one occasion in his PR, but in #3624 (comment) he said:

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.

So I am a bit confused now :)

[maggu2810]

I agree with the above, but I don't see any comment that states we "must not" use @nullable on arguments.
Why not using annotations where we can state was is allowed and what is forbidden?
At least if it helps using other nullness checker in third party code then the Eclipse one.

[kaikreuzer]

The Checker Framework assumes that a non @nullable marked parameter must not be null.

Doesn't that mean that you will actually HAVE TO put @Nullable annotations now everywhere? While I agree that we didn't say that they must not be used, I would also want to avoid a situation where we have to use them everywhere, since this was against the original idea, which was to be fully backward compatible (i.e. assuming that a not-annotated parameter is to be assumed to be nullable by default).

[maggu2810]

Doesn't that mean that you will actually HAVE TO put @nullable annotations now everywhere?

Do you want to tell me that our framework accepts null for every non annotated argument and can return null on every non annotated return value?

since this was against the original idea, which was to be fully backward compatible

Adding annotations does break compatibility? Sorry this is new to me.

I do not get the point against adding @Nullable and @NonNull on every place (as an ongoing process) where we know that a value can be null or must not be null.
Annotations shouldn't matter but allow tools to check if the types are used correctly.

[triller-telekom]

Do you want to tell me that our framework accepts null for every non annotated argument and can return null on every non annotated return value?

I would say: Yes. Because that is the default for Java and we do not want to touch it.

I do not get the point against adding @nullable and @nonnull on every place (as an ongoing process) where we know that a value can be null or must not be null.

The point against that is that we do not want to "flood" our code with annotations.

Regarding the checker framework: Can it be configured in a way that it accepts the fact that non annotated parameters can be null?

[maggu2810]

A default is applied to every type, so arguments and return value.
If we could configure a tool that every type in a package org.eclipse.smarthome is default nullable it would mean that also every return type is nullable.
Every value that is supplied by a non annotated ESH function and de-referenced without an explicit null check will result in a nullness violation.

I also don't agree that nullness annotation does "flood" the code.
Sure, the character of methods are longer but it will help tools to check for correct code at compile time.

I really don't understand your arguments against that annotations.
break compatibility? at which place
flooding code? is it about source code size? really?

[triller-telekom]

break compatibility? at which place

That I don't know.

flooding code? is it about source code size? really?

Not code size, for me its about readability, but if we are just arguing about the parameter of methods I could also live with @NonNull AND @Nullable annotations there.

[kaikreuzer]

Do you want to tell me that our framework accepts null for every non annotated argument and can return null on every non annotated return value?

Didn't we already discuss that here?

I also don't agree that nullness annotation does "flood" the code.

Sounds to me as a very similar discussion as putting "final" on every single parameter in every method. We all agree that it would be more correct, but nonetheless, it makes method signatures less readable as they are bloated.

[maggu2810]

No, this is not the same as final in every method.
final on a method is for the framework code itself.
The annotations are also to check the correctness of third party code.
The only reason against the annotations is "flooding" and perhaps your personal interest.
I never read a technical reason that such annotations are not good at all.
And if we don't want to flood the code it is an argument against all the nullness annotations and not only about special ones.

The annotations would be useful for third party code of e.g. my company.
Currently I maintain all that additional annotations in separate files myself.
So, I really don't understand why we use the annotations at some places and not on the others. Shouldn't we also support other tools like the one our company uses?

[kaikreuzer]

The only reason against the annotations is "flooding"

@triller-telekom already pointed to this example to show what this actually means in effect. I think there should be a strong reason FOR adding the annotations and not a reason against not adding them.

I never read a technical reason that such annotations are not good at all.

I thought I gave it here: "this feature is minimal invasive and really only brings new behavior at the places where we desire it" - so it is about not adding more than is actually needed. It is simply that parameters are nullable by default, if nothing else is specified.

The annotations would be useful for third party code of e.g. my company.

If I understand it correctly, they are only useful, because your tooling (CheckerFramework) invariable assumes a "NonNullByDefault", while for Java (and thus for ESH as long as there is no other decision) it is "NullableByDefault". Or am I missing something?

Shouldn't we also support other tools like the one our company uses?

Nobody wants to make anything hard for anyone. But if some external tools makes assumptions that contradict the assumptions that exist in the projects code base, I wonder how that should be resolved...

[maggu2810]

@triller-telekom already pointed to this example to show what this actually means in effect. I think there should be a strong reason FOR adding the annotations and not a reason against not adding them.

Hm, so it is okay to have this information in the JavaDoc, so the one that reads the API does the correct thing, but it is not okay to add it to function declarations, so tools can use it for checks (e.g. https://github.com/eclipse/smarthome/compare/master...maggu2810:nullable?expand=1#diff-a1ada05baa22b325df4f26fba6959571R53)?

"this feature is minimal invasive and really only brings new behavior at the places where we desire it"
So, who is "we" that is allowed to desire something?

It is simply that parameters are nullable by default, if nothing else is specified.

I assume one reason that we have Optional in Java 8 and many more languages is because this behavior is not the best one.

"Note the significant difference between @nullable and omitting a null annotation: This annotation explicitly states that null is OK and must be expected. By contrast, no annotation simply means, we don't know what's the intention. This is the old situation where sometimes both sides (caller and callee) redundantly check for null, and some times both sides wrongly assume that the other side will do the check. This is where NullPointerExceptions originate from. Without an annotation the compiler will not give specific advice, but with a @nullable annotation every unchecked dereference will be flagged." (http://help.eclipse.org/kepler/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-using_null_annotations.htm)

If I understand it correctly, they are only useful, because your tooling (CheckerFramework) invariable assumes a "NonNullByDefault", while for Java (and thus for ESH as long as there is no other decision) it is "NullableByDefault". Or am I missing something?

No, it is useful because it is independent of the default a tool is using.
After using such annotations we should not make pre-conditions like NonNullByDefault or NullableByDefault if it can be prevented.
And isn't that the intention of the type annotations, you don't need to know it yourself, but you annotate it.

[sjsf]

I stayed out of this discussion for long because I'm really torn...

On one hand, I really see the value they can add to our (in the sense of "the Eclipse SmartHome project's") quality assurance.

My personal fear however is (and I know the comparison is not fair, it won't happen just with null annotations alone) that we one day end up with something like this:

https://github.com/eclipse/smarthome/blob/df90c5ddab20fffe7bbd399b0a60194812c538b2/bundles/io/org.eclipse.smarthome.io.rest.core/src/main/java/org/eclipse/smarthome/io/rest/core/thing/ThingResource.java#L251-L261

The problem here is: It can be read pretty well by tools, can automatically generate consistent documentation, saves tons of code and of course is absolutely valuable - but it takes a while for humans (or maybe just for me) just to figure out what the signature is of this stupid method and if it takes one, two, three or a hundred parameters. I would like to avoid this.

So don't get me wrong: I really support the approach of adding null annotations! But I would appreciate if we could keep them to a minimum, i.e. not stating the obvious. Adding an annotation to every method parameter somehow feels wrong: A method parameter either is allowed to be null or is not. And if we assume a default, then we can save 50%+ of such annotations (if we pick the right one...) in favor of human readability:

void stateUpdated(@NonNull EventPublisher eventPublisher, @NonNull ItemChannelLink link, 
    @Nullable State state, @NonNull Item item);

I would love to exchange many of those annotations by just a single @NonNullByDefault annotation per class:

@NonNullByDefault
void stateUpdated(EventPublisher eventPublisher, ItemChannelLink link, 
    @Nullable State state, Item item);

And by the way - javadoc is a completely different story. Thanks to highlighting and folding, (at least my) brain became pretty good at ignoring them because they are optically separated and don't clutter the important information.

I don't have much experience with the checker framework and have no idea if and how easy it can be adapted to obey such default annotations. And while "we" (in the sense of ESH) are not using it, I don't mind adding the necessary clues in order for you and others to be able to use it. However, IMHO we shouldn't do that to the price that it starts making our code less readable than strictly necessary.

[maggu2810]

And by the way - javadoc is a completely different story. Thanks to highlighting and folding

So, things will change if the Eclipse IDE supports folding / hiding of annotations some time? 😉 Joke...

As written already above it is not about the Checker Framework / an external tool that makes some assumptions. I assume I can change my tooling (why not, it is only code) to interpret every argument as nullable and return type as non-null of the ESH package namespace.
So, am I correct that you choosen "@nonnull annotations on parameters and @nullable on return values " as the annotations to use? So, arguments are default nullable and return values are default non-null?

I have been told multiple times that we interpret every argument as could be null (which I don't think so), how do we handle functions like this one (just an example that I saw while looking at the latest PRs):
https://github.com/eclipse/smarthome/pull/4109/files#diff-810126e50ad4f2c2ad5d985ffd577ed4R35
Doesn't that function needs a @NonNull as it will throw a NPE as soon as name is equal to null? Do we need to implement such function to be null tolerant (e.g. using "constant_string".equals(...))?

I still think (not becaue it is the default of the Checker Framework) that more arguments are not allowed to be null then are handled null-tolerant.

[triller-telekom]

So, am I correct that you choosen "@nonnull annotations on parameters and @nullable on return values " as the annotations to use? So, arguments are default nullable and return values are default non-null?

I think we also allowed @Nullable on return values, didn't we @kaikreuzer ? But I agree that this makes it even more confusing :( In the end we only wanted to express with annotations what was already written as strings in the javadoc...

https://github.com/eclipse/smarthome/pull/4109/files#diff-810126e50ad4f2c2ad5d985ffd577ed4R35
Doesn't that function needs a @nonnull as it will throw a NPE as soon as name is equal to null? Do we need to implement such function to be null tolerant (e.g. using "constant_string".equals(...))?

This example is a private function, and we only wanted to annotate our public API. But I agree that this method should have a check for null or if possible use "constant_string".equals(...).

[kaikreuzer]

if the Eclipse IDE supports folding / hiding of annotations some time?

THAT would be cool 😎

So, am I correct that you choosen "@nonnull annotations on parameters and @nullable on return values " as the annotations to use?

Yes, exactly, see this #3624 (comment).

So, arguments are default nullable

Yes.

and return values are default non-null?

No - default is also nullable (as this is what you have to expect if there is no annotation at all).
By adding a nullable annotation, we specifically inform the IDE that it can be null, so that it activates nullness checks (which are not activated if there is no annotation as this would break existing code).

Doesn't that function needs a @nonnull as it will throw a NPE as soon as name is equal to null?

Yes, absolutely! This is a perfect example where @NonNull should be added! The only reason why it isn't there is that we do not (yet?) enforce the use of null annotations for contributions.

that more arguments are not allowed to be null then are handled null-tolerant.

Agreed. This is a best practice for framework code anyhow - independently of any annotation discussion.

[kaikreuzer]

I would love to exchange many of those annotations by just a single @NonNullByDefault annotation per class:

I actually didn't yet think about that. This might be a good compromise for all of us then - the classes which we want to (or already have) cover by annotations could be marked as @NonNullByDefault, which would relieve us from many annotations throughout the class. And indeed in that case we would then add @Nullable annotations to parameters - but only those. Imho the main issue we all want to avoid having annotations on every single parameter (it is not against using @Nullable in general).

[maggu2810]

IIRC @NonNullByDefault could be defined for package, class, method, ... (correct?) So, we should be strict how to use it because it could be very unclear which default is used.

Now you confused me completely.

• no annotation for return value or argument, both could be null but no checks
• you allow @nonnull for arguments only to enable checks that a method is not called by a known null candidate
• you allow @nullable for return value to ensure the caller handles a potential null dereference
• you don't allow @nullable for arguments to ensure the null checker warns for potential null dereference in the method implementation
• you don't allow @nonnull for return types to state that null must not be returned (warnings on returning potential null candidates)

[kaikreuzer]

Now you confused me completely.

Sorry, that's not my intention 😕

• Correct.
• Correct.
• Correct.
• We do not require @nullable here, because if there is no annotation, we have to handle null anyhow. And therefore we do not wish @nullable annotations there, since it only adds further clutter to the method signature.
• Thinking about it, this is actually something where I wonder if we also have to add @NonNull or if otherwise our consumers see many unnecessary warnings if they pass on such values to annotated methods.

[maggu2810]

We do not require @nullable here, because if there is no annotation, we have to handle null anyhow. And therefore we do not wish @nullable annotations there, since it only adds further clutter to the method signature.

Sure, we have to handle null anyhow. But I don't agree that the annotation "only" adds further cluttern.
The same point as you already stated above is valid:

By adding a nullable annotation, we specifically inform the IDE that it can be null, so that it activates nullness checks (which are not activated if there is no annotation as this would break existing code).

    public static void myFrameworkMethod(String paramB) {
        System.out.print(paramB.length());
    }

This will NOT trigger a warning / error (wrt to the chosen severity) if we call a method on a potential nullable variable. Only if we add the annotation that paramB could be nullable, we will get a warning that paramB.length() will be a potential null pointer access.

Or do I miss the magic point here? Do we assume that our implementations are such perfect that we don't need the annotation for our method implementations?

[triller-telekom]

Thank you @maggu2810 for summing up our discussion on this wiki page.

Since also other contributors are start to use null annotations we should come to an agreement on what to use where soon, so we do not follow different approaches in different parts of the code.

The "problems" in our discussion are the fear that we might clutter the code and the need for some (potentially more) annotations for tools.

@maggu2810 said that type annotations where designed with the idea in mind that a developer just states if the return value is @nullable or @nonnull and also which parameter is @nullable or @nonnull so the tool has all the information it needs to draw conclusions.

If we follow that path, this would leave our codebase "open" for every "null.checking tool". Since we are developing a framework that will be used by others in their solutions this might be a valid point to consider. However, if we would pick a default such as @NullableByDefault or @NonNullByDefault as @SJKA suggested we could potentially save around 50% of annotations.

If we pick @NullableByDefault we would be in line with the default of Java, but instead of compiling the code without issues we would enable the compiler to raise a warning and the developer can be informed in his IDE. Every method would need null checks for its parameters (IMHO a good idea for framework code in general)), or we would have to clutter its signature with @nonnull annotations... In this scenario we would allow @nonnull on parameters AND return values.

On the other side if we pick @NonNullByDefault we would say that in the case of method parameters, we would force the consumer of the method to do the null check and can remove them from our framework. I do not know if this is a good idea because we are a framework and users of it might not use null checkers and accidentally pass null values to our functions which do not have checks anymore. However, I think that most functions are designed in a way that they really need the information from their parameters and thus they should be not null. So with this option we might need less @nullable annotations, which we then would allow on return values AND parameters.

[maggu2810]

If we pick @nullablebydefault we would be in line with the default of Java

Really? If a method uses nullness annotations it states that it assume that a parameter if not null or it allows that a parameter could be null.
Java without that annotations does IMHO not state that it allows e.g. that an argument could be null.
There are several methods in the JRE that we are not allowed to call with a nullable argument.

So the default of Java is that you can use null all the time as an argument but it could crash.
The same is true for the annotations. A function that uses non-null annotation for an argument could still be called with a nullable argument. It is only about the tooling.

So I don't think it is correct to say that default Java is equal to "assume nullable annotation for every type". There is no statement done that could be used by tools. It is not "nullable" and is also not "non-null".

If the framework itself would check every parameter, what do you want to do if there is a violation?
If we state that a parameter must not be null (regardless if it is JavaDoc or annotation), then the caller should not provide null. Do you think that check for null and throw an IllegalArgumentException is much better then a NPE that is called a line below?

I agree that annotate every argument is noisy.
I think it is the only solution to support as much tools as possible.
If we think about a "by default" annotation I would vote for "non-null by default" because if you think about a fully annotated framework I am pretty sure that the most annotations would be "non-null".

[maggu2810]

Can't we allow @NonNull and @Nullable annotations for a transition phase but prefer to use @NonNullByDefault for the whole class / interface (for a transition phase but prefer to use @NonNullByDefault for the whole package using package-info.java)?

We could also use @NullableByDefault for the classes and packages, but I am pretty sure that if we think about a fully annotated framework there are more non-null ones that nullable ones so if we set a default for a class / package it would make sense to use non-null.

I don't think it would make sense to use nullable by default for some classes and non-null by default for other classes. There should be only one default.

[triller-telekom]

If we think about a "by default" annotation I would vote for "non-null by default" because if you think about a fully annotated framework I am pretty sure that the most annotations would be "non-null".

Alright, so let's go for this option then. Since we have packages split over several bundles and to keep it readable, shall we add this @NonNullByDefault annotation to the class rather than to the package?

We would also remove null checks on parameters where we throw IllegalArgumentExceptions at the moment and let the execution run into the NPE, otherwise we would have dead code there.

With the agreed default we would only allow @nullable on parameters and return values and prohibit the use of @nonnull there. The only place where we need to allow @nonnull would be this example:

@NonNullByDefault
public MyNonNullClass() {
   public void method(String s) {}
}

public MyStandardClass() {
   private getString() {
      return "foo";
   }

   public void method() {
        @SuppressWarnings("null")
        @NonNull
        String s = getString();

        MyNonNullClass cl = new MyNonNullClass();
        cl.method(s);
  }
}

In this example we need the @nonnull annotation together with the @SuppressWarnings("null") because otherwise there will be a warning that we try to pass a variable which potentially is null to the method since getString is not annotated (Could also be a non annotated library method in which case we could then think about creating an external annotation).

WDYT @maggu2810? Shall we create a PR with this decision for our coding guidelines? And shall we create a separate PR to remove the use of @nonnull from the code again?

[triller-telekom]

Sorry I was typing and trying things out in my IDE and didn't refresh my browser so I just read your comment now:

Can't we allow @nonnull and @nullable annotations for a transition phase

Agreed, but either we should have a PR that removes them or everyone who touches the locations removes the wroingly placed ones.

but prefer to use @NonNullByDefault for the whole class / interface

That's what I suggest.

(for a transition phase but prefer to use @NonNullByDefault for the whole package using package-info.java)?

Will that work with our split packages? Also I think it might be more readable if the class has the @NonNullByDefault annotation instead of a "hidden" package-info.java file.

[maggu2810]

Since we have packages split over several bundles
Will that work with our split packages?

Sorry, I was not aware of that. Really? Can you give me some examples?

Also I think it might be more readable if the class has the @nonnullbydefault annotation instead of a "hidden" package-info.java file.

Okay for me (regardless of split packages or not).

... example ...

Give me some minutes to check something...

[maggu2810]

@triller-telekom As long as MyStandardClass does not use @NonNullByDefault because e.g. the class is such a big one that it would be a hard work, we should annotate the return type of getString() with a @NonNull annotation.

public class MyStandardClass {

    private @NonNull String getString() {
        return "foo";
    }

    public void method() {
        final String s = getString();

        final MyNonNullClass cl = new MyNonNullClass();
        cl.method(s);
    }

}

So, no need to suppress a warning.

[triller-telekom]

Since we have packages split over several bundles
Will that work with our split packages?

Sorry, I was not aware of that. Really? Can you give me some examples?

At least we have them in the test bundles and normal bundles, for example: org.eclipse.smarthome.core.thing.internal

As long as MyStandardClass does not use @NonNullByDefault because e.g. the class is such a big one that it would be a hard work, we should annotate the return type of getString() with a @nonnull annotation.

I was thinking of classes which we do not (yet) have annotated or which are external and we do not have an external annotation (yet). I don't know what is the better option: either force the developers to create these external annotations once they come across them or have a SurpressWarning in the code... Also is the goal that we have annotations EVERYWHERE, i.e. no more warnings from the eclipse null checker?

[maggu2810]

Hm, wouldn't be an external annotation the better solution as a suppressed warning in our code? I played just a little bit with the external annotation support in the Eclipse IDE, but it seems very easy to use.

[triller-telekom]

Indeed having an annotation is better than suppressing a warning, I agree. Let's start with requiring the developers to add external annotations rather than suppressing the warnings they get and see if its feasible. If not, we can still revert back to the suppressing a warning method. WDYT?

So shall we create a PR to update our coding guidelines to what we now agreed on, regarding the use of null annotations?

[maggu2810]

@triller-telekom Do you agree that Latest summary (2) is correct?

[triller-telekom]

The final result of our null annotation usage should be that classes are annotated by @NonNullByDefault and return types, parameter types etc. are annotated with @nullable only. There is no need for a @nonnull annotation because it is set as default.

Agreed.

As long as we are in a transition phase it could be hard work to annotate the whole class. As long as the whole class cannot be annotated to use a default annotation we are allowed to use @nullable and @nonnull annotations for return types, parameter types etc. As soon as it is possible to set the default annotation for the class all redundant usages of @nonnull should be removed.

Hmm, I thought we only keep those "wrongly" added @nullable and @nonnull annotations until the class gets its @NonNullByDefault and then remove them, but do not add further such annotations... If we do that, how do we decide on "when" is a good time to add the @NonNullByDefault?

@nonnull and @nullable will be (not only in the transition phase but all the time) allowed for generic type arguments. So we could mark that e.g. a map must contain only non-null values of a collection does not contain a nullable entry etc.

If I add @NonNullByDefault on a class which has a non-annotated method in there that returns a List<String> my IDE shows me that this method returns a @NonNull List<@NonNull String>. So why do we need @nonnull on generics?

We are using the Eclipse External Annotation support to add annotations for the JRE and third party libraries.

Agreed.

[maggu2810]

If I add @nonnullbydefault on a class which has a non-annotated method in there that returns a List my IDE shows me that this method returns a @nonnull List<@nonnull String>. So why do we need @nonnull on generics?

Really? For me it seems that the annotations are not applied to generic type arguments. But I could be wrong.

Hmm, I thought we only keep those "wrongly" added @nullable and @nonnull annotations until the class gets its @nonnullbydefault and then remove them, but do not add further such annotations... If we do that, how do we decide on "when" is a good time to add the @nonnullbydefault?

We could also state that there is no transition phase.
My fear has been that if e.g. you would like to change some interface method to state that the return value is not null on some function and you need to annotate the whole interface with a non-null by default annotation, you will stop because you have to fix all implementations and all usages of every function of the interface...

[maggu2810]

@triller-telekom It seems you are right about about the inheritance of the type parameter annotations.

There is only one thing that IMHO is not really correctly handled by the Eclipse Nullness checker (but this does not change anything of our usage):

Null type mismatch (type annotations): required 'List<@Nullable String>' but this expression has type 'List<@NonNull String>'

If someone receives a list that members could be nullable it should also be correct to provide a list with non-null entries.

[triller-telekom]

Regarding the "transition phase problem": How about we do not allow classes which are "half way annotated"? So while coding if you come across an interface method that you want to annotate you can try do add @NonNullByDefault to the interface and if it only impacts the code you are about to touch anyway, then you can fix it right away.

But if the effects of the @NonNullByDefault are so widely distributed over the code base, just refrain from adding it in the PR you are about to create and do it in a separate PR, or leave it for someone else to do it in a separate PR.

If someone receives a list that members could be nullable it should also be correct to provide a list with non-null entries.

is that a warning in our current IDE and maven setup? If so we could live with it and wait until this has been fixed in the Eclipse nullness checker.

[maggu2810]

How about we do not allow classes which are "half way annotated"

Perhaps it would be the best option to keep things (not the ESH ones 😉 ) clean.
So, no transition phase at all. Do it or leave it.

is that a warning in our current IDE and maven setup?

Currently I tested the IDE only.

If so we could live with it and wait until this has been fixed in the Eclipse nullness checker.

Agree, perhaps my understanding is wrong or the Eclipse nullness checker needs a bugfix.
But this should not affect our (hopefully) correct usage of that annotations.

FYI:

import java.util.LinkedList;
import java.util.List;
import java.util.Objects;

import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.eclipse.jdt.annotation.NonNullByDefault;

class NullnessTest {

    @NonNullByDefault
    protected static class NonNullIsMyDefault {
        private final List<String> lst;

        public NonNullIsMyDefault(final List<String> lst) {
            this.lst = lst;
        }

        protected boolean foo() {
            final @Nullable String stringNullable = lst.get(0); // no warning - as expected
            final @NonNull String stringNonNull = lst.get(0); // no warning - as expected
            return Objects.equals(stringNullable, stringNonNull);
        }

    }

    protected static class NullableList {
        private final List<@Nullable String> lst;

        public NullableList(final List<@Nullable String> lst) {
            this.lst = lst;
        }

        protected boolean foo() {
            final @Nullable String stringNullable = lst.get(0); // no warning - as expected
            final @NonNull String stringNonNull = lst.get(0); // warning as expected
            return Objects.equals(stringNullable, stringNonNull);
        }

    }

    protected static class NonAnnotatedList {
        private final List<String> lst;

        public NonAnnotatedList(final List<String> lst) {
            this.lst = lst;
        }

        protected boolean foo() {
            final @Nullable String stringNullable = lst.get(0); // no warning - as expected
            final @NonNull String stringNonNull = lst.get(0); // warning as expected
            return Objects.equals(stringNullable, stringNonNull);
        }

    }

    protected static void test() {
        final List<@NonNull String> listWithNonNullEntries = new LinkedList<>();
        final List<@Nullable String> listWithNullableEntries = new LinkedList<>();

        new NonNullIsMyDefault(listWithNonNullEntries); // no warning - as expected
        new NonNullIsMyDefault(listWithNullableEntries); // warning as expected
        new NullableList(listWithNonNullEntries); // warning -- NOT EXPECTED
        new NullableList(listWithNullableEntries); // no warning - as expected
        new NonAnnotatedList(listWithNonNullEntries); // no warning - as expected
        new NonAnnotatedList(listWithNullableEntries); // no warning - as expected

    }
}

[triller-telekom]

So I think we finally have come to an agreement, right? :)

Let's use @NonNullByDefault on classes and only allow @nullable on return values and parameters and generics. For third party code we will use the external annotations and create additional ones they do not exist yet.

Since you have already created the wiki page, do you want to update its content and create a PR against our coding guidelines documentation with it?

[maggu2810]

@triller-telekom Do you agree that Latest summary (3) is correct?

[maggu2810]

@triller-telekom We should have a look at the external annotation support to get this into master as soon as possible.

[triller-telekom]

@triller-telekom Do you agree that Latest summary (3) is correct?

Agreed.

I will take a look into the external annotations again. I remember that the options you can set in the IDE for the nullness checker (performed by the jdt compiler) somehow differ from the ones you can set on the command line, i.e. in our maven pom file. because those two settings are not consistent that might be the reason why the warning doesn't show up in maven in the first place...

[maggu2810]

Perhaps interesting for the SAT

I tried the @NonNullByDefault on private projects and I need to disable the nullness checks of FindBugs / SpotBugs as it seems they are not play very well together.
But IMHO this is okay, as nullness checks are done by another party now.

Currently this is part of my "findbugs_exclude.xml":

  <Match>
    <Bug pattern="RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE" />
  </Match>
  <Match>
    <Bug pattern="NP_STORE_INTO_NONNULL_FIELD" />
  </Match>
  <Match>
    <Bug pattern="NP_NONNULL_RETURN_VIOLATION" />
  </Match>
  <Match>
    <Bug pattern="NP_NONNULL_PARAM_VIOLATION" />
  </Match>
  <Match>
    <Bug pattern="NP_NULL_PARAM_DEREF" />
  </Match>
  <Match>
    <Bug pattern="NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR" />
  </Match>
  <Match>
    <Bug pattern="NP_NULL_PARAM_DEREF_NONVIRTUAL" />
  </Match>

[kaikreuzer]

@triller-telekom Do you agree that Latest summary (3) is correct?
Agreed.

In the light of #4217, I would say that the external annotation should be left out of scope (while being clear that we want to use them, but that we still have to work out the details), so that we can very quickly have a decision on the "internal" null annotation use.

So shall we create a PR to update our coding guidelines to what we now agreed on, regarding the use of null annotations?

@triller-telekom Yes, will you do so? We then would not have to block PRs like #4262 anymore...

[triller-telekom]

Done, see PR #4275

@maggu2810 I have used your summary on the wiki page, I hope that's fine for you.