diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java index 73390d7e9..3d09022e4 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,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 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 +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 validate() { @@ -92,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<>()); @@ -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 + 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 +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. - * - * @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) { + 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 (FindAnnotations.find(vd, "@" + fqn).isEmpty()) { + if (!hasNullRelatedAnnotation(vd)) { candidates.add(vd); } } @@ -170,6 +190,28 @@ private List findCandidateParameters(J.MethodDeclaration 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() { + @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 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(