Skip to content

Improve AnnotateNullableMethods to avoid duplicate annotations and annotation placement issues#845

Merged
timtebeek merged 2 commits intoopenrewrite:mainfrom
stefanodallapalma:fix-annotate-nullable-methods
Apr 1, 2026
Merged

Improve AnnotateNullableMethods to avoid duplicate annotations and annotation placement issues#845
timtebeek merged 2 commits intoopenrewrite:mainfrom
stefanodallapalma:fix-annotate-nullable-methods

Conversation

@stefanodallapalma
Copy link
Copy Markdown
Contributor

@stefanodallapalma stefanodallapalma commented Mar 31, 2026

Problem

Same issues as #843 (AnnotateNullableParameters), but for method return types:

1. Duplicate annotations when a different nullable annotation is already present

// Input: already annotated with @CheckForNull
@CheckForNull
public String getName() { return null; }

// After: ❌ duplicate
@CheckForNull
@Nullable
public String getName() { return null; }

Also fails to detect TYPE_USE annotations already on the return type (e.g. Outer.@Nullable Inner), causing re-annotation on subsequent Repeat.repeatUntilStable cycles:

// Input
public static Outer.@Nullable Inner test() { return null; }

// After: ❌ double-annotated
public static @Nullable Outer.@Nullable Inner test() { return null; }

2. Compilation error with non-TYPE_USE annotations on nested/array return types

// ❌ @CheckForNull not applicable in this type context
public Outer.@CheckForNull Inner bar() { return null; }

Solution

Mirrors the approach from PR #843:

  • Simple name matching — skip methods already carrying any annotation named Nullable, CheckForNull, regardless of package.

  • Non-TYPE_USE list — non-type use annotations (such as CheckForNull) stay as method-level annotations. All others default to TYPE_USE behavior.

Requested review

@timtebeek

NOTE: Test case typescriptCode() fails locally due to RPC process shut down early with exit code 1. I've no context on what that test is meant for. Worth checking it.

@timtebeek
Copy link
Copy Markdown
Member

Thanks! Can we resolve the conflicts (revert the changes) to AnnotateNullableParameters?

@timtebeek timtebeek self-requested a review March 31, 2026 13:15
@stefanodallapalma
Copy link
Copy Markdown
Contributor Author

Good catch. Haven't noticed AnnotateNullableParameter slipped through

@stefanodallapalma stefanodallapalma force-pushed the fix-annotate-nullable-methods branch from 3cfd633 to 33c856a Compare March 31, 2026 13:28
* are assumed to be TYPE_USE and will be moved to the return type position.
*/
private static final Set<String> NON_TYPE_USE_FQN = new HashSet<>(Arrays.asList(
"javax.annotation.CheckForNull"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit odd to have a set of one; do we see this grow in the future? Might there be some overfit for the test cases that uses the same?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps better use TYPE_USE_NULLABLE_ANNOTATIONS as in AnnotateNullableParameters

private static final Set<String> 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"
    ));

Comment on lines +43 to +46
private static final Set<String> NULLABLE_ANNOTATION_SIMPLE_NAMES = new HashSet<>(Arrays.asList(
"Nullable",
"CheckForNull"
));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we're not using the same contains("null") as we did for parameters? it's hard to be exhaustive, and we don't know if folks have their own "@NullAllowed" annotation for instance.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason tbh, I've started updating the recipe yesterday alongside the AnnotateNullableParameters and haven't had time to align with all changes in AnnotateNullableParameter

- Replace NULLABLE_ANNOTATION_SIMPLE_NAMES set with case-insensitive
  contains("null") check to handle custom annotations (e.g. @NullAllowed)
- Replace NON_TYPE_USE_FQN negative list with TYPE_USE_NULLABLE_ANNOTATIONS
  positive list, matching the approach in AnnotateNullableParameters
- Update tests to expect custom/unknown annotations as declaration-target
@github-project-automation github-project-automation bot moved this from In Progress to Ready to Review in OpenRewrite Apr 1, 2026
@timtebeek timtebeek merged commit 4ded8a7 into openrewrite:main Apr 1, 2026
1 check passed
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants