spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.74k stars 38.15k forks source link

Replace `@Nonnull(when = When.MAYBE) ` by `@CheckForNull` in `@Nullable` #27183

Closed xenoterracide closed 5 months ago

xenoterracide commented 3 years ago

it appears this is found in the findbugs jsr305 annotations my reason why is I want to enable -Werror and I'm using java modules module-info.java and it appears you can't use jsr305 as the automatic module name. sadly I don't actually know what When does or if there's a replacement. most tools seem to respect annotations named Nullable or NonNull now.

/home/xeno/.gradle/caches/modules-2/files-2.1/org.springframework/spring-core/5.3.8/da9b87dacaa5bbf80fad0f7b483988372a00a152/spring-core-5.3.8.jar(/org/springframework/lang/Nullable.class): warning: Cannot find annotation method 'when()' in type 'Nonnull': class file for javax.annotation.Nonnull not found
warning: unknown enum constant When.MAYBE
  reason: class file for javax.annotation.meta.When not found
error: warnings found and -Werror specified
1 error
2 warnings
❯ ./gradlew --version && ./gradlew :authn:dependencyInsight --configuration runtimeClasspath --dependency spring-core                                                                      # backend -> master + !

------------------------------------------------------------
Gradle 7.1.1
------------------------------------------------------------

Build time:   2021-07-02 12:16:43 UTC
Revision:     774525a055494e0ece39f522ac7ad17498ce032c

Kotlin:       1.4.31
Groovy:       3.0.7
Ant:          Apache Ant(TM) version 1.10.9 compiled on September 27 2020
JVM:          11.0.11 (AdoptOpenJDK 11.0.11+9)
OS:           Linux 5.10.42-1-MANJARO amd64

Type-safe dependency accessors is an incubating feature.
Type-safe project accessors is an incubating feature.

> Task :authn:dependencyInsight
org.springframework:spring-core:5.3.8
serv-inc commented 2 years ago

Does https://stackoverflow.com/questions/61140322/jetty-run-error-java-lang-typenotpresentexception-type-javax-annotation-meta-wh help ?

    <dependency>
        <groupId>com.google.code.findbugs</groupId>
        <artifactId>jsr305</artifactId>
        <version>3.0.2</version>
    </dependency>
sbrannen commented 2 years ago

Do you have jsr305-3.0.2.jar in the class path or module path?

it appears you can't use jsr305 as the automatic module name.

What happens when you attempt to do that?

Are you encountering issues with split packages?

chirag-ji commented 2 years ago

I've also getting this, but adding jsr305 worked.

I wonder why it is not working as it was available as a transitive dependency in the classpath, but working fine after adding it as 'direct dependency`. Any one please share some views on it.

sdeleuze commented 2 years ago

This is a known side effect of current JSR 305 support, the Java compiler generates warning on annotation with enum values when not present in the classpath while it is lenient for the annotations themselves. As suggested by Sam, the workaround is to have the jsr305 in the classpath or module path.

If I am not mistaken, this only occurs when user code is using Spring null-safety annotations, so that's on purpose that we don't add jsr305 to the transitive dependencies in a mandatory way.

The middle term fix will be to switch to https://github.com/jspecify/jspecify (where Spring team is involved) and remove those JSR305 annotations from Spring ones.

@sbrannen What about creating an issue dedicated to switching from JSR 305 to JSpecify in the 6.x Backlog in order to give visibility to our users and closing this one?

sbrannen commented 2 years ago
cpovirk commented 7 months ago

I am all in favor of a migration to JSpecify, but FYI, I suspect that you can prevent this warning by changing...

https://github.com/spring-projects/spring-framework/blob/6f95af978e6b923fa229e6752ef9c119627e3b97/spring-core/src/main/java/org/springframework/lang/Nullable.java#L53

...to just...

@CheckForNull

We do this for a common annotation inside Google, and it works well with Kotlin.

sdeleuze commented 7 months ago

Thanks for the insight, I am going to give it a try.

xenoterracide commented 7 months ago

curious, what problem does using meta annotations actually solve in todays world? how is that consumed? most tools that would have used it now support these things just by looking at Nonnull/Nullable, I think.

cpovirk commented 7 months ago

I think that the tool that people usually care most about is Kotlin:

The meta annotations are also supported by IntelliJ when editing Java code.

And I assume that SpotBugs has inherited FindBugs's support for them.

I'd be interested in knowing of other tools that are of interest to people, too.

xenoterracide commented 7 months ago

Yes, but spotbugs and IntelliJ also support not having those specific meta annotations. Other annotations work now. Unless you know something that I don't about how those tools work. Which is quite possible. I guess I'm just questioning on whether or not there's any tooling that requires those annotations or any part of those toolings that require those meta annotations. There's a lot of nullity annotations out there right now that don't have the meta annotations. So a lot of tools are supporting without, I believe.

It's probably worth saying that my primary nullness checker is errorprone null away. Checker framework is also pretty popular.

I think it might be worth testing the assumption that these meta annotations are still really buying you anything at all.

cpovirk commented 7 months ago

I suspect that they don't buy a ton except for Kotlin.

I'd have to look into IntelliJ and SpotBugs:

xenoterracide commented 7 months ago

Fair, I was just suggesting verifying that the world is still needing this for something.

Uh, If you see that very inappropriate thing show up in an email... I was speech to texting and Google translated something from my TV. Sorry I honestly want you to know I didn't say that.

sdeleuze commented 7 months ago

Seems to work as expected based on my tests:

sdeleuze commented 7 months ago

I had sadly to revert the related commit as, as observed by @jhoeller, IntelliJ IDEA is not able to recognize @Nullable semantics with that change.

@cpovirk Let's maybe discuss that with JetBrains.

cpovirk commented 7 months ago

Oh, that's a shame :( Thanks for letting me know. I may try to switch the one we're using in Google now.

I wondered if maybe IntelliJ would do what we want if we were to use @Nullable instead of @CheckForNull, but it looks like it does not. (I'm actually not surprised by that: The JSR-305 @Nullable technically is more like "unknown nullness" than it is what people normally mean by "nullable." I don't remember offhand if Kotlin behaves the same way.)

I've filed IDEA-351380 to see if we can get support for @TypeQualifierNickname @CheckForNull. Star, vote, etc. :)

nkonev commented 5 months ago

According to https://youtrack.jetbrains.com/issue/IDEA-351380/Support-CheckForNull-with-TypeQualifierNickname seems that IDE's behavior is fixed in IntelliJ IDEA 2024.1.2