Add nullability annotations and compile-time checks by marcphilipp · Pull Request #4557 · junit-team/junit-framework

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

 	requires java.management; // needed by RuntimeUtils to determine input arguments
+
 	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

+
+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

+	@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

 	 * 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:

https://github.com/junit-team/junit5/blob/791778e4189e9e19a7908500b49113d21d20014e/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/TypeDescriptor.java#L61-L64

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

https://github.com/junit-team/junit5/blob/261c2f5a1576ea29add753b92499e7cf29342f23/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java#L117

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

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

+- 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

Closed

12 tasks

marcphilipp added 9 commits May 23, 2025 15:37

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 (JSpecify) -- for 3.2? 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

mkurz mentioned this pull request Jul 11, 2025

Adopt JSpecify? playframework/playframework#13398

Open

magicprinc mentioned this pull request Jul 20, 2025

Add nullability annotations nats-io/nats.java#1217

Closed

armandino mentioned this pull request Feb 24, 2026

Breaking change: replace JetBrains annotations with JSpecify instancio/instancio#1604

Merged

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marcphilipp commented May 20, 2025 •

edited

Loading

scordio May 24, 2025 •

edited

Loading

scordio Jun 7, 2025 •

edited

Loading