From 8bf01d4b2cbb96269a9cd1096e3edb9ac12cc406 Mon Sep 17 00:00:00 2001 From: Niels de Bruin Date: Mon, 14 Oct 2024 10:41:22 +0200 Subject: [PATCH 01/18] Add AnnotateNullableMethodsRecipe --- .../AnnotateNullableMethods.java | 183 ++++++++++++++++ .../AnnotateNullableMethodsTest.java | 198 ++++++++++++++++++ 2 files changed, 381 insertions(+) create mode 100644 src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java create mode 100644 src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java new file mode 100644 index 0000000000..4b94bfdf4e --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -0,0 +1,183 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import lombok.AllArgsConstructor; +import org.openrewrite.Cursor; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +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 java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + +public class AnnotateNullableMethods extends Recipe { + private static final String NULLABLE_ANN_PACKAGE = "org.jspecify.annotations"; + private static final String NULLABLE_ANN_CLASS = NULLABLE_ANN_PACKAGE + ".Nullable"; + private static final AnnotationMatcher NULLABLE_ANNOTATION_MATCHER = + new AnnotationMatcher("@" + NULLABLE_ANN_CLASS); + private static final String NULLABLE_ANN_ARTIFACT = "jspecify"; + + @Override + public String getDisplayName() { + return "Annotate methods which may return null with @Nullable"; + } + + @Override + public String getDescription() { + return "Automatically adds the @org.jspecify.annotation.Nullable to non-private methods" + + "that may return null. 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 TreeVisitor getVisitor() { + return new AnnotateNullableMethodsVisitor(); + } + + private static class AnnotateNullableMethodsVisitor extends JavaIsoVisitor { + @Override + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { + if (md.hasModifier(J.Modifier.Type.Private)) { + return md; + } + + if (md.getMethodType() != null && md.getMethodType().getReturnType() instanceof JavaType.Primitive) { + return md; + } + + if (service(AnnotationService.class).matches(getCursor(), NULLABLE_ANNOTATION_MATCHER)) { + return md; + } + + md = super.visitMethodDeclaration(md, ctx); + updateCursor(md); + + if (FindNullableReturnStatements.find(md).get()) { + maybeAddImport(NULLABLE_ANN_CLASS); + return JavaTemplate.builder("@Nullable") + .imports(NULLABLE_ANN_CLASS) + .javaParser(JavaParser.fromJavaVersion().classpath(NULLABLE_ANN_ARTIFACT)) + .build() + .apply(getCursor(), md.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))); + } + return md; + } + } + + @AllArgsConstructor + private static class FindNullableReturnStatements extends JavaIsoVisitor { + private static final List KNOWN_NULLABLE_METHODS = getMatchersKnownNullableMethods(); + + static AtomicBoolean find(J subtree) { + return new FindNullableReturnStatements().reduce(subtree, new AtomicBoolean()); + } + + private static List getMatchersKnownNullableMethods() { + List matchers = new ArrayList<>(); + + matchers.add(new MethodMatcher("java.util.Map compute(..)")); + matchers.add(new MethodMatcher("java.util.Map computeIfAbsent(..)")); + matchers.add(new MethodMatcher("java.util.Map computeIfPresent(..)")); + matchers.add(new MethodMatcher("java.util.Map get(..)")); + matchers.add(new MethodMatcher("java.util.Map merge(..)")); + matchers.add(new MethodMatcher("java.util.Map put(..)")); + matchers.add(new MethodMatcher("java.util.Map putIfAbsent(..)")); + + matchers.add(new MethodMatcher("java.util.Queue poll(..)")); + matchers.add(new MethodMatcher("java.util.Queue peek(..)")); + + matchers.add(new MethodMatcher("java.util.Deque peekFirst(..)")); + matchers.add(new MethodMatcher("java.util.Deque pollFirst(..)")); + matchers.add(new MethodMatcher("java.util.Deque peekLast(..)")); + + matchers.add(new MethodMatcher("java.util.NavigableSet lower(..)")); + matchers.add(new MethodMatcher("java.util.NavigableSet floor(..)")); + matchers.add(new MethodMatcher("java.util.NavigableSet ceiling(..)")); + matchers.add(new MethodMatcher("java.util.NavigableSet higher(..)")); + matchers.add(new MethodMatcher("java.util.NavigableSet pollFirst(..)")); + matchers.add(new MethodMatcher("java.util.NavigableSet pollLast(..)")); + + matchers.add(new MethodMatcher("java.util.NavigableMap lowerEntry(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap floorEntry(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap ceilingEntry(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap higherEntry(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap lowerKey(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap floorKey(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap ceilingKey(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap higherKey(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap firstEntry(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap lastEntry(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap pollFirstEntry(..)")); + matchers.add(new MethodMatcher("java.util.NavigableMap pollLastEntry(..)")); + + matchers.add(new MethodMatcher("java.util.Spliterator trySplit(..)")); + + return Collections.unmodifiableList(matchers); + } + + @Override + public J.Return visitReturn(J.Return retrn, AtomicBoolean containsNullableReturn) { + if (containsNullableReturn.get()) { + return retrn; + } + + J.Return r = super.visitReturn(retrn, containsNullableReturn); + updateCursor(r); + + // If the returns is contained within a lambda statement, we don't consider it. + Cursor ex = getCursor().dropParentUntil(e -> e instanceof J.MethodDeclaration || e instanceof J.Lambda); + if (!(ex.getValue() instanceof J.MethodDeclaration)) { + return r; + } + + if (r.getExpression() != null && maybeIsNull(r.getExpression())) { + containsNullableReturn.set(true); + } + + return r; + } + + private boolean maybeIsNull(Expression returnExpression) { + if (returnExpression instanceof J.Literal && ((J.Literal) returnExpression).getValue() == null) { + return true; + } else if (returnExpression instanceof J.MethodInvocation) { + return isKnowNullableMethod((J.MethodInvocation) returnExpression); + } + return false; + } + + private boolean isKnowNullableMethod(J.MethodInvocation methodInvocation) { + for (MethodMatcher m : KNOWN_NULLABLE_METHODS) { + if (m.matches(methodInvocation)) { + return true; + } + } + return false; + } + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java new file mode 100644 index 0000000000..ca65cdf3c7 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -0,0 +1,198 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.junit.jupiter.api.Test; +import org.openrewrite.java.JavaParser; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; +import static org.openrewrite.java.Assertions.javaVersion; + +class AnnotateNullableMethodsTest implements RewriteTest { + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new AnnotateNullableMethods()).parser(JavaParser.fromJavaVersion() + .classpath("jspecify")) + .allSources(sourceSpec -> sourceSpec.markers(javaVersion(17))); + } + + @Test + void methodReturnsNullLiteral() { + rewriteRun( + spec -> spec.recipe(new AnnotateNullableMethods()), + //language=java + java( + """ + public class Test { + public String getString() { + return null; + } + + public String getStringWithMultipleReturn() { + if (System.currentTimeMillis() % 2 == 0) { + return "Not null"; + } + return null; + } + } + """, + """ + import org.jspecify.annotations.Nullable; + + public class Test { + @Nullable + public String getString() { + return null; + } + + @Nullable + public String getStringWithMultipleReturn() { + if (System.currentTimeMillis() % 2 == 0) { + return "Not null"; + } + return null; + } + } + """ + ) + ); + } + + @Test + void methodReturnNullButIsAlreadyAnnotated() { + rewriteRun( + spec -> spec.recipe(new AnnotateNullableMethods()), + //language=java + java(""" + import org.jspecify.annotations.Nullable; + + public class Test { + @Nullable + public String getString() { + return null; + } + + @Nullable + public String getStringWithMultipleReturn() { + if (System.currentTimeMillis() % 2 == 0) { + return "Not null"; + } + return null; + } + } + """ + ) + ); + } + + @Test + void methodDoesNotReturnNull() { + rewriteRun( + spec -> spec.recipe(new AnnotateNullableMethods()), + //language=java + java( + """ + package org.example; + + public class Test { + public String getString() { + return "Hello"; + } + } + """ + ) + ); + } + + @Test + void methodReturnsDelegateKnowNullableMethod() { + rewriteRun( + spec -> spec.recipe(new AnnotateNullableMethods()), + //language=java + java( + """ + import java.util.HashMap; + import java.util.Map; + + public class Test { + public String getString() { + Map map = new HashMap<>(); + return map.get("key"); + } + } + """, + """ + import org.jspecify.annotations.Nullable; + + import java.util.HashMap; + import java.util.Map; + + public class Test { + @Nullable + public String getString() { + Map map = new HashMap<>(); + return map.get("key"); + } + } + """ + ) + ); + } + + @Test + void methodWithLambdaShouldNotBeAnnotated() { + rewriteRun( + //language=java + java( + """ + import java.util.stream.Stream; + class A { + public Runnable getRunnable() { + return () -> null; + } + + public Integer someStream(){ + // Stream with lambda class. + return Stream.of(1, 2, 3) + .map(i -> {if (i == 2) return null; else return i;}) + .reduce((a, b) -> a + b) + .orElse(null); + } + } + """ + ) + ); + } + + @Test + void privateMethodsShouldNotBeAnnotated() { + rewriteRun( + spec -> spec.recipe(new AnnotateNullableMethods()), + //language=java + java( + """ + public class Test { + private String getString() { + return null; + } + } + """ + ) + ); + } +} From ae890d757c0b687b6d745c47dcf159863a5e0532 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 14 Oct 2024 12:08:33 +0200 Subject: [PATCH 02/18] Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../staticanalysis/AnnotateNullableMethodsTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index ca65cdf3c7..47c4321f40 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -16,6 +16,7 @@ package org.openrewrite.staticanalysis; import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; import org.openrewrite.java.JavaParser; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; @@ -31,6 +32,7 @@ public void defaults(RecipeSpec spec) { .allSources(sourceSpec -> sourceSpec.markers(javaVersion(17))); } + @DocumentExample @Test void methodReturnsNullLiteral() { rewriteRun( @@ -78,7 +80,8 @@ void methodReturnNullButIsAlreadyAnnotated() { rewriteRun( spec -> spec.recipe(new AnnotateNullableMethods()), //language=java - java(""" + java( + """ import org.jspecify.annotations.Nullable; public class Test { From 1a914522fcaed95386efefba7b6f1cfa2258dbbd Mon Sep 17 00:00:00 2001 From: Niels de Bruin Date: Mon, 14 Oct 2024 12:39:03 +0200 Subject: [PATCH 03/18] Apply suggestions from code review Co-authored-by: Tim te Beek --- .../staticanalysis/AnnotateNullableMethods.java | 8 +++----- .../staticanalysis/AnnotateNullableMethodsTest.java | 4 +--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 4b94bfdf4e..93b957a050 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -33,11 +33,9 @@ import java.util.concurrent.atomic.AtomicBoolean; public class AnnotateNullableMethods extends Recipe { - private static final String NULLABLE_ANN_PACKAGE = "org.jspecify.annotations"; - private static final String NULLABLE_ANN_CLASS = NULLABLE_ANN_PACKAGE + ".Nullable"; + private static final String NULLABLE_ANN_CLASS = "org.jspecify.annotations.Nullable"; private static final AnnotationMatcher NULLABLE_ANNOTATION_MATCHER = new AnnotationMatcher("@" + NULLABLE_ANN_CLASS); - private static final String NULLABLE_ANN_ARTIFACT = "jspecify"; @Override public String getDisplayName() { @@ -62,7 +60,7 @@ public TreeVisitor getVisitor() { private static class AnnotateNullableMethodsVisitor extends JavaIsoVisitor { @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { - if (md.hasModifier(J.Modifier.Type.Private)) { + if (!md.hasModifier(J.Modifier.Type.Public)) { return md; } @@ -81,7 +79,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, Execut maybeAddImport(NULLABLE_ANN_CLASS); return JavaTemplate.builder("@Nullable") .imports(NULLABLE_ANN_CLASS) - .javaParser(JavaParser.fromJavaVersion().classpath(NULLABLE_ANN_ARTIFACT)) + .javaParser(JavaParser.fromJavaVersion().classpath("jspecify")) .build() .apply(getCursor(), md.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))); } diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index 47c4321f40..e287e57238 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -27,9 +27,7 @@ class AnnotateNullableMethodsTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new AnnotateNullableMethods()).parser(JavaParser.fromJavaVersion() - .classpath("jspecify")) - .allSources(sourceSpec -> sourceSpec.markers(javaVersion(17))); + spec.recipe(new AnnotateNullableMethods()).parser(JavaParser.fromJavaVersion().classpath("jspecify")); } @DocumentExample From 0bfebc4381a6df44e7b96fe0ae57d584030b4331 Mon Sep 17 00:00:00 2001 From: Niels de Bruin Date: Mon, 14 Oct 2024 16:27:59 +0200 Subject: [PATCH 04/18] Update src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java Co-authored-by: Tim te Beek --- .../staticanalysis/AnnotateNullableMethodsTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index e287e57238..b9dd47a4d8 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -83,8 +83,7 @@ void methodReturnNullButIsAlreadyAnnotated() { import org.jspecify.annotations.Nullable; public class Test { - @Nullable - public String getString() { + public @Nullable String getString() { return null; } From e2413d1eb997b0f29a23319786ac117a66172a60 Mon Sep 17 00:00:00 2001 From: Niels de Bruin Date: Mon, 14 Oct 2024 16:31:27 +0200 Subject: [PATCH 05/18] Apply suggestions from code review Co-authored-by: Tim te Beek --- .../staticanalysis/AnnotateNullableMethodsTest.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index b9dd47a4d8..1176cb2953 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -60,8 +60,7 @@ public String getString() { return null; } - @Nullable - public String getStringWithMultipleReturn() { + public @Nullable String getStringWithMultipleReturn() { if (System.currentTimeMillis() % 2 == 0) { return "Not null"; } @@ -87,8 +86,7 @@ public class Test { return null; } - @Nullable - public String getStringWithMultipleReturn() { + public @Nullable String getStringWithMultipleReturn() { if (System.currentTimeMillis() % 2 == 0) { return "Not null"; } @@ -143,8 +141,7 @@ public String getString() { import java.util.Map; public class Test { - @Nullable - public String getString() { + public @Nullable String getString() { Map map = new HashMap<>(); return map.get("key"); } From 21873f9ce19a9a6775a8e1d7217900a41a29d78b Mon Sep 17 00:00:00 2001 From: Niels de Bruin Date: Mon, 14 Oct 2024 16:49:35 +0200 Subject: [PATCH 06/18] Process PR feedback --- .../AnnotateNullableMethods.java | 86 ++++++++++--------- .../AnnotateNullableMethodsTest.java | 40 +++++---- 2 files changed, 65 insertions(+), 61 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 93b957a050..8caaf60c48 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -26,13 +26,13 @@ import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; -import java.util.ArrayList; -import java.util.Collections; +import java.util.Arrays; import java.util.Comparator; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; public class AnnotateNullableMethods extends Recipe { + private static final String NULLABLE_ANN_CLASS = "org.jspecify.annotations.Nullable"; private static final AnnotationMatcher NULLABLE_ANNOTATION_MATCHER = new AnnotationMatcher("@" + NULLABLE_ANN_CLASS); @@ -58,6 +58,8 @@ public TreeVisitor getVisitor() { } private static class AnnotateNullableMethodsVisitor extends JavaIsoVisitor { + AtomicBoolean annotatedNullable = new AtomicBoolean(false); + @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { if (!md.hasModifier(J.Modifier.Type.Public)) { @@ -77,6 +79,9 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, Execut if (FindNullableReturnStatements.find(md).get()) { maybeAddImport(NULLABLE_ANN_CLASS); + if (!annotatedNullable.getAndSet(true)) { + doAfterVisit(new NullableOnMethodReturnType().getVisitor()); + } return JavaTemplate.builder("@Nullable") .imports(NULLABLE_ANN_CLASS) .javaParser(JavaParser.fromJavaVersion().classpath("jspecify")) @@ -96,46 +101,43 @@ static AtomicBoolean find(J subtree) { } private static List getMatchersKnownNullableMethods() { - List matchers = new ArrayList<>(); - - matchers.add(new MethodMatcher("java.util.Map compute(..)")); - matchers.add(new MethodMatcher("java.util.Map computeIfAbsent(..)")); - matchers.add(new MethodMatcher("java.util.Map computeIfPresent(..)")); - matchers.add(new MethodMatcher("java.util.Map get(..)")); - matchers.add(new MethodMatcher("java.util.Map merge(..)")); - matchers.add(new MethodMatcher("java.util.Map put(..)")); - matchers.add(new MethodMatcher("java.util.Map putIfAbsent(..)")); - - matchers.add(new MethodMatcher("java.util.Queue poll(..)")); - matchers.add(new MethodMatcher("java.util.Queue peek(..)")); - - matchers.add(new MethodMatcher("java.util.Deque peekFirst(..)")); - matchers.add(new MethodMatcher("java.util.Deque pollFirst(..)")); - matchers.add(new MethodMatcher("java.util.Deque peekLast(..)")); - - matchers.add(new MethodMatcher("java.util.NavigableSet lower(..)")); - matchers.add(new MethodMatcher("java.util.NavigableSet floor(..)")); - matchers.add(new MethodMatcher("java.util.NavigableSet ceiling(..)")); - matchers.add(new MethodMatcher("java.util.NavigableSet higher(..)")); - matchers.add(new MethodMatcher("java.util.NavigableSet pollFirst(..)")); - matchers.add(new MethodMatcher("java.util.NavigableSet pollLast(..)")); - - matchers.add(new MethodMatcher("java.util.NavigableMap lowerEntry(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap floorEntry(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap ceilingEntry(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap higherEntry(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap lowerKey(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap floorKey(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap ceilingKey(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap higherKey(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap firstEntry(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap lastEntry(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap pollFirstEntry(..)")); - matchers.add(new MethodMatcher("java.util.NavigableMap pollLastEntry(..)")); - - matchers.add(new MethodMatcher("java.util.Spliterator trySplit(..)")); - - return Collections.unmodifiableList(matchers); + return Arrays.asList( + new MethodMatcher("java.util.Map computeIfAbsent(..)"), + new MethodMatcher("java.util.Map computeIfPresent(..)"), + new MethodMatcher("java.util.Map get(..)"), + new MethodMatcher("java.util.Map merge(..)"), + new MethodMatcher("java.util.Map put(..)"), + new MethodMatcher("java.util.Map putIfAbsent(..)"), + + new MethodMatcher("java.util.Queue poll(..)"), + new MethodMatcher("java.util.Queue peek(..)"), + + new MethodMatcher("java.util.Deque peekFirst(..)"), + new MethodMatcher("java.util.Deque pollFirst(..)"), + new MethodMatcher("java.util.Deque peekLast(..)"), + + new MethodMatcher("java.util.NavigableSet lower(..)"), + new MethodMatcher("java.util.NavigableSet floor(..)"), + new MethodMatcher("java.util.NavigableSet ceiling(..)"), + new MethodMatcher("java.util.NavigableSet higher(..)"), + new MethodMatcher("java.util.NavigableSet pollFirst(..)"), + new MethodMatcher("java.util.NavigableSet pollLast(..)"), + + new MethodMatcher("java.util.NavigableMap lowerEntry(..)"), + new MethodMatcher("java.util.NavigableMap floorEntry(..)"), + new MethodMatcher("java.util.NavigableMap ceilingEntry(..)"), + new MethodMatcher("java.util.NavigableMap higherEntry(..)"), + new MethodMatcher("java.util.NavigableMap lowerKey(..)"), + new MethodMatcher("java.util.NavigableMap floorKey(..)"), + new MethodMatcher("java.util.NavigableMap ceilingKey(..)"), + new MethodMatcher("java.util.NavigableMap higherKey(..)"), + new MethodMatcher("java.util.NavigableMap firstEntry(..)"), + new MethodMatcher("java.util.NavigableMap lastEntry(..)"), + new MethodMatcher("java.util.NavigableMap pollFirstEntry(..)"), + new MethodMatcher("java.util.NavigableMap pollLastEntry(..)"), + + new MethodMatcher("java.util.Spliterator trySplit(..)") + ); } @Override diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index 1176cb2953..3d834fd4ea 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -22,7 +22,6 @@ import org.openrewrite.test.RewriteTest; import static org.openrewrite.java.Assertions.java; -import static org.openrewrite.java.Assertions.javaVersion; class AnnotateNullableMethodsTest implements RewriteTest { @Override @@ -39,6 +38,7 @@ void methodReturnsNullLiteral() { java( """ public class Test { + public String getString() { return null; } @@ -54,9 +54,9 @@ public String getStringWithMultipleReturn() { """ import org.jspecify.annotations.Nullable; - public class Test { - @Nullable - public String getString() { + public class Test { + + public @Nullable String getString() { return null; } @@ -78,22 +78,22 @@ void methodReturnNullButIsAlreadyAnnotated() { spec -> spec.recipe(new AnnotateNullableMethods()), //language=java java( - """ - import org.jspecify.annotations.Nullable; - - public class Test { - public @Nullable String getString() { - return null; - } - - public @Nullable String getStringWithMultipleReturn() { - if (System.currentTimeMillis() % 2 == 0) { - return "Not null"; - } - return null; - } - } """ + import org.jspecify.annotations.Nullable; + + public class Test { + public @Nullable String getString() { + return null; + } + + public @Nullable String getStringWithMultipleReturn() { + if (System.currentTimeMillis() % 2 == 0) { + return "Not null"; + } + return null; + } + } + """ ) ); } @@ -128,6 +128,7 @@ void methodReturnsDelegateKnowNullableMethod() { import java.util.Map; public class Test { + public String getString() { Map map = new HashMap<>(); return map.get("key"); @@ -141,6 +142,7 @@ public String getString() { import java.util.Map; public class Test { + public @Nullable String getString() { Map map = new HashMap<>(); return map.get("key"); From 791b7fd155097ef1649a805a6674aa277f5617e5 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Oct 2024 10:53:48 +0200 Subject: [PATCH 07/18] Apply formatting suggestions --- .../staticanalysis/AnnotateNullableMethods.java | 12 +++++++----- .../staticanalysis/AnnotateNullableMethodsTest.java | 7 ++++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 8caaf60c48..e27f8d0248 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -45,11 +45,11 @@ public String getDisplayName() { @Override public String getDescription() { return "Automatically adds the @org.jspecify.annotation.Nullable to non-private methods" + - "that may return null. 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."; + "that may return null. 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 @@ -58,6 +58,7 @@ public TreeVisitor getVisitor() { } private static class AnnotateNullableMethodsVisitor extends JavaIsoVisitor { + AtomicBoolean annotatedNullable = new AtomicBoolean(false); @Override @@ -94,6 +95,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, Execut @AllArgsConstructor private static class FindNullableReturnStatements extends JavaIsoVisitor { + private static final List KNOWN_NULLABLE_METHODS = getMatchersKnownNullableMethods(); static AtomicBoolean find(J subtree) { diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index 3d834fd4ea..6cdbca2750 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -24,6 +24,7 @@ import static org.openrewrite.java.Assertions.java; class AnnotateNullableMethodsTest implements RewriteTest { + @Override public void defaults(RecipeSpec spec) { spec.recipe(new AnnotateNullableMethods()).parser(JavaParser.fromJavaVersion().classpath("jspecify")); @@ -38,7 +39,7 @@ void methodReturnsNullLiteral() { java( """ public class Test { - + public String getString() { return null; } @@ -54,8 +55,8 @@ public String getStringWithMultipleReturn() { """ import org.jspecify.annotations.Nullable; - public class Test { - + public class Test { + public @Nullable String getString() { return null; } From f2eb66ae57b5f8997756af800678c4f8062a56a9 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Oct 2024 11:13:00 +0200 Subject: [PATCH 08/18] Slight polish --- .../AnnotateNullableMethods.java | 159 ++++++++---------- 1 file changed, 69 insertions(+), 90 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index e27f8d0248..0ed9071713 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -15,11 +15,7 @@ */ package org.openrewrite.staticanalysis; -import lombok.AllArgsConstructor; -import org.openrewrite.Cursor; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Recipe; -import org.openrewrite.TreeVisitor; +import org.openrewrite.*; import org.openrewrite.java.*; import org.openrewrite.java.service.AnnotationService; import org.openrewrite.java.tree.Expression; @@ -34,114 +30,97 @@ public class AnnotateNullableMethods extends Recipe { private static final String NULLABLE_ANN_CLASS = "org.jspecify.annotations.Nullable"; - private static final AnnotationMatcher NULLABLE_ANNOTATION_MATCHER = - new AnnotationMatcher("@" + NULLABLE_ANN_CLASS); + private static final AnnotationMatcher NULLABLE_ANNOTATION_MATCHER = new AnnotationMatcher("@" + NULLABLE_ANN_CLASS); @Override public String getDisplayName() { - return "Annotate methods which may return null with @Nullable"; + return "Annotate methods which may return `null` with `@Nullable`"; } @Override public String getDescription() { - return "Automatically adds the @org.jspecify.annotation.Nullable to non-private methods" + - "that may return null. 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."; + return "Automatically adds the `@org.jspecify.annotation.Nullable` to non-private methods " + + "that may return `null`. 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 TreeVisitor getVisitor() { - return new AnnotateNullableMethodsVisitor(); - } - - private static class AnnotateNullableMethodsVisitor extends JavaIsoVisitor { - - AtomicBoolean annotatedNullable = new AtomicBoolean(false); - - @Override - public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration md, ExecutionContext ctx) { - if (!md.hasModifier(J.Modifier.Type.Public)) { - return md; - } - - if (md.getMethodType() != null && md.getMethodType().getReturnType() instanceof JavaType.Primitive) { - return md; - } - - if (service(AnnotationService.class).matches(getCursor(), NULLABLE_ANNOTATION_MATCHER)) { - return md; - } - - md = super.visitMethodDeclaration(md, ctx); - updateCursor(md); + return new JavaIsoVisitor() { + @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(), NULLABLE_ANNOTATION_MATCHER)) { + return methodDeclaration; + } - if (FindNullableReturnStatements.find(md).get()) { - maybeAddImport(NULLABLE_ANN_CLASS); - if (!annotatedNullable.getAndSet(true)) { - doAfterVisit(new NullableOnMethodReturnType().getVisitor()); + J.MethodDeclaration md = super.visitMethodDeclaration(methodDeclaration, ctx); + updateCursor(md); + + if (FindNullableReturnStatements.find(md).get()) { + maybeAddImport(NULLABLE_ANN_CLASS); + J.MethodDeclaration annotatedMethod = JavaTemplate.builder("@Nullable") + .imports(NULLABLE_ANN_CLASS) + .javaParser(JavaParser.fromJavaVersion().dependsOn( + "package org.jspecify.annotations;public @interface Nullable {}")) + .build() + .apply(getCursor(), md.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))); + return (J.MethodDeclaration) new NullableOnMethodReturnType().getVisitor().visitNonNull(annotatedMethod, ctx, getCursor().getParentTreeCursor()); } - return JavaTemplate.builder("@Nullable") - .imports(NULLABLE_ANN_CLASS) - .javaParser(JavaParser.fromJavaVersion().classpath("jspecify")) - .build() - .apply(getCursor(), md.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))); + return md; } - return md; - } + }; } - @AllArgsConstructor private static class FindNullableReturnStatements extends JavaIsoVisitor { - private static final List KNOWN_NULLABLE_METHODS = getMatchersKnownNullableMethods(); + private static final List KNOWN_NULLABLE_METHODS = Arrays.asList( + new MethodMatcher("java.util.Map computeIfAbsent(..)"), + new MethodMatcher("java.util.Map computeIfPresent(..)"), + new MethodMatcher("java.util.Map get(..)"), + new MethodMatcher("java.util.Map merge(..)"), + new MethodMatcher("java.util.Map put(..)"), + new MethodMatcher("java.util.Map putIfAbsent(..)"), + + new MethodMatcher("java.util.Queue poll(..)"), + new MethodMatcher("java.util.Queue peek(..)"), + + new MethodMatcher("java.util.Deque peekFirst(..)"), + new MethodMatcher("java.util.Deque pollFirst(..)"), + new MethodMatcher("java.util.Deque peekLast(..)"), + + new MethodMatcher("java.util.NavigableSet lower(..)"), + new MethodMatcher("java.util.NavigableSet floor(..)"), + new MethodMatcher("java.util.NavigableSet ceiling(..)"), + new MethodMatcher("java.util.NavigableSet higher(..)"), + new MethodMatcher("java.util.NavigableSet pollFirst(..)"), + new MethodMatcher("java.util.NavigableSet pollLast(..)"), + + new MethodMatcher("java.util.NavigableMap lowerEntry(..)"), + new MethodMatcher("java.util.NavigableMap floorEntry(..)"), + new MethodMatcher("java.util.NavigableMap ceilingEntry(..)"), + new MethodMatcher("java.util.NavigableMap higherEntry(..)"), + new MethodMatcher("java.util.NavigableMap lowerKey(..)"), + new MethodMatcher("java.util.NavigableMap floorKey(..)"), + new MethodMatcher("java.util.NavigableMap ceilingKey(..)"), + new MethodMatcher("java.util.NavigableMap higherKey(..)"), + new MethodMatcher("java.util.NavigableMap firstEntry(..)"), + new MethodMatcher("java.util.NavigableMap lastEntry(..)"), + new MethodMatcher("java.util.NavigableMap pollFirstEntry(..)"), + new MethodMatcher("java.util.NavigableMap pollLastEntry(..)"), + + new MethodMatcher("java.util.Spliterator trySplit(..)") + ); static AtomicBoolean find(J subtree) { return new FindNullableReturnStatements().reduce(subtree, new AtomicBoolean()); } - private static List getMatchersKnownNullableMethods() { - return Arrays.asList( - new MethodMatcher("java.util.Map computeIfAbsent(..)"), - new MethodMatcher("java.util.Map computeIfPresent(..)"), - new MethodMatcher("java.util.Map get(..)"), - new MethodMatcher("java.util.Map merge(..)"), - new MethodMatcher("java.util.Map put(..)"), - new MethodMatcher("java.util.Map putIfAbsent(..)"), - - new MethodMatcher("java.util.Queue poll(..)"), - new MethodMatcher("java.util.Queue peek(..)"), - - new MethodMatcher("java.util.Deque peekFirst(..)"), - new MethodMatcher("java.util.Deque pollFirst(..)"), - new MethodMatcher("java.util.Deque peekLast(..)"), - - new MethodMatcher("java.util.NavigableSet lower(..)"), - new MethodMatcher("java.util.NavigableSet floor(..)"), - new MethodMatcher("java.util.NavigableSet ceiling(..)"), - new MethodMatcher("java.util.NavigableSet higher(..)"), - new MethodMatcher("java.util.NavigableSet pollFirst(..)"), - new MethodMatcher("java.util.NavigableSet pollLast(..)"), - - new MethodMatcher("java.util.NavigableMap lowerEntry(..)"), - new MethodMatcher("java.util.NavigableMap floorEntry(..)"), - new MethodMatcher("java.util.NavigableMap ceilingEntry(..)"), - new MethodMatcher("java.util.NavigableMap higherEntry(..)"), - new MethodMatcher("java.util.NavigableMap lowerKey(..)"), - new MethodMatcher("java.util.NavigableMap floorKey(..)"), - new MethodMatcher("java.util.NavigableMap ceilingKey(..)"), - new MethodMatcher("java.util.NavigableMap higherKey(..)"), - new MethodMatcher("java.util.NavigableMap firstEntry(..)"), - new MethodMatcher("java.util.NavigableMap lastEntry(..)"), - new MethodMatcher("java.util.NavigableMap pollFirstEntry(..)"), - new MethodMatcher("java.util.NavigableMap pollLastEntry(..)"), - - new MethodMatcher("java.util.Spliterator trySplit(..)") - ); - } - @Override public J.Return visitReturn(J.Return retrn, AtomicBoolean containsNullableReturn) { if (containsNullableReturn.get()) { From 341a056abd4ef293eea24db3d090e1c7df107f75 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Oct 2024 11:21:13 +0200 Subject: [PATCH 09/18] Further polish --- .../AnnotateNullableMethods.java | 42 +++++++++---------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 0ed9071713..938a0063dc 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -15,6 +15,7 @@ */ package org.openrewrite.staticanalysis; +import org.jspecify.annotations.Nullable; import org.openrewrite.*; import org.openrewrite.java.*; import org.openrewrite.java.service.AnnotationService; @@ -62,7 +63,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl J.MethodDeclaration md = super.visitMethodDeclaration(methodDeclaration, ctx); updateCursor(md); - if (FindNullableReturnStatements.find(md).get()) { + if (FindNullableReturnStatements.find(md)) { maybeAddImport(NULLABLE_ANN_CLASS); J.MethodDeclaration annotatedMethod = JavaTemplate.builder("@Nullable") .imports(NULLABLE_ANN_CLASS) @@ -117,36 +118,31 @@ private static class FindNullableReturnStatements extends JavaIsoVisitor e instanceof J.MethodDeclaration || e instanceof J.Lambda); - if (!(ex.getValue() instanceof J.MethodDeclaration)) { - return r; - } + public J.Lambda visitLambda(J.Lambda lambda, AtomicBoolean atomicBoolean) { + // Do not evaluate return statements in lambdas + return lambda; + } - if (r.getExpression() != null && maybeIsNull(r.getExpression())) { - containsNullableReturn.set(true); + @Override + public J.Return visitReturn(J.Return retrn, AtomicBoolean found) { + if (found.get()) { + return retrn; } - + J.Return r = super.visitReturn(retrn, found); + found.set(maybeIsNull(r.getExpression())); return r; } - private boolean maybeIsNull(Expression returnExpression) { - if (returnExpression instanceof J.Literal && ((J.Literal) returnExpression).getValue() == null) { - return true; - } else if (returnExpression instanceof J.MethodInvocation) { + private boolean maybeIsNull(@Nullable Expression returnExpression) { + if (returnExpression instanceof J.Literal) { + return ((J.Literal) returnExpression).getValue() == null; + } + if (returnExpression instanceof J.MethodInvocation) { return isKnowNullableMethod((J.MethodInvocation) returnExpression); } return false; From 90a3e733d4d65ad6311ce99d01b310de71f945a3 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Oct 2024 11:23:13 +0200 Subject: [PATCH 10/18] Polish tests --- .../AnnotateNullableMethodsTest.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index 6cdbca2750..2aa4f94be0 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -27,14 +27,15 @@ class AnnotateNullableMethodsTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new AnnotateNullableMethods()).parser(JavaParser.fromJavaVersion().classpath("jspecify")); + spec + .recipe(new AnnotateNullableMethods()) + .parser(JavaParser.fromJavaVersion().classpath("jspecify")); } @DocumentExample @Test void methodReturnsNullLiteral() { rewriteRun( - spec -> spec.recipe(new AnnotateNullableMethods()), //language=java java( """ @@ -76,7 +77,6 @@ public class Test { @Test void methodReturnNullButIsAlreadyAnnotated() { rewriteRun( - spec -> spec.recipe(new AnnotateNullableMethods()), //language=java java( """ @@ -102,7 +102,6 @@ public class Test { @Test void methodDoesNotReturnNull() { rewriteRun( - spec -> spec.recipe(new AnnotateNullableMethods()), //language=java java( """ @@ -121,17 +120,14 @@ public String getString() { @Test void methodReturnsDelegateKnowNullableMethod() { rewriteRun( - spec -> spec.recipe(new AnnotateNullableMethods()), //language=java java( """ - import java.util.HashMap; import java.util.Map; public class Test { - public String getString() { - Map map = new HashMap<>(); + public String getString(Map map) { return map.get("key"); } } @@ -139,13 +135,11 @@ public String getString() { """ import org.jspecify.annotations.Nullable; - import java.util.HashMap; import java.util.Map; public class Test { - public @Nullable String getString() { - Map map = new HashMap<>(); + public @Nullable String getString(Map map) { return map.get("key"); } } @@ -182,7 +176,6 @@ public Integer someStream(){ @Test void privateMethodsShouldNotBeAnnotated() { rewriteRun( - spec -> spec.recipe(new AnnotateNullableMethods()), //language=java java( """ From 0de3a5faa822615f6b1429591d273183caf8c1cb Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Oct 2024 11:27:50 +0200 Subject: [PATCH 11/18] Verify handling of J.NewClass with nested return --- .../AnnotateNullableMethods.java | 2 - .../AnnotateNullableMethodsTest.java | 44 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 938a0063dc..7895b89eb7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -61,8 +61,6 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl } J.MethodDeclaration md = super.visitMethodDeclaration(methodDeclaration, ctx); - updateCursor(md); - if (FindNullableReturnStatements.find(md)) { maybeAddImport(NULLABLE_ANN_CLASS); J.MethodDeclaration annotatedMethod = JavaTemplate.builder("@Nullable") diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index 2aa4f94be0..b2a371a91f 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -188,4 +188,48 @@ private String getString() { ) ); } + + @Test + void returnWithinNewClass() { + rewriteRun( + //language=java + java( + """ + import java.util.concurrent.Callable; + + public class Test { + + public Callable getString() { + return new Callable() { + @Override + public String call() throws Exception { + return null; + } + }; + } + + } + """, + """ + import org.jspecify.annotations.Nullable; + + import java.util.concurrent.Callable; + + public class Test { + + public Callable getString() { + return new Callable() { + + @Override + public @Nullable String call() throws Exception { + return null; + } + }; + } + + } + """ + ) + ); + } } From 717a25998cabe84d250ad6201a455a4679f9b82d Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Oct 2024 11:31:53 +0200 Subject: [PATCH 12/18] Update src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../openrewrite/staticanalysis/AnnotateNullableMethods.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 7895b89eb7..f52720891f 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -16,7 +16,9 @@ package org.openrewrite.staticanalysis; import org.jspecify.annotations.Nullable; -import org.openrewrite.*; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; import org.openrewrite.java.*; import org.openrewrite.java.service.AnnotationService; import org.openrewrite.java.tree.Expression; From 44cd307760867a619b60d6feea21c774a582a573 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 22 Oct 2024 12:20:28 +0200 Subject: [PATCH 13/18] Only visit method body, to avoid arguments with a nested return --- .../staticanalysis/AnnotateNullableMethods.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index f52720891f..4ec6ed260a 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -42,12 +42,11 @@ public String getDisplayName() { @Override public String getDescription() { - return "Automatically adds the `@org.jspecify.annotation.Nullable` to non-private methods " + - "that may return `null`. 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."; + return "Add the `@org.jspecify.annotation.Nullable` to non-private methods that may return `null`. " + + "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 @@ -63,7 +62,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl } J.MethodDeclaration md = super.visitMethodDeclaration(methodDeclaration, ctx); - if (FindNullableReturnStatements.find(md)) { + if (FindNullableReturnStatements.find(md.getBody())) { maybeAddImport(NULLABLE_ANN_CLASS); J.MethodDeclaration annotatedMethod = JavaTemplate.builder("@Nullable") .imports(NULLABLE_ANN_CLASS) @@ -118,7 +117,7 @@ private static class FindNullableReturnStatements extends JavaIsoVisitor Date: Tue, 22 Oct 2024 12:39:21 +0200 Subject: [PATCH 14/18] Reduce using the updated parent tree cursor --- .../staticanalysis/AnnotateNullableMethods.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 4ec6ed260a..741cdf29b5 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -16,6 +16,7 @@ package org.openrewrite.staticanalysis; import org.jspecify.annotations.Nullable; +import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; @@ -62,7 +63,8 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl } J.MethodDeclaration md = super.visitMethodDeclaration(methodDeclaration, ctx); - if (FindNullableReturnStatements.find(md.getBody())) { + updateCursor(md); + if (FindNullableReturnStatements.find(md.getBody(), getCursor().getParentTreeCursor())) { maybeAddImport(NULLABLE_ANN_CLASS); J.MethodDeclaration annotatedMethod = JavaTemplate.builder("@Nullable") .imports(NULLABLE_ANN_CLASS) @@ -117,8 +119,8 @@ private static class FindNullableReturnStatements extends JavaIsoVisitor Date: Thu, 24 Oct 2024 12:22:34 +0200 Subject: [PATCH 15/18] Fix check for static return annotations --- .../staticanalysis/AnnotateNullableMethods.java | 3 ++- .../AnnotateNullableMethodsTest.java | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 741cdf29b5..ef77d9c1cb 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -58,7 +58,8 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl if (!methodDeclaration.hasModifier(J.Modifier.Type.Public) || methodDeclaration.getMethodType() == null || methodDeclaration.getMethodType().getReturnType() instanceof JavaType.Primitive || - service(AnnotationService.class).matches(getCursor(), NULLABLE_ANNOTATION_MATCHER)) { + service(AnnotationService.class).matches(getCursor(), NULLABLE_ANNOTATION_MATCHER) || + service(AnnotationService.class).matches(new Cursor(null, methodDeclaration.getReturnTypeExpression()), NULLABLE_ANNOTATION_MATCHER)) { return methodDeclaration; } diff --git a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java index b2a371a91f..59d3848fbd 100644 --- a/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java @@ -232,4 +232,21 @@ public Callable getString() { ) ); } + + @Test + void returnStaticNestInnerClassAnnotation() { + rewriteRun( + //language=java + java( + """ + import org.jspecify.annotations.Nullable; + + public class Outer { + public static Outer.@Nullable Inner test() { return null; } + static class Inner {} + } + """ + ) + ); + } } From b7c993e83e0d4e37165b66924577795faf1dc466 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Thu, 24 Oct 2024 12:37:51 +0200 Subject: [PATCH 16/18] Insert a qualified annotation that we then possibly shorten --- .../openrewrite/staticanalysis/AnnotateNullableMethods.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index ef77d9c1cb..624100d069 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -66,13 +66,12 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl J.MethodDeclaration md = super.visitMethodDeclaration(methodDeclaration, ctx); updateCursor(md); if (FindNullableReturnStatements.find(md.getBody(), getCursor().getParentTreeCursor())) { - maybeAddImport(NULLABLE_ANN_CLASS); - J.MethodDeclaration annotatedMethod = JavaTemplate.builder("@Nullable") - .imports(NULLABLE_ANN_CLASS) + J.MethodDeclaration annotatedMethod = JavaTemplate.builder("@" + NULLABLE_ANN_CLASS) .javaParser(JavaParser.fromJavaVersion().dependsOn( "package org.jspecify.annotations;public @interface Nullable {}")) .build() .apply(getCursor(), md.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))); + doAfterVisit(new ShortenFullyQualifiedTypeReferences().getVisitor()); return (J.MethodDeclaration) new NullableOnMethodReturnType().getVisitor().visitNonNull(annotatedMethod, ctx, getCursor().getParentTreeCursor()); } return md; From 0e0e2637910b48392cb72ff99996fcd91b4d93fe Mon Sep 17 00:00:00 2001 From: Niels de Bruin Date: Thu, 24 Oct 2024 13:02:23 +0200 Subject: [PATCH 17/18] Limit scope of import fix --- .../org/openrewrite/staticanalysis/AnnotateNullableMethods.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index 624100d069..fee97ae8e7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -71,7 +71,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl "package org.jspecify.annotations;public @interface Nullable {}")) .build() .apply(getCursor(), md.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))); - doAfterVisit(new ShortenFullyQualifiedTypeReferences().getVisitor()); + doAfterVisit(ShortenFullyQualifiedTypeReferences.modifyOnly(annotatedMethod)); return (J.MethodDeclaration) new NullableOnMethodReturnType().getVisitor().visitNonNull(annotatedMethod, ctx, getCursor().getParentTreeCursor()); } return md; From aa52bfb1a4de291f223a94ca882d5dc332286890 Mon Sep 17 00:00:00 2001 From: Niels de Bruin Date: Thu, 24 Oct 2024 14:17:38 +0200 Subject: [PATCH 18/18] Add extra null check --- .../openrewrite/staticanalysis/AnnotateNullableMethods.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java index fee97ae8e7..94763084af 100644 --- a/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java +++ b/src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java @@ -59,7 +59,8 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration methodDecl methodDeclaration.getMethodType() == null || methodDeclaration.getMethodType().getReturnType() instanceof JavaType.Primitive || service(AnnotationService.class).matches(getCursor(), NULLABLE_ANNOTATION_MATCHER) || - service(AnnotationService.class).matches(new Cursor(null, methodDeclaration.getReturnTypeExpression()), NULLABLE_ANNOTATION_MATCHER)) { + (methodDeclaration.getReturnTypeExpression() != null && + service(AnnotationService.class).matches(new Cursor(null, methodDeclaration.getReturnTypeExpression()), NULLABLE_ANNOTATION_MATCHER))) { return methodDeclaration; }