From 4c7735016b2edb6766261c2882e17ff3662d607f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Deleuze?= Date: Tue, 19 Mar 2024 16:30:05 +0100 Subject: [PATCH] Refine null-safety with NullAway build-time checks This commit introduces null-safety checks for spring-core at build-time in order to validate the consistency of Spring null-safety annotations and generate errors when inconsistencies are detected during a build (similar to what is done with Checkstyle). In order to make that possible, this commit also introduces a new org.springframework.lang.Contract annotation inspired from org.jetbrains.annotations.Contract, which allows to specify semantics of methods like Assert#notNull in order to prevent artificial additional null checks in Spring Framework code base. This commit only checks org.springframework.core package, follow-up commits will also extend the analysis to other modules, after related null-safety refinements. See gh-32475 --- build.gradle | 1 + gradle/spring-module.gradle | 18 +++++ .../core/annotation/RepeatableContainers.java | 2 + .../org/springframework/lang/Contract.java | 73 +++++++++++++++++++ .../java/org/springframework/util/Assert.java | 9 +++ .../org/springframework/util/ClassUtils.java | 1 + .../springframework/util/CollectionUtils.java | 2 + .../util/ConcurrentLruCache.java | 2 +- .../org/springframework/util/MimeType.java | 1 + .../springframework/util/MimeTypeUtils.java | 1 + .../org/springframework/util/NumberUtils.java | 1 + .../org/springframework/util/StringUtils.java | 5 ++ .../util/concurrent/ListenableFuture.java | 1 + .../util/concurrent/ListenableFutureTask.java | 1 + 14 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 spring-core/src/main/java/org/springframework/lang/Contract.java diff --git a/build.gradle b/build.gradle index fb9774658382..302675faf6cb 100644 --- a/build.gradle +++ b/build.gradle @@ -9,6 +9,7 @@ plugins { id 'de.undercouch.download' version '5.4.0' id 'me.champeau.jmh' version '0.7.2' apply false id 'me.champeau.mrjar' version '0.1.1' + id "net.ltgt.errorprone" version "3.1.0" apply false } ext { diff --git a/gradle/spring-module.gradle b/gradle/spring-module.gradle index 42ff9f94c21f..883712b5b756 100644 --- a/gradle/spring-module.gradle +++ b/gradle/spring-module.gradle @@ -6,12 +6,15 @@ apply plugin: 'org.springframework.build.optional-dependencies' // apply plugin: 'com.github.johnrengelman.shadow' apply plugin: 'me.champeau.jmh' apply from: "$rootDir/gradle/publications.gradle" +apply plugin: 'net.ltgt.errorprone' dependencies { jmh 'org.openjdk.jmh:jmh-core:1.37' jmh 'org.openjdk.jmh:jmh-generator-annprocess:1.37' jmh 'org.openjdk.jmh:jmh-generator-bytecode:1.37' jmh 'net.sf.jopt-simple:jopt-simple' + errorprone 'com.uber.nullaway:nullaway:0.10.24' + errorprone 'com.google.errorprone:error_prone_core:2.9.0' } pluginManager.withPlugin("kotlin") { @@ -109,3 +112,18 @@ publishing { // Disable publication of test fixture artifacts. components.java.withVariantsFromConfiguration(configurations.testFixturesApiElements) { skip() } components.java.withVariantsFromConfiguration(configurations.testFixturesRuntimeElements) { skip() } + +tasks.withType(JavaCompile).configureEach { + options.errorprone { + disableAllChecks = true + option("NullAway:CustomContractAnnotations", "org.springframework.lang.Contract") + option("NullAway:AnnotatedPackages", "org.springframework.core") + option("NullAway:UnannotatedSubPackages", "org.springframework.instrument,org.springframework.context.index," + + "org.springframework.asm,org.springframework.cglib,org.springframework.objenesis," + + "org.springframework.javapoet,org.springframework.aot.nativex.substitution") + } +} +tasks.compileJava { + // The check defaults to a warning, bump it up to an error for the main sources + options.errorprone.error("NullAway") +} \ No newline at end of file diff --git a/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java b/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java index 581ffeb9c8b8..9f7bf61ad2d6 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java @@ -22,6 +22,7 @@ import java.util.Map; import java.util.Objects; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ConcurrentReferenceHashMap; @@ -80,6 +81,7 @@ Annotation[] findRepeatedAnnotations(Annotation annotation) { @Override + @Contract("null -> false") public boolean equals(@Nullable Object other) { if (other == this) { return true; diff --git a/spring-core/src/main/java/org/springframework/lang/Contract.java b/spring-core/src/main/java/org/springframework/lang/Contract.java new file mode 100644 index 000000000000..416b6ff4f297 --- /dev/null +++ b/spring-core/src/main/java/org/springframework/lang/Contract.java @@ -0,0 +1,73 @@ +/* + * Copyright 2002-2024 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.lang; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Inspired from {@code org.jetbrains.annotations.Contract}, this variant has been introduce in the + * {@code org.springframework.lang} package to avoid requiring an extra dependency, while still following the same semantics. + * + *

Specifies some aspects of the method behavior depending on the arguments. Can be used by tools for advanced data flow analysis. + * Note that this annotation just describes how the code works and doesn't add any functionality by means of code generation. + * + *

Method contract has the following syntax:
+ * contract ::= (clause ';')* clause
+ * clause ::= args '->' effect
+ * args ::= ((arg ',')* arg )?
+ * arg ::= value-constraint
+ * value-constraint ::= 'any' | 'null' | '!null' | 'false' | 'true'
+ * effect ::= value-constraint | 'fail' + * + * The constraints denote the following:
+ *

+ *

Examples: + * @Contract("_, null -> null") - method returns null if its second argument is null
+ * @Contract("_, null -> null; _, !null -> !null") - method returns null if its second argument is null and not-null otherwise
+ * @Contract("true -> fail") - a typical assertFalse method which throws an exception if true is passed to it
+ * + * @author Sebastien Deleuze + * @since 6.2 + * @see NullAway custom contract annotations + */ +@Documented +@Retention(RetentionPolicy.CLASS) +@Target(ElementType.METHOD) +public @interface Contract { + + /** + * Contains the contract clauses describing causal relations between call arguments and the returned value. + */ + String value() default ""; + + /** + * Specifies if this method is pure, i.e. has no visible side effects. This may be used for more precise data flow analysis, and + * to check that the method's return value is actually used in the call place. + */ + boolean pure() default false; +} diff --git a/spring-core/src/main/java/org/springframework/util/Assert.java b/spring-core/src/main/java/org/springframework/util/Assert.java index ad045071d486..9b0cba8e9fdf 100644 --- a/spring-core/src/main/java/org/springframework/util/Assert.java +++ b/spring-core/src/main/java/org/springframework/util/Assert.java @@ -20,6 +20,7 @@ import java.util.Map; import java.util.function.Supplier; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; /** @@ -71,6 +72,7 @@ public abstract class Assert { * @param message the exception message to use if the assertion fails * @throws IllegalStateException if {@code expression} is {@code false} */ + @Contract("false, _ -> fail") public static void state(boolean expression, String message) { if (!expression) { throw new IllegalStateException(message); @@ -92,6 +94,7 @@ public static void state(boolean expression, String message) { * @throws IllegalStateException if {@code expression} is {@code false} * @since 5.0 */ + @Contract("false, _ -> fail") public static void state(boolean expression, Supplier messageSupplier) { if (!expression) { throw new IllegalStateException(nullSafeGet(messageSupplier)); @@ -106,6 +109,7 @@ public static void state(boolean expression, Supplier messageSupplier) { * @param message the exception message to use if the assertion fails * @throws IllegalArgumentException if {@code expression} is {@code false} */ + @Contract("false, _ -> fail") public static void isTrue(boolean expression, String message) { if (!expression) { throw new IllegalArgumentException(message); @@ -124,6 +128,7 @@ public static void isTrue(boolean expression, String message) { * @throws IllegalArgumentException if {@code expression} is {@code false} * @since 5.0 */ + @Contract("false, _ -> fail") public static void isTrue(boolean expression, Supplier messageSupplier) { if (!expression) { throw new IllegalArgumentException(nullSafeGet(messageSupplier)); @@ -137,6 +142,7 @@ public static void isTrue(boolean expression, Supplier messageSupplier) * @param message the exception message to use if the assertion fails * @throws IllegalArgumentException if the object is not {@code null} */ + @Contract("!null, _ -> fail") public static void isNull(@Nullable Object object, String message) { if (object != null) { throw new IllegalArgumentException(message); @@ -154,6 +160,7 @@ public static void isNull(@Nullable Object object, String message) { * @throws IllegalArgumentException if the object is not {@code null} * @since 5.0 */ + @Contract("!null, _ -> fail") public static void isNull(@Nullable Object object, Supplier messageSupplier) { if (object != null) { throw new IllegalArgumentException(nullSafeGet(messageSupplier)); @@ -167,6 +174,7 @@ public static void isNull(@Nullable Object object, Supplier messageSuppl * @param message the exception message to use if the assertion fails * @throws IllegalArgumentException if the object is {@code null} */ + @Contract("null, _ -> fail") public static void notNull(@Nullable Object object, String message) { if (object == null) { throw new IllegalArgumentException(message); @@ -185,6 +193,7 @@ public static void notNull(@Nullable Object object, String message) { * @throws IllegalArgumentException if the object is {@code null} * @since 5.0 */ + @Contract("null, _ -> fail") public static void notNull(@Nullable Object object, Supplier messageSupplier) { if (object == null) { throw new IllegalArgumentException(nullSafeGet(messageSupplier)); diff --git a/spring-core/src/main/java/org/springframework/util/ClassUtils.java b/spring-core/src/main/java/org/springframework/util/ClassUtils.java index 5e2ab066a971..062a386c3973 100644 --- a/spring-core/src/main/java/org/springframework/util/ClassUtils.java +++ b/spring-core/src/main/java/org/springframework/util/ClassUtils.java @@ -544,6 +544,7 @@ public static boolean isPrimitiveWrapperArray(Class clazz) { * @param clazz the class to check * @return the original class, or a primitive wrapper for the original primitive type */ + @SuppressWarnings("NullAway") public static Class resolvePrimitiveIfNecessary(Class clazz) { Assert.notNull(clazz, "Class must not be null"); return (clazz.isPrimitive() && clazz != void.class ? primitiveTypeToWrapperMap.get(clazz) : clazz); diff --git a/spring-core/src/main/java/org/springframework/util/CollectionUtils.java b/spring-core/src/main/java/org/springframework/util/CollectionUtils.java index 4127d1adf178..17bf57391467 100644 --- a/spring-core/src/main/java/org/springframework/util/CollectionUtils.java +++ b/spring-core/src/main/java/org/springframework/util/CollectionUtils.java @@ -34,6 +34,7 @@ import java.util.function.BiFunction; import java.util.function.Consumer; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; /** @@ -71,6 +72,7 @@ public static boolean isEmpty(@Nullable Collection collection) { * @param map the Map to check * @return whether the given Map is empty */ + @Contract("null -> true") public static boolean isEmpty(@Nullable Map map) { return (map == null || map.isEmpty()); } diff --git a/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java b/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java index 6ab26b8f6153..926bb671ce89 100644 --- a/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java +++ b/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java @@ -45,7 +45,7 @@ * @param the type of the cached values, does not allow null values * @see #get(Object) */ -@SuppressWarnings({"unchecked"}) +@SuppressWarnings({"unchecked", "NullAway"}) public final class ConcurrentLruCache { private final int capacity; diff --git a/spring-core/src/main/java/org/springframework/util/MimeType.java b/spring-core/src/main/java/org/springframework/util/MimeType.java index a178489598aa..8b68a0a2e451 100644 --- a/spring-core/src/main/java/org/springframework/util/MimeType.java +++ b/spring-core/src/main/java/org/springframework/util/MimeType.java @@ -573,6 +573,7 @@ public int compareTo(MimeType other) { else { String thisValue = getParameters().get(thisAttribute); String otherValue = other.getParameters().get(otherAttribute); + Assert.notNull(thisValue, "Parameter for " + thisAttribute + " must not be null"); if (otherValue == null) { otherValue = ""; } diff --git a/spring-core/src/main/java/org/springframework/util/MimeTypeUtils.java b/spring-core/src/main/java/org/springframework/util/MimeTypeUtils.java index 494d3fcc77d7..93c946ec6760 100644 --- a/spring-core/src/main/java/org/springframework/util/MimeTypeUtils.java +++ b/spring-core/src/main/java/org/springframework/util/MimeTypeUtils.java @@ -213,6 +213,7 @@ public static MimeType parseMimeType(String mimeType) { return cachedMimeTypes.get(mimeType); } + @SuppressWarnings("NullAway") private static MimeType parseMimeTypeInternal(String mimeType) { int index = mimeType.indexOf(';'); String fullType = (index >= 0 ? mimeType.substring(0, index) : mimeType).trim(); diff --git a/spring-core/src/main/java/org/springframework/util/NumberUtils.java b/spring-core/src/main/java/org/springframework/util/NumberUtils.java index 11febb67f180..505088913a72 100644 --- a/spring-core/src/main/java/org/springframework/util/NumberUtils.java +++ b/spring-core/src/main/java/org/springframework/util/NumberUtils.java @@ -238,6 +238,7 @@ else if (BigDecimal.class == targetClass || Number.class == targetClass) { * @see #convertNumberToTargetClass * @see #parseNumber(String, Class) */ + @SuppressWarnings("NullAway") public static T parseNumber( String text, Class targetClass, @Nullable NumberFormat numberFormat) { diff --git a/spring-core/src/main/java/org/springframework/util/StringUtils.java b/spring-core/src/main/java/org/springframework/util/StringUtils.java index 9394b6836c08..028e9d074261 100644 --- a/spring-core/src/main/java/org/springframework/util/StringUtils.java +++ b/spring-core/src/main/java/org/springframework/util/StringUtils.java @@ -36,6 +36,7 @@ import java.util.TimeZone; import java.util.stream.Collectors; +import org.springframework.lang.Contract; import org.springframework.lang.Nullable; /** @@ -122,6 +123,7 @@ public static boolean isEmpty(@Nullable Object str) { * @see #hasLength(String) * @see #hasText(CharSequence) */ + @Contract("null -> false") public static boolean hasLength(@Nullable CharSequence str) { return (str != null && str.length() > 0); } @@ -135,6 +137,7 @@ public static boolean hasLength(@Nullable CharSequence str) { * @see #hasLength(CharSequence) * @see #hasText(String) */ + @Contract("null -> false") public static boolean hasLength(@Nullable String str) { return (str != null && !str.isEmpty()); } @@ -158,6 +161,7 @@ public static boolean hasLength(@Nullable String str) { * @see #hasLength(CharSequence) * @see Character#isWhitespace */ + @Contract("null -> false") public static boolean hasText(@Nullable CharSequence str) { if (str == null) { return false; @@ -188,6 +192,7 @@ public static boolean hasText(@Nullable CharSequence str) { * @see #hasLength(String) * @see Character#isWhitespace */ + @Contract("null -> false") public static boolean hasText(@Nullable String str) { return (str != null && !str.isBlank()); } diff --git a/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java b/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java index 9c0514b24b84..cd6125c9d501 100644 --- a/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java +++ b/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFuture.java @@ -62,6 +62,7 @@ public interface ListenableFuture extends Future { * Expose this {@link ListenableFuture} as a JDK {@link CompletableFuture}. * @since 5.0 */ + @SuppressWarnings("NullAway") default CompletableFuture completable() { CompletableFuture completable = new DelegatingCompletableFuture<>(this); addCallback(completable::complete, completable::completeExceptionally); diff --git a/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFutureTask.java b/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFutureTask.java index b0bc7b6c1c66..71601200a70d 100644 --- a/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFutureTask.java +++ b/spring-core/src/main/java/org/springframework/util/concurrent/ListenableFutureTask.java @@ -70,6 +70,7 @@ public void addCallback(SuccessCallback successCallback, FailureCallb } @Override + @SuppressWarnings("NullAway") public CompletableFuture completable() { CompletableFuture completable = new DelegatingCompletableFuture<>(this); this.callbacks.addSuccessCallback(completable::complete);