Add `AnnotateNullableMethods` recipe #364

nielsdebruin · 2024-10-14T09:24:58Z

What's changed?

The PR implements the recipe outlined in #328. The new recipe will automatically add a @org.jspecify.annotation.Nullable to non-private methods that may return null. It does so by scanning methods and checking their return statements for potential null values. Apart from the literal return null it will also take methods on collection which are know to be null (based on : error-prone), such as Map.get, Map.put, Queue.poll..etc. Null that are the result of lambda, or streams, are currently ignored.

What's your motivation?

To quote this issue #328:

Nullability annotations on method return types help tools and devs to better understand and reason about the code. Folks might forget to add these annotations though. A recipe could look through the body of a method and add the missing nullability annotations on the return type.

Fixes Add missing nullable annotation on method declaration return type #328

Anything in particular you'd like reviewers to focus on?

This is my first recipe, so I might have formatted make some call, etc.

Anyone you would like to review specifically?

@timtebeek

Checklist

I've added unit tests to cover both positive and negative cases
I've read and applied the recipe conventions and best practices
I've used the IntelliJ IDEA auto-formatter on affected files

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-35
src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java
- lines 28-28
- lines 59-60

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-35
src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java
- lines 28-28
- lines 59-60

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java

Co-authored-by: Tim te Beek <[email protected]>

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java

nielsdebruin · 2024-10-14T10:50:07Z

Hi @timtebeek I see that in your suggested change you have moved the @Nullable after the access modifier instead of before, which is counter to the issue #328. Can I assume that this will be the final way, or should it be place before after all?

Pankraz76

Leverage functional style and existing APIs over boilerplate imperative approaches to maximize code efficiency and readability.

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java

…ethodsTest.java Co-authored-by: Tim te Beek <[email protected]>

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java

Co-authored-by: Tim te Beek <[email protected]>

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java

…ethods.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

timtebeek · 2024-10-22T10:23:57Z

I've been running this recipe at scale against serialized LSTs from OpenRewrite and Apache Maven projects, but so far do not see the recipe making any changes there; any thoughts on that? I'll try a few more iterations, but might make sense to quickly sync so you can do the same.

Seeing the suggested changes now after setting my recipe as the active recipe from my IDE and then running

 mod run . --active-recipe

One minor imperfection on the second one here; notice the Nullable annotation appears twice.

Maybe it's good to either fix NullableOnMethodReturnType to not allow these duplicates, or not rely on NullableOnMethodReturnType for pushing the annotation down from the method to the return type, but immediately adding it on the return type.

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-36

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java
- lines 35-35

timtebeek · 2024-10-24T10:30:29Z

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ChainStringBuilderAppendCalls.java

lines 35-35

If you see these types of messages your branch is likely behind on the main branch that contains the fixes. Be sure to click this button in such cases to reduce the noise on your PRs:

timtebeek

Great to see! Would you might once more running it against OpenRewrite from your IDE as a preview, and then once we merge this run it through the Moderne platform across all OpenRewrite repositories & created PRs to add these missing annotations and potentially resolve any nullability issues that then can reported downstream.

timtebeek · 2024-10-24T10:46:02Z

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java

+                    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)) {


My IDE gives me a warning on this line; something this recipe intends to make more common:

Be sure to enable that warning in your IDE if you haven't already, and keep pushing F2 and Alt + Enter to resolve issues before a commit/push and definitely before a merge.

Once that's no longer yellow we can merge. :)

github-actions bot reviewed Oct 14, 2024

View reviewed changes

nielsdebruin force-pushed the add-nullable-annotations branch from e295fc3 to 4a6a356 Compare October 14, 2024 09:31

Add AnnotateNullableMethodsRecipe

8bf01d4

nielsdebruin force-pushed the add-nullable-annotations branch from 4a6a356 to 8bf01d4 Compare October 14, 2024 09:31

github-actions bot reviewed Oct 14, 2024

View reviewed changes

timtebeek and others added 2 commits October 14, 2024 12:08

Apply suggestions from code review

ae890d7

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into add-nullable-annotations

efbda59

timtebeek assigned nielsdebruin Oct 14, 2024

timtebeek added the recipe label Oct 14, 2024

github-actions bot reviewed Oct 14, 2024

View reviewed changes

timtebeek reviewed Oct 14, 2024

View reviewed changes

Apply suggestions from code review

1a91452

Co-authored-by: Tim te Beek <[email protected]>

github-actions bot reviewed Oct 14, 2024

View reviewed changes

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java Outdated Show resolved Hide resolved

Pankraz76 suggested changes Oct 14, 2024

View reviewed changes

Update src/test/java/org/openrewrite/staticanalysis/AnnotateNullableM…

0bfebc4

…ethodsTest.java Co-authored-by: Tim te Beek <[email protected]>

github-actions bot reviewed Oct 14, 2024

View reviewed changes

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java Outdated Show resolved Hide resolved

Apply suggestions from code review

e2413d1

Co-authored-by: Tim te Beek <[email protected]>

github-actions bot reviewed Oct 14, 2024

View reviewed changes

src/test/java/org/openrewrite/staticanalysis/AnnotateNullableMethodsTest.java Outdated Show resolved Hide resolved

Process PR feedback

21873f9

nielsdebruin force-pushed the add-nullable-annotations branch from 13907c2 to 21873f9 Compare October 16, 2024 08:44

github-actions bot reviewed Oct 16, 2024

View reviewed changes

nielsdebruin requested a review from timtebeek October 16, 2024 08:55

Apply formatting suggestions

791b7fd

github-actions bot reviewed Oct 22, 2024

View reviewed changes

Slight polish

f2eb66a

github-actions bot reviewed Oct 22, 2024

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java Outdated Show resolved Hide resolved

timtebeek added 2 commits October 22, 2024 11:21

Further polish

341a056

Polish tests

90a3e73

github-actions bot reviewed Oct 22, 2024

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java Outdated Show resolved Hide resolved

Verify handling of J.NewClass with nested return

0de3a5f

github-actions bot reviewed Oct 22, 2024

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/AnnotateNullableMethods.java Outdated Show resolved Hide resolved

Update src/main/java/org/openrewrite/staticanalysis/AnnotateNullableM…

717a259

…ethods.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot reviewed Oct 22, 2024

View reviewed changes

Only visit method body, to avoid arguments with a nested return

44cd307

github-actions bot reviewed Oct 22, 2024

View reviewed changes

Reduce using the updated parent tree cursor

f3d71d1

github-actions bot reviewed Oct 22, 2024

View reviewed changes

Fix check for static return annotations

4382ab2

github-actions bot reviewed Oct 24, 2024

View reviewed changes

timtebeek added 2 commits October 24, 2024 12:30

Merge branch 'main' into add-nullable-annotations

f9a7d0b

Insert a qualified annotation that we then possibly shorten

b7c993e

timtebeek approved these changes Oct 24, 2024

View reviewed changes

timtebeek reviewed Oct 24, 2024

View reviewed changes

nielsdebruin added 2 commits October 24, 2024 13:02

Limit scope of import fix

0e0e263

Add extra null check

aa52bfb

timtebeek merged commit 81459cb into openrewrite:main Oct 25, 2024

nielsdebruin deleted the add-nullable-annotations branch October 27, 2024 12:46

Add AnnotateNullableMethods recipe #364

Add AnnotateNullableMethods recipe #364

Uh oh!

Conversation

nielsdebruin commented Oct 14, 2024 • edited by timtebeek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's changed?

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Checklist

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nielsdebruin commented Oct 14, 2024

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Add `AnnotateNullableMethods` recipe #364

Add `AnnotateNullableMethods` recipe #364

nielsdebruin commented Oct 14, 2024 •

edited by timtebeek

Loading

timtebeek commented Oct 22, 2024 •

edited

Loading