Add nullability annotations and compile-time checks #4557

marcphilipp · 2025-05-20T08:52:20Z

Overview

This PR uses JSpecify's annotations to annotate packages as @NullMarked which implies fields, parameters, return types etc. in those package are not nullable by default. If null is allowed, the @Nullable annotation is used to indicate that.

At compile-time Error Prone with the NullAway plugin is used to verify consistency of the annotations causing the build to fail, for example, if null is passed as a parameter where it isn't allowed.

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes

sormuras · 2025-05-20T08:57:44Z

This PR uses JSpecify's annotations to annotate packages as @NullMarked which implies fields, parameters, return types etc. in those package are not nullable by default. If null is allowed, the @Nullable annotation is used to indicate that.

Would it make sense to annotate modules instead of packages with @NullMarked instead?

marcphilipp · 2025-05-20T09:48:45Z

Would it make sense to annotate modules instead of packages with @NullMarked instead?

Yes, I think that's a good idea! I went package by package but when a module is done moving the annotation to the module declaration makes sense. 👍

I'll give that a shot.

marcphilipp · 2025-05-20T10:11:09Z

Unfortunately, NullAway does not support that:

Support Nullmarked on module-info (JPMS) uber/NullAway#1083

marcphilipp · 2025-05-20T10:29:49Z

I think a found a good "interim" solution:

For NullAway, the config is now in the build setup because it doesn't yet support module descriptors
For IntelliJ, the config is now in the module descriptors

marcphilipp · 2025-05-20T10:32:06Z

junit-platform-commons/src/main/java/module-info.java

 	requires static transitive org.apiguardian.api;
+	requires static transitive org.jspecify;


We should discuss whether this is the right "scope". It requires downstream projects to have the JSpecify jar on their module path (should they be compiled using the module path).

requires static would make it optional, but we'd have to suppress warnings like these.

Mh, yeah. It ships with a compiled module descriptor, though: org.jspecify

marcphilipp · 2025-05-20T11:29:40Z

gradle/plugins/common/src/main/kotlin/junitbuild.java-nullability-conventions.gradle.kts

+
+tasks.withType<JavaCompile>().configureEach {
+	options.errorprone {
+		disableAllChecks = true


We can later (after this PR) enable additional checks provided by Error Prone but I didn't want to mix those changes into this already large set of changes.

marcphilipp · 2025-05-22T05:39:07Z

I'll put annotations back on the packages after the discussion in uber/NullAway#1083 (comment).

marcphilipp · 2025-05-22T06:26:31Z

...rm-tooling-support-tests/src/archUnit/java/platform/tooling/support/tests/ArchUnitTests.java

+	@ArchTest
+	void packagesShouldBeNullMarked(JavaClasses classes) {
+		var exclusions = Stream.of( //
+			"..shadow..", //
+			"org.junit.jupiter.api..", //
+			"org.junit.jupiter.engine..", //
+			"org.junit.jupiter.migrationsupport..", //
+			"org.junit.jupiter.params..", //
+			"org.junit.platform.launcher.." //
+		).map(PackageMatcher::of).toList();
+
+		var subpackages = Stream.of("org.junit.platform", "org.junit.jupiter", "org.junit.vintage") //
+				.map(classes::getPackage) //
+				.flatMap(rootPackage -> rootPackage.getSubpackagesInTree().stream()) //
+				.filter(pkg -> exclusions.stream().noneMatch(it -> it.matches(pkg.getName()))) //
+				.filter(pkg -> !pkg.getClasses().isEmpty()) //
+				.toList();
+		assertThat(subpackages).isNotEmpty();
+
+		var violations = subpackages.stream() //
+				.filter(pkg -> !pkg.isAnnotatedWith(NullMarked.class)) //
+				.map(JavaPackage::getName) //
+				.sorted();
+		assertThat(violations).describedAs("The following packages are missing the @NullMarked annotation").isEmpty();
+	}


This check will help us to avoid forgetting to add the annotation on new packages.

scordio · 2025-05-24T06:52:29Z

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java

@@ -565,7 +566,7 @@ static boolean isWideningConversion(Class<?> sourceType, Class<?> targetType) {
 	 * @return the corresponding wrapper type or {@code null} if the
 	 * supplied type is {@code null} or not a primitive type
 	 */
-	public static Class<?> getWrapperType(Class<?> type) {
+	public static @Nullable Class<?> getWrapperType(Class<?> type) {


@marcphilipp given that the Javadoc mentions:

* @return the corresponding wrapper type or {@code null} if the * supplied type is {@code null} or not a primitive type

should the parameter also have @Nullable?

public static @Nullable Class<?> getWrapperType(@Nullable Class<?> type)

(I spotted it during the rebase of #4219)

After a quick look, I think there are no unit tests for getWrapperType() in ReflectionUtilsTests.

Happy to submit a PR to address both topics, if you agree 🙂

Good catch! On main, there are no usages that potentially pass null, are there? Do you need that for #4219? If so, adding @Nullable along with tests sounds good to me. If not, I think we should rather change the Javadoc.

From my branch:

junit5/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/TypeDescriptor.java

Lines 61 to 64 in 791778e

if (type == null) { // FIXME parameter of ReflectionUtils.getWrapperType should be @Nullable

return null;

}

Class<?> wrapperType = ReflectionUtils.getWrapperType(type);

triggered this, which is somehow related to the following on main:

junit5/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java

Line 117 in 261c2f5

Class<?> targetTypeToUse = toWrapperType(targetType);

I try to figure out why the warning doesn't happen on main 🤔

scordio · 2025-06-07T09:20:44Z

CONTRIBUTING.md

+- The descriptor of each module is annotated with `@NullMarked` for IDEs such as IntelliJ
+  IDEA to treat code correctly.


@marcphilipp IIUC, this eventually didn't happen due to uber/NullAway#1083, and you instead went for annotating the packages, right?

Good catch, thank you! Updated in df24efb

marcphilipp self-assigned this May 20, 2025

marcphilipp force-pushed the marc/4550-nullability-checks branch from f61aa87 to f8f1e3d Compare May 20, 2025 09:45

marcphilipp mentioned this pull request May 20, 2025

Support Nullmarked on module-info (JPMS) uber/NullAway#1083

Closed

marcphilipp force-pushed the marc/4550-nullability-checks branch from f8f1e3d to e2cd369 Compare May 20, 2025 10:28

marcphilipp commented May 20, 2025

View reviewed changes

marcphilipp force-pushed the marc/4550-nullability-checks branch 2 times, most recently from e950738 to 914277e Compare May 20, 2025 11:28

marcphilipp commented May 20, 2025

View reviewed changes

marcphilipp force-pushed the marc/4550-nullability-checks branch 3 times, most recently from 7f62e96 to 907cdd0 Compare May 20, 2025 14:03

marcphilipp linked an issue May 20, 2025 that may be closed by this pull request

Add JSpecify nullability annotations to Java APIs #4550

Closed

2 tasks

marcphilipp added theme: programming model component: Jupiter component: Platform component: Kotlin labels May 22, 2025

marcphilipp force-pushed the marc/4550-nullability-checks branch from f3fca05 to 11adeb2 Compare May 22, 2025 06:24

marcphilipp commented May 22, 2025

View reviewed changes

marcphilipp force-pushed the marc/4550-nullability-checks branch 6 times, most recently from cd8776e to 29ede48 Compare May 22, 2025 15:37

mpkorstanje mentioned this pull request May 22, 2025

Consider nullability annotations and compile-time checks cucumber/cucumber-jvm#3000

Open

9 tasks

marcphilipp added 12 commits May 23, 2025 15:37

Annotate nullability in org.junit.jupiter.api

b3ca703

Check org.junit.jupiter.api..

0b7b463

Enable NullAway in junit-jupiter-engine

169e5d7

Annotate nullability in org.junit.jupiter.engine.config

6b2a64a

Annotate nullability in org.junit.jupiter.engine

179b3e9

Annotate nullability in org.junit.jupiter.engine.descriptor

63377d7

Check org.junit.jupiter.engine..

b6386c7

Annotate nullability in remaining org.junit.jupiter.engine.. packages

ddd17a9

Enable NullAway in junit-jupiter-migrationsupport

f38fb2d

Enable NullAway in junit-jupiter-params

dc3ae6e

Add JSpecify to all module descriptors

5587851

Enable NullAway in tests

2cccb29

marcphilipp force-pushed the marc/4550-nullability-checks branch from 29ede48 to 834e903 Compare May 23, 2025 13:38

marcphilipp added 4 commits May 23, 2025 15:55

Enable NullAway in documentation

62797b0

Annotate nullability in org.junit.platform.commons.test

c0bb36a

Treat compiler warnings as errors for all source sets

5da1c40

Fix compilation of JMH benchmarks

aa121ee

marcphilipp force-pushed the marc/4550-nullability-checks branch from 834e903 to aa121ee Compare May 23, 2025 13:55

marcphilipp added 3 commits May 23, 2025 16:17

Move @Nullable to return type for methods

02c8d81

Fix IDEA-detected nullability problems in main source sets

d80c226

Add to release notes

6c11b2b

marcphilipp marked this pull request as ready for review May 23, 2025 14:49

marcphilipp merged commit 261c2f5 into main May 23, 2025
13 checks passed

marcphilipp deleted the marc/4550-nullability-checks branch May 23, 2025 15:06

scordio reviewed May 24, 2025

View reviewed changes

This was referenced May 24, 2025

Evaluate JSpecify annotations for nullability constraints assertj/assertj#3727

Open

Remove null support from getWrapperType input, add test coverage #4571

Merged

This was referenced Jun 4, 2025

Explicitly indicate Nullability of arguments and return values FasterXML/jackson-core#1105

Open

Tracking issue for potential adoption of JSpecify nullness annotations jOOQ/jOOQ#12949

Open

scordio reviewed Jun 7, 2025

View reviewed changes

		requires static transitive org.apiguardian.api;
		requires static transitive org.jspecify;

	if (type == null) { // FIXME parameter of ReflectionUtils.getWrapperType should be @Nullable
	return null;
	}
	Class<?> wrapperType = ReflectionUtils.getWrapperType(type);

		- The descriptor of each module is annotated with `@NullMarked` for IDEs such as IntelliJ
		IDEA to treat code correctly.

Uh oh!

Add nullability annotations and compile-time checks #4557

Add nullability annotations and compile-time checks #4557

Uh oh!

Conversation

marcphilipp commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

sormuras commented May 20, 2025

Uh oh!

marcphilipp commented May 20, 2025

Uh oh!

marcphilipp commented May 20, 2025

Uh oh!

marcphilipp commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcphilipp commented May 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scordio May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scordio Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcphilipp commented May 20, 2025 •

edited

Loading

scordio May 24, 2025 •

edited

Loading

scordio Jun 7, 2025 •

edited

Loading