Skip to content

Commit

Permalink
Refine null-safety with NullAway build-time checks
Browse files Browse the repository at this point in the history
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 spring-projectsgh-32475
  • Loading branch information
sdeleuze committed Mar 19, 2024
1 parent 0a715bc commit 4c77350
Show file tree
Hide file tree
Showing 14 changed files with 117 additions and 1 deletion.
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 18 additions & 0 deletions gradle/spring-module.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down Expand Up @@ -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")
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -80,6 +81,7 @@ Annotation[] findRepeatedAnnotations(Annotation annotation) {


@Override
@Contract("null -> false")
public boolean equals(@Nullable Object other) {
if (other == this) {
return true;
Expand Down
73 changes: 73 additions & 0 deletions spring-core/src/main/java/org/springframework/lang/Contract.java
Original file line number Diff line number Diff line change
@@ -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.
*
* <p>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.
*
* <p>Method contract has the following syntax:<br/>
* contract ::= (clause ';')* clause<br/>
* clause ::= args '->' effect<br/>
* args ::= ((arg ',')* arg )?<br/>
* arg ::= value-constraint<br/>
* value-constraint ::= 'any' | 'null' | '!null' | 'false' | 'true'<br/>
* effect ::= value-constraint | 'fail'
*
* The constraints denote the following:<br/>
* <ul>
* <li> _ - any value
* <li> null - null value
* <li> !null - a value statically proved to be not-null
* <li> true - true boolean value
* <li> false - false boolean value
* <li> fail - the method throws an exception, if the arguments satisfy argument constraints
* </ul>
* <p>Examples:
* <code>@Contract("_, null -> null")</code> - method returns null if its second argument is null<br/>
* <code>@Contract("_, null -> null; _, !null -> !null")</code> - method returns null if its second argument is null and not-null otherwise<br/>
* <code>@Contract("true -> fail")</code> - a typical assertFalse method which throws an exception if <code>true</code> is passed to it<br/>
*
* @author Sebastien Deleuze
* @since 6.2
* @see <a href="https://github.com/uber/NullAway/wiki/Configuration#custom-contract-annotations">NullAway custom contract annotations</a>
*/
@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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Map;
import java.util.function.Supplier;

import org.springframework.lang.Contract;
import org.springframework.lang.Nullable;

/**
Expand Down Expand Up @@ -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);
Expand All @@ -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<String> messageSupplier) {
if (!expression) {
throw new IllegalStateException(nullSafeGet(messageSupplier));
Expand All @@ -106,6 +109,7 @@ public static void state(boolean expression, Supplier<String> 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);
Expand All @@ -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<String> messageSupplier) {
if (!expression) {
throw new IllegalArgumentException(nullSafeGet(messageSupplier));
Expand All @@ -137,6 +142,7 @@ public static void isTrue(boolean expression, Supplier<String> 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);
Expand All @@ -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<String> messageSupplier) {
if (object != null) {
throw new IllegalArgumentException(nullSafeGet(messageSupplier));
Expand All @@ -167,6 +174,7 @@ public static void isNull(@Nullable Object object, Supplier<String> 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);
Expand All @@ -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<String> messageSupplier) {
if (object == null) {
throw new IllegalArgumentException(nullSafeGet(messageSupplier));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.function.BiFunction;
import java.util.function.Consumer;

import org.springframework.lang.Contract;
import org.springframework.lang.Nullable;

/**
Expand Down Expand Up @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
* @param <V> the type of the cached values, does not allow null values
* @see #get(Object)
*/
@SuppressWarnings({"unchecked"})
@SuppressWarnings({"unchecked", "NullAway"})
public final class ConcurrentLruCache<K, V> {

private final int capacity;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "";
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ else if (BigDecimal.class == targetClass || Number.class == targetClass) {
* @see #convertNumberToTargetClass
* @see #parseNumber(String, Class)
*/
@SuppressWarnings("NullAway")
public static <T extends Number> T parseNumber(
String text, Class<T> targetClass, @Nullable NumberFormat numberFormat) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.TimeZone;
import java.util.stream.Collectors;

import org.springframework.lang.Contract;
import org.springframework.lang.Nullable;

/**
Expand Down Expand Up @@ -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);
}
Expand All @@ -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());
}
Expand All @@ -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;
Expand Down Expand Up @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public interface ListenableFuture<T> extends Future<T> {
* Expose this {@link ListenableFuture} as a JDK {@link CompletableFuture}.
* @since 5.0
*/
@SuppressWarnings("NullAway")
default CompletableFuture<T> completable() {
CompletableFuture<T> completable = new DelegatingCompletableFuture<>(this);
addCallback(completable::complete, completable::completeExceptionally);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public void addCallback(SuccessCallback<? super T> successCallback, FailureCallb
}

@Override
@SuppressWarnings("NullAway")
public CompletableFuture<T> completable() {
CompletableFuture<T> completable = new DelegatingCompletableFuture<>(this);
this.callbacks.addSuccessCallback(completable::complete);
Expand Down

0 comments on commit 4c77350

Please sign in to comment.