Consider adding @NotNull or similar to the code base

gregw commented 4 months ago

Jetty version(s) Jetty 9.x is now at End of Community Support

Enhancement Description

But which one? There is some suggestion that we can just make our own and modern IDEs will pattern match it?

sbordet commented 4 months ago

Not a big fan, as it adds boilerplate.

janbartel commented 4 months ago

I'm not a fan either. There are so many annotations that can be applied to code that we're in danger having more annotations than code. Besides, if our AI overlords are as smart as predicted, then soon they won't need annotations to understand the code and suggest null checks etc to developers.

joakime commented 4 months ago

I am a fan.

This would benefit our users the most. I think we should do this!

Based on comment from @cowwoc at https://github.com/jetty/jetty.project/pull/11843#issuecomment-2143871757

The following could be used... (copied from https://github.com/jetty/jetty.project/pull/11843#issuecomment-2145166049)

The org.eclipse.jdt.annotation.Nullable would be easy for us to use, as it's very license compatible.

https://central.sonatype.com/artifact/org.eclipse.jdt/org.eclipse.jdt.annotation/2.3.0/versions

<dependency>
    <groupId>org.eclipse.jdt</groupId>
    <artifactId>org.eclipse.jdt.annotation</artifactId>
    <version>2.3.0</version>
</dependency>

It's pretty lightweight too ...

$ jar -tvf org.eclipse.jdt.annotation-2.3.0.jar 
  2741 Fri Jan 12 17:26:36 CST 2024 META-INF/MANIFEST.MF
  2348 Fri Jan 12 17:26:38 CST 2024 META-INF/ECLIPSE_.SF
  9555 Fri Jan 12 17:26:38 CST 2024 META-INF/ECLIPSE_.RSA
     0 Fri Jan 12 17:26:36 CST 2024 META-INF/
     0 Fri Jan 12 17:26:36 CST 2024 org/
     0 Fri Jan 12 17:26:36 CST 2024 org/eclipse/
     0 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/
     0 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/
   478 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/NotOwning.class
   132 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/package-info.class
   580 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/NonNullByDefault.class
   436 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/Nullable.class
  9257 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/Checks.class
   476 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/Owning.class
   434 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/NonNull.class
  1435 Fri Jan 12 17:26:36 CST 2024 org/eclipse/jdt/annotation/DefaultLocation.class
     0 Fri Jan 12 17:26:36 CST 2024 src/
     0 Fri Jan 12 17:26:36 CST 2024 src/org/
     0 Fri Jan 12 17:26:36 CST 2024 src/org/eclipse/
     0 Fri Jan 12 17:26:36 CST 2024 src/org/eclipse/jdt/
     0 Fri Jan 12 17:26:36 CST 2024 src/org/eclipse/jdt/annotation/
 20426 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/Checks.java
  4520 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/DefaultLocation.java
  2574 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/NonNull.java
  3198 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/NonNullByDefault.java
  1833 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/NotOwning.java
  1947 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/Nullable.java
  4826 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/Owning.java
  1060 Fri Jan 12 17:15:38 CST 2024 src/org/eclipse/jdt/annotation/package-info.java
   216 Fri Jan 12 17:26:36 CST 2024 .api_description
  1460 Fri Jan 12 17:15:38 CST 2024 about.html
   608 Fri Jan 12 17:15:38 CST 2024 bundle.properties

The pom also has no dependencies, or even a parent pom.

olamy commented 4 months ago

Why not, as those static analysis tools are getting really popular and can be useful but is there any build tool plugin supporting this annotation org.eclipse.jdt.annotation.NonNull? (Maven and Gradle).

I would prefer something we can set up to be integrated into build (CI etc..) for us or for users (for such static analysis we cannot rely only on IDE). And something popular or this could be a bit useless for our users. The defacto standard tool (at least most popular) is spotbugs. I'm not sure if this is supporting this annotation.

sbordet commented 4 months ago

I don't see any value added, and a lot of work to be done to change our code to add annotations, that would only work on parameters but not return values.

It's like adding final to parameters and variables, everybody says "+1", but it's now considered bad practice.

Plus, there are a million places where the parameter would actually be non-null in the real code, but we pass null in tests.

And another million cases where the code flow is such that the parameter or field is assigned only once with a non-null value, but technically must be nullable (for example when we recycle objects).

Sure, we can do 10x the work, and workaround all the above cases and more that I have not mentioned.

Not worth it, and we have so much else more important to do than this.

olamy commented 4 months ago

I don't see any value added, and a lot of work to be done to change our code to add annotations, that would only work on parameters but not return values.

Not sure what you mean with not return values. You can annotate a method and it's taking into account as well:

stupid example but

   class App

    @Nullable
    public String getFoo() {
        if (true) {
            return null;
        }
        return "Foo";
    }

    @NonNull
    public String getBar() {
        if (random.nextBoolean()) {
            return null;
        }
        return "Bar";
    }

And another class

  class Yup

    public void test() {
        App app = new App();
        String foo = app.getFoo().trim();
        System.out.println("foo:"+foo);
    }

The static analysis tool will fail and detect 2 possible errors which can help us and potentially our users:

method documented as @NonNull but possibly returning null
using a method marked @Nullable but not doing any null check

Currently we are just writing javadoc but it's nothing but automated. Have a quick look here https://github.com/olamy/not-null mvn verify spotbugs:check

It's like adding final to parameters and variables, everybody says "+1", but it's now considered bad practice.

yes I definitely agree on useless final everywhere! but here it's a bit different and not so useless as adding final everywhere.

Plus, there are a million places where the parameter would actually be non-null in the real code, but we pass null in tests.

And another million cases where the code flow is such that the parameter or field is assigned only once with a non-null value, but technically must be nullable (for example when we recycle objects).

Sure, we can do 10x the work, and workaround all the above cases and more that I have not mentioned.

Not worth it, and we have so much else more important to do than this.

cowwoc commented 4 months ago

Your mileage may vary...

The only annotation that I personally use is @CheckReturnValue.

IntelliJ (and possibly others IDEs) are able to detect whether a method may return null or not, regardless of whether you annotate the method. What they can't detect is thread-safety and "intent" (how the API is meant to be used).

For example, if your API exposes a builder pattern and the user forgets to invoke build() then using the @CheckReturnValue annotation will warn your users that they did something wrong. Given Builder().setA().setB(), you'd annotate all the methods except for build() with @CheckReturnValue. That way, if the user forgets to invoke build() they'll get a warning.

When it comes to tools like SpotBugs I opted to use PMD instead, mostly because I like that it scans source-code instead of class files. Both SpotBugs and PMD contain false-positives that will annoy you. So they will definitely add to your workload. At the same time, they will catch low-hanging bugs proactively before your users run into them.

My suggestion is that if you use a tool like SpotBugs or PMD, don't enable all the rules. Pick a small subset of rules that are dependable (low false-positive rate) and run with them.

Some rules, like the all-popular "cyclomatic complexity" measure, are very opaque so it becomes very difficult to tune them to a reasonable value. I just turn them off. On the other hand, simple rules like detecting unused imports or fields, or ensuring a consistent naming convention for constants... those are very transparent and useful, especially when it comes to PRs. They catch a lot of the low-hanging fruit for you.

olamy commented 4 months ago

we already have spotbugs report available in CI https://jenkins.webtide.net/job/jetty.project/job/jetty-12.0.x/2057/analysis-jdk17/origin.-1831077759/ But to be honest, we do not look at them. I'm not really about adding this tooling as the default of the build, as it is slowing down the build a lot. We used to have PMD in the past, but we removed it as nobody really cares about it. As we do not really actively look at those reports, I'm a bit sceptic about adding it to the default build (we have the cache now, so it's a bit better in terms of local build). ATM, it's in CI for curiosity if someone wants to pick up some of the reported issues.

cowwoc commented 4 months ago

@olamy I fail the build on any violations so reports can't be ignored. But that's just me :)

gregw commented 4 months ago

I'm not interested in anything for post analysis like spot-bugs. It is only worthwhile if the code actually fails to compile and you get warnings as you write the code.

My limited experience with this is from working in the spring-framework and the benefits that I see are:

using an annotation can give more clarity than an explicit null check (although Objects.requiresNotNull is pretty good also).
There may be a tiny runtime benefit as null checks in the code need to be JITed out
It does detect bugs when that are nullable are not checked.

That code base appears to assume that all returns and parameters are not null unless annotated otherwise. That does remove the need to put in lots of annotations, but I fear we have lots of nullables, so probably not so much.

I think there may be some small net benefit, but a bit effort would be needed to retrofit all the code.... now is probably not the time.

Perhaps we could go with the (current) assumption that all returns and params are nullable, unless annotated otherwise. We could then optionally start using a notNull in new code.... but I'd like to see an experiment done before we added a dependency for this.

olamy commented 4 months ago

springframework is using error-prone compiler with this https://github.com/uber/NullAway so this is failing for build tool at compile phase with Maven. something similar is achievable using spotbugs as well (not exactly at compilation but the analysis can be bind after compilation phase which will have the same effect) I will try some experiment with both.

gregw commented 2 months ago

I don't think we have time to do this before 12.1, so I'm putting this into the roadmap for later consideration.

lachlan-roberts commented 2 months ago

I am not a fan of these annotations.

In our PR for spring-framework, I was working on a websocket endpoint where we know onOpen() is always called before onMessage(), so we know the session field is always set before the call to onMessage(). However because session was nullable I was forced to do a null check for every call to onMessage() even though it was impossible for it to be null.

So these annotations are restrictive in these situations where you have some context to know why it is not null, and if the null check is not there the code won't compile.

It could be quite an effort to update all these cases in the code base and it could introduce a lot of unnecessary null checks.

jetty / jetty.project

Consider adding @NotNull or similar to the code base #11848