#2838 standardize nullability annotations

[AlexanderSkrock]

• Fixes Standardize nullability annotations #2838

[knutwannheden]

I will also ask @sambsnyd to weigh in with his view on this.

[sambsnyd]

Looks useful to me. Thanks!

[AlexanderSkrock]

Between holidays, I had time to think about this recipe and committed a work in progress version. In my opinion, we can solve the standardization issue even better this way. Would be more flexible and robust.

What do you think about this approach, @knutwannheden and @sambsnyd ? Anyways, I you prefer the original approach, I will roll back to the last commit before the overhaul and simply add your proposed testcase and nullability annotation. Otherwise I would continue implementing the StandardizeNullabilityAnnotationsVisitor.

[knutwannheden]

Apart from the last remark by Tim I think this PR is good to go.

[knutwannheden]

There is actually one case which isn't addressed yet. The Java language for some reason allows annotations to appear in-between modifiers. So it is actually allowed to have a class like this:

            class Test {
                private
                @Nonnull
                final
                String variable = "";
            }

In OpenRewrite the LST is also a bit unusual in this case. There is a J.Modifier type which declares an annotations field. I think it should probably be straight-forward to extend the recipe to also support this corner case, or what do you think @AlexanderSkrock ?

[AlexanderSkrock]

There is actually one case which isn't addressed yet. The Java language for some reason allows annotations to appear in-between modifiers. So it is actually allowed to have a class like this:

            class Test {
                private
                @Nonnull
                final
                String variable = "";
            }

In OpenRewrite the LST is also a bit unusual in this case. There is a J.Modifier type which declares an annotations field. I think it should probably be straight-forward to extend the recipe to also support this corner case, or what do you think @AlexanderSkrock ?

Oh, I overlooked that one! Yeah, I could implement it within visitClassDeclaration, but for annotations that belong to modifiers visitAnnotation would not be called, am I right? I came to this conclusion because J.Modifiers are not visisted with their specialized type, but as casual statements. (I will verify this with a testcase quickly.) In consequence we could only collect annotations manually from class declaration. Writing that, I realize the code currently does not check whether an annotation was found within "annotations on a classes kind" or "leading annotations".
This adds another question, similary to what you asked earlier about "insertion point of annotations", should we always insert at leading annotations? That's what we currently do (and probably is the most used option), still it is possible to do it between modifiers and after modifiers.

Edit: I added a few test cases about afore discussed problems with this commit.

1. For annotations on the Kind of ClassDeclarations, it seems there is no api yet to change annotations which are assigned to the Kind. We would need to remove matched annotations.
2. I could add checking Modifiers for ClassDeclaration, VariableDeclarations etc., but first we need to actually visit Modifiers like other elements, so that we can detect these occurences within visitAnnotation.

Last, but not least, for the first and second bullet, I would propose we define inserting annotations in front (e. g. for ClassDeclaration as leading annotation) as standard.

[knutwannheden]

FWIW, I found another source with common nullness annotations: The annotations supported by the Kotlin compiler.

[knutwannheden]

Oh, I overlooked that one! Yeah, I could implement it within visitClassDeclaration, but for annotations that belong to modifiers visitAnnotation would not be called, am I right? I came to this conclusion because J.Modifiers are not visisted with their specialized type, but as casual statements. (I will verify this with a testcase quickly.) In consequence we could only collect annotations manually from class declaration. Writing that, I realize the code currently does not check whether an annotation was found within "annotations on a classes kind" or "leading annotations". This adds another question, similary to what you asked earlier about "insertion point of annotations", should we always insert at leading annotations? That's what we currently do (and probably is the most used option), still it is possible to do it between modifiers and after modifiers.

For J.Modifier you could add a visitModifier() overload to your visitor to collect data about these annotations. Please also note that all of J.ClassDeclaration, J.MethodDeclaration, and J.VariableDeclarations declare an getAllAnnotations() method, which collects all annotations from the places they can appear in.

Last, but not least, for the first and second bullet, I would propose we define inserting annotations in front (e. g. for ClassDeclaration as leading annotation) as standard.

In the JLS there is this remark:

It is customary, though not required, to write declaration annotations before all other modifiers, and type annotations immediately before the type to which they apply.

So, either we could try to replace the annotations exactly in the same place as they were declared (but as previously discussed, this is currently not as easy as one would like it to be) or we could instead just always add the new annotations as leading annotations (all of the previously mentioned types also declare a leadingAnnotations field), which would be in line with the remark from the JLS and your proposal.

[sambsnyd]

If the recipe moves the strangely-placed annotations in-place that is fine, this recipe's purpose will have been accomplished.
If the recipe moves the annotation to the usual/canonical place for annotations that is a bonus.
Regarding either result as acceptable, I would probably go with whichever is easier/clearer to implement.

[AlexanderSkrock]

Just for your information. Currently I got some private issues and I try spend as much as time as possible with my family to make it through this period. Also, I am not sure, when I will be able to continue my work on this pull request. I am sorry!

As mentioned earlier, I collected the remaning issues and wrote, currently disabled, testcases for those. If any of you got capacities, feel free to finish the implementation.

Kind regards,
Adagatiya

[knutwannheden]

@AlexanderSkrock Thanks for the heads-up and I wish you and your family all the best 🍀 Since the PR is nearly done, I think we will try to finish it. Thank you again for all the work you put into it!

[timtebeek]

With JSpecify coming out we've done a write up of our migration from internal annotations to JSpecify:
https://www.moderne.ai/blog/mass-migration-of-nullability-annotations-to-jspecify
The lessons learned and recipes developed there are likely to be valuable in this effort as well.

[timtebeek]

FYI: There's now a parallel effort in

• Recipes to migrate from Javax, Jakarta and JetBrains nullability annotations to JSpecify rewrite-migrate-java#541

[timtebeek]

With the release of JSpecify we've created separate recipes in rewrite-migrate-java to migrate from various other libraries to JSpecify: https://docs.openrewrite.org/recipes/java/jspecify/migratetojspecify

I propose we close this effort, and pull over anything that's still applicable to rewrite-migrate-java instead. Thanks again @AlexanderSkrock !