pippo icon indicating copy to clipboard operation
pippo copied to clipboard

Add nullability annotations to methods

Open Wavesonics opened this issue 6 years ago • 13 comments

It would be nice over all, but especially useful for Kotlin users, if Pippo's methods had nullability annotations.

I can start adding these as I go, but in some cases it's not clear if something should be null or not to me.

For instance, in the Application ctor that accepts a settings object: public Application(PippoSettings settings) is that nullable?

Anyway, maybe just as you go along, if you're in a file that you know the nullability for, slap some annotations on things?

Wavesonics avatar Sep 07 '18 22:09 Wavesonics

I prefer to avoid nullability from two reasons:

  • requires to add a new dependency (main reason)
  • increase the verbosity of the method's signature (maybe it's subjective)

I think that they are few places where a null argument/parameter is allow. If we need to do something for these situations, I think that is better to use builtin Optional.

decebals avatar Sep 08 '18 10:09 decebals

Understandable, I've gotten so used to looking at arguments with them I don't think I notice anymore, but it is much more verbose with them in there, so I understand. (Mine are often final as well as a null ability annotation, so... it's a lot lol)

There is a pure javax version of them if that makes a difference to you: (not google or jetbrains)

<dependency>
    <groupId>javax.annotation</groupId>
    <artifactId>javax.annotation-api</artifactId>
    <version>1.2</version>
</dependency>

Even if they were only used for return types of methods it would be a big help to Kotlin developers.

Wavesonics avatar Sep 10 '18 23:09 Wavesonics

Related to javax.annotation-api, it contains the annotations from JSR 250 and it doesn't contain @Nullable or @NotNull. On the other side, this library contains only the annotations but we need an implementation to have something that work. In the end, I preserve my idea that Optional is the best for us (it's already here and it's work).

decebals avatar Sep 20 '18 17:09 decebals

The correct dependency is

<dependency>
    <groupId>com.google.code.findbugs</groupId>
    <artifactId>jsr305</artifactId>
    <version>${jsr305.version}</version>
</dependency>

requires to add a new dependency (main reason)

There is no need to keep the annotations in runtime, <scope>provided</scope> is enough. Kotlin users will deeply appreciate that :)

I preserve my idea that Optional is the best for us (it's already here and it's work).

This looks like religious question, however the initial intention was not to make a replacement for maybe type: https://stackoverflow.com/a/26328555

@whyicantusemyemailasusername

There is no need to keep the annotations in runtime, provided is enough. Kotlin users will deeply appreciate that :)

I wish to make Kotlin users happy and maybe we will find a solution.

I see that are a lot of libraries that resolve this problem (analyzers):

  • Checker Framework
  • FindBugs
  • Eclipse
  • NetBeans
  • IntelliJ

Probably we need at runtime, in our Pippo based application, to add a dependecy to one of above libraries to activate these checkers (@NotNull, ...). Corect? I wish that this step to be optional by default.

On the other side, I understand that we need the definition of annotation(s) (for example @NotNull) at compilation and runtime. Correct?

In this case we have two options:

  • add a tiny depndency to pippo-core, something that define only annotation(s)
  • include the annotation(s) as java code in pippo-core

There is no need to keep the annotations in runtime, provided is enough.

The provided scope means that you must supply yourself this dependency in your application. But if the depdency is not available, you are in trouble.

@Wavesonics, @whyicantusemyemailasusername So, what is your propose (the complet solution) to resolve the issue - what depenencies, scope, ...? I ask you because I don't use these kind of annotation in my project but I want to make (Kotlin) users that use this approach happy 😄.

decebals avatar Dec 04 '18 10:12 decebals

But if the depedency is not available, you are in trouble.

Not really - java annotations doesn't require runtime classes until someone decides to use them - see https://stackoverflow.com/questions/3567413/why-doesnt-a-missing-annotation-cause-a-classnotfoundexception-at-runtime

what dependencies

We usually use com.google.code.findbugs:jsr305 - this one is small, provides standard javax.annotation.* classes and also have some annotations in javax.annotation.concurrent.* . It also fully supported by Intellij Idea.

see also https://github.com/google/guava/issues/2960 - this one is against findbugs:jsr305

@whyicantusemyemailasusername You convinced me. Your arguments are solid and the benefit is clear. Please submit a PR. Thanks!

decebals avatar Dec 04 '18 11:12 decebals

All I wish is a stable annotations API that is supported by (allmost) all major analyzers (FindBugs, Checker Framework, IntelliJ, ...)

decebals avatar Dec 04 '18 11:12 decebals

Looks like the Checker Framework is a way to go. jsr305 is almost dead (see guava's thread). Intellij Idea also supports CF. edit: Kotlin seems to support CF annotations too: https://github.com/JetBrains/kotlin/blob/master/core/descriptors.jvm/src/org/jetbrains/kotlin/load/java/JvmAnnotationNames.kt

Ya I think @whyicantusemyemailasusername covered it, just including the annotations at compile time is all that is required (Besides actually annotating the arguments and return types in the API of course).

I think this is the dependency you need if you want to use Checker Framework's annotations: https://mvnrepository.com/artifact/org.checkerframework/checker-qual/2.5.7

Should be pretty minimal.

Wavesonics avatar Dec 04 '18 19:12 Wavesonics

Is this something you'd accept pull requests on? I'd be happy to start annotating things.

Wavesonics avatar Dec 28 '18 21:12 Wavesonics

Is this something you'd accept pull requests on? I'd be happy to start annotating things.

Sure. Me and @whyicantusemyemailasusername will assist you.

decebals avatar Dec 28 '18 21:12 decebals