Skip to content

Commit 3f3cd50

Browse files
stefanodallapalmastedapatimtebeek
authored
Improve AnnotateNullableParameters to avoid duplicate annotations and annotation placement issues (#843)
* fix duplicate nullable annotations fix annotation position * 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. * Use .reduce for immediate return in hasNullRelatedAnnotation --------- Co-authored-by: stefanod <stefano.dallapalma@adyen.com> Co-authored-by: Tim te Beek <tim@moderne.io>
1 parent 0a3f001 commit 3f3cd50

2 files changed

Lines changed: 235 additions & 33 deletions

File tree

‎src/main/java/org/openrewrite/staticanalysis/AnnotateNullableParameters.java‎

Lines changed: 75 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import org.openrewrite.*;
2222
import org.openrewrite.internal.ListUtils;
2323
import org.openrewrite.java.*;
24-
import org.openrewrite.java.search.FindAnnotations;
2524
import org.openrewrite.java.search.SemanticallyEqual;
2625
import org.openrewrite.java.tree.Expression;
2726
import org.openrewrite.java.tree.J;
@@ -30,6 +29,7 @@
3029
import org.openrewrite.staticanalysis.java.MoveFieldAnnotationToType;
3130

3231
import java.util.*;
32+
import java.util.concurrent.atomic.AtomicBoolean;
3333

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

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

44+
/**
45+
* Subset of nullable annotations that are meta-annotated with {@code @Target(TYPE_USE)}.
46+
* Only these annotations can be positioned before the inner type of nested type
47+
* (e.g. {@code Outer.@Nullable Inner}) or on array brackets (e.g. {@code String @Nullable[]}).
48+
* Declaration-target annotations like {@code @CheckForNull} must remain as leading annotations.
49+
*/
50+
private static final Set<String> TYPE_USE_NULLABLE_ANNOTATIONS = new HashSet<>(Arrays.asList(
51+
"jakarta.annotation.Nullable",
52+
"org.checkerframework.checker.nullness.qual.Nullable",
53+
"org.eclipse.jdt.annotation.Nullable",
54+
"org.jspecify.annotations.Nullable"
55+
));
56+
4457
@Option(displayName = "`@Nullable` annotation class",
45-
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`",
58+
description = "The fully qualified name of the @Nullable annotation to add. " +
59+
"Both `@Target(TYPE_USE)` and declaration annotations (e.g. `javax.annotation.CheckForNull`) are supported. " +
60+
"Defaults to `org.jspecify.annotations.Nullable`.",
4661
example = "org.jspecify.annotations.Nullable",
4762
required = false)
4863
@Nullable
@@ -61,10 +76,11 @@ public class AnnotateNullableParameters extends Recipe {
6176
String displayName = "Annotate null-checked method parameters with `@Nullable`";
6277

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

6985
@Override
7086
public Validated<Object> validate() {
@@ -92,7 +108,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl
92108
return md;
93109
}
94110

95-
Map<J.VariableDeclarations, J.Identifier> candidateIdentifiers = buildIdentifierMap(findCandidateParameters(md, fullyQualifiedName));
111+
Map<J.VariableDeclarations, J.Identifier> candidateIdentifiers = buildIdentifierMap(findCandidateParameters(md));
96112
Set<J.Identifier> nullCheckedIdentifiers = new NullCheckVisitor(candidateIdentifiers.values(), additionalNullCheckingMethods)
97113
.reduce(md.getBody(), new HashSet<>());
98114

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

111-
// For array types, move annotation from leading annotations to array brackets
112-
if (annotated.getTypeExpression() instanceof J.ArrayType) {
113-
// Find the annotation we just added
114-
J.Annotation nullableAnnotation = null;
115-
for (J.Annotation ann : annotated.getLeadingAnnotations()) {
116-
if (ann.getSimpleName().equals(simpleName)) {
117-
nullableAnnotation = ann;
118-
break;
127+
// TYPE_USE annotations can be positioned on array brackets and before inner types
128+
// of nested types; declaration-target annotations stay as leading annotations
129+
if (TYPE_USE_NULLABLE_ANNOTATIONS.contains(fullyQualifiedName)) {
130+
// For array types, move annotation from leading annotations to array brackets
131+
if (annotated.getTypeExpression() instanceof J.ArrayType) {
132+
// Find the annotation we just added
133+
J.Annotation nullableAnnotation = null;
134+
for (J.Annotation ann : annotated.getLeadingAnnotations()) {
135+
if (ann.getSimpleName().equals(simpleName)) {
136+
nullableAnnotation = ann;
137+
break;
138+
}
119139
}
120-
}
121-
if (nullableAnnotation != null) {
122-
J.Annotation finalAnnotation = nullableAnnotation;
123-
J.ArrayType arrayType = (J.ArrayType) annotated.getTypeExpression();
124-
annotated = annotated.withLeadingAnnotations(ListUtils.map(annotated.getLeadingAnnotations(),
125-
a -> a == finalAnnotation ? null : a));
126-
arrayType = arrayType.withAnnotations(singletonList(finalAnnotation.withPrefix(Space.SINGLE_SPACE)));
127-
if (annotated.getLeadingAnnotations().isEmpty()) {
128-
arrayType = arrayType.withPrefix(Space.EMPTY);
140+
if (nullableAnnotation != null) {
141+
J.Annotation finalAnnotation = nullableAnnotation;
142+
J.ArrayType arrayType = (J.ArrayType) annotated.getTypeExpression();
143+
annotated = annotated.withLeadingAnnotations(ListUtils.map(annotated.getLeadingAnnotations(),
144+
a -> a == finalAnnotation ? null : a));
145+
arrayType = arrayType.withAnnotations(singletonList(finalAnnotation.withPrefix(Space.SINGLE_SPACE)));
146+
if (annotated.getLeadingAnnotations().isEmpty()) {
147+
arrayType = arrayType.withPrefix(Space.EMPTY);
148+
}
149+
annotated = annotated.withTypeExpression(arrayType);
129150
}
130-
annotated = annotated.withTypeExpression(arrayType);
131151
}
152+
153+
// For nested types, move annotation before the inner type (e.g. Outer.@Nullable Inner)
154+
doAfterVisit(new MoveFieldAnnotationToType(fullyQualifiedName).getVisitor());
132155
}
133156

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

152174
/**
153175
* Finds method parameters that are candidates for @Nullable annotation.
154-
* A parameter is a candidate if it doesn't already have the target nullable annotation.
155-
*
156-
* @param md the method declaration to analyze
157-
* @param fqn the fully qualified name of the nullable annotation
158-
* @return list of parameter declarations that could receive the annotation
176+
* A parameter is a candidate if it doesn't already have any annotation whose simple name
177+
* contains "null" (case-insensitive). This catches @Nullable, @CheckForNull, @NullableDecl,
178+
* @NonNull, @NotNull, etc. from any framework, preventing duplication and conflicts.
159179
*/
160-
private List<J.VariableDeclarations> findCandidateParameters(J.MethodDeclaration md, String fqn) {
180+
private static List<J.VariableDeclarations> findCandidateParameters(J.MethodDeclaration md) {
161181
List<J.VariableDeclarations> candidates = new ArrayList<>();
162182
for (Statement parameter : md.getParameters()) {
163183
if (parameter instanceof J.VariableDeclarations) {
164184
J.VariableDeclarations vd = (J.VariableDeclarations) parameter;
165-
if (FindAnnotations.find(vd, "@" + fqn).isEmpty()) {
185+
if (!hasNullRelatedAnnotation(vd)) {
166186
candidates.add(vd);
167187
}
168188
}
169189
}
170190
return candidates;
171191
}
172192

193+
private static boolean hasNullRelatedAnnotation(J.VariableDeclarations vd) {
194+
for (J.Annotation ann : vd.getLeadingAnnotations()) {
195+
if (isNullAnnotation(ann)) {
196+
return true;
197+
}
198+
}
199+
// Also check type-use annotations on the type expression (e.g., String @Nullable[] or Outer.@Nullable Inner)
200+
return new JavaIsoVisitor<AtomicBoolean>() {
201+
@Override
202+
public J.Annotation visitAnnotation(J.Annotation annotation, AtomicBoolean f) {
203+
if (isNullAnnotation(annotation)) {
204+
f.set(true);
205+
}
206+
return annotation;
207+
}
208+
}.reduce(vd.getTypeExpression(), new AtomicBoolean(false)).get();
209+
}
210+
211+
private static boolean isNullAnnotation(J.Annotation ann) {
212+
return ann.getSimpleName().toLowerCase(Locale.ROOT).contains("null");
213+
}
214+
173215
private Map<J.VariableDeclarations, J.Identifier> buildIdentifierMap(List<J.VariableDeclarations> parameters) {
174216
Map<J.VariableDeclarations, J.Identifier> identifierMap = new HashMap<>();
175217
for (J.VariableDeclarations vd : parameters) {

‎src/test/java/org/openrewrite/staticanalysis/AnnotateNullableParametersTest.java‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package org.openrewrite.staticanalysis;
1717

18+
import org.jspecify.annotations.Nullable;
1819
import org.junit.jupiter.api.Nested;
1920
import org.junit.jupiter.api.Test;
2021
import org.junit.jupiter.params.ParameterizedTest;
@@ -50,6 +51,36 @@ public void defaults(RecipeSpec spec) {
5051
public @interface Nullable {}
5152
""",
5253
// language=java
54+
"""
55+
package javax.annotation;
56+
public @interface CheckForNull {}
57+
""",
58+
// language=java
59+
"""
60+
package javax.annotation;
61+
public @interface Nullable {}
62+
""",
63+
// language=java
64+
"""
65+
package edu.umd.cs.findbugs.annotations;
66+
public @interface Nullable {}
67+
""",
68+
// language=java
69+
"""
70+
package edu.umd.cs.findbugs.annotations;
71+
public @interface CheckForNull {}
72+
""",
73+
// language=java
74+
"""
75+
package org.jetbrains.annotations;
76+
public @interface Nullable {}
77+
""",
78+
// language=java
79+
"""
80+
package org.springframework.lang;
81+
public @interface Nullable {}
82+
""",
83+
// language=java
5384
"""
5485
package org.apache.commons.lang3;
5586
@@ -233,6 +264,46 @@ public PersonBuilder setName(B.@Nullable C name) {
233264
);
234265
}
235266

267+
@Test
268+
void nestedTypeWithNonTypeUseAnnotation() {
269+
rewriteRun(
270+
spec -> spec.recipe(new AnnotateNullableParameters("javax.annotation.CheckForNull", null)),
271+
//language=java
272+
java(
273+
"package a; public class B { public static class C {} }",
274+
SourceSpec::skip
275+
),
276+
//language=java
277+
java(
278+
"""
279+
import a.B;
280+
public class PersonBuilder {
281+
public PersonBuilder setName(B.C name) {
282+
if (name == null) {
283+
return this;
284+
}
285+
return this;
286+
}
287+
}
288+
""",
289+
"""
290+
import a.B;
291+
292+
import javax.annotation.CheckForNull;
293+
294+
public class PersonBuilder {
295+
public PersonBuilder setName(@CheckForNull B.C name) {
296+
if (name == null) {
297+
return this;
298+
}
299+
return this;
300+
}
301+
}
302+
"""
303+
)
304+
);
305+
}
306+
236307
@Test
237308
void finalVariableSpacing() {
238309
rewriteRun(
@@ -342,6 +413,95 @@ public PersonBuilder setName(@Nullable String first, @Nullable String last) {
342413
);
343414
}
344415

416+
@CsvSource({
417+
", javax.annotation.CheckForNull",
418+
", javax.annotation.Nullable",
419+
", edu.umd.cs.findbugs.annotations.Nullable",
420+
", edu.umd.cs.findbugs.annotations.CheckForNull",
421+
", org.jetbrains.annotations.Nullable",
422+
", org.springframework.lang.Nullable",
423+
"org.openrewrite.jgit.annotations.Nullable, javax.annotation.CheckForNull",
424+
"org.openrewrite.jgit.annotations.Nullable, javax.annotation.Nullable",
425+
"org.openrewrite.jgit.annotations.Nullable, edu.umd.cs.findbugs.annotations.Nullable",
426+
"org.openrewrite.jgit.annotations.Nullable, edu.umd.cs.findbugs.annotations.CheckForNull",
427+
"org.openrewrite.jgit.annotations.Nullable, org.jetbrains.annotations.Nullable",
428+
"org.openrewrite.jgit.annotations.Nullable, org.springframework.lang.Nullable",
429+
})
430+
@ParameterizedTest
431+
void parameterAlreadyAnnotatedWithKnownNullableAnnotation(@Nullable String targetAnnotation, String existingAnnotation) {
432+
String simpleAnnotation = existingAnnotation.substring(existingAnnotation.lastIndexOf('.') + 1);
433+
rewriteRun(
434+
spec -> spec.recipe(new AnnotateNullableParameters(targetAnnotation, null)),
435+
//language=java
436+
java(
437+
"""
438+
import %s;
439+
440+
public class PersonBuilder {
441+
private String name = "Unknown";
442+
443+
public PersonBuilder setName(@%s String name) {
444+
if (name == null) {
445+
return this;
446+
}
447+
this.name = name.substring(0, 1).toUpperCase() + name.substring(1);
448+
return this;
449+
}
450+
}
451+
""".formatted(existingAnnotation, simpleAnnotation)
452+
)
453+
);
454+
}
455+
456+
@Test
457+
void mixedAnnotationsOnlyAnnotatesUnannotatedParams() {
458+
rewriteRun(
459+
//language=java
460+
java(
461+
"""
462+
import javax.annotation.CheckForNull;
463+
464+
public class PersonBuilder {
465+
private String first = "Unknown";
466+
private String last = "Unknown";
467+
468+
public PersonBuilder setName(@CheckForNull String first, String last) {
469+
if (first != null) {
470+
this.first = first;
471+
}
472+
if (last == null) {
473+
return this;
474+
}
475+
this.last = last;
476+
return this;
477+
}
478+
}
479+
""",
480+
"""
481+
import org.jspecify.annotations.Nullable;
482+
483+
import javax.annotation.CheckForNull;
484+
485+
public class PersonBuilder {
486+
private String first = "Unknown";
487+
private String last = "Unknown";
488+
489+
public PersonBuilder setName(@CheckForNull String first, @Nullable String last) {
490+
if (first != null) {
491+
this.first = first;
492+
}
493+
if (last == null) {
494+
return this;
495+
}
496+
this.last = last;
497+
return this;
498+
}
499+
}
500+
"""
501+
)
502+
);
503+
}
504+
345505
@Test
346506
void parameterIsNotNullChecked() {
347507
rewriteRun(

0 commit comments

Comments
 (0)
⚡