Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to JSpecify annotations for nullability constraints #28797

Closed
sbrannen opened this issue Jul 12, 2022 · 13 comments
Closed

Migrate to JSpecify annotations for nullability constraints #28797

sbrannen opened this issue Jul 12, 2022 · 13 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@sbrannen
Copy link
Member

sbrannen commented Jul 12, 2022

Overview

Once JSpecify releases a relatively stable (potentially beta) version, we should migrate to JSpecify annotations for nullability constraints.

The previous plan was to meta-annotate annotations in the org.springframework.lang package with JSpecify annotations alongside the JSR-305 annotations, but JSpecify won't provide such meta annotations for various reasons. So the new plan is to leverage directly JSpecify annotations and deprecate Spring Framework null-safety annotations.

Related Issues

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Jul 12, 2022
@sbrannen sbrannen added this to the 6.x Backlog milestone Jul 12, 2022
@sdeleuze sdeleuze modified the milestones: 6.x Backlog, 6.1.x Nov 22, 2022
@sdeleuze
Copy link
Contributor

sdeleuze commented Nov 22, 2022

An interesting related blog post by Meta.

An important improvement would be IMO to update the Spring Framework build to check null-safety at build-time, not just rely on IntelliJ IDEA warnings, and potentially to publish guidelines for Spring portfolio projects and Spring applications that want to do the same.

@sdeleuze
Copy link
Contributor

In addition to the change of artifact and meta-annotations, the switch to jSpecify is expected to allow Spring to refine the following aspects of null-safety:

  • Support null-safety at generics, varargs and array elements levels (to be checked depending on which jSpecify version we leverage).
  • Better tooling integration, with a build-time check in Spring Framework Gradle build.
  • Prevent the unknown enum constant When.MAYBE compile-time warning when Spring null-safety annotations are used without JSR 305 available in the classpath.

@xenoterracide
Copy link

usage of jsr305 jar breaks JPMS. So requires monkey patching to deal with this if -Werror is used. and then makes the build also incompatible with javax annotations-api (v1). Although I'm sure people here are well aware of the issues around the 3? jars... I'd hope.

@sdeleuze
Copy link
Contributor

sdeleuze commented Feb 14, 2024

I have dropped a comment in jakartaee/common-annotations-api#124 (comment) as it seems not realistic to me to just drop a bunch of annotations there, this topic is much more complex than that.

Hey, Spring Framework committer here. I am not sure that would be as simple as adding those 2 additional annotations since:

  • Spring use case is leveraging meta annotations
  • The scope where null-safety applies needs to be specified, in JSR 305, we use @TypeQualifierDefault
  • Annotation attributes like When.MAYBE need to be expressed in another way in order to avoid warnings from javac
  • Need to have specifications and tooling support

In a nutshell, going beyond simple @Nonnull and @Nullable annotations is a huge task that is much more involved that it seems, as proven in https://github.com/jspecify/jspecify related work.

@xenoterracide Could you please give a try to JSpecify 0.3.0 on a sample application and see if it fulfils your JPMS needs (mostly to provide a feedback to the JSpecify team)?

Myself I plan to experiment with a Spring Framework branch using JSpecify annotations instead of JSR 305 to provide also feedback to JSpecify as they need that for an upcoming release.

@sdeleuze
Copy link
Contributor

It looks like meta-annotation capabilities have been removed from JSpecify 1.0 so using them to meta annotate Spring annotations will not be possible.

The other issue is that the "annote package to set the default to non-null" works differently in JSpecify than with JSR 305. It is expressed with the @NullMarked annotation which does not allow to differentiate the scope where those defaults are applied (while Spring allow to target APIs and field distinctly). I need to have a deeper look to @NullMarked to understand what would be the best path to move forward.

@xenoterracide
Copy link

xenoterracide commented Mar 27, 2024

They have decided that for version 1.0 they aren't covering lazy initialized fields. So for situations like JPA it is not a complete solution. This message is simply an FYI.

@sdeleuze
Copy link
Contributor

sdeleuze commented Jul 18, 2024

JSpecify 1.0.0 has been released. It does not provide meta annotation / implication capabilities.

@sdeleuze
Copy link
Contributor

sdeleuze commented Oct 1, 2024

After evaluating various options, we have decided to tentatively migrate Spring Framework 7 to JSpecify annotations and deprecate Spring null-safety annotations.

Even more important than the annotations themselves, the specifications as well as the resolution of the split package issue caused by the JSR 305 annotations impacting JPMS, the ongoing work in the ecosystem for JSpecify support in tooling or Kotlin and the capability of specifying generic type, array and varargs element null-safety have been strong reasons for this choice.

This will also enable consistency with other libraries not based on Spring that we maintain (if the project leads agree to do such move as well) like Reactor or Micrometer, and hopefully other ones we don't, helping the wider JVM ecosystem to gradually check more and more for null-safety at build-time rather seeing NullPointerException thrown on production.

@hantsy
Copy link
Contributor

hantsy commented Oct 9, 2024

It seems JDK plans Nonnull/Nullable support in the future. https://bugs.openjdk.org/browse/JDK-8303099

@sdeleuze
Copy link
Contributor

Indeed, and we are tracking this JDK effort, which will have a different timeline and some specific goals (JVM optimization angle, backward compatibility), closely.

We expect null-safety information provided by JSpecify annotation to be useful to build a bridge between JDKs not supporting null-safety and those which will.

The Spring team also intends to provide feedback to the Java Platform team on the related JEP.

@xenoterracide
Copy link

Hopefully we see a more bulk option/default soon TM. I might comment that I'd like to know if it will be able to do inference which is something that no other solution has been able to do and is probably not completely possible outside of java itself.

@xenoterracide
Copy link

xenoterracide commented Dec 14, 2024

could @Nullmarked be added to any packages that have NonNullApi and NonNullFields. Nullaway already supports this but it doesn't support springs annotations at the time of this comment. I might be willing to put in the work; also I think that it's supported by name so if spring wanted it's own "copy" as with the above. uber/NullAway#1084

@sdeleuze
Copy link
Contributor

That will be the case in Spring Framework 7 and Spring will use directly JSpecify annotations.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
This commit updates the whole Spring Framework codebase to use JSpecify
annotations instead of Spring null-safety annotations with JSR 305
semantics.

JSpecify provides signficant enhancements such as properly defined
specifications, a canonical dependency with no split-package issue,
better tooling, better Kotlin integration and the capability to specify
generic type, array and varargs element null-safety. Generic type
null-safety is not defined by this commit yet and will be specified
later.

A key difference is that Spring null-safety annotations, following
JSR 305 semantics, apply to fields, parameters and return values,
while JSpecify annotations apply to type usages. That's why this
commit moves nullability annotations closer to the type for fields
and return values.

See spring-projectsgh-28797
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
JSpecify annotations should be used instead.

See spring-projectsgh-28797
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
This commit documents using JSpecify instead of the now deprecated
Spring null-safety annotations.

Closes spring-projectsgh-28797
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
This commit updates the whole Spring Framework codebase to use JSpecify
annotations instead of Spring null-safety annotations with JSR 305
semantics.

JSpecify provides signficant enhancements such as properly defined
specifications, a canonical dependency with no split-package issue,
better tooling, better Kotlin integration and the capability to specify
generic type, array and varargs element null-safety. Generic type
null-safety is not defined by this commit yet and will be specified
later.

A key difference is that Spring null-safety annotations, following
JSR 305 semantics, apply to fields, parameters and return values,
while JSpecify annotations apply to type usages. That's why this
commit moves nullability annotations closer to the type for fields
and return values.

See spring-projectsgh-28797
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
JSpecify annotations should be used instead.

See spring-projectsgh-28797
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants