Skip to content

Improve AnnotateNullableMethods to avoid duplicate annotations and annotation placement issues #845

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 2 commits into from
Apr 1, 2026
Merged

Improve AnnotateNullableMethods to avoid duplicate annotations and annotation placement issues #845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33c856a
fix annotations duplication and position
stedapa Mar 31, 2026
b5b355a
Align nullable detection with AnnotateNullableParameters
timtebeek Apr 1, 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
103 changes: 68 additions & 35 deletions src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,41 +20,54 @@
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.java.*;
import org.openrewrite.java.service.AnnotationService;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.TypeTree;
import org.openrewrite.staticanalysis.java.MoveFieldAnnotationToType;

import java.util.Arrays;
import java.util.Comparator;
import java.util.List;
import java.util.Optional;
import java.util.*;
import java.util.Locale;
import java.util.concurrent.atomic.AtomicBoolean;

@EqualsAndHashCode(callSuper = false)
@Value
public class AnnotateNullableMethods extends Recipe {

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

/**
* FQNs of nullable annotations that are meta-annotated with {@code @Target(TYPE_USE)}.
* These annotations can be positioned before the inner type of a nested type or on array brackets.
* All other nullable annotations are assumed to be declaration-target only and will remain
* as method-level 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
String nullableAnnotationClass;

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

String displayName = "Annotate methods which may return `null` with `@Nullable`";

String description = "Add `@Nullable` to non-private methods that may return `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 a `@Nullable` annotation and checks their return " +
"statements for potential null values. It also identifies known methods from standard libraries that may " +
"return null, such as methods from `Map`, `Queue`, `Deque`, `NavigableSet`, and `Spliterator`. " +
"The return of streams, or lambdas are not taken into account.";
"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. " +
"Methods that already carry a known nullable annotation (matched by simple name) are skipped to avoid duplication. " +
"This recipe scans for methods that do not already have a `@Nullable` annotation and checks their return " +
"statements for potential null values. It also identifies known methods from standard libraries that may " +
"return null, such as methods from `Map`, `Queue`, `Deque`, `NavigableSet`, and `Spliterator`. " +
"The return of streams, or lambdas are not taken into account.";

@Override
public Validated<Object> validate() {
Expand All @@ -68,14 +81,15 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
String fullyQualifiedName = nullableAnnotationClass != null ? nullableAnnotationClass : DEFAULT_NULLABLE_ANN_CLASS;
String fullyQualifiedPackage = fullyQualifiedName.substring(0, fullyQualifiedName.lastIndexOf('.'));
String simpleName = fullyQualifiedName.substring(fullyQualifiedName.lastIndexOf('.') + 1);
boolean isTypeUseAnnotation = TYPE_USE_NULLABLE_ANNOTATIONS.contains(fullyQualifiedName);

JavaIsoVisitor<ExecutionContext> javaIsoVisitor = new JavaIsoVisitor<ExecutionContext>() {
@Override
public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDeclaration, ExecutionContext ctx) {
if (!methodDeclaration.hasModifier(J.Modifier.Type.Public) ||
methodDeclaration.getMethodType() == null ||
methodDeclaration.getMethodType().getReturnType() instanceof JavaType.Primitive ||
service(AnnotationService.class).matches(getCursor(), new AnnotationMatcher("@" + fullyQualifiedName)) ||
hasNullableAnnotation(methodDeclaration.getReturnTypeExpression(), fullyQualifiedName)) {
hasAnyNullableAnnotation(methodDeclaration)) {
return methodDeclaration;
}

Expand All @@ -88,33 +102,52 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl
.build()
.apply(getCursor(), md.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName)));
doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(annotatedMethod));
doAfterVisit(new MoveFieldAnnotationToType(fullyQualifiedName).getVisitor());
return (J.MethodDeclaration) new NullableOnMethodReturnType().getVisitor()
.visitNonNull(annotatedMethod, ctx, getCursor().getParentTreeCursor());

// TYPE_USE annotations are moved to the return type position (e.g. public @Nullable String foo())
// and positioned before inner types of nested types (e.g. Outer.@Nullable Inner).
// Declaration-target annotations stay as method-level annotations (e.g. @CheckForNull \n public String foo()).
if (isTypeUseAnnotation) {
doAfterVisit(new MoveFieldAnnotationToType(fullyQualifiedName).getVisitor());
return (J.MethodDeclaration) new NullableOnMethodReturnType().getVisitor()
.visitNonNull(annotatedMethod, ctx, getCursor().getParentTreeCursor());
}
return annotatedMethod;
}
return md;
}

private boolean hasNullableAnnotation(@Nullable TypeTree returnType, String annotationFqn) {
if (returnType == null) {
return false;
}

// Check if the return type itself is annotated
if (service(AnnotationService.class).matches(new Cursor(null, returnType), new AnnotationMatcher("@" + annotationFqn))) {
return true;
}

// For array types, check if the element type is annotated
if (returnType instanceof J.ArrayType) {
J.ArrayType arrayType = (J.ArrayType) returnType;
if (arrayType.getElementType() instanceof J.AnnotatedType) {
return service(AnnotationService.class).matches(new Cursor(null, arrayType.getElementType()), new AnnotationMatcher("@" + annotationFqn));
/**
* Checks whether the method declaration already has any known nullable annotation,
* either as a method-level annotation or anywhere on the return type.
*/
private boolean hasAnyNullableAnnotation(J.MethodDeclaration methodDeclaration) {
// Check method-level annotations
for (J.Annotation annotation : methodDeclaration.getLeadingAnnotations()) {
if (isNullAnnotation(annotation)) {
return true;
}
}

// Scan the entire return type tree for any annotation with a known nullable simple name.
// Uses a TreeVisitor to reliably traverse all AST node types regardless of structure
// (J.AnnotatedType, J.FieldAccess with annotated names, J.ArrayType with bracket annotations, etc.)
TypeTree returnType = methodDeclaration.getReturnTypeExpression();
if (returnType != null) {
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
public J.Annotation visitAnnotation(J.Annotation annotation, AtomicBoolean found) {
if (isNullAnnotation(annotation)) {
found.set(true);
}
return annotation;
}
}.reduce(returnType, new AtomicBoolean(false), getCursor()).get();
}
return false;
}

private boolean isNullAnnotation(J.Annotation ann) {
return ann.getSimpleName().toLowerCase(Locale.ROOT).contains("null");
}
};
return Repeat.repeatUntilStable(javaIsoVisitor, 5);
}
Expand Down
112 changes: 110 additions & 2 deletions src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,43 @@ public class Test {
);
}

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

public class Test {
@CheckForNull
public String getString() {
return null;
}
}
"""
)
);
}

@Test
void methodReturnNullButArrayIsAlreadyAnnotated() {
rewriteRun(
//language=java
java(
"""
import org.jspecify.annotations.Nullable;

public class Test {
public String @Nullable [] getArray() {
return null;
}
}
"""
)
);
}

@Test
void methodDoesNotReturnNull() {
rewriteRun(
Expand Down Expand Up @@ -256,7 +293,37 @@ public String getString() {

public class Test {

public @Nullable String getString() {
@Nullable
public String getString() {
return null;
}
}
"""
)
);
}

@Test
void provideCustomNonTypeUseNullableAnnotationOption() {
rewriteRun(
spec -> spec.recipe(new AnnotateNullableMethods("javax.annotation.CheckForNull")),
//language=java
java(
"""
public class Test {

public String getString() {
return null;
}
}
""",
"""
import javax.annotation.CheckForNull;

public class Test {

@CheckForNull
public String getString() {
return null;
}
}
Expand Down Expand Up @@ -375,6 +442,46 @@ public class Foo {
);
}

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

import javax.annotation.CheckForNull;

public class Foo {
@CheckForNull
public B.C bar() {
return null;
}
}
"""
)
);
}

@Test
void nullableMethodsInvocationsWithDefaultNullableClass() {
rewriteRun(
Expand Down Expand Up @@ -445,7 +552,8 @@ public class Test {
return new Random().nextBoolean() ? "Not null" : null;
}

public @Nullable String getString() {
@Nullable
public String getString() {
return maybeNullString();
}
}
Expand Down
Loading