Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Enable Null annotations warnings/errors in IDE and Maven #3906

Merged
merged 4 commits into from
Jul 31, 2017

Conversation

triller-telekom
Copy link
Contributor

Enables a warning on a potential Nullpointer access and an Error if a null value is passed to a @nonnull argument

Fixes #3624

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@@ -19,6 +19,7 @@ Require-Bundle: org.eclipse.xtext;visibility:=reexport,
org.eclipse.smarthome.model.lazygen;resolution:=optional
Import-Package: com.google.common.base,
com.google.common.collect,
javax.annotation,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should mark it optional, so that it is only required at compile time, but not at runtime.

@@ -19,7 +19,8 @@ Require-Bundle: org.eclipse.xtext;visibility:=reexport,
org.objectweb.asm;bundle-version="[5.0.1,6.0.0)";resolution:=optional,
org.eclipse.smarthome.model.lazygen;resolution:=optional,
org.eclipse.xtext.xbase.lib
Import-Package: org.apache.log4j,
Import-Package: javax.annotation,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should mark it optional, so that it is only required at compile time, but not at runtime.

pom.xml Outdated
</extraClasspathElements>
<compilerArgs>
<arg>-err:+nullAnnot(javax.annotation.Nullable|javax.annotation.Nonnull|org.eclipse.jdt.annotation.NonNullByDefault),+inheritNullAnnot</arg>
<arg>-warn:+null</arg>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace tabs by spaces on this line

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks.
@maggu2810 Are you ok with this solution for the null annotations? Imho it brings a good value as we can more precisely specify our APIs with it now.

@maggu2810
Copy link
Contributor

I would like to give it a try, add some annotations on the API, remove the annotation stubs in my projects and check if it play well together...

One question about the optional import...
I have done it some time in the Eclipse IDE. Add a Nullable or NonNull annotation on a method used by other bundles (e.g. in the base module handler). The Eclipse IDE complains about missing stuff if the bindings does not import (optional or not) the annotation bundle itself. It is not a problem for the runtime, but for the Eclipse IDE only. Does it work with that solution?
I can test it myself if I test the other stuff as well, but perhaps you already done it...

@triller-telekom
Copy link
Contributor Author

At first I should mention that we are using @Nonnull with a lower-case n in the middle, otherwise you might wonder why it is not working ;)

I just did the following test but I am not sure if it is what you meant:

Inside bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java I annotated the State parameter of the updateState(ChannelUid, State) with @Nonnull and had to add the optional javax.annotation dependency to the bundle org.eclipse.smarthome.core.thing.

In a binding like the astro binding for example, I can call that method and get a warning if I pass null as the State parameter to that method. But I do NOT need to add a javax.annotation dependency to the astro binding bundle.

Is that what you wanted to know?

@maggu2810 maggu2810 merged commit 6487a19 into eclipse-archived:master Jul 31, 2017
@maggu2810
Copy link
Contributor

@triller-telekom Thanks

@triller-telekom triller-telekom deleted the nullAnnotations branch August 1, 2017 07:49
@sjsf
Copy link
Contributor

sjsf commented Sep 11, 2017

How about adding a word to the documentation here:
https://github.com/eclipse/smarthome/blob/master/docs/documentation/development/guidelines.md?

@triller-telekom
Copy link
Contributor Author

I guess first we have to agree on what to use where in #4080 before we write a documentation.

@maggu2810
Copy link
Contributor

How about adding a word to the documentation here:
https://github.com/eclipse/smarthome/blob/master/docs/documentation/development/guidelines.md?

I started with a summary in the wiki (as @triller-telekom also referenced in his linked comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants