Maps missing @Nullable for value types

[wborn]

While adding null annotations in bindings I found some methods with maps that don't have a @Nullable annotation on their value types. As a result you're forced to use the same kind of map which may cause NPEs because map.get(key) can return null.

BaseThingHandler

https://github.com/eclipse/smarthome/blob/c3e9726c55e2a7465c60c1b1db03bec0c4c4bf46/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java#L181

https://github.com/eclipse/smarthome/blob/c3e9726c55e2a7465c60c1b1db03bec0c4c4bf46/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java#L214

https://github.com/eclipse/smarthome/blob/c3e9726c55e2a7465c60c1b1db03bec0c4c4bf46/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java#L293

https://github.com/eclipse/smarthome/blob/c3e9726c55e2a7465c60c1b1db03bec0c4c4bf46/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java#L548

https://github.com/eclipse/smarthome/blob/c3e9726c55e2a7465c60c1b1db03bec0c4c4bf46/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java#L559

DiscoveryResultBuilder

https://github.com/eclipse/smarthome/blob/c3e9726c55e2a7465c60c1b1db03bec0c4c4bf46/bundles/config/org.eclipse.smarthome.config.discovery/src/main/java/org/eclipse/smarthome/config/discovery/DiscoveryResultBuilder.java#L77

---

When I search the code base I see this issue in more classes such as: BridgeType, ThingType, ThingTypeBuilder.

It might be a good idea to also add a SAT check for this in NullAnnotationsCheck.

[maggu2810]

@wborn The value type should be annotated only with @Nullable if you explicit allow that a key can have a null value. The map's get method can always return null regardless if the value is nullable or non-null. Most of the time we miss the EEAs that I would like to integrate since a long time -- see respective issue(s) and PR(s).

[wborn]

If the APIs stay annotated this way, should people then start using workarounds such as utility methods that map the types to their own maps (which allow @Nullables) or add annotations for suppressing the warnings about values that can never be null?

[maggu2810]

@wborn Sorry, I did not get the point.

Example given: The BaseThingHandler's handleConfigurationUpdate method currently uses configurationParameters that consists of a map which entries' values must not be null.

What do you complain about? Is this wrong? Should the value of an entry be allowed to be null? So, should we differ between a non-existing key and a key with a null value?

[wborn]

For instance when you use BaseThingHandler.editProperties() you are forced to declare your variable as:

Map<String, String> map = editProperties();

Now if you write some code that gets and uses a value from this map the warning "Null comparison always yields false: The variable XXXXXX cannot be null at this location" shows.

Map<String, String> map = editProperties();

String value = map.get("key");
if (value != null) {
   ...
}

The same would happen if you override BaseThingHandler.updateProperties(Map<String, String>) and have logic that checks values from this map. If you open BaseThingHandler in Eclipse you see the class is suffering from this issue itself because the same warning shows at:

https://github.com/eclipse/smarthome/blob/8ed062ffe30ff130858f6865ac27d7e5c98ce7d9/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java#L565

[cdjackson]

I think this is the same as https://github.com/eclipse/smarthome/issues/4915?

[wborn]

Yes it has the same root cause the missing EEA.

All PRs to get these added in the end did not make it: https://github.com/eclipse/smarthome/pull/4217, https://github.com/eclipse/smarthome/pull/4513

Most maps nowadays seem to workaround this issue by adding @Nullable to the value type.

[maggu2810]

If you add @Nullable to the value type you change the semantic!

As long as EEAs are not integrated you should ignore such warnings. Adding "wrong" annotations just to remove that warnings doesn't make sense.

I never understand why you keep all this warnings and people uses workarounds to get rid of them instead of integrate EEAs.

[kaikreuzer]

Just stumbled over this and want to additionally refer to the discussion at https://github.com/eclipse/smarthome/pull/6095#issuecomment-416389259. In short: Imho the @NonNullByDefault annotation incorrectly changes the semantics, which is the cause of our problem. Having to add supress warning annotations everywhere where Maps are used is indeed an ugly workaround for that. Adding EEAs would only really resolve it, if we have a complete coverage of all jdk and used external libraries, which is highly unlikely to ever get there (despite the fact that we didn't figure out any suitable way to easily include and maintain EEAs at all).