Null annotations - observations and discussion

[htreu]

After being confronted with the new null annotations for the first time now I would like to show some issues I came across and discuss with you how we manage to deal with them. The original PR and discussion was here: #4080.

All my examples assume the @NonNullByDefault annotation on class level. This implies all members, return values and method parameters are expected to be non null.

Class members of type Map<K,V>
The compiler treads members of type Map<K,V> as @NonNull Map<@NonNull K, @NonNull V>. This implies that a call to myMap.get("invalidKey") will return a non null value, even if the Map does not contain the key. In the process the following code will be reported as dead code:

Object value = myMap.get("invalidKey");
if (value == null) {
    throw ... // this block reported as "dead code"
}

This behaviour does introduce a NPE which is dangerous.

Implicit null checks
Given a class with constructor MyThing(String param1) which is all non null we get the following behaviour during creation:

@Nullable String param1 = getConfigParam(...);
if (param1 != null) {
    new MyThing(param1); // this will work okay, null check accepted
}

if (StringUtils.isBlank(param1) {
   new MyThing(param1); // this will fail with error, implicit null check not accepted
}

This could very easily lead to a change in programming style which is required to adopt to the null annotations: To express the semantically blank check a StringUtils.isBlank(str) will become a str == null || str.isEmpty() which is technically equal but imho feels wrong.

Dependency injection
Class members which are injected by OSGi DeclarativeService (DS) are not supported well: When doing no extra annotation, the often used mechanism to set these members to null on unbind will resolve to error. When giving the member a @Nullable (which semantically is correct) all invocations now require a null check since the compiler warns about potential null pointer access.
Both "positions" do not reflect the actual behaviour since we know the framework will take care of the dependency (in most cases) and we can be sure not to have a null pointer at hand.

Some of the above issues where already addressed here but my feeling is we need to have another discussion on how to deal with the new situation. Right now my IDE floods my with warnings which do not seem to resolve magically.

[htreu]

Another observation I want to add regards JUnit test cases:

JUnit tests
My Tests often follow this scheme:

private static final String CONST = "value";

private MyClass unitUnderTest;

public setup() {
    unitUnderTest = new MyClass(CONST)
}

assuming the above this will give me compiler warnings which state that CONST "needs unchecked conversion to conform to @NonNull String" (because MyClass constructor requires a non null value). One thing here is that the compiler does not recognise final members which are initialised non null as conform with @NonNull.
When adding @NoNull to CONST the following line gives a warning:

assertThat(unitUnderTest.getNullableValue(), is(CONST));

because the non null CONST does not conform to the nullable String the Matcher infers from the @Nullable annotated MyClass.getNullableValue method.

[triller-telekom]

While looking at the PRs i found in #4319 something where the code has been changed to satisfy the null annotation compiler:

for (String id : listUIDs.keySet()) 
    String uid = listUIDs.get(id);

has been changed to

for (final Entry<String, String> entry : listUIDs.entrySet()) {
    final String id = entry.getKey();
    final String uid = entry.getValue();

Do we really have to iterate over the entryset whenever we loop through a map?

[maggu2810]

Class members of type Map<K,V>

The compiler treads members of type Map<K,V> as @NonNull Map<@NonNull K, @NonNull V>. This implies that a call to myMap.get("invalidKey") will return a non null value, even if the Map does not contain the key.

If thats the case then it is not correctly handled by the Eclipse Annotation processor.
The get method of a map is not annotated, so it does not state that if it returns T using the annotation assigned to the type or a @Nullable T.

But this would be solved as soon EEAs are used.
I already created a PR that should eliminate that error.
See this line: https://github.com/eclipse/smarthome/pull/4217/files#diff-797cf9b12794986000209fda8c79263fR13

Implicit null checks

This could very easily lead to a change in programming style which is required to adopt to the null annotations: To express the semantically blank check a StringUtils.isBlank(str) will become a str == null || str.isEmpty() which is technically equal but imho feels wrong.

It depends.
E.g. using null annotations you can already express that an argument must not be null and so you can most of the time check for isEmpty() only and does not need to handle the null case, too.
Using your example that takes the return value from "somewhere" it is also not a general use case that null and empty are handled same. Most of the time you should consider if a non available string and an explicit empty one should be handled equal.
But there are cases where you would like to use such a construct. As already stated in the other comment, this is not about nullness annotations in general but about what can be explicit expressed by a function (e.g. if we return true we know the argument is not null).

Dependency injection

For dependency injected variable the null check should perhaps be deactivated at all (if possible). We should have a look at which options are available using the Eclipse nullness annotation processor.

[maggu2810]

Do we really have to iterate over the entryset whenever we loop through a map?

If you need the key and the value you should use an entrySet (this does not depend on nullness annotations at all).

As long as a map get function could return null how should the annotation processor know that a non null value is returned? It needs to check if no other thread is able to change the map between that calls etc.

[sjsf]

I would like to add another one to the list:

Inline Field Initialization

Is there any trick to make JDT be just a little smart about those?

(argh, just realized it's the same as #4321 (comment) - this not only applies to unit tests...)

[triller-telekom]

I agree that the JDT compiler should recognize that your final field are initialized and that they cannot be changed to null later on. But if you add @NonNullByDefault to your class your 2 fields should become annotated with @NonNull and the warnings should vanish.

[sjsf]

But if you add @NonNullByDefault to your class your 2 fields should become annotated with @nonnull and the warnings should vanish.

Right, the warning vanishes. But it gets worse, so no, I can't do that if I'm using OSGi service references - because then it will give me an error on my service instance field:

If I then tell it that my field is @nullable (which is a lie, because it will never be null), I get the error down there because JDT feels pretty confident that the contract is violated:

Big Fail!

Plus: the "Dependency injection" point described in #4321 (comment) hits.

To me this whole nullable story with JDT feels pretty half-baked. The annotations are nice for documenting the API, but rather a pain when trying to use them for static analysis.

[maggu2810]

Dependency injection

Using nullness checks involves that we need to differ between an optional injected reference (e.g. OSGi DS using a cardinality of 0..1 or 0..n) and a mandatory injected reference. And surely also between "we trust that injected variables are injected" and "we want to check that the mandatory injection has been done". But let's assume we never want to check that the injection framework does it work correctly. Just added this case for completeness.

I assume we can neglect the optional injected reference because this one should be marked as nullable as it could be missed. Agree?
So, let's have a look at mandatory references.

For OSGi there are at least two cases:

• injection per field (not used in ESH because of min. OSGi comp spec 4.2)
• injection per bind method / and removal per unbind method

Dependency Injection per field

The variable is set and unset by the injection framework.
We don't need to set a null value in the unbind method our-self.
We would like to mark the variable as @NonNull and two options

• initialize the variable with a dummy object
• don't initialize the variable at all

Initialize the variable with a dummy object is possible but doesn't make sense as we rely on the injection framework and don't want to create a class implementation that e.g. contains a throw new UnsupportedOperation() for every method.

Don't initialize the variable at all.
The Eclipse nullness checker complains about a missing initialization. It is right, because we marked the variable as @NonNull and the checker identify that it will be null because of the missing initialization. What is missing is a way to state "it is okay, because this variable will be initialized by another one".
The Checker Framework (just to have a look at what is possible) brings its own "SuppressWarnings" values. You can introduce constants and assign the special strings to it.
So I use something like this one:

@SuppressWarnings(SprWrnValues.OSGI_MANDATORY_SERVICE_REF)
protected @NonNull Object ref;

The nullness check framework knows I does not need to care about the missing initialization and also every developer knows "the variable is not initialized because it is a mandatory OSGi service reference".

Dependency Injection per bind ...

Mostly the same situation as above and the additional situation that you will assign null to the "non-null" variable. You know that your component will be deactivate and so you assign null, so the garbage collection could clean up the unused service.
Also in this situation a suppress warning constant for the unbind method could state "I am allowed to set null here to the non-null variable because it is an mandatory OSGi reference and so I will be not active anymore".

I don't know if there are methods to inform the Eclipse nullness checker about that situations, too.
It would be great if this option would be added to the tool.

[maggu2810]

As long as there is no option by the official JDT tools, we can use our own workaround to use dependency injection and nullness aware code.

Let's have a look at the static method nonNulledNull
We know a reference is non null in our instance because it is injected, the type system can't know about it. So we can't tell "it is non-null" but don't initialize the variable or assign null in the unbind method.
But what we can do?
We could workaround the type system, so it does not know that null is assigned.
We could still use correct nullness checks and we could track all of that usages.

So instead of writing

public class ComponentFoo {
    protected ServiceBar serviceBar;

    protected void setServiceBar(ServiceBar serviceBar) {
        this.serviceBar = serviceBar;
    }

    protected void unsetServiceBar(ServiceBar serviceBar) {
        this.serviceBar = null;
    }
}

without using nullness checks, we could write

@NonNullByDefault
public class ComponentFoo {
    protected ServiceBar serviceBar = NullnessUtils.nonNulledNull();

    protected void setServiceBar(ServiceBar serviceBar) {
        this.serviceBar = serviceBar;
    }

    protected void unsetServiceBar(ServiceBar serviceBar) {
        this.serviceBar = NullnessUtils.nonNulledNull();
    }
}

and use nullness checks for the whole class (so, all non dependency injected code).

If there is sometime a better solution, we could easily look at all locations NullnessUtils.nonNulledNull() is called and fix that usage.

[htreu]

Hi guys,
thanks for the discussion. We seem to have two discussions in the room:

1. Should null annotations serve as static analysis on the whole codebase or do we mainly want to document the public API (which brings on the next discussion, where the public API starts/ends, do we need to split the API into separate bundles, etc.).
2. What are the technical issues with null annotations and what are the ways to work around them.

[htreu]

On the transition between 1. and 2. I would like to make a proposal:
The main thing which annoys me about the null annotations is the flood of warnings in Eclipse. What I like about the annotations is the error marker when setting @NonNull fields through constructors and setters.
What about switching the warnings off completely and concentrate on the errors only? This would at least give a benefit but does not require workarounds at all (which imho are not exactable).

[maggu2810]

• Class members of type Map<K,V>: shouldn't be a problem using EEAs (see comment above)
• Implicit null checks: StringUtils.isBlank(String) cannot be solved easily using the current JDT tools (IMHO). In general I don't know if null and empty should always be handled the same and if we need to check for null using nullness annotations at all. We could use the separate checks, create our custom util method or perhaps we should talk with the developers of the nullness checks. (see comment above)
• Dependency injection: see comment above
• JUnit tests: the exmaples above does not use @NonNullByDefault at all, so using not annotated stuff. Correct? If there is another general problem in JUnit tests we could perhaps not using nullness checks in tests.

The main thing which annoys me about the null annotations is the flood of warnings in Eclipse.

Aren't the most of the warnings caused by the classes that doesn't use nullness annotations at all or correctly? If so, it would be better if we found a agreement about solutions / workaround for problems like dependency injection and fix the warnings.

[htreu]

@maggu2810 as stated above: for me the workarounds to please the new tooling do not raise the quality of the code, they make it worse. I can see the benefit from null annotations and I would like to make a concrete proposal which incorporates @sjka`s proposal for API documentation:

• We should expect (read force) null annotations on all public API, meaning all exported classes should annotate their public constructors/methods.
• We should allow null annotations on all internal API if we think it helps improving quality (just like its documented right now using @NonNullByDefault). This will also allow @maggu2810 to further extend classes by null annotations and complement the external tooling like the checker framework.
• for the ESH-only tooling we disable all JDT null annotation warnings as they would require afore mentioned workarounds and a lot of work to fully resolve. The JDT annotation error reports are fine imho since they only show up if Eclipse is very certain about a violation (like passing null to a non null parameter).

This way could have the best for external consumers (documented and validated API), external tooling can still check the whole codebase, ESH-only tooling will only check the obvious violation.

wdyt?

[maggu2810]

workaround

Which one of the above topics?

We have a class that states that a variable is non-null.
A variable is injected by a framework like SCR.
I assume regardless of the tooling you choose, we need to tell the nullness annotation tool that it is okay that this variable is not initialized or you set null in the unbind method on a non-null variable reference.
Or you do you think should this be detected by the nullness tooling? Should the tool know about all injection framework, read the OSGI-INF ...

I don't think it is only a workaround but we state that we violate the non-null annotation here by intention.

[htreu]

Regarding the dependency injection I think an injected service should be @Nullable since thats what it actually is from a compiler point of view. Only from the higher level dependency injection we (read developer, also a higher level authority) knows that the reference will not be null at runtime. Setting it to null on unbind is also a desired thing to do to make GC happy. This will result in warnings for all invoking places and my approach is to disable warnings completely for JDT annotation validation.

[maggu2810]

From the compiler (javac) point of view also a @NonNull annotated variable is nullable as he don't care about that annotations 😉

I agree that it should be set to null in unbind to let the GC do its job (I hope I have written it above, don't know exactly). I used "workaround" for NullnessUtils.nonNulledNull() because I think it is a workaround as long as there is no official way to do so. But I don't think the whole mechanism is a workaround.

disable warnings completely for JDT annotation validation

For me an annotated API is already a big win.

If you prefer to disable nullness checks to reduce warning, okay -- I don't like that decision but if the others agree...

I would prefer

• to support EEAs
• use = NullnessUtils.nonNulledNull() instead of = null; for mandatory references
• fix the implementations
• create a summary which false nullness warnings remains and improve tooling

[triller-telekom]

Regarding eeas, please see #4217 (comment)

I agree with @htreu that we should not implement NullnessUtils.nonNulledNull() to make a tool happy. So the JDT compiler should be enhanced to support the OSGI framework and not flood our code with warnings.

I also agree with him, that now as we have some experience with those null annotations, we should go back to the main idea that we had in mind for them: annotate our public api with what is written in the javadocs. That is because unfortunately it turned out that the tooling with its full static analysis is not able to scan the code as it is (without any workarounds for this tool).

[htreu]

Don´t get me wrong, I also like the goal and the bigger picture we try to reach with null annotations. Just right now there are imho too many open questions. So lets start by disabling the warnings and concentrate on annotating the API. All other steps (EEAs, fix internal implementation, fix/extend tooling) can then be done step by step.
Once we reach a point with the additional tooling where warnings are reduced to a minimum we can again decide how to proceed.

[htreu]

Here is my proposal for the severity settings:

[ppieczul]

My observations:

• Warnings when accessing nullable compound data, for example those used for deserialization of JSON data:

        if (json.details != null) {
            String format = json.details.format;

This will give a warning Potential null pointer access as json.details is nullable type.
But it can't really be null, I explain this as a precaution to a potential multithreaded access to the json structure. Theoretically other thread could make json.details null in between, as json is not a local variable but an argument to this method.
This warning will go away if added an extra local variable for json.details at the front.
The same warnings will be given for basically all accesses to method's fields that are nullable.

• There is a class of these that causes mvn to throw error instead of warning. IDE gives warning only. For example:

if (json.details != null && json.details.format != null) {
    format = json.details.format;    <-- format is nonnull and error is because json.details.format is nullable 

for (LxJsonApp3.LxJsonRoom room : config.rooms.values()) {
    if (room.uuid != null && room.name != null) {
        addOrUpdateRoom(new LxUuid(room.uuid), room.name);  <-- both arguments are nonnull and error given for both since they are nullable

These errors go away when putting the nonnull values into local variable and using it instead of accessing multiple fields.

[ppieczul]

One more observation, which probably is the same class as above, but it can be fixed differently:

        if (json.states != null) {
            logger.trace("Reading states for LxControl: {}", json.type);
            for (Map.Entry<String, JsonElement> jsonState : json.states.entrySet()) {

This will give warning for json.states that can be null.
But when logger line is totally removed, it will figure out that it can't be null and warning disappears. I suppose this is an another case for potential modification of the json.states in a multithreaded environment. Without the logger it may result in check+use being atomic from preemption perspective. I don't know how it really is in Java, this is just a suspicion.

[ppieczul]

Setting the severity setting to the values proposed by @htreu helps me to eliminate half of the 60 false-positive warnings I had remaining in my binding.
The other 30 still left are all for the potential null pointer access that I described above in my two comments above. I think there are two ways to eliminate them now:

• change the potential null pointer access to ignore
• add extra local variables to the code to prevent the potential modifications of the nested structures (but which will never happen)

For the extra variables, sometimes the code looks agreeable with them, but sometimes it looks awkward, for example:
With the warning:

    private void stopResponseTimeout() {
        synchronized (state) {
            if (timeout != null) {
                timeout.cancel(true);
                timeout = null;
            }
        }
    }

Without the warning:

    private void stopResponseTimeout() {
        synchronized (state) {
           ScheduledFuture<?> thread = timeout;
            if (thread != null) {
                thread.cancel(true);
                timeout = null;
            }
        }
    }

Do you think such solution for the warnings sake only is acceptable? Or maybe we should change the severity further?

[htreu]

FMPOV the tooling should add value and not confuse during coding. With the examples @ppieczul provides at least some of the potential null pointer access warnings seem not to be that obvious. I would rather decrease the severity then have warnings that never go away w/o tricks or SuppressWarnings directive.

[triller-telekom]

@ppieczul Thank you for your observations with the null annotations!

I have just had a look on this again so we hopefully find some final settings for these annotations.

Examples

First I will go through the examples from @ppieczul:

if (json.details != null) {
            String format = json.details.format;

This is a Potential null pointer access warning where I would have preferred to have a local variable anyway because json.details is used twice:

JDetails jDetails = json.details;
if (jDetails != null) {
            String format = jDetails.format;

This will make the warning disappear and is also IMHO better coding style.

if (json.details != null && json.details.format != null) {
    format = json.details.format;    <-- format is nonnull and error is because json.details.format is nullable 

In my IDE this results in an Null type mismatch (type annotations): required '@NonNull String' but this expression has type '@Nullable String' error. Also this is an error in Maven. Maybe your settings have not been correctly applied regarding the severity settings? (My Eclipse installation is a brand new checkout from last week though).

for (LxJsonApp3.LxJsonRoom room : config.rooms.values()) {
    if (room.uuid != null && room.name != null) {
        addOrUpdateRoom(new LxUuid(room.uuid), room.name);  <-- both arguments are nonnull and error given for both since they are nullable

Again I would say that room.uuid and room.name should be stored in local variables since they are used twice. IMHO better coding style and it would make the warning disappear.

if (json.states != null) {
            logger.trace("Reading states for LxControl: {}", json.type);
            for (Map.Entry<String, JsonElement> jsonState : json.states.entrySet()) {

This will give warning for json.states that can be null.
But when logger line is totally removed, it will figure out that it can't be null and warning disappears.

I somehow cannot reproduce this example. I have build my own simple one like this:

private void test(String str) {
        @NonNull
        String nonNullStr = "foobar";
        @Nullable
        String nullableStr = str;

        nonNullStr = nullableStr; // this is an error in my IDE

        if (nullableStr != null) {
            System.out.println("bla bla");
            logger.debug("bla bla");
            nonNullStr = nullableStr; // this is totally fine in my IDE since nullableStr has been checked before assigning it
        }
}

So even the two logging statements my IDE detects that the null check has been done.

private Object state;
private ScheduledFuture<?> timeout;

private void stopResponseTimeout() {
        synchronized (state) {
            if (timeout != null) {
                timeout.cancel(true);
                timeout = null;
            }
        }
    }

This last example has no warning or error in my IDE, maybe your settings were not correctly applied (see the example above)?

OSGI service injection

This part stays a real issue with the current implementation of the nullness analysis of the Eclipse IDE.

Technically correct the definition for a service should look ass follow, i.e. the service should be declared as @Nullable:

@Component
public class MyComponent {
    protected @Nullable MyService myService;

    public setMyService(MyService ms) {
        this.myService = ms;
    }
    public unsetMyService(MyService ms) {
        this.myService = null;
    }

    public void doSomething() {
        myService.method(); //raises a "potential null pointer access" warning

        @SuppressWarnings("null") //does NOT work if myService.method() has return type void
        myService.method();

        @SuppressWarnings("null") //works but is ugly...
        ReturnType rt = myService.method();

        @SuppressWarnings("null") // does NOT work
         for (Foo f : myService.getAll()) {}

        if(myService != null) { //works but this check is always needed although OSGI guarantees  that it is not null
            myService.method();
        }

    }
}

If instead we declare the injected service as @NonNull which is only correct if there is no bug in the OSGI framework, we have to initialize the myService variable in the above example with a value that is not null... This works only with the workaround hack from @maggu2810 here where he introduces a NullnessUtils helper class with a static nonNulledNull method. We would need to use it for every service injection in the ESH framework :(

The Eclipse nullness analysis should offer another annotation, maybe @injected where it knows that this variable will be somehow initialized and disables the "potential null pointer access" for these variables...

potential null pointer access warning

If we disable this warning we obviously do not have a problem with it for dependency injections, so let's disable it and everything is fine? Unfortunately not...

Consider this example:

String abc = getNullableStr(str);
System.out.println(abc.length()); // we want this warning here!!!

private @Nullable String getNullableStr(@Nullable String s) {
        return s;
}

With the null annotations we explicitly want to define those cases where we intentionally return a null value. If we disable the potential null pointer access warning, the warning mentioned in the example will disappear. Since this is a case where we want the warning but do not get it if the check is turned off, why do we enable the nullness analysis? In other words, if we disable this warning, we practically disable a huge part of the analysis that we wanted to introduce as a helper for us.

[htreu]

After reading the analysis from @triller-telekom I agree we should stay on the "warning" level for potential null pointer access. We gain more then we loose on confusing code style (which is not confusing at all on a second look).
Only the DS dependency injection situation is somehow unsatisfying. I guess we should go with a mix of @SuppressWarnings("null") and if (service != null) {} style, dependent on the surrounding code.

[maggu2810]

OSGI service injection

The Eclipse nullness analysis should offer another annotation, maybe @injected where it knows that this variable will be somehow initialized and disables the "potential null pointer access" for these variables...

The problem is how should be identified if a function that access the injected service is called if the service is active only? E.g. a method could be called in the bind function of a service before it is activated and so on.
I don't see a real solution for it...

[htreu]

One additional thing for #4321 (comment) 1.: We must not use Checks.requireNonNull() because the org.eclipse.jdt.annotation package is an optional import and not present at runtime.

Regarding @triller-telekom´s last comment: we can get around some of these warnings when we actually do annotate fields with @NonNull in classes where we use the reduced default scope. Its only for the injected services which should not be annotated. I guess we rarely use injected fields as method arguments, so this might do the trick.

[htreu]

On another note, I´m also okay with the proposal to only decrease the default scope by DefaultLocation.FIELD and use all other scopes provided for classes with injected services.

[triller-telekom]

I have just created PR #4513 which re-enables "Unchecked conversion from non-annotated type to @nonnull type" and adds support for external annotations.

In contrast to PR #4217 I am using a zip file for the annotations that will be hosted by an external project (possibly lastnpe.org). I am downloading the zip file via a maven plugin or an eclipse setup task.

But we still can use parts of PR #4217, I am not replacing everything!

@maggu2810 Can you please have a look on my PR? I left some more details in the commit message. WDYT?

[htreu]

Before we miss it: I just came across the situation where variable arguments needed to be annotated, like private void myMethod(@Nullable Object... params) and the compiler didn´t like it. We need to figure out how to solve this.

edit: never mind, works as expected

[kaikreuzer]

Just some feedback after having worked on some "real" code with those settings:

1. We should definitely treat our handlers similar to OSGi components. A lot of code in them expects that initialize() has been called before anything else is used and that dispose() is called in the end. Many fields are therefore also only set in initialize(), so they cannot be marked as NonNull. Nonetheless, many can be treated as such and if they are accessed before initialize() is called or after dispose() has been called, it can be considered an illegal state of the handler.

2. I wonder if we should also remove some of the NonNullByDefault on types. An annoying case that I very often came across is the access of a collection. If get() is called, with the proposed annotations the compiler assumes that the result is NonNull, which simply does not make sense as this call is often done without knowing whether a value for the key exists at all.

[triller-telekom]

Regarding your second point: If we disable the check "Unchecked conversion from non-annotated type to @nonnull type" and the use of external annotations we only get errors if an annotation for the external code exists, but not a warning that we should add an external annotation to the library. In other words there is no warning that the non-annotated library code is used as a return value for a @nonnull method for example. So we are not encouraged to add an external annotation for that library. But IF an external annotation for the library code exists we will still get an error in the IDE id the annotation says @nullable and we want to use it as @nonnull. If we are fine with that we can certainly let this setting disabled.

@maggu2810 Since you have worked with annotations (i.e. checker framework in your case) before. Can you estimate briefly what impact it has if we only use annotations for return values and parameters?I.e. reduce their scope to:

@NonNullByDefault({ DefaultLocation.PARAMETER, DefaultLocation.RETURN_TYPE })

Will the analysis results be much less accurate if type parameter, array contents etc. are omitted?

I also tried to combine the parameters for the @NonNullByDefault annotation in a static array definition, because @martinvw mentioned that a large definition is ugly. But array variables do not exist at compile time where annotations are evaluated so this is unfortunately not possible.

[maggu2810]

I wonder if we should also remove some of the NonNullByDefault on types. An annoying case that I very often came across is the access of a collection. If get() is called, with the proposed annotations the compiler assumes that the result is NonNull, which simply does not make sense as this call is often done without knowing whether a value for the key exists at all.

Really, that is wired. But perhaps I did not understand that comment correctly.
But I don't know how this is related? If a class Foo is annotated with "@NonNullByDefault" it does not add annotations to member function of non-annotated classes.
If Collection is not annotated, the interpretation of "get" should not change regardless how Foo is annotated.
If the current IDE settings interpret the return type this way there is something wrong.

[maggu2810]

@triller-telekom I found out another mechanism to handle injected references or let's say the exclusion of some field.

Assume this class:

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;

@Component
public class ComponentImpl {

    public static interface DummyService {
        void foo();
    }

    private DummyService service;

    @Reference
    protected void setDummyService(DummyService service) {
        this.service = service;
    }

    protected void unsetDummyService(DummyService service) {
        this.service = null;
    }

    public void foo() {
        service.foo();
    }
}

If you add @NonNullByDefault to the class non-null is applied to "PARAMETER, RETURN_TYPE, FIELD, TYPE_BOUND, TYPE_ARGUMENT".

If you would like to avoid errors for the injected service reference your suggestion has been to exclude every field.

My suggestion is: Exclude only the fields that are injected.

Annotate the field you want to exclude with ′@NonNullByDefault, too AND exclude that it is applied to fields. As you annotate the one field only, only this one is excluded from the nullness analysis.

@Component
@NonNullByDefault
public class ComponentImpl {

    public static interface DummyService {
        void foo();
    }

    @NonNullByDefault({ PARAMETER, RETURN_TYPE, TYPE_BOUND, TYPE_ARGUMENT })
    private DummyService service;

    @Reference
    protected void setDummyService(DummyService service) {
        this.service = service;
    }

    protected void unsetDummyService(DummyService service) {
        this.service = null;
    }

    public void foo() {
        service.foo();
    }
}

WDYT?

As we only need the "long annotation" for injected stuff that should be ignored, I assume it is something we could do.

If we find another way some time, we can change the lines again.

-- edit --

You could also disable the assignment using `

    @NonNullByDefault({})
    private DummyService service;

[triller-telekom]

@maggu2810 I appreciate that you invested the time to figure out another way for our dependency injection problem. However I think that writing @NonNullByDefault({ PARAMETER, RETURN_TYPE, TYPE_BOUND, TYPE_ARGUMENT }) or @NonNullByDefault({}) over each field is IMHO more annoying than writing

@NonNullByDefault({ DefaultLocation.PARAMETER, DefaultLocation.RETURN_TYPE, DefaultLocation.ARRAY_CONTENTS,
        DefaultLocation.TYPE_ARGUMENT, DefaultLocation.TYPE_BOUND, DefaultLocation.TYPE_PARAMETER })

or
@NonNullByDefault({ DefaultLocation.PARAMETER, DefaultLocation.RETURN_TYPE })

ONCE over the class. I think we already agreed that we would like to have as little annotations as possible?

So out of your experience with the checker framework and annotations could you please say something about my question from #4321 (comment) ? I.e. if you could estimate the impact of using only @NonNullByDefault({ DefaultLocation.PARAMETER, DefaultLocation.RETURN_TYPE }) over the class, than the long annotation with everything but the field?

If @NonNullByDefault({ DefaultLocation.PARAMETER, DefaultLocation.RETURN_TYPE }) would be sufficient for annotating our API, which, to recap, is our goal with these null annotations, we could write that over every class and make no exceptions for dependency injected fields. WDYT?

[maggu2810]

It is not the same, you disable the nullness check for every field, regardless if it is an injected one or not.
Adding an annotation to some fields that should be ignored is IMHO much better, because we want to use the nullness checks in general but disable it for stuff that could not be handled correctly because the static analysis does not know such precondition etc.

If no fields are respected, how should a non-annotated field be interpret?
If it is ignored, then it make no sense, as we can violate our annotated API all the time without any notification (if we call our API with a field).

Adding @NonNullByDefault({}) to the OSGi service reference members (how much do you expect) is IMHO a more win then ignoring all field in a component.

[htreu]

@maggu2810 this is really nice! I like the method of disabling the assignment the most. With that we could again concentrate on the default we would like to use on all classes. Exceptions to injected or other, later initialised fields can then be handled like you described.

[triller-telekom]

It is not the same, you disable the nullness check for every field, regardless if it is an injected one or not.

True, I am totally with you, these are 2 distinct things. However our intention was to annotate the public API, which is certainly not a private field... I am just trying to figure out a configuration that makes it most usable for our goal, even if that means we loose some precision in the null analysis.

If no fields are respected, how should a non-annotated field be interpret?
If it is ignored, then it make no sense, as we can violate our annotated API all the time without any notification (if we call our API with a field).

We already ignore it because we disable "Unchecked conversion from non-annotated type to @nonnull type". So everything that is not annotated (especially return values from 3rd party libraries) can theoretically go everywhere.

Adding @NonNullByDefault({}) to the OSGi service reference members (how much do you expect) is IMHO a more win then ignoring all field in a component.

Since @htreu jumped in here while I was still typing... Maybe we should give it a try.

So to sum it up:

• We want @NonNullByDefault on every class.
• We put @NonNullByDefault({}) before an OSGI injected field
• We put @NonNullByDefault({}) before a field that is set by initialize within a ThingHandler

Warning settings stay as they are at the moment within the IDE. Are you OK with that?

[htreu]

If a class Foo is annotated with "@nonnullbydefault" it does not add annotations to member function of non-annotated classes.
If Collection is not annotated, the interpretation of "get" should not change regardless how Foo is annotated.

This is certainly right, I guess @kaikreuzer refers to the behaviour on typed Collections, where the non-nullness also is propagated to the generic type arguments:

In an @NonNullByDefault annotated class the following happens:

Map<String, String> myMap = new HashMap<>();
myMap.put("key", "value"); // succeeds
myMap.put(null, "value"); // error: "Null type mismatch: required '@NonNull String' but the provided value is null"
myMap.put("key", null); // error: "Null type mismatch: required '@NonNull String' but the provided value is null"

String value = myMap.get("key");
if (value != null) { // warning: "Redundant null check: The variable value cannot be null at this location"
    // do smth
}

This is not about annotating the method signature but about annotating the generic types used. The definition of the map would look like @NonNull Map<@NonNull String, @NonNull String> myMap when @NonNullByDefault is applied.

[maggu2810]

@htreu Ah, okay. But also this topic will be eliminated as soon as external annotations are used.

So, to move forward (at least to remove the big points we identified at the moment), we need

• a more recent Tycho (to bump the JDT compiler used by the Maven build)
• support for EEAs

[maggu2810]

@htreu Do get more into details:
A collection will mark (using the correct EEAs) that it not only returns V on get, but a @Nullable V. So it could be return null if V is marked @NonNull, too.

[triller-telekom]

@maggu2810 Do you mean that @htreu should explicitly declare his map within a @NonNullByDefault declared class like this?

Map<String, @Nullable String> myMap = new HashMap<>();
String value = myMap.get("key");
if (value != null) { // all good no DEAD code
    // do smth
}

And by "will be solved by external annotations" you mean that these external annotation will have such a definition with the @Nullable?

PS: I just read your email, will have a look on your branch now.

[htreu]

You guys will sort this out :-) From what I understand the EEAs with an @Nullable V get(K key) method will override the @NonNull V type annotation from the map declaration, right?

[maggu2810]

@htreu You are right. The method signature win.
So, if there is an explicit @NonNull V or @Nullable V this one will taken.

@triller-telekom I have comment "my obersavations" (they could be wrong) about the EEA 0,1,+ syntax on the respective project already (lastnpe's external annotations).

[triller-telekom]

Just an update on this "null analysis front" :)

Maven does not stop the build on null analysis errors (see #4604) until we have a more recent tycho version. So we intentionally accept the fact that people MIGHT contribute stuff that breaks, but since the IDE checks are still on we assume that this won't be much code and could easily be fixed once we have the new tycho version.

I have edited our coding guidelines regarding the null analysis usage, especially the part about excluding injected service fields in PR #4546:

@NonNullByDefault({})
    private DummyService service;

Also I have updated my PR "Corrected null annotations in thing package" #4550 to match these guidelines.

[tmrobert8]

I'd like to add one thing to this discussion (unless you'd also like me to add situations where a null is marked even though there is no chance of a null) - you simply can't eliminate null checks on method parameters (ie Objects.requireNotNull, etc). Both should be required!

Two reasons:

1. The attributes are great at catching static analyzed nulls and go along way to helping a programmer write good code. But they fail miserably for runtime nulls. Runtime nulls are the hardest to track down and by not having an Objects.requireNotNull check - the NPE can be much harder to figure out (especially if it's on a statement, like a stream(), that has multiple references of which any could be null. In that case, the Objects.requireNotNull is invaluable because it tells you the parameter was null, not some other variable was null.

2. As a designer, when I create and document a method - I'm specifying the contract for the method. If I specify a parameter shouldn't be null, that parameter should not be null and receiving a null should be an error (NPE). While null annotations catch a majority - they do NOT GUARANTEE them method will not receive a null and thus BREAK the contract that I created for the method. Because we don't have a guarentee - we don't know what the method will do with that null (pass it to other methods and create really hard to find issues because of that?).

Bottom line - I really like the null annotations but they CANNOT replace runtime checks

[triller-telekom]

@tmrobert8 Thank you for participating in our discussion. I have created PR #5360 to explain why we introduced those null annotations. Does this answer your concerns?

[bugabinga]

Hi!

Thanks for having this discussion in the open. It captures my experiences with null annotations well.

Is there a meaningful difference between:

@NonNullByDefault({})
    private DummyService service;

@SuppressWarnings( "null" )
    private DummyService service;

Also, how are the workarounds working out for you?

[htreu]

Without the @NonNullByDefault({}) annotation the field is marked with an error: The @NonNull field resourceBundleTracker may not have been initialized. And suppressing any warnings does not help in this case.