#2838 standardize nullability annotations

[AlexanderSkrock]

• Fixes #2838

[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.

[AlexanderSkrock]

Sadly, I'm only able to do small steps, as I barely have coherent time slots. The current state of my draft merge request is as follows:

• Replacement works on various elements
• Replacement works with different scopes
• Replacement works with edge cases like replacing one annotation with two if necessary to cover scopes
• Adding and removing import works

ToDos:

• A general problem when inserting annotations that type resolution of inserted annotations does not work. Testcases get the expected code but type validation assertion fails. That's my core problem that remains. I understand that type is unknown as the import is added afterVisit, so after the template was executed. Tho, I do not know a solution yet.
• For return type annotations the spacing is odd, as it misses a whitespace between the visibility modifier and the annotation. E. g. public@NonNull. I attempted to modify autoformatting, but I did not find a suitable solution yet, which works in all cases.
• Adding targets and scopes for all known nullability annotations

[timtebeek]

Sadly, I'm only able to do small steps, as I barely have coherent time slots.

We appreciate all the work you've put in already! No worries about timing; do let us know if you'd like to hand off some of the work, otherwise we'll wait for you as we don't want to take away the fun seeing a feature through.

• A general problem when inserting annotations that type resolution of inserted annotations does not work. Testcases get the expected code but type validation assertion fails. That's my core problem that remains. I understand that type is unknown as the import is added afterVisit, so after the template was executed. Tho, I do not know a solution yet.

Hoping for instance @knutwannheden can chime in here; as he went through your PR before already, before I dive in as well.

• For return type annotations the spacing is odd, as it misses a whitespace between the visibility modifier and the annotation. E. g. public@NonNull. I attempted to modify autoformatting, but I did not find a suitable solution yet, which works in all cases.

Typically in such cases you might be lacking a prefix, and I don't see a mention of prefix in StandardizeNullabilityAnnotationsVisitor so far. You can likely find other examples where we add the prefix of the original element on the replacement element, which might fix the issues you're having here as well.

[knutwannheden]

Hoping for instance @knutwannheden can chime in here; as he went through your PR before already, before I dive in as well.

@AlexanderSkrock Yes, please don't hesitate to reach out if you have questions or problems you don't know how to solve. We will do our best to help out. I will try to take a look at your PR. So far it looks like you have made a lot of good progress!

[AlexanderSkrock]

Hoping for instance @knutwannheden can chime in here; as he went through your PR before already, before I dive in as well.

@AlexanderSkrock Yes, please don't hesitate to reach out if you have questions or problems you don't know how to solve. We will do our best to help out. I will try to take a look at your PR. So far it looks like you have made a lot of good progress!

Thank for your feedback already!

Except for completing the list of known nullability annotations, I am almost done. So far, one problem left when the replacing annotation has the same simple name.

Consider the following snippet:

maybeRemoveMatchedAnnotationImports(matchedNullabilityAnnotations);
maybeAddReplacementAnnotationImports(annotationsForReplacement);

List<J.Annotation> currentNullabilityAnnotations = matchedNullabilityAnnotations.stream().map(MatchedNullabilityAnnotation::getJAnnotation).collect(Collectors.toList());
List<J.Annotation> cleanedAnnotations = new LinkedList<>(classDeclaration.getLeadingAnnotations());
cleanedAnnotations.removeAll(currentNullabilityAnnotations);

J.ClassDeclaration cleanClassDeclaration = classDeclaration.withLeadingAnnotations(cleanedAnnotations);
return annotationsForReplacement.stream().reduce(
        cleanClassDeclaration,
        (J.ClassDeclaration currentDeclaration, NullabilityAnnotation annotation) -> currentDeclaration.withTemplate(
                annotationTemplate(annotation),
                currentDeclaration.getCoordinates().addAnnotation(Comparator.comparing(J.Annotation::getSimpleName))
        ), (oldDeclaration, newDeclaration) -> newDeclaration
);

The annotations are already replaced, but adding and removing imports is delayed to afterVisit. Now, when we use the template to add the replacement annotations, we have the old imports and new imports (that we manually added to the template). If the old and the new annotation now have the same simple name, the parser can not resolve the annotation's type properly because both imports match. Do you have an idea on this, @knutwannheden ? As far as I understood, I would not have the option at this point to modify imports directly while visiting e. g. the ClassDeclaration, right?

[knutwannheden]

That is indeed a tricky problem at the moment. We intend to improve this by allowing a template to contain fully qualified references, which then optionally (and if possible without conflict) get "shortened" again with a corresponding import when updating the LST.

I have a few ideas how to handle this. You could try using ChangeType, but I don't know if it can handle this case either. Otherwise you could first remove the import and annotations (and remember the locations) and in a second step (using doAfterVisit() or by hooking it in inside a visitSourceFile() override) add the new annotations (and import). Alternatively, but this should be last resort, directly attempt to update the type annotations. I am saying last resort, because this is often rather tricky to get exactly right.

Please let me know how it goes and if you don't find a good solution I will try to dig deeper.

[AlexanderSkrock]

First of all, thank you for your ideas!

I like that two-staged approach by splitting removal of old annotations and insertion of new annotations. I almost started implementing, when I realized there are really odd edge-cases which may require me to possibly use fully-qualified identifiers anyways if necessary, which is more complex. How do you think about these edge-cases which may occur. Should we possibly "ignore" them for now?

Case 1

Suppose following:

• Library A has annotation NonNullFields and NonNullApi
• Library B has annotation NonNullFields

Within one source file we now have two locations:

@NonNullFields
public static class X {
}

and

@NonNullApi
@NonNullFields
public static class Y {
}

On class X we can easily replace A's @NonNullFields with B's @NonNullFields. On class Y we will realize we can not replace both annotations, because our library B only supports @NonNullFields. As consequence, we do not touch class Y. => In the end we kept importing A's @NonNullFields, but also added B's @NonNullFields which now clash.

Case 2

Suppose following:

• Library A has annotation NonNull which does not work on packages (e. g. org.openrewrite.internal.lang.NonNull)
• Library B has annotation NonNull which does not work on packages and classes (e. g. org.springframework.lang.NonNull)

Within our code base we have the following class:

@NonNull
public static class Y {

    @NonNull
    String nonNullVariable = "";
}

On the member variable we can easily replace A's @NonNull with B's @NonNull On the class we might not have a replacement (for the example spring-core, there would be the option to replace with @NonNullFields and @NonNullApi, but let us forget that for a second). As consequence we do not touch the annotation on the class. => In the end we kept importing A's @NonNull, but also added B's @NonNull which now clash.

[knutwannheden]

Yes, your use cases sound valid, unfortunately. And there are probably also other recipes which could have similar issues, which is why we are thinking about solving it in JavaTemplate.

Maybe we could for the time being do it this way then: Replace the annotations with fully qualified annotations, remove the import and finally have a visitor which adds an import (assuming there is no more conflict) and replaces the fully qualified references. This latter step was recently requested as a recipe anyway and would be fairly easy to achieve even without JavaTemplate.

So you don't have to solve yet another problem, I could look into providing this last recipe. Unfortunately I am on vacation next week. So unless you feel inclined to tackle that as well (in a separate PR) or find some other solution, you might have to wait another week, unless I find an hour or so to look into this.

[AlexanderSkrock]

Sounds promising and I would be glad if you could implement that functionality. Anyway, if you feel having too few time, I could also give it a try. Either way, no hurry on that.

Meanwhile I will finish the collection of known nullability annotations (edit: just finished :D ) and will prepare a couple yaml recipes for migrating to at least javax, jakarta, openrewrite (for internal usage possibly) and spring-core annotations. For some of the other providers I feel they are not as complete to build a "complete migration recipe". Oh, and not to forget I missed some logging yet, to inform the user in cases, we were not able to find replacements.

[AlexanderSkrock]

@knutwannheden , the recipe you thought of is the same as in this issue it seems: https://github.com/openrewrite/rewrite/issues/3094

[knutwannheden]

Correct, that's the one.

[knutwannheden]

@AlexanderSkrock #3094 is now fixed and you should be able to use doNext() to shorten the fully qualified type references.

[AlexanderSkrock]

Thank you a lot, @knutwannheden ! I will try to integrate that recipe asap.

By the way, I love that building block system that OpenRewrite allows. Combining core functionality to build bigger recipes while focusing on what actually matters to your recipe.

[knutwannheden]

I fixed a couple of more edge cases around the shortening of qualified names, including what you mentioned regarding types in the same package.

[AlexanderSkrock]

Almost done! Seems only logging is missing to notify the user, whenever replacement was not possible, so users can initiate manuell steps to change these occurences. Tho, I would keep these on debug level, to not spam on casual runs.

I saw that slf4j is used throughout the project. Does OpenRewrite also come if an specific implementation of that api, that I should add as dependency to rewrite-java? I was not able to spot one. Or doesn't OpenRewrite come with a specific implementation and it is up to the user of OpenRewrite?

[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]

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!