Created AnnotateNullableParameters recipe

[stefanodallapalma]

Overview

Automatically adds @Nullable annotations to method parameters that are explicitly null-checked within the method body. This helps teams incrementally adopt null safety annotations in existing codebases without manual effort.

What's your motivation?

Many legacy codebases already contain defensive null checks but lack formal null safety annotations. If developers write if (param == null), they clearly intended that parameter to be nullable. This recipe makes that existing intent explicit through annotations, enabling better static analysis and API documentation.

What it detects

• Direct comparisons: param == null, param != null
• Utility methods: Objects.isNull(param), StringUtils.isBlank(param), etc.
• Negated checks: !Objects.isNull(param), !StringUtils.isNotBlank(param)
• Compound conditions: param == null || param.isEmpty()

Before:

public PersonBuilder setName(String name) {
    if (name != null) {
        this.name = name;
    }
    return this;
}

After:

import org.jspecify.annotations.Nullable;

public PersonBuilder setName(@Nullable String name) {
    if (name != null) {
        this.name = name;
    }
    return this;
}

Anything in particular you'd like reviewers to focus on?

Configuration

The default annotation is org.jspecify.annotations.Nullable as a widely used standard annotation and already default for the AnnotateNullableMethods recipe. The annotation can be customized via the recipe param nullableAnnotationClass (kept the same name as in AnnotateNullableMethods.java for consistency).

Do note that the annotation detection only processes parameters not already annotated with the default or provided annotation (based on fully qualified name). This means that if the parameter has already a @Nullable annotation from a different package (e.g., javax.annotation.Nullable vs org.jspecify.annotations.Nullable), this recipe may end up adding a duplicate annotation. Should we compare against simple name instead to avoid this?.

Limitations

I want to ensure these design decisions make sense:

• Public methods only: The recipe only analyzes public methods to avoid over-annotating internal utility methods that may change more frequently than public API contracts. Public methods represent stable interfaces where null safety documentation provides the most value. Is this scope appropriate or too restrictive?

• Direct if conditions only: Currently only detects null checks in direct if conditions (not in nested method calls or complex control flow). This only annotates when there's clear evidence of null checking while keeping the implementation simple and covering the most common patterns, but reviewers should consider if this misses too many real-world cases.

• Predefined method matchers: The recipe uses a curated list of known null-checking methods rather than a more flexible pattern. This ensures precision but may miss custom utility methods. Should we provide a way to extend this list, or is the current approach sufficient?

• No data flow analysis: The recipe assumes parameters aren't dereferenced before null checks. However, this assumption is intentionally conservative and aligns with the recipe's core purpose: documenting developer intent rather than verifying correctness. The presence of if (param == null) anywhere in the method is strong evidence of nullable intent, regardless of whether that check is technically correct or optimally placed. Should we add documentation clarifying this intent-focused approach, or is this reasoning sufficient?

Anyone you would like to review specifically?

@timtebeek

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Great to see! Your design decisions make sense to me; indeed best to start conservatively and iterate from there. I imagine we'll discover more as we run these recipes, same as we saw with the recent ternary addition.

Reading your description I'd briefly wondered if we'd want to cover cases like assert a != null, but I'll go over your implementation before we look to see if that makes sense.

[stefanodallapalma]

Reading your description I'd briefly wondered if we'd want to cover cases like assert a != null, but I'll go over your implementation before we look to see if that makes sense.

I had a similar thought about including methods like Objects.requireNonNull(). However, my understanding of @Nullable on parameters is that they signal the method can safely handle null values (e.g., by using defaults or graceful degradation). Methods that use assert or Objects.requireNonNull() are actually expressing the opposite intent, aren't they?

[timtebeek]

Great addition @stefanodallapalma ! Look forward to seeing what undocumented nullable arguments this will turn up when run against OpenRewrite, Moderne and even Spring Framework 7. 🙏🏻 Hope it helps you there as well! I'm thinking of bundling up this recipe along with a few others into a JSpecify best practices either here or in rewrite-migrate-java.

[stefanodallapalma]

@timtebeek Thank you for taking care of additional changes needed to clean up and tweak the recipe. They provide useful learning material for me to develop future recipes!

One more consideration. We'll likely have to compare params' annotations against the target annotation's simple name (along with similar known annotations like CheckForNull rather than their fqn, in order to avoid duplicates). Think of the many Jakarta Nullable annotations used for DTOs or by Spotbugs, for example.

I'll work on it after my holidays if that makes sense.

[stefanodallapalma]

On a different note, I love the idea of a JSpecify best practices bundle!

[timtebeek]

@timtebeek Thank you for taking care of additional changes needed to clean up and tweak the recipe. They provide useful learning material for me to develop future recipes!

You're welcome! Didn't want to leave you with a puzzle, but also wanted to not forget to cover those cases, hence why I only added the test earlier before I found time for the fix.

One more consideration. We'll likely have to compare params' annotations against the target annotation's simple name (along with similar known annotations like CheckForNull rather than their fqn, in order to avoid duplicates). Think of the many Jakarta Nullable annotations used for DTOs or by Spotbugs, for example.

I'll work on it after my holidays if that makes sense.

Thanks! We already have a few recipes to migrate off of alternative nullable annotations; I'd recommend folks run those first, possible as part of the JSpecify best practices before adding new annotations:
https://github.com/openrewrite/rewrite-migrate-java/blob/d2a747c437bc4a585d4e9d10aa36b78c2845513a/src/main/resources/META-INF/rewrite/jspecify.yml#L18-L28

Thanks again!

[timtebeek]

On a different note, I love the idea of a JSpecify best practices bundle!

• 🚀 Add JSpecify best practices recipe rewrite-migrate-java#744

[timtebeek]

Ran the recipe just now, and saw a somewhat curious case:

diff --git a/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java b/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java
index d8e85f0..a1fa7c9 100644
--- a/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java
+++ b/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java
@@ -16,6 +16,8 @@ org.openrewrite.staticanalysis.AnnotateNullableParameters
 
 package org.openrewrite.java.dependencies.internal;
 
+import org.jspecify.annotations.Nullable;
+
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -92,7 +94,7 @@
         }
 
         @Override
-        public boolean equals(Object obj) {
+        public boolean equals(@Nullable Object obj) {
             if (obj == this) {
                 return true;
             }

Any quick thoughts? Should we exclude any methods that are overrides and assume annotations are placed on the parent class/interface?

[stefanodallapalma]

Thanks! We already have a few recipes to migrate off of alternative nullable annotations; I'd recommend folks run those first, possible as part of the JSpecify best practices before adding new annotations:

https://github.com/openrewrite/rewrite-migrate-java/blob/d2a747c437bc4a585d4e9d10aa36b78c2845513a/src/main/resources/META-INF/rewrite/jspecify.yml#L18-L28

I understand, and that's the approach I would take as well. However, to provide context: I was recently standardizing Nullable annotations to jspecify in one of my repositories (using a simple type change) and encountered numerous conflicts where classes using Jakarta Nullable had fields that aren't present in jspecify. This is why running those recipes first may not be sufficient. I'll take a closer look at the ones you shared, and search for some reproducible examples.

[stefanodallapalma]

Ran the recipe just now, and saw a somewhat curious case:

diff --git a/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java b/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java

index d8e85f0..a1fa7c9 100644

--- a/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java

+++ b/src/main/java/org/openrewrite/java/dependencies/internal/VersionParser.java

@@ -16,6 +16,8 @@ org.openrewrite.staticanalysis.AnnotateNullableParameters

 

 package org.openrewrite.java.dependencies.internal;

 

+import org.jspecify.annotations.Nullable;

+

 import java.util.ArrayList;

 import java.util.List;

 import java.util.Map;

@@ -92,7 +94,7 @@

         }

 

         @Override

-        public boolean equals(Object obj) {

+        public boolean equals(@Nullable Object obj) {

             if (obj == this) {

                 return true;

             }

Any quick thoughts? Should we exclude any methods that are overrides and assume annotations are placed on the parent class/interface?

I can't see it in this diff, but usually equals have null checks. So, I'd say the recipe works as expected, but yes, we can think of excluding overrides. Maybe we can start with known methods like equals? Then see if it makes sense to exclude overridden methods as well?

Tbh, I've no clues how static analysis tools behave if annotations are defined in the parent class but not in its subclasses.

[timtebeek]

Thanks! For now I've pushed these two small fixes to reduce some of the noise otherwise seen after running this recipe:

• 0ad21be

We're aiming for clear improvements here, and can look at a more fine grained approach for overridden methods later if needed.

[timtebeek]

Another couple small fixes in 9deb55d ; we really ought to get you the tools to test these at scale. :)