Introduce null-safety of Spring Framework API [SPR-15540]

[spring-projects-issues]

Sébastien Deleuze opened SPR-15540 and commented

The proposal brought by this issue is to make nullability of return values and parameters of Spring Framework API explicit via annotations, and leverage this information in Kotlin user Spring projects.

More details can be found in these 2 Square blog posts Rolling out @Nullable and Non-null is the default.

If at some points @ParametersAreNonnullByDefault is supported at package level, it seems it could be possible to add @Nullable annotation only on nullable parameter and return values without bloating our code source with non-nullability annotations.

Notice that the com.google.code.findbugs:jsr305 dependency used is applied with a provided scope which seems to be enough to make it taken in account by Kotlin (without that Kotlin consider Java nullability as unknown). We need to check this would not create some classloader related issues.

---

Reference URL: https://youtrack.jetbrains.com/issue/KT-10942

Issue Links:

• DATACASS-459 Adopt to changed AnnotationUtils.getValue(…) behavior
• DATAJPA-1131 Adopt to changed AnnotationUtils.getValue(…) behavior
• DATAMONGO-1710 Adopt to changed AnnotationUtils.getValue(…) and OperatorNode.getRightOperand() behavior
• DATASOLR-396 Adopt to changed AnnotationUtils.getValue(…) behavior
• BeanNotOfRequiredTypeException (NullBean instead of null) when calling ApplicationContext.getBean(name, type) [SPR-16342] #20889 BeanNotOfRequiredTypeException (NullBean instead of null) when calling ApplicationContext.getBean(name, type)
• NPE in AnnotationUtils.getValue [SPR-15642] #20201 NPE in AnnotationUtils.getValue
• Setting user header on CONNECT message stopped working [SPR-15822] #20377 Setting user header on CONNECT message stopped working
• AbstractMessageSource does not properly interact with DelegatingMessageSource parent [SPR-16047] #20596 AbstractMessageSource does not properly interact with DelegatingMessageSource parent
• Null path after UriComponents.normalize() results in NullPointerException [SPR-16364] #20911 Null path after UriComponents.normalize() results in NullPointerException
• Inconsistent @Nullable on AbstractDestinationResolvingMessagingTemplate [SPR-15670] #20229 Inconsistent @Nullable on AbstractDestinationResolvingMessagingTemplate
• AnnotationAwareOrderComparator doesn't handle null values anymore [SPR-15823] #20378 AnnotationAwareOrderComparator doesn't handle null values anymore
• Fix overridden methods nullability [SPR-15869] #20424 Fix overridden methods nullability
• AbstractMessageSource does not support null as default message anymore [SPR-16127] #20675 AbstractMessageSource does not support null as default message anymore
• Nullability inconsistency in DataAccessUtils requiredSingleResult vs requiredUniqueResult [SPR-16225] #20773 Nullability inconsistency in DataAccessUtils requiredSingleResult vs requiredUniqueResult
• *Utils.find### to return Optional [SPR-17484] #22016 *Utils.find### to return Optional
• BeanDefinitionBuilder method arguments not annotated with @Nullable [SPR-15841] #20396 BeanDefinitionBuilder method arguments not annotated with @Nullable
• STOMP SEND should not map to @SubscribeMapping methods [SPR-16109] #20657 STOMP SEND should not map to @SubscribeMapping methods
• Consistent use of Collection.toArray with zero-sized array argument [SPR-16523] #21066 Consistent use of Collection.toArray with zero-sized array argument
• Decouple o.s.w.reactive.result.view.UrlBasedViewResolver from ApplicationContext [SPR-15219] #19784 Decouple o.s.w.reactive.result.view.UrlBasedViewResolver from ApplicationContext
• Revisit java.util.Optional declarations in reactive API signatures [SPR-15576] #20135 Revisit java.util.Optional declarations in reactive API signatures
• Extend null-safety to field level [SPR-15720] #20276 Extend null-safety to field level
• Revisit nullability annotations towards GA [SPR-15756] #20311 Revisit nullability annotations towards GA
• Backport selected refinements from the nullability efforts in 5.0 [SPR-15656] #20215 Backport selected refinements from the nullability efforts in 5.0
• Make getters and setters null-safety consistent [SPR-15792] #20347 Make getters and setters null-safety consistent

6 votes, 5 watchers

[spring-projects-issues]

Sébastien Deleuze commented

After a deeper look and discussing on the KT-10942, here is what I found.

First, this issue is not only interesting for Kotlin users but also for all Java developers since IntelliJ IDEA supports JSR 305 annotations provided by findbugs package as well as Eclipse.

Second, while @ParametersAreNonnullByDefault only applies to parameters and not return values, the good news is that this annotation is itself meta-annotated with meta-annotations that could allow us to craft an annotation suitable for our needs. for example :

@Documented
@Nonnull
@TypeQualifierDefault({ElementType.METHOD, ElementType.PARAMETER})
@Retention(RetentionPolicy.RUNTIME)
public @interface NonNullApiByDefault {
}

IDEA already supports these meta annotations so that could enable us to move forward even before Kotlin support that. Andrey Breslav has expressed his interest in supporting these meta-annotations in Kotlin so that could be a nice outcome.

I am going to create a branch right now to experiment on this. We should have shortly some news from the Kotlin side.

[spring-projects-issues]

Sébastien Deleuze commented

Work in progress commit : master...sdeleuze:null-safety

My goal is to reach a state where if have processed all Spring Framework modules Javadoc nullability related comments and applied the matching @Nullable annotations (should be done tomorrow) and ship that with RC2. Kotlin team is currently evaluating when KT-10942 will be fixed, but it seems doable in a reasonable amount of time based on their first thoughts.

I will perform additional tests with IDEA and Eclipse to see how these annotations are taken in account in Java ASAP.

[spring-projects-issues]

Sébastien Deleuze commented

After some tests I confirm that works with IntelliJ which reports warnings if null valurs are passed on non-nullable parameters for example.
Annotations need to be set on each package (it is not transitive for child packages).

Juergen Hoeller Could you please have a quick look on the modules I did on this branch to give me your advice, especially to validate the @NonNullableApiByDefault principle, maybe to propose a better name if you think this one is not appropriate, and to say me if you are ok with this first scope of @Nullable annotations (the ones specified in the Javadoc, I used a search on "code null"). I am sure a lot of other ones are missing but that's a beginning that we could improve after RC2 (this is just a warning in the IDE, not on command line compilation).

[spring-projects-issues]

Sébastien Deleuze commented

I am approaching a stage where I will be able to merge in master. After discussing with Juergen Hoeller, we have chosen to leverage JSR 305 meta-annotations in 2 new Spring annotations. This seems the right tradeoff since it avoid us to directly expose dormant JSR 305 annotations (they could potentially change), give us the possibility to add additional meta-annotation later if needed, avoid to spread to JSR 305 with provided scope "trick" across all other Spring projects while IntelliJ (and Kotlin) will be able to recognize them without any kind of Spring specific support. We intend to use the same strategy on Reactor, and possibly other Spring projects.

For package level:

package org.springframework.lang;

@Documented
@Nonnull
@Target({ElementType.METHOD, ElementType.PARAMETER})
@TypeQualifierDefault({ElementType.METHOD, ElementType.PARAMETER})
@Retention(RetentionPolicy.RUNTIME)
public @interface NonNullApi {
}

Mostly for parameter or return value level:

package org.springframework.lang;

@Documented
@javax.annotation.Nullable
@Target({ElementType.METHOD, ElementType.PARAMETER})
@TypeQualifierDefault({ElementType.METHOD, ElementType.PARAMETER})
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {
}

[spring-projects-issues]

Sébastien Deleuze commented

I am doing a last pass on the 1752 return null; we have in our codebase and I will merge my branch in master (previous @Nullable addtions was based on the Javadoc).

[spring-projects-issues]

Sébastien Deleuze commented

Merged in master via this commit.

[spring-projects-issues]

Sébastien Deleuze commented

Unlike what I saw in my tests (not sure why), it seems IntelliJ does not recognize Spring @Nullable annotations meta annotated with JSR 305 ones. I have created IDEA-173544 and requested a quick feedback from IntelliJ team in order to have a status before our RC2 release.

[spring-projects-issues]

Sébastien Deleuze commented

Workaround for Spring projects developers: it is possible to make Spring @Nullable annotation recognized in IntelliJ IDEA by going to menu File -> Settings -> Editor -> Inspections -> @NotNull / @Nullable problems -> Options -> Configure Annotations and adding org.springframework.lang.Nullable to the list of nullable annotations.

[spring-projects-issues]

Sébastien Deleuze commented

After a useful discussion with Peter Gromov about JSR 305 and IDEA support for custom @Nullable, it seems we have reached a consensus. See also these interesting slides for more background https://www.cs.umd.edu/~pugh/JSR-305.pdf.

On Spring Framework side, I am going to update our @org.springframework.lang.Nullable annotation in order to leverage JSR 305 @TypeQualifierNickname meta-annotation + NonNull(when=MAYBE) since this is a more suitable semantic than NonNull(when=UNKNOWN) carried by @javax.annotation.Nullable. Also the removal of @TypeQualifierDefault will make nullable only applied to the return value when set at method level.

@Documented
@TypeQualifierNickname 
@Nonnull(when=MAYBE)
@Target({ElementType.METHOD, ElementType.PARAMETER})
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {
}

IntelliJ IDEA 2017.1 next update will provide pre-configured support for @org.springframework.lang.Nullable in order to provide a good developer experience by default for Spring Framework 5.0 GA. Before we get that, Spring developers should configure the annotation manually as explained in my previous comment. IntelliJ IDEA 2017.2 will provide full support for JSR 305 meta annotations, allowing to support Spring null(safety in a fully generic way).

Eclipse support nullable + non-null by default via configuration of the custom annotations so they will be able to leverage null-safety as well.
Netbeans does not support non-null by default strategy, but I will open a feature request on their bug tracker.

I will add a documentation page providing more detail about null-safety for Spring developers and tooling vendors.

[spring-projects-issues]

Sébastien Deleuze commented

See also related null-safety support on Reactor side, with a similar strategy for @NonNullApi and usage of javax.annotation.Nullable directly in order to avoid adding an additional @Nullable annotation in the classpath and because they don't need to be as defensive as Spring about JSR 305 annotations.

After Juergen Hoeller epic commit I am waiting feedbacks from other projects like Boot (#20201) or Data before resolving this issue.

[spring-projects-issues]

Sébastien Deleuze commented

After another huge commit from Juergen Hoeller, nullability declaration is now extended to the field level as well.

[spring-projects-issues]

Matthew commented

Hello. I have just raised issue #20657 which was introduced by a commit from this issue.