Add optional org.eclipse.jdt.annotation dependency to binding bundles

[triller-telekom]

Signed-off-by: Stefan Triller stefan.triller@telekom.de

[triller-telekom]

I noticed that some Manifest.MF were not autogenerated and thus did not have the imports in alphabetical order. That's why some diffs are larger than my "one line" addition. But I have checked that non is missing and that is really is just a reordering of imports.

[martinvw]

What is the story about this change?
And more important why are the builds failing?

[triller-telekom]

Are you refering to build error such as reported here?
eclipse-archived/smarthome#4042

It is an Eclipse JDT Bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=463359

The story behind that is we introduced null annotations ins ESH and also placed them in some Generics, such as public void handleConfigurationUpdate(Map<@NonNull String, Object> configurationParameters)

Usually the Eclipse JDT compiler should inherit this @NonNull setting and it also does that for normal parameters and return values, but unfortunately not (yet) for generics.

EDIT: And this change is "just" about making the IDE aware of these null annotations on the binding bundles, so the null checks are executed inside the IDE. I just have to find out what's different for ESH and OH why the package org.eclipse.jdt.annotation is not available in the target.

[martinvw]

And more important why are the builds failing?

@openhab-bot
Jenkins — Looks like there's a problem with this pull request
Details

continuous-integration/travis-ci/pr — The Travis CI build failed
Details

[martinvw]

The story behind that is we introduced null annotations ins ESH and also placed them in some Generics, such as public void handleConfigurationUpdate(Map<@nonnull String, Object> configurationParameters)

Personally I would expect such annotations (especially on Generics) only to be needed at compile time and not at runtime, is the manifest really the proper place to add it?

[triller-telekom]

Having it as an optional dependency in the Manifest.mf means that we do not need it at runtime, or am I missing something?

Also the Eclipse documentation says we should include it as an optional Require-Bundle inside Manifest.mf. And since we are using package imports instead of Require-Bundle, I included them as package imports.

[martinvw]

Only one thing remains, can you look into and fix the builds for this PR?

[kaikreuzer]

is the manifest really the proper place to add it?

@martinvw Just ftr as this probably escaped you - the full discussion around that feature was done here: eclipse-archived/smarthome#3624

[triller-telekom]

Problem is here: openhab/openhab-core#189

Edit: Together with this one: eclipse-archived/smarthome#4046 So I am not sure if Travis builds against the current ESH master or snapshot? If it builds against a snapshot it will still fail, because we need this revert PR.

[triller-telekom]

close/reopen to re-trigger travis build

EDIT: Yay, passed Travis build

[martinvw]

Is this now required for every bundle should I refuse bindings without this?
Should we add static analysis for this?

[triller-telekom]

The new archtype that will be released with the next ESH stable release already includes this as a default, see here. So please refuse bindings without this dependency.

A static analysis would be nice to have, yes. In the end it is a benefit for the developers as they get a direct warning in there IDE if they pass a null value to a @NonNull parameter of a method for example.

[ppieczul]

With default non nulls, what is the recommendation for handling fields that should be non-null but are initialized using functions not suited for null annotations, for example:

String str;
...
str = otherNonNullString.toLowerCase();

There will be a mismatch that toLowerCase() may return null, so it can't be assigned to this string. I could check this null before I assign it, but there is nothing else I could assign to the string, this seems not right. I don't want to make this variable nullable too, as it should be nonnull and I'd like to rely on toLowerCase to never be null.

[triller-telekom]

As long as we do not have external annotations for libraries such as the JDK you can suppress the warning like Kai suggested here: eclipse-archived/smarthome#3930 (comment)

[martinvw]

I have the following error which I find to hard correctly categorize is it a false positive or a missing annotation in the framework:

[WARNING] D:\Java\eclipse\openhab\git\openhab2-addons\addons\binding\org.openhab.binding.rfxcom\src\main\java\org\openhab\binding\rfxcom\internal\RFXComHandlerFactory.java:[67] 
	} else if (supportsThingType(thingTypeUID)) {
	                             ^^^^^^^^^^^^
Null type safety (type annotations): The expression of type 'ThingTypeUID' needs unchecked conversion to conform to '@NonNull ThingTypeUID'

Fragment:

@Override
protected ThingHandler createHandler(Thing thing) {

   ThingTypeUID thingTypeUID = thing.getThingTypeUID();

   if (RFXComBindingConstants.SUPPORTED_BRIDGE_THING_TYPES_UIDS.contains(thingTypeUID)) {
       RFXComBridgeHandler handler = new RFXComBridgeHandler((Bridge) thing);
       registerDeviceDiscoveryService(handler);
       return handler;
   } else if (supportsThingType(thingTypeUID)) {
       return new RFXComHandler(thing);
   }

   return null;
}

https://github.com/openhab/openhab2-addons/blob/master/addons/binding/org.openhab.binding.rfxcom/src/main/java/org/openhab/binding/rfxcom/internal/RFXComHandlerFactory.java

Because of the call to thing.getThingTypeUID() we do require a null check but I would suppose a thing always has an ThingTypeUID what do you think is this an issue (and if so where in the framework or binding) or should every binding check this?

[ppieczul]

I started moving my binding code to non-null by default. Already removed many redundant checks and dead code parts. Also, use of Optional results in not only safer code, but the methods that come with Optional allow for more esthetic and simpler code. I am very pleased with the results and hope to finish it soon.
However, there are a few things that I find uneasy:

• Errors for mismatching null constraints
  For example implementation of ChannelTypeProvidermethods gives me errors at all return values that are of legacy type:

    @Override
    public Collection<ChannelType> getChannelTypes(Locale locale) {
        return channelTypes;
    }

Error is at Collection: The return type is incompatible with 'Collection<ChannelType>' returned from ChannelTypeProvider.getChannelTypes(Locale) (mismatching null constraints)

And compiler suggests to change the definition of the interface (no combination of nullable at implementation gets rid of this error). Any ideas how to overcome this?

• Warnings at points where legacy code is used.
  • The main points are string and collection methods. They seem to be manageable in my case by adding explicit nonnull and suppressing warning, because the amount of such cases is pretty low comparing to the whole code size.
  • However, warnings also appear at all calls to Optional. Now this is really problematic, because this class is intended to eliminate null references and compiler doubts about this. Now I have over 60 of these warnings. Eliminating them by suppressing inside methods looks really bad and kind of breaks the whole idea of calling flows you can create with Optional. Unfortunately this class is final and there is no way to wrap around it. I read there is a possibility to fix it by using external annotation. Any ideas?
  • Some confusing recommendations from compiler, for example:

        @Nullable
        XxxConfig cfg = getConfig().as(XxxConfig.class); // as() from Configuration class
        if (cfg != null) {
            ...
        } else {
           Here compiler will claim this is dead code, but if you look at as() implementation it returns null explicitly.            
        }

[triller-telekom]

Because of the call to thing.getThingTypeUID() we do require a null check but I would suppose a thing always has an ThingTypeUID what do you think

I think there is an annotation missing in the Thing class because a thing should always have a UID.

@ppieczul Regarding the use of Optional we had a brief discussion already here and decided to not use it on out public APIs and in other cases only where we really need to work with streams...

The ChannelTypeProvider does not have any annotations, so how can you get a warning regarding the null annotations if you implement it? Can you please give an example?

@ppieczul Regarding your last example: The static code analysis from Eclipse to check these null annotations is for sure not 100% fail safe and will generate false positives. Why it does that in this case I honestly don't know.

But using external annotations, especially to annotate JDK code would be helpful, I agree. Does anyone have an idea how to use them and also where to get a annotations for the JDK so we do not have to do that again manually?

[kaikreuzer]

@triller-telekom Check https://github.com/lastnpe/eclipse-null-eea-augments.

[ppieczul]

There's one more interesting case with maps. Because map.get() get return null when key is not found, the correct definition of maps is: Map<KeyT, @Nullable ValueT>. If we miss this nullable, we will get dead code warnings at each check of whether entry is present with get(). But when we add that nullable, the bonus is that propagates everywhere the value type is used, also to elements of the collection that map.values() returns. This causes the compiler to force us check on null elements of this collection, even if we know they are non null. Seems no good way out of that.