From 8bdc4204aec489b20dea3e72c0c1d7fad22346c9 Mon Sep 17 00:00:00 2001 From: stefanod Date: Mon, 30 Mar 2026 18:48:52 +0200 Subject: [PATCH 1/3] fix duplicate nullable annotations fix annotation position --- .../AnnotateNullableParameters.java | 112 +++++++++--- .../AnnotateNullableParametersTest.java | 160 ++++++++++++++++++ 2 files changed, 246 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java index 73390d7e9..d0bb879b0 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java @@ -41,8 +41,43 @@ public class AnnotateNullableParameters extends Recipe { private static final String DEFAULT_NULLABLE_ANN_CLASS = "org.jspecify.annotations.Nullable"; + /** + * Well-known nullable annotations from various frameworks. If any of these is already present + * on a parameter, we skip annotating it to avoid duplication such as {@code @CheckForNull @Nullable Type param}. + */ + private static final List KNOWN_NULLABLE_ANNOTATIONS = Arrays.asList( + "android.support.annotation.Nullable", + "androidx.annotation.Nullable", + "edu.umd.cs.findbugs.annotations.Nullable", + "edu.umd.cs.findbugs.annotations.CheckForNull", + "javax.annotation.Nullable", + "jakarta.annotation.Nullable", + "javax.annotation.CheckForNull", + "org.checkerframework.checker.nullness.qual.Nullable", + "org.checkerframework.checker.nullness.compatqual.NullableDecl", + "org.eclipse.jdt.annotation.Nullable", + "org.jetbrains.annotations.Nullable", + "org.jspecify.annotations.Nullable", + "org.springframework.lang.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 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 @@ -61,10 +96,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 validate() { @@ -108,31 +144,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 + 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))); } } @@ -151,18 +193,24 @@ private static boolean containsIdentifierByName(Collection 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. + * A parameter is a candidate if it doesn't already have the target nullable annotation + * or any other well-known nullable annotation. This prevents duplication such as + * {@code @CheckForNull @Nullable Type param} when a different nullable annotation is already present. * * @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 */ private List findCandidateParameters(J.MethodDeclaration md, String fqn) { + // Build the set of annotations to check: the target annotation plus all known nullable annotations + Set annotationsToCheck = new LinkedHashSet<>(KNOWN_NULLABLE_ANNOTATIONS); + annotationsToCheck.add(fqn); + List 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 (!hasAnyNullableAnnotation(vd, annotationsToCheck)) { candidates.add(vd); } } @@ -170,6 +218,18 @@ private List findCandidateParameters(J.MethodDeclaration return candidates; } + /** + * Checks whether the given variable declaration already has any of the specified nullable annotations. + */ + private static boolean hasAnyNullableAnnotation(J.VariableDeclarations vd, Set annotationFqns) { + for (String annotationFqn : annotationFqns) { + if (!FindAnnotations.find(vd, "@" + annotationFqn).isEmpty()) { + return true; + } + } + return false; + } + private Map buildIdentifierMap(List parameters) { Map identifierMap = new HashMap<>(); for (J.VariableDeclarations vd : parameters) { diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableParametersTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableParametersTest.java index af8f9348d..72010461a 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableParametersTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableParametersTest.java @@ -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; @@ -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; @@ -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( @@ -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( From b509b8d121c041902390e7898ae0383cb274e382 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 31 Mar 2026 11:26:14 +0200 Subject: [PATCH 2/3] Replace known nullable annotations list with simple name matching Instead of maintaining a hardcoded list of 13 nullable annotation FQNs, detect any annotation whose simple name contains "null" (case-insensitive). This automatically covers all frameworks and prevents duplication/conflicts. --- .../AnnotateNullableParameters.java | 68 +++++++------------ 1 file changed, 26 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java index d0bb879b0..afa959539 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java @@ -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; @@ -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; @@ -41,26 +41,6 @@ public class AnnotateNullableParameters extends Recipe { private static final String DEFAULT_NULLABLE_ANN_CLASS = "org.jspecify.annotations.Nullable"; - /** - * Well-known nullable annotations from various frameworks. If any of these is already present - * on a parameter, we skip annotating it to avoid duplication such as {@code @CheckForNull @Nullable Type param}. - */ - private static final List KNOWN_NULLABLE_ANNOTATIONS = Arrays.asList( - "android.support.annotation.Nullable", - "androidx.annotation.Nullable", - "edu.umd.cs.findbugs.annotations.Nullable", - "edu.umd.cs.findbugs.annotations.CheckForNull", - "javax.annotation.Nullable", - "jakarta.annotation.Nullable", - "javax.annotation.CheckForNull", - "org.checkerframework.checker.nullness.qual.Nullable", - "org.checkerframework.checker.nullness.compatqual.NullableDecl", - "org.eclipse.jdt.annotation.Nullable", - "org.jetbrains.annotations.Nullable", - "org.jspecify.annotations.Nullable", - "org.springframework.lang.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 @@ -128,7 +108,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl return md; } - Map candidateIdentifiers = buildIdentifierMap(findCandidateParameters(md, fullyQualifiedName)); + Map candidateIdentifiers = buildIdentifierMap(findCandidateParameters(md)); Set nullCheckedIdentifiers = new NullCheckVisitor(candidateIdentifiers.values(), additionalNullCheckingMethods) .reduce(md.getBody(), new HashSet<>()); @@ -193,24 +173,16 @@ private static boolean containsIdentifierByName(Collection 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 - * or any other well-known nullable annotation. This prevents duplication such as - * {@code @CheckForNull @Nullable Type param} when a different nullable annotation is already present. - * - * @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 findCandidateParameters(J.MethodDeclaration md, String fqn) { - // Build the set of annotations to check: the target annotation plus all known nullable annotations - Set annotationsToCheck = new LinkedHashSet<>(KNOWN_NULLABLE_ANNOTATIONS); - annotationsToCheck.add(fqn); - + private static List findCandidateParameters(J.MethodDeclaration md) { List candidates = new ArrayList<>(); for (Statement parameter : md.getParameters()) { if (parameter instanceof J.VariableDeclarations) { J.VariableDeclarations vd = (J.VariableDeclarations) parameter; - if (!hasAnyNullableAnnotation(vd, annotationsToCheck)) { + if (!hasNullRelatedAnnotation(vd)) { candidates.add(vd); } } @@ -218,16 +190,28 @@ private List findCandidateParameters(J.MethodDeclaration return candidates; } - /** - * Checks whether the given variable declaration already has any of the specified nullable annotations. - */ - private static boolean hasAnyNullableAnnotation(J.VariableDeclarations vd, Set annotationFqns) { - for (String annotationFqn : annotationFqns) { - if (!FindAnnotations.find(vd, "@" + annotationFqn).isEmpty()) { + private static boolean hasNullRelatedAnnotation(J.VariableDeclarations vd) { + for (J.Annotation ann : vd.getLeadingAnnotations()) { + if (isNullAnnotation(ann)) { return true; } } - return false; + // Also check type-use annotations on the type expression (e.g., String @Nullable[] or Outer.@Nullable Inner) + AtomicBoolean found = new AtomicBoolean(false); + new JavaIsoVisitor() { + @Override + public J.Annotation visitAnnotation(J.Annotation annotation, AtomicBoolean f) { + if (isNullAnnotation(annotation)) { + f.set(true); + } + return annotation; + } + }.visit(vd.getTypeExpression(), found); + return found.get(); + } + + private static boolean isNullAnnotation(J.Annotation ann) { + return ann.getSimpleName().toLowerCase(Locale.ROOT).contains("null"); } private Map buildIdentifierMap(List parameters) { From 9c5414cc984e9a8b83f54bb9ffaccabdc0102f9e Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 31 Mar 2026 13:46:19 +0200 Subject: [PATCH 3/3] Use .reduce for immediate return in hasNullRelatedAnnotation --- .../staticanalysis/AnnotateNullableParameters.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java index afa959539..3d09022e4 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java @@ -197,8 +197,7 @@ private static boolean hasNullRelatedAnnotation(J.VariableDeclarations vd) { } } // Also check type-use annotations on the type expression (e.g., String @Nullable[] or Outer.@Nullable Inner) - AtomicBoolean found = new AtomicBoolean(false); - new JavaIsoVisitor() { + return new JavaIsoVisitor() { @Override public J.Annotation visitAnnotation(J.Annotation annotation, AtomicBoolean f) { if (isNullAnnotation(annotation)) { @@ -206,8 +205,7 @@ public J.Annotation visitAnnotation(J.Annotation annotation, AtomicBoolean f) { } return annotation; } - }.visit(vd.getTypeExpression(), found); - return found.get(); + }.reduce(vd.getTypeExpression(), new AtomicBoolean(false)).get(); } private static boolean isNullAnnotation(J.Annotation ann) {