pippo-java / pippo

Micro Java Web Framework
http://www.pippo.ro
Apache License 2.0
788 stars 128 forks source link

Add nullability annotations to methods #445

Open Wavesonics opened 6 years ago

Wavesonics commented 6 years ago

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?

decebals commented 6 years ago

I prefer to avoid nullability from two reasons:

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.

Wavesonics commented 6 years ago

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.

decebals commented 6 years ago

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).

whyicantusemyemailasusername commented 5 years ago

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

decebals commented 5 years ago

@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):

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:

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 😄.

whyicantusemyemailasusername commented 5 years ago

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.

whyicantusemyemailasusername commented 5 years ago

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

decebals commented 5 years ago

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

decebals commented 5 years ago

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

whyicantusemyemailasusername commented 5 years ago

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

Wavesonics commented 5 years ago

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 commented 5 years ago

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

decebals commented 5 years ago

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

Sure. Me and @whyicantusemyemailasusername will assist you.