Skip to content

Improve AnnotateNullableParameters to avoid duplicate annotations and annotation placement issues #843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
timtebeek merged 3 commits into from
Mar 31, 2026
Merged

Improve AnnotateNullableParameters to avoid duplicate annotations and annotation placement issues #843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8bdc420
fix duplicate nullable annotations
stedapa Mar 30, 2026
b509b8d
Replace known nullable annotations list with simple name matching
timtebeek Mar 31, 2026
9c5414c
Use .reduce for immediate return in hasNullRelatedAnnotation
timtebeek Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 75 additions & 33 deletions src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.openrewrite.*;
import org.openrewrite.internal.ListUtils;
import org.openrewrite.java.*;
import org.openrewrite.java.search.FindAnnotations;
import org.openrewrite.java.search.SemanticallyEqual;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
Expand All @@ -30,6 +29,7 @@
import org.openrewrite.staticanalysis.java.MoveFieldAnnotationToType;

import java.util.*;
import java.util.concurrent.atomic.AtomicBoolean;

import static java.util.Collections.singletonList;
import static java.util.Comparator.comparing;
Expand All @@ -41,8 +41,23 @@ public class AnnotateNullableParameters extends Recipe {

private static final String DEFAULT_NULLABLE_ANN_CLASS = "org.jspecify.annotations.Nullable";

/**
* Subset of nullable annotations that are meta-annotated with {@code @Target(TYPE_USE)}.
* Only these annotations can be positioned before the inner type of nested type
* (e.g. {@code Outer.@Nullable Inner}) or on array brackets (e.g. {@code String @Nullable[]}).
* Declaration-target annotations like {@code @CheckForNull} must remain as leading annotations.
*/
private static final Set<String> TYPE_USE_NULLABLE_ANNOTATIONS = new HashSet<>(Arrays.asList(
"jakarta.annotation.Nullable",
"org.checkerframework.checker.nullness.qual.Nullable",
"org.eclipse.jdt.annotation.Nullable",
"org.jspecify.annotations.Nullable"
));

@Option(displayName = "`@Nullable` annotation class",
description = "The fully qualified name of the @Nullable annotation. The annotation should be meta annotated with `@Target(TYPE_USE)`. Defaults to `org.jspecify.annotations.Nullable`",
description = "The fully qualified name of the @Nullable annotation to add. " +
"Both `@Target(TYPE_USE)` and declaration annotations (e.g. `javax.annotation.CheckForNull`) are supported. " +
"Defaults to `org.jspecify.annotations.Nullable`.",
example = "org.jspecify.annotations.Nullable",
required = false)
@Nullable
Expand All @@ -61,10 +76,11 @@ public class AnnotateNullableParameters extends Recipe {
String displayName = "Annotate null-checked method parameters with `@Nullable`";

String description = "Add `@Nullable` to parameters of public methods that are explicitly checked for `null`. " +
"By default `org.jspecify.annotations.Nullable` is used, but through the `nullableAnnotationClass` option a custom annotation can be provided. " +
"When providing a custom `nullableAnnotationClass` that annotation should be meta annotated with `@Target(TYPE_USE)`. " +
"This recipe scans for methods that do not already have parameters annotated with `@Nullable` annotation and checks their usages " +
"for potential null checks. Additional null-checking methods can be specified via the `additionalNullCheckingMethods` option.";
"By default `org.jspecify.annotations.Nullable` is used, but through the `nullableAnnotationClass` option a custom annotation can be provided. " +
"Both `@Target(TYPE_USE)` and declaration annotations (e.g. `javax.annotation.CheckForNull`) are supported. " +
"Parameters that already carry a known nullable annotation are skipped to avoid duplication. " +
"This recipe scans for methods that do not already have parameters annotated with a nullable annotation and checks their usages " +
"for potential null checks. Additional null-checking methods can be specified via the `additionalNullCheckingMethods` option.";

@Override
public Validated<Object> validate() {
Expand Down Expand Up @@ -92,7 +108,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl
return md;
}

Map<J.VariableDeclarations, J.Identifier> candidateIdentifiers = buildIdentifierMap(findCandidateParameters(md, fullyQualifiedName));
Map<J.VariableDeclarations, J.Identifier> candidateIdentifiers = buildIdentifierMap(findCandidateParameters(md));
Set<J.Identifier> nullCheckedIdentifiers = new NullCheckVisitor(candidateIdentifiers.values(), additionalNullCheckingMethods)
.reduce(md.getBody(), new HashSet<>());

Expand All @@ -108,31 +124,37 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl
.apply(new Cursor(getCursor(), vd),
vd.getCoordinates().addAnnotation(comparing(J.Annotation::getSimpleName)));

// For array types, move annotation from leading annotations to array brackets
if (annotated.getTypeExpression() instanceof J.ArrayType) {
// Find the annotation we just added
J.Annotation nullableAnnotation = null;
for (J.Annotation ann : annotated.getLeadingAnnotations()) {
if (ann.getSimpleName().equals(simpleName)) {
nullableAnnotation = ann;
break;
// TYPE_USE annotations can be positioned on array brackets and before inner types
// of nested types; declaration-target annotations stay as leading annotations
if (TYPE_USE_NULLABLE_ANNOTATIONS.contains(fullyQualifiedName)) {
// For array types, move annotation from leading annotations to array brackets
Comment on lines +127 to +130
Copy link
Copy Markdown
Member

@timtebeek timtebeek Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, thanks. Don't see an immediate good way to avoid the existing collection here, as the new fullyQualifiedName might not be on the recipe classpath.

if (annotated.getTypeExpression() instanceof J.ArrayType) {
// Find the annotation we just added
J.Annotation nullableAnnotation = null;
for (J.Annotation ann : annotated.getLeadingAnnotations()) {
if (ann.getSimpleName().equals(simpleName)) {
nullableAnnotation = ann;
break;
}
}
}
if (nullableAnnotation != null) {
J.Annotation finalAnnotation = nullableAnnotation;
J.ArrayType arrayType = (J.ArrayType) annotated.getTypeExpression();
annotated = annotated.withLeadingAnnotations(ListUtils.map(annotated.getLeadingAnnotations(),
a -> a == finalAnnotation ? null : a));
arrayType = arrayType.withAnnotations(singletonList(finalAnnotation.withPrefix(Space.SINGLE_SPACE)));
if (annotated.getLeadingAnnotations().isEmpty()) {
arrayType = arrayType.withPrefix(Space.EMPTY);
if (nullableAnnotation != null) {
J.Annotation finalAnnotation = nullableAnnotation;
J.ArrayType arrayType = (J.ArrayType) annotated.getTypeExpression();
annotated = annotated.withLeadingAnnotations(ListUtils.map(annotated.getLeadingAnnotations(),
a -> a == finalAnnotation ? null : a));
arrayType = arrayType.withAnnotations(singletonList(finalAnnotation.withPrefix(Space.SINGLE_SPACE)));
if (annotated.getLeadingAnnotations().isEmpty()) {
arrayType = arrayType.withPrefix(Space.EMPTY);
}
annotated = annotated.withTypeExpression(arrayType);
}
annotated = annotated.withTypeExpression(arrayType);
}

// For nested types, move annotation before the inner type (e.g. Outer.@Nullable Inner)
doAfterVisit(new MoveFieldAnnotationToType(fullyQualifiedName).getVisitor());
}

doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(annotated));
doAfterVisit(new MoveFieldAnnotationToType(fullyQualifiedName).getVisitor());
return annotated.withModifiers(ListUtils.mapFirst(annotated.getModifiers(), first -> first.withPrefix(Space.SINGLE_SPACE)));
}
}
Expand All @@ -151,25 +173,45 @@ private static boolean containsIdentifierByName(Collection<J.Identifier> identif

/**
* Finds method parameters that are candidates for @Nullable annotation.
* A parameter is a candidate if it doesn't already have the target nullable annotation.
*
* @param md the method declaration to analyze
* @param fqn the fully qualified name of the nullable annotation
* @return list of parameter declarations that could receive the annotation
* A parameter is a candidate if it doesn't already have any annotation whose simple name
* contains "null" (case-insensitive). This catches @Nullable, @CheckForNull, @NullableDecl,
* @NonNull, @NotNull, etc. from any framework, preventing duplication and conflicts.
*/
private List<J.VariableDeclarations> findCandidateParameters(J.MethodDeclaration md, String fqn) {
private static List<J.VariableDeclarations> findCandidateParameters(J.MethodDeclaration md) {
List<J.VariableDeclarations> candidates = new ArrayList<>();
for (Statement parameter : md.getParameters()) {
if (parameter instanceof J.VariableDeclarations) {
J.VariableDeclarations vd = (J.VariableDeclarations) parameter;
if (FindAnnotations.find(vd, "@" + fqn).isEmpty()) {
if (!hasNullRelatedAnnotation(vd)) {
candidates.add(vd);
}
}
}
return candidates;
}

private static boolean hasNullRelatedAnnotation(J.VariableDeclarations vd) {
for (J.Annotation ann : vd.getLeadingAnnotations()) {
if (isNullAnnotation(ann)) {
return true;
}
}
// Also check type-use annotations on the type expression (e.g., String @Nullable[] or Outer.@Nullable Inner)
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
public J.Annotation visitAnnotation(J.Annotation annotation, AtomicBoolean f) {
if (isNullAnnotation(annotation)) {
f.set(true);
}
return annotation;
}
}.reduce(vd.getTypeExpression(), new AtomicBoolean(false)).get();
}

private static boolean isNullAnnotation(J.Annotation ann) {
return ann.getSimpleName().toLowerCase(Locale.ROOT).contains("null");
}

private Map<J.VariableDeclarations, J.Identifier> buildIdentifierMap(List<J.VariableDeclarations> parameters) {
Map<J.VariableDeclarations, J.Identifier> identifierMap = new HashMap<>();
for (J.VariableDeclarations vd : parameters) {
Expand Down
160 changes: 160 additions & 0 deletions src/test/java/org/openrewrite/staticanalysis/AnnotateNullableParametersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.openrewrite.staticanalysis;

import org.jspecify.annotations.Nullable;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
Expand Down Expand Up @@ -50,6 +51,36 @@ public void defaults(RecipeSpec spec) {
public @interface Nullable {}
""",
// language=java
"""
package javax.annotation;
public @interface CheckForNull {}
""",
// language=java
"""
package javax.annotation;
public @interface Nullable {}
""",
// language=java
"""
package edu.umd.cs.findbugs.annotations;
public @interface Nullable {}
""",
// language=java
"""
package edu.umd.cs.findbugs.annotations;
public @interface CheckForNull {}
""",
// language=java
"""
package org.jetbrains.annotations;
public @interface Nullable {}
""",
// language=java
"""
package org.springframework.lang;
public @interface Nullable {}
""",
// language=java
"""
package org.apache.commons.lang3;

Expand Down Expand Up @@ -233,6 +264,46 @@ public PersonBuilder setName(B.@Nullable C name) {
);
}

@Test
void nestedTypeWithNonTypeUseAnnotation() {
rewriteRun(
spec -> spec.recipe(new AnnotateNullableParameters("javax.annotation.CheckForNull", null)),
//language=java
java(
"package a; public class B { public static class C {} }",
SourceSpec::skip
),
//language=java
java(
"""
import a.B;
public class PersonBuilder {
public PersonBuilder setName(B.C name) {
if (name == null) {
return this;
}
return this;
}
}
""",
"""
import a.B;

import javax.annotation.CheckForNull;

public class PersonBuilder {
public PersonBuilder setName(@CheckForNull B.C name) {
if (name == null) {
return this;
}
return this;
}
}
"""
)
);
}

@Test
void finalVariableSpacing() {
rewriteRun(
Expand Down Expand Up @@ -342,6 +413,95 @@ public PersonBuilder setName(@Nullable String first, @Nullable String last) {
);
}

@CsvSource({
", javax.annotation.CheckForNull",
", javax.annotation.Nullable",
", edu.umd.cs.findbugs.annotations.Nullable",
", edu.umd.cs.findbugs.annotations.CheckForNull",
", org.jetbrains.annotations.Nullable",
", org.springframework.lang.Nullable",
"org.openrewrite.jgit.annotations.Nullable, javax.annotation.CheckForNull",
"org.openrewrite.jgit.annotations.Nullable, javax.annotation.Nullable",
"org.openrewrite.jgit.annotations.Nullable, edu.umd.cs.findbugs.annotations.Nullable",
"org.openrewrite.jgit.annotations.Nullable, edu.umd.cs.findbugs.annotations.CheckForNull",
"org.openrewrite.jgit.annotations.Nullable, org.jetbrains.annotations.Nullable",
"org.openrewrite.jgit.annotations.Nullable, org.springframework.lang.Nullable",
})
@ParameterizedTest
void parameterAlreadyAnnotatedWithKnownNullableAnnotation(@Nullable String targetAnnotation, String existingAnnotation) {
String simpleAnnotation = existingAnnotation.substring(existingAnnotation.lastIndexOf('.') + 1);
rewriteRun(
spec -> spec.recipe(new AnnotateNullableParameters(targetAnnotation, null)),
//language=java
java(
"""
import %s;

public class PersonBuilder {
private String name = "Unknown";

public PersonBuilder setName(@%s String name) {
if (name == null) {
return this;
}
this.name = name.substring(0, 1).toUpperCase() + name.substring(1);
return this;
}
}
""".formatted(existingAnnotation, simpleAnnotation)
)
);
}

@Test
void mixedAnnotationsOnlyAnnotatesUnannotatedParams() {
rewriteRun(
//language=java
java(
"""
import javax.annotation.CheckForNull;

public class PersonBuilder {
private String first = "Unknown";
private String last = "Unknown";

public PersonBuilder setName(@CheckForNull String first, String last) {
if (first != null) {
this.first = first;
}
if (last == null) {
return this;
}
this.last = last;
return this;
}
}
""",
"""
import org.jspecify.annotations.Nullable;

import javax.annotation.CheckForNull;

public class PersonBuilder {
private String first = "Unknown";
private String last = "Unknown";

public PersonBuilder setName(@CheckForNull String first, @Nullable String last) {
if (first != null) {
this.first = first;
}
if (last == null) {
return this;
}
this.last = last;
return this;
}
}
"""
)
);
}

@Test
void parameterIsNotNullChecked() {
rewriteRun(
Expand Down
Loading