Standardize nullability annotations

[timtebeek]

There's no shortage of nullability annotations; we even have our own, which are not always consistently applied. Lombok has a decent list of mostly-interchangeable nullability annotations, although they are missing Jakarta & OpenRewrite. And while we already have the ChangeType recipe to switch these over individually, it might make sense to have a configurable dedicated recipe to switch between implementations.

non-null	nullable
javax.annotation.Nonnull	javax.annotation.Nullable
jakarta.annotation.Nonnull	jakarta.annotation.Nullable
org.eclipse.jdt.annotation.NonNull	org.eclipse.jdt.annotation.Nullable
org.jetbrains.annotations.NotNull	org.jetbrains.annotations.Nullable
org.netbeans.api.annotations.common.NonNull	org.netbeans.api.annotations.common.NullAllowed
androidx.annotation.NonNull	androidx.annotation.Nullable
android.support.annotation.NonNull	android.support.annotation.Nullable
org.checkerframework.checker.nullness.qual.NonNull	org.checkerframework.checker.nullness.qual.Nullable
edu.umd.cs.findbugs.annotations.NonNull	edu.umd.cs.findbugs.annotations.Nullable
org.springframework.lang.NonNull	org.springframework.lang.Nullable
org.jmlspecs.annotation.NonNull	org.jmlspecs.annotation.Nullable
org.openrewrite.internal.lang.NonNull	org.openrewrite.internal.lang.Nullable

[AlexanderSkrock]

I would like to take on this one.

[timtebeek]

Great to hear! I think in the past we'd discussed to maybe add parameters to declarative yaml recipes, which could be a good fit here, although I can't find that issue right now. Likely best tackled separately and out of scope for this one.

In this case an imperative Java recipe that merely calls out a bunch of ChangeType recipes could already help a lot, potentially aided by some applicability checks on whether the corresponding dependency containing the target annotation is available, and/or add required dependency if needed. Not all aspects have been thought through fully yet, but curious to hear your thoughts on these!

[AlexanderSkrock]

I read your comment just now, @timtebeek, two questions came to my mind.

• What do you mean by parameters for declarative recipes. the variables annotated with @Option?
• Does ChangeType also support replacing usage of types as Annotations? If that is possible we would not need another recipe. Later in my comment, I thought about implementing a ReplaceAnnation recipe ,which would not be necessary anymore.

Beside that, these were my thoughts, when I initially read the issue.

A recipe to standardize nullability annotations

Interface for usage

@Value
@EqualsAndHashCode(callSuper = false)
public class UnifyNullabilityAnnotations extends Recipe {

    @Option(displayName = "Nullable Annotation to use",
            description = "All other nullable annotations will be replaced by this one",
            example = "org.openrewrite.internal.lang.Nullable")
    String nullableAnnotation;

    @Option(displayName = "Non-null annotation to use",
            description = "All other non-null annotations will be replaced by this one",
            example = "org.openrewrite.internal.lang.NonNull")
    String nonNullAnnotation;

    @Override
    public String getDisplayName() {
        return "Unify nullability annotations";
    }

    @Override
    public String getDescription() {
        return "Define one null and one non-null annotation to be used. All divergent annotations will be replaced.";
    }
}

Also, I thought about the following use-cases:

1. A project uses an internal self-developed annotation and wants to migrate to e. g. Lombok
2. A project uses a less common annotation which is not part of our list of known nullability annotations.

That would not be possible with above configuration parameters, as our known nullability annotations do not include that one. As potential solution we could offer two more optional parameters as follows:

@Option(displayName = "Additional annotations to be recognized as nullable annotations",
        description = "These annotations will also be recognized as nullable annotations. Use this parameter to replace additional annotations which are not contained in our default list.",
        example = "org.openrewrite.internal.lang.Nullable",
        required = false)
@Nullable
List<String> additionalNullableAnnotations;

@Option(displayName = "Additional annotations to be recognized as non-null annotations",
        description = "These annotations will also be recognized as non-null annotations. Use this parameter to replace additional annotations which are not contained in our default list.",
        example = "org.openrewrite.internal.lang.NonNull",
        required = false)
@Nullable
List<String> additionalNonNullAnnotations;

As alternative to those additional parameters, I will describe another possible approach on those edge-cases in section Execution.

Validation

The main parameters nullableAnnotation and nonNullAnnotation definitely should be validated to accessible. If not, there is no point in executing this recipe. Same goes two additional parameters we might include: additionalNullableAnnotations and additionalNonNullAnnotations. For replacement, we do not necessarily need to the to be on classpath, as it will not be used in the rewritten code anyway. In my opinion, If the annotations are not available, changes are high that there is a typo.

Execution

I would implement another recipe ReplaceAnnotation similar to the existing RemoveAnnotation (see Github). It will also match used annotations in the code base, but instead of removing it will replace them with another annotation. For the sake of documentation we need to clean up imports of removed annotations and add an import of the replacement annotation, if necessary.

This logic could be directly implemented within UnifyNullabilityAnnotations, but I would like this one to be more reusable. I could think of other use cases where to want to migrate from framework A to framework B. The recipe UnifyNullabilityAnnotations will then be a very tailored implementation, focusing on simple usability instead of a high grade of flexibility (e. g. annotation pattern matching).

Actually, simplicity would be a good reason to refrain from the two aforementioned additional parameters additionalNullableAnnotations and additionalNonNullAnnotations. I am very ambivalent about those. For described use-cases it would be much easier to utilize. Contrariwise, if we remove these parameters, users would need to fall back utilizing a more complex configuration of ReplaceAnnotation.

Thoughts on annotations and their dependencies

In case an annotation can not be found on classpath, we could either give a hint which dependency is to be added or even automatically add the dependency to the project. Obviously, this only applies to the annotations configured via nullableAnnotation and nonNullAnnotation.

As requirement for both ideas, is the knowledge about corresponding dependencies containing each annotation. For our list of known annotations, we can simply add this information. Of course, it would decrease maintainability, tho chances that coordinates of these dependencies change are pretty low. I would refrain from introducing this logic to possible configured additional annotations (if we implement them for two reasons. Firstly, it would make configuring additional annotations more complex and secondly users should know coordinates of their special annotations as they are already part of their code base.

An additional feature could be automatically add necessary dependencies. Sounds pretty cool, but I had no solution on determining what version to use. As we do not know better, we could only add the newest version by default. As workaround we could offer another configuration option to define a version, but there is a huge disadvantage to this as it would introduce another location to keep in mind when upgrading dependencies. Concluding, if we would like to offer this feature, we should always add the latest version of the corresponding dependency. For adding dependencies there is already a recipe for both Gradle and Maven.

[timtebeek]

Thank for the detailed analysis! Here's a reply to your questions at the top:

What do you mean by parameters for declarative recipes. the variables annotated with @Option?

So with declarative recipes I mean the yaml format to configure recipes. We've discussed in the past to potentially add parameters to recipes purely defined in yaml, but have not gotten around to that yet. This could have been an approach for this recipe: only define the recipe in yaml as taking two options, and apply a list of ChangeType recipes that way.

Defining recipes purely in Yaml is limited though, and might not have worked all the way through with some of the checks that we want to do on availability of annotations and adding dependencies where necessary. I suggest we stick to a Java recipe here for now, which links together other known recipes. That keeps the logic out of the implementation as much as possible, and allows us to potentially revisit this as a yaml only recipe in the future.

Does ChangeType also support replacing usage of types as Annotations? If that is possible we would not need another recipe. Later in my comment, I thought about implementing a ReplaceAnnation recipe ,which would not be necessary anymore.

ChangeType does indeed support swapping one annotation for another; here is an example in rewrite-spring. So for anyone using a homegrown nullable annotation, I'd suggest they first run a ChangeType themselves, before running this recipe to standardize other known annotations onto a single pair of annotations.

That removes the need for any specific ReplaceAnnotation recipe, or additionalNullableAnnotations / additionalNonNullAnnotations options, making this recipe easier to implement.

Thoughts on annotations and their dependencies

I think it's fine to add the latest version of the corresponding dependency, only if using the annotations, and not already available transitively. Both of these restrictions are options of using Add dependency.

---

General suggestion

I think this implementation would work best as a Java recipe that takes two options, as you've sketched already, and then replaces a suite of known alternative nullability annotations. This Java recipe can then be used a number of times from yaml to define a couple of suggested recipes that people can run without further configuration. For example:

type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.nullability.UseJavaxNullabilityAnnotations
displayName: Use javax nullability annotations
description: Replace other well known Java nullability annotations with javax.
recipeList:
  - org.openrewrite.java.nullability.StandardizeNullabililtyAnnotations
      nullableAnnotation: javax.annotation.Nullable
      nonNullAnnotation: javax.annotation.Nonnull
  - org.openrewrite.maven.AddDependency:
      groupId: javax.annotation
      artifactId: javax.annotation-api
      version: 1.x
      onlyIfUsing: javax.annotation.*
      acceptTransitive: "True"
---
type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.nullability.UseJakartaNullabilityAnnotations
displayName: Use jakarta nullability annotations
description: Replace other well known Java nullability annotations with jakarta.
recipeList:
  - org.openrewrite.java.nullability.StandardizeNullabililtyAnnotations
      nullableAnnotation: jakarta.annotation.Nullable
      nonNullAnnotation: jakarta.annotation.Nonnull
  - org.openrewrite.maven.AddDependency:
      groupId: jakarta.annotation
      artifactId: jakarta.annotation-api
      version: 2.x
      onlyIfUsing: jakarta.annotation.*
      acceptTransitive: "True"
---
...

[AlexanderSkrock]

Thank you for your extensive response and also some useful links!

Having the already existent recipes ChangeType and AddDependency, we will barely have any additional logic to implement except for validations. Pretty charming!

I am a fan of your ready-to-use recipes, thank you for these. Should I embed them somewhere as documentation or did you think of eample implementations, which can directly be used?

One last thought on your example recipes. I think we would either have to integrate the AddDependency recipe as sub recipe inside StandardizeNullabililtyAnnotations or swap the order and potentially remove onlyIfUsing: xxx. My concern, is that either StandardizeNullabililtyAnnotations already adds the latest version before AddDependency ran or AddDependency runs before StandardizeNullabililtyAnnotations but these annotations are not yet, and in consequence the dependency will not be added.

I could think of following:

• Refrain on auto-adding the neccessary dependency inside StandardizeNullabililtyAnnotations and only check if the annotation is available, possibly giving a hint on the necessary dependency
• Implement read-to-use recipes as you already commented:

type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.nullability.UseJakartaNullabilityAnnotations
displayName: Use jakarta nullability annotations
description: Replace other well known Java nullability annotations with jakarta.
recipeList:
    - org.openrewrite.maven.AddDependency:
      groupId: jakarta.annotation
      artifactId: jakarta.annotation-api
      version: 2.x
      onlyIfUsing: jakarta.annotation.*
      acceptTransitive: "True"
  - org.openrewrite.java.nullability.StandardizeNullabililtyAnnotations
      nullableAnnotation: jakarta.annotation.Nullable
      nonNullAnnotation: jakarta.annotation.Nonnull

(This would required us to constantly update the version as new versions are released.)

Advantages are, these are easy-to-use, require zero configuraton and allow to switch between annotations as easy as changing the recipe name. As downside, if the recipe StandardizeNullabililtyAnnotations is used aside the preconfigured recipes, users would probably need to an AddDependency recipe manually because they are tightly coupled or manually change dependencies. Anyway, this seems okay to me.

[timtebeek]

Should I embed them somewhere as documentation or did you think of example implementations, which can directly be used?

I think the ready to use preconfigured recipes can go into a new file in this folder: https://github.com/openrewrite/rewrite/tree/main/rewrite-java/src/main/resources/META-INF/rewrite We already have a number of recipes there that are no more than a combination of existing recipes. By providing these ready made recipes we ensure people are able to easily target a pair of annotations.

We can play around with the required order of the AddDependency versus ChangeType once we have a suite of tests, optionally using for instance mavenProject to verify that we add the required dependency only when necessary. The reason I put it last was because I would not want us to add any dependency when we don't use any nullability annotations in a project (yet), but that could maybe also be achieved with an applicability test.

This would required us to constantly update the version as new versions are released.

There would be no need to update the dependency version when we use dependency version selectors. I wasn't explicit about this earlier, but 2.x actually does work by itself, and was not a placeholder for a particular version. :)

We'd prefer to have recipes that do not have any manual steps, either in maintaining the recipe, or after running it later. I think that'd be achievable here as well, and is much preferred when running at scale.

[AlexanderSkrock]

Basically I was done with the initial implementation, just missing some more preconfigured recipes for the various frameworks. While thinking about more specific annotation, I realized the replacement of nullability annotations might be slightly more complex than I initially thought.

So, what's the problem: There are some annotations that can be defined in a higher level, e. g. on a class or a package. Some might be applicable on those in addition to methods or fields, but some may not be. A possible example would be org.openrewrite.internal.lang.NonNullApi which is not applicable on all element types. For example Javax annotations have no limitations on their annotations, but rewrite's own or lombok do have. Furthermore, there is an additional problem. The scope of afore mentioned example NonNullApi is only methods including their parameters and return values. => When, having one of these edge cases, a simple replacement is not possible. Further, it might break code.

A possible approach We could define all known nullability annotations as follows:

private static final Set<NullabilityAnnotation> KNOWN_NULLABILITY_ANNOTATIONS = Stream.of(
        nullable("javax.annotation.Nullable").withAllTargets().withAllScopes(),
        nonNull("javax.annotation.Nonnull").withAllTargets().withAllScopes(),

        nullable("org.openrewrite.internal.lang.Nullable").withAllTargets().withAllScopes(),
        nullable("org.openrewrite.internal.lang.NullFields").withTargets(ElementType.PACKAGE, ElementType.TYPE).withScopes(ElementType.FIELD),
        nonNull("org.openrewrite.internal.lang.NonNull").withAllTargets().withAllScopes(),
        nonNull("org.openrewrite.internal.lang.NonNullFields").withTargets(ElementType.PACKAGE, ElementType.TYPE).withScopes(ElementType.FIELD),
        nonNull("org.openrewrite.internal.lang.NonNullApi").withTargets(ElementType.PACKAGE, ElementType.TYPE).withScopes(ElementType.METHOD, ElementType.PARAMETER)
).collect(Collectors.toSet());

Also we would change the api and allow multiple nullable and not-null annotations to be configured:

@Option(displayName = "Nullable annotations to use",
        description = "All other nullable annotations will be replaced by this one.",
        example = "javax.annotation.Nullable")
List<NullabilityAnnotation> nullableAnnotations;

@Option(displayName = "Non-null annotations to use",
        description = "All other non-null annotations will be replaced by this one.",
        example = "javax.annotation.Nonnull")
List<NullabilityAnnotation> nonNullAnnotations;

For example our UseJavaxNullabilityAnnotations recipe would change as follows:

type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.maven.java.UseJavaxNullabilityAnnotations
displayName: Use javax nullability annotations
description: Replace other well known Java nullability annotations with javax.
recipeList:
  - org.openrewrite.java.StandardizeNullabilityAnnotations:
      nullableAnnotations:
        - fqn: javax.annotation.Nullable
      nonNullAnnotations:
        - fqn: javax.annotation.Nonnull
  - org.openrewrite.maven.AddDependency:
      groupId: javax.annotation
      artifactId: javax.annotation-api
      version: 1.x
      onlyIfUsing: javax.annotation.*
      acceptTransitive: "True"

For the sake of usability, I would say do not have to define targets and scopes again for annotations that are contained in our known pool of annotations, these will be inserted on the fly. Still, it would be possible to add custom annotations as standardized annotations, by simply by adding those fields. (Only question would remain, regarding migrating away from custom annotations, which would again need another parameters, with the same structure for manually adding known annotations). When it comes to implementation, it would be a bit more than just chaining a couple ChangeType recipes. When ever we approach a nullability annotation, we could lookup a matching annotation with an acceptable target type. For, scopes we could either only 1:1 replacement or splitting/combing 1 in 2. An example could be replacing @Nonnull of Javax on class level with NonNullFields and NonNullApi from rewrite and vice versa. That way, we could make sure the semantics stay the same.

Of course, we could still stick to the minimalistic approach, tho it might not be feature-complete and lead to issues in some edge cases. So, what do you think, @timtebeek ?

Besides, that question, I was not able to successfully run my testcases, because sub recipes were not called. My sub recipes were added, but then skipped. I explained it a bit more in detail here. Could you also have a look on this, @timtebeek . I would really appreciate help on this!

Kind regards Alexander Skrock

[timtebeek]

Thanks a lot for your thorough work on this one @AlexanderSkrock ; I'd already been following along for a bit in your branch; feel free to open a draft pull request such that it's easier to checkout a local copy here, see the test run results, provide feedback and potentially push small changes.

I agree with you that we need some way to limit recipe execution to only produce valid changes; not yet sure about the best way to achieve that, but perhaps we can start by only providing pre-made nullability recipes to annotations with a wide enough scope & target? That could simplify the implementation and allow us to get a first version with which we can then gather feedback. If people then themselves use the base StandardizeNullabilityAnnotations with incompatible annotation scope/target that's on them. Not sure what the options would be to detect such inaccurate use, but at least it'll clearly fail with compilation errors. Does that sound feasible?

On running the test cases: typically when we see no changes made it's down to missing types on the test execution classpath. I don't fully know if the buildGradle that you use is also taken into account when determining the test classpath, but it might be missing a dependency before you're able to detect and replace org.openrewrite.internal.lang.NonNull. More often we use the RecipeSpec to define the classpath, as that has less boilerplate to setup. Does that give you some leads to try to get the tests to run? I could have a go as well if you open a draft and allow pushing changes.

Great to see your progress on this!

[AlexanderSkrock]

I just had some time to have another look on this. For convenience I created a draft pull request, as you proposed.

Regarding that "simple-first" approach, I will try to check which replacements would be safe to do. Depending on the result, it might also not be possible to do it this way and rather implement a separate visitor, which evaluates every use of annotation and tries to match possible alternatives as I described earlier. We will see.

Sadly, the pipeline also fails to execute my testcases. The recipe does not do any changes at all, thereby I assume it is the similar reason as for local execution. All ChangeType recipes are propertly added to sub results and available during execution.

From my understanding, the process for recipe execution currently is as follows:

1. We schedule a RecipeRun
2. We create a RecipeRunStats object, which immediately contains the recipe itself and its sub recipes as called
3. We do test for applicability
4. We do recipe validation
5. We retrieve the recipe's visitor and execute
6. We get the recipe list
7. We skip recipes within the list that are already contained in the called list
8. We execute sub recipes

But if I was right, this would be a general problem. Searching the code base, I was only able to find uses of doNext for applicability checks and during visitor retrieval which both work differently in context of afore described process. But, then I wonder what's the difference to DeclarativeRecipes because they use exactly the same mechanism. They add sub recipes ahead of execution, not within the constructor, but on initialization. Still, their usage is similar enough.

Oh, I also checked your hint on classpath configuration, but the ChangeType recipe is within the same artifact or module and this recipe contains no validation whether the type to replace is available or not. So I doubt, it could be the reason in my case.

I would greatly appreciate any additional ideas. Also, feel free to checkout my draft to execute test cases locally.

Kind regards Alexander Skrock

[timtebeek]

Quick note here to say I'm traveling this week, so can't fully dive in now. Maybe @kunli2 or @knutwannheden can have a look at fixing your test execution and answering the above? 🙏🏻

[AlexanderSkrock]

After a long break I was able to go trough my changes and the open issues regarding annotations on modifiers. But this needs some additional work around the java visitor which should not be implemented by the way in my opinion. => So I created #3502. After that issue is solved I will have a look on this one again.

[timtebeek]

Folks subscribed to this issue might be interested to read our latest blog: https://www.moderne.ai/blog/mass-migration-of-nullability-annotations-to-jspecify

The recipes we used there are available in OpenRewrite to migrate our internal nullability annotations to JSpecify. https://github.com/openrewrite/rewrite/blob/26f62e2080730f83c85592ecfc6ee583c24f1f4d/rewrite-core/src/main/resources/META-INF/rewrite/jspecify.yml#L17-L43

Might be a good moment to get the PR attached here across the line; that's been dormant for a while.