From 4fdf40e39ec011103e316a52414ea4c56b5af4a7 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 15 Jul 2025 13:45:34 +0200 Subject: [PATCH] Fix Checkstyle configuration for nullability annotations While assessing #35195, I noticed the following issues with our Checkstyle configuration regarding nullability annotations. - "^(?!org\.jspecify|\.annotations).*(NonNull|Nullable)$" contains a "|". - "^(?!org\.jspecify|\.annotations).*(NonNull|Nullable)$" matches against NonNull but not against Nonnull, and therefore incorrectly permits usage of javax.annotation.Nonnull. - Some of the Checkstyle suppressions no longer apply. This commit addresses all of the above issues and updates several tests to use example annotations other than javax.annotation.Nonnull where feasible. See gh-35195 Closes gh-35205 --- .../AnnotatedElementUtilsTests.java | 38 ++++++++++--------- .../annotation/AnnotationFilterTests.java | 10 ++--- .../AnnotationTypeMappingsTests.java | 4 +- .../core/annotation/AnnotationUtilsTests.java | 9 +++-- src/checkstyle/checkstyle-suppressions.xml | 7 +--- src/checkstyle/checkstyle.xml | 16 ++++++-- 6 files changed, 45 insertions(+), 39 deletions(-) diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java index 9490fb0712f..21fe0ec480d 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java @@ -31,8 +31,10 @@ import java.util.Date; import java.util.List; import java.util.Set; -import javax.annotation.Nonnull; -import javax.annotation.ParametersAreNonnullByDefault; +import javax.annotation.RegEx; +import javax.annotation.Syntax; +import javax.annotation.concurrent.ThreadSafe; +import javax.annotation.meta.TypeQualifierNickname; import javax.annotation.meta.When; import jakarta.annotation.Resource; @@ -46,6 +48,7 @@ import org.springframework.core.annotation.AnnotationUtilsTests.WebController; import org.springframework.core.annotation.AnnotationUtilsTests.WebMapping; import org.springframework.core.testfixture.stereotype.Component; import org.springframework.core.testfixture.stereotype.Indexed; +import org.springframework.lang.Contract; import org.springframework.util.MultiValueMap; import static java.util.Arrays.asList; @@ -237,9 +240,8 @@ class AnnotatedElementUtilsTests { @SuppressWarnings("deprecation") void isAnnotatedForPlainTypes() { assertThat(isAnnotated(Order.class, Documented.class)).isTrue(); - assertThat(isAnnotated(org.springframework.lang.NonNullApi.class, Documented.class)).isTrue(); - assertThat(isAnnotated(org.springframework.lang.NonNullApi.class, Nonnull.class)).isTrue(); - assertThat(isAnnotated(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue(); + assertThat(isAnnotated(Inherited.class, Documented.class)).isTrue(); + assertThat(isAnnotated(Contract.class, Documented.class)).isTrue(); } @Test @@ -278,9 +280,8 @@ class AnnotatedElementUtilsTests { @SuppressWarnings("deprecation") void hasAnnotationForPlainTypes() { assertThat(hasAnnotation(Order.class, Documented.class)).isTrue(); - assertThat(hasAnnotation(org.springframework.lang.NonNullApi.class, Documented.class)).isTrue(); - assertThat(hasAnnotation(org.springframework.lang.NonNullApi.class, Nonnull.class)).isTrue(); - assertThat(hasAnnotation(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue(); + assertThat(hasAnnotation(Inherited.class, Documented.class)).isTrue(); + assertThat(hasAnnotation(Contract.class, Documented.class)).isTrue(); } @Test @@ -346,17 +347,16 @@ class AnnotatedElementUtilsTests { @SuppressWarnings("deprecation") void getAllAnnotationAttributesOnLangType() { MultiValueMap attributes = getAllAnnotationAttributes( - org.springframework.lang.NonNullApi.class, Nonnull.class.getName()); - assertThat(attributes).as("Annotation attributes map for @Nonnull on NonNullApi").isNotNull(); - assertThat(attributes.get("when")).as("value for NonNullApi").isEqualTo(List.of(When.ALWAYS)); + org.springframework.lang.NonNullApi.class, javax.annotation.Nonnull.class.getName()); + assertThat(attributes).as("Annotation attributes map for @Nonnull on @NonNullApi").isNotNull(); + assertThat(attributes.get("when")).as("value for @NonNullApi").isEqualTo(List.of(When.ALWAYS)); } @Test void getAllAnnotationAttributesOnJavaxType() { - MultiValueMap attributes = getAllAnnotationAttributes( - ParametersAreNonnullByDefault.class, Nonnull.class.getName()); - assertThat(attributes).as("Annotation attributes map for @Nonnull on NonNullApi").isNotNull(); - assertThat(attributes.get("when")).as("value for NonNullApi").isEqualTo(List.of(When.ALWAYS)); + MultiValueMap attributes = getAllAnnotationAttributes(RegEx.class, Syntax.class.getName()); + assertThat(attributes).as("Annotation attributes map for @Syntax on @RegEx").isNotNull(); + assertThat(attributes.get("when")).as("value for @RegEx").isEqualTo(List.of(When.ALWAYS)); } @Test @@ -855,8 +855,10 @@ class AnnotatedElementUtilsTests { @Test void javaxMetaAnnotationTypeViaFindMergedAnnotation() { - assertThat(findMergedAnnotation(ParametersAreNonnullByDefault.class, Nonnull.class)).isEqualTo(ParametersAreNonnullByDefault.class.getAnnotation(Nonnull.class)); - assertThat(findMergedAnnotation(ResourceHolder.class, Nonnull.class)).isEqualTo(ParametersAreNonnullByDefault.class.getAnnotation(Nonnull.class)); + assertThat(findMergedAnnotation(ThreadSafe.class, Documented.class)) + .isEqualTo(ThreadSafe.class.getAnnotation(Documented.class)); + assertThat(findMergedAnnotation(ResourceHolder.class, TypeQualifierNickname.class)) + .isEqualTo(RegEx.class.getAnnotation(TypeQualifierNickname.class)); } @Test @@ -1536,7 +1538,7 @@ class AnnotatedElementUtilsTests { } @Resource(name = "x") - @ParametersAreNonnullByDefault + @RegEx static class ResourceHolder { } diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java index 8a48353d627..6bf99d15784 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java @@ -19,9 +19,8 @@ package org.springframework.core.annotation; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; -import javax.annotation.Nonnull; +import javax.annotation.concurrent.ThreadSafe; -import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; import org.springframework.lang.Contract; @@ -86,12 +85,13 @@ class AnnotationFilterTests { @Test void javaWhenJavaxAnnotationReturnsTrue() { - assertThat(AnnotationFilter.JAVA.matches(Nonnull.class)).isTrue(); + assertThat(AnnotationFilter.JAVA.matches(ThreadSafe.class)).isTrue(); } @Test + @SuppressWarnings("deprecation") void javaWhenSpringLangAnnotationReturnsFalse() { - assertThat(AnnotationFilter.JAVA.matches(Nullable.class)).isFalse(); + assertThat(AnnotationFilter.JAVA.matches(org.springframework.lang.Nullable.class)).isFalse(); } @Test @@ -103,7 +103,7 @@ class AnnotationFilterTests { @SuppressWarnings("deprecation") void noneReturnsFalse() { assertThat(AnnotationFilter.NONE.matches(Retention.class)).isFalse(); - assertThat(AnnotationFilter.NONE.matches(Nullable.class)).isFalse(); + assertThat(AnnotationFilter.NONE.matches(org.springframework.lang.Nullable.class)).isFalse(); assertThat(AnnotationFilter.NONE.matches(TestAnnotation.class)).isFalse(); } diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationTypeMappingsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationTypeMappingsTests.java index a34e4f73ea9..0eac833cc5e 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationTypeMappingsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationTypeMappingsTests.java @@ -29,8 +29,6 @@ import java.util.List; import java.util.Map; import java.util.stream.IntStream; -import javax.annotation.Nullable; - import org.junit.jupiter.api.Test; import org.springframework.core.annotation.AnnotationTypeMapping.MirrorSets; @@ -442,7 +440,7 @@ class AnnotationTypeMappingsTests { return result; } - private @Nullable Method getAliasMapping(AnnotationTypeMapping mapping, int attributeIndex) { + private Method getAliasMapping(AnnotationTypeMapping mapping, int attributeIndex) { int mapped = mapping.getAliasMapping(attributeIndex); return mapped != -1 ? mapping.getRoot().getAttributes().get(mapped) : null; } diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java index a38fba676bd..f1364c73cec 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java @@ -30,8 +30,9 @@ import java.util.Map; import java.util.NoSuchElementException; import java.util.Set; -import javax.annotation.Nonnull; -import javax.annotation.ParametersAreNonnullByDefault; +import javax.annotation.RegEx; +import javax.annotation.Syntax; +import javax.annotation.concurrent.ThreadSafe; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -428,8 +429,8 @@ class AnnotationUtilsTests { @Test void isAnnotationMetaPresentForPlainType() { assertThat(isAnnotationMetaPresent(Order.class, Documented.class)).isTrue(); - assertThat(isAnnotationMetaPresent(ParametersAreNonnullByDefault.class, Documented.class)).isTrue(); - assertThat(isAnnotationMetaPresent(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue(); + assertThat(isAnnotationMetaPresent(ThreadSafe.class, Documented.class)).isTrue(); + assertThat(isAnnotationMetaPresent(RegEx.class, Syntax.class)).isTrue(); } @Test diff --git a/src/checkstyle/checkstyle-suppressions.xml b/src/checkstyle/checkstyle-suppressions.xml index ad2ca8f7b56..3fe0922a831 100644 --- a/src/checkstyle/checkstyle-suppressions.xml +++ b/src/checkstyle/checkstyle-suppressions.xml @@ -21,9 +21,6 @@ - - - @@ -40,9 +37,7 @@ - - - + diff --git a/src/checkstyle/checkstyle.xml b/src/checkstyle/checkstyle.xml index d7ba423332f..e6947f32357 100644 --- a/src/checkstyle/checkstyle.xml +++ b/src/checkstyle/checkstyle.xml @@ -104,10 +104,20 @@ - + - + + + + + + + + + + + +