Improve AnnotateNullableParameters to avoid duplicate annotations and annotation placement issues

[stefanodallapalma]

Problem

The AnnotateNullableParameters recipe has two issues when used with nullable annotations from different frameworks:

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

The recipe only checks for the target annotation before adding it. If a parameter already carries a different nullable annotation (e.g. @CheckForNull), the recipe blindly adds the target on top, producing inconsistent code:

// Before: parameter already annotated with @CheckForNull
public void process(@CheckForNull String name) { ... }

// After: ❌ duplicate nullable semantics
public void process(@CheckForNull @Nullable String name) { ... }

This is problematic because users often want to run the recipe as a first step before a separate annotation-replacement recipe (e.g. @CheckForNull → @Nullable). The duplication forces manual cleanup.

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

The recipe unconditionally calls MoveFieldAnnotationToType, which repositions the annotation before the inner type of a nested type (Outer.@Nullable Inner). This is only valid for @Target(TYPE_USE) annotations. Declaration-target annotations like @CheckForNull cannot appear in that position:

// ❌ Compilation error: @CheckForNull not applicable in this type context
public boolean hasConfig(Account account, Units.@CheckForNull Currencies currency) { ... }

Solution

1. Skip parameters that already carry a known nullable annotation

Added a KNOWN_NULLABLE_ANNOTATIONS list covering well-known nullable annotations across frameworks. findCandidateParameters now checks against all of them, not just the target:

• org.jspecify.annotations.Nullable
• javax.annotation.Nullable / jakarta.annotation.Nullable
• javax.annotation.CheckForNull
• edu.umd.cs.findbugs.annotations.Nullable / CheckForNull
• org.checkerframework.checker.nullness.qual.Nullable
• org.eclipse.jdt.annotation.Nullable
• org.jetbrains.annotations.Nullable
• org.springframework.lang.Nullable
• android.support.annotation.Nullable / androidx.annotation.Nullable

// @CheckForNull already present → parameter is skipped
public void process(@CheckForNull String name) { ... }  // ✅ No change

// Only unannotated parameters get the target annotation
public void process(@CheckForNull String first, String last) { ... }
// becomes:
public void process(@CheckForNull String first, @Nullable String last) { ... }  // ✅ Only `last` annotated

2. Position annotation correctly based on @Target type

Added a TYPE_USE_NULLABLE_ANNOTATIONS set (jspecify, Jakarta, Checker Framework, Eclipse). Array bracket positioning (String @Nullable[]) and nested type positioning (Outer.@Nullable Inner) are now gated behind this check. Declaration-target annotations stay as leading annotations:

// TYPE_USE annotation (jspecify) → positioned before inner type
public void process(Units.@Nullable Currencies currency) { ... }  // ✅

// Declaration annotation (CheckForNull) → stays as leading annotation
public void process(@CheckForNull Units.Currencies currency) { ... }  // ✅

Updated recipe description

Removed the @Target(TYPE_USE) requirement from the option and recipe descriptions, since both TYPE_USE and declaration annotations are now properly supported.

Testing

• ✅ Parameterized test covering 6 known annotations × 2 recipe configurations (default + custom) = 12 combinations for skip-when-already-annotated
• ✅ Mixed parameter test: one param with @CheckForNull (skipped), one without (annotated)
• ✅ Nested type with TYPE_USE annotation → B.@Nullable C positioning
• ✅ Nested type with non-TYPE_USE annotation → @CheckForNull B.C positioning
• ✅ All existing tests pass unchanged

Requested reviewer

@timtebeek

Co-authored with claude.ai - Opus 4.6

[timtebeek]

Hey thanks! Makes sense, mostly. Had a quick look: on our side we avoid the duplicate annotations by running migrate to JSpecify before adding annotations. Are you using something else that you saw this?

[timtebeek]

Would we even need to be exhaustive here? Or can we match any annotation that contains Null? Want to avoid that we have this explicit list that we need to maintain, or folks asking to add their own when this heuristic is unlikely to have mismatches.

[stefanodallapalma]

I agree. But I'd be a little more specific by checking against the simple names CheckForNull and Nullable. I've seen parameters annotated with NotNull while still being checked against null. Whether that's correct, wrong, or just too defensive, if we only match any annotation that contains "Null", those parameters may end up being annotated as @Nullable @NotNull param, which is confusing.

[timtebeek]

Ah I would have thought if we see any annotation containing Null then we skip the whole thing, and assume the user knows what they're doing, or at least wouldn't appreciate a second annotation added.

[stefanodallapalma]

forget about it, my brain is not braining atm. So, if we check against contains("null"), parameters annotated with NotNull and similar annotations will be skipped, although they still check against null. Which is ok, as the code will be null-safe while letting static analysis tools like NullAway know that the param shall not be null.

[stefanodallapalma]

Ah I would have thought if we see any annotation containing Null then we skip the whole thing, and assume the user knows what they're doing, or at least wouldn't appreciate a second annotation added.

100%. I could not see your comment while typing my answer

[timtebeek]

This makes sense, thanks. Don't see an immediate good way to avoid the existing collection here, as the new fullyQualifiedName might not be on the recipe classpath.

[stefanodallapalma]

Hey thanks! Makes sense, mostly. Had a quick look: on our side we avoid the duplicate annotations by running migrate to JSpecify before adding annotations. Are you using something else that you saw this?

Running "migrate to JSpecify" before AnnotateNullable* makes sense, but in a large codebase like ours we prefer using a more conservative approach: annotate first and then decide whether and if makes sense migrate to jspecify or other annotations based on needs and constraints. Plus, codebases that still rely on FindBugs with @CheckForNull may not benefit from this recipe in its current state

[timtebeek]

Thanks for the help here! Good to avoid duplicates no matter how we'd arrive at those.