Move type-use annotations to array brackets during JSpecify migration#1038
Move type-use annotations to array brackets during JSpecify migration#1038
Conversation
Move the recipe before ChangeType in each migration pipeline so it matches on the old annotation type (e.g. javax.annotation.*). This avoids incorrectly moving pre-existing JSpecify annotations where @nullable String[] intentionally means "array of nullable Strings."
…ected Verifies that a project with both javax annotations (to migrate) and pre-existing JSpecify @nullable String[] (meaning array of nullable elements) only migrates the javax annotations without touching the already-correct JSpecify annotations.
src/main/java/org/openrewrite/java/migrate/jspecify/MoveAnnotationToArrayType.java
Show resolved
Hide resolved
| List<J.Annotation> leading = ListUtils.map(mv.getLeadingAnnotations(), a -> { | ||
| if (match[0] == null && matchesType(a)) { | ||
| match[0] = a; | ||
| return null; | ||
| } | ||
| return a; | ||
| }); | ||
| if (leading == mv.getLeadingAnnotations()) { | ||
| return mv; | ||
| } | ||
| mv = mv.withLeadingAnnotations(leading); |
There was a problem hiding this comment.
I suppose I'm trying to figure out if we have recipes that are going to be fragile in light of leading annotations being dropped and whether any of our recipes need to be adapted to handle the change in position for the annotations (have not had a chance to dig into this further yet)
There was a problem hiding this comment.
Let me know what you'd need to be comfortable seeing this merged!
| void moveNullableToNestedClassArrayField() { | ||
| rewriteRun( | ||
| //language=java | ||
| java( | ||
| """ | ||
| import javax.annotation.Nullable; | ||
| import java.util.Map; | ||
|
|
||
| class Foo { | ||
| @Nullable | ||
| public Map.Entry<String, String>[] entries; | ||
| } | ||
| """, | ||
| """ | ||
| import javax.annotation.Nullable; | ||
| import java.util.Map; | ||
|
|
||
| class Foo { | ||
| public Map.Entry<String, String> @Nullable[] entries; | ||
| } | ||
| """ | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think this test may actually be incorrect. Based on this, these could be different meanings:
@Nullable X[] x
// I believe the nested equivalent would be
y.@Nullable X[] xmeans individual elements can be null, but not the array itself
X @Nullable [] x
// I believe the nested equivalent would be
y.X @Nullable [] xmeans individual elements cannot be null, but the array can
@Nullable X @Nullable [] x
// I believe the nested equivalent would be
y.@Nullable X @Nullable [] xmeans both individual elements and the array itself can be null
There was a problem hiding this comment.
hmm; I thought it was different for the javax nullable annotation:
javax.annotation.Nullable is NOT a TYPE_USE annotation — it only targets METHOD, FIELD, PARAMETER, etc. When someone writes @javax.annotation.Nullable Map.Entry[], the annotation is in declaration position — it doesn’t have the element-vs-array distinction. It’s syntactically equivalent to “this whole thing might be null.”
The recipe deliberately targets pre-migration annotations (before ChangeType renames them to JSpecify). By the time the annotation becomes org.jspecify.annotations.Nullable (which IS TYPE_USE), it’s already in the correct array-bracket position with the intended meaning: “the array reference can be null.”
There was a problem hiding this comment.
The annotations targeted by
MoveAnnotationToArrayTypeare:
javax.annotation.*ull*— No@Targetat all (JSR-305). Implicitly applies everywhere including TYPE_USE.jakarta.annotation.*ull*—@Target({METHOD, PARAMETER, FIELD, TYPE_USE})— yes, TYPE_USEorg.jetbrains.annotations.*ull*—@Target({METHOD, FIELD, PARAMETER, LOCAL_VARIABLE, TYPE_USE})— yes, TYPE_USEio.micrometer.core.lang.*ull*—@Target({METHOD, FIELD, PARAMETER, LOCAL_VARIABLE})— no TYPE_USEorg.springframework.lang.*ull*—@Target({METHOD, PARAMETER, FIELD})— no TYPE_USEio.micronaut.core.annotation.*ull*—@Target({METHOD, PARAMETER, FIELD, TYPE_USE, ANNOTATION_TYPE})— yes, TYPE_USESo three of the six (jakarta, JetBrains, Micronaut) are explicitly TYPE_USE annotations, and javax is implicitly usable in TYPE_USE positions. Only Micrometer and Spring are purely declaration-position.
Your concern is valid for at least half the migration paths — for those annotations,
@Nullable X[]on the element type genuinely has different semantics thanX @Nullable[]on the array. The recipe is making a deliberate semantic choice to reposition the annotation to mean "array can be null" rather than "elements can be null."
There was a problem hiding this comment.
You could be correct on that. I guess I'm just not as familiar with the intricacies of different targets for annotations and how they affect the interpretation of the statement. Given we're targetting specific sets of annotations, it's probably fine, if just possible to misuse given there aren't target guards in the recipe if you were for example to give it JSpecify annotation FQNs to start with. Would it make sense to just guard against asking to move JSpecify annotations to JSpecify annotations?
Jakarta, JetBrains, and Micronaut annotations target TYPE_USE, so `@Nullable X[]` (element nullable) already has distinct semantics from `X @nullable[]` (array nullable). Moving them would change meaning.
github-project-automation
bot
moved this from In Progress
to Ready to Review
in OpenRewrite
2 participants
Summary
Fixes MigrateFromJavaxAnnotationApi does not modify annotations to the type level #934
Adds a new
MoveAnnotationToArrayTyperecipe that moves type-use annotations (e.g.@Nullable) from declaration position to the array brackets (e.g.@Nullable byte[]→byte @Nullable[]), as required by the JSpecify spec.Wires the recipe into all 6 JSpecify migration recipes in
jspecify.yml, running afterMoveFieldAnnotationToType.Handles method return types, method parameters, and field declarations.
Test plan
MoveAnnotationToArrayType: return types, parameters, fields, non-array no-ops, multi-dimensional arraysJSpecifyBestPracticesTestfor end-to-end javax → JSpecify migration with array types