NullableOnMethodReturnType seems incomplete

[Bananeweizen]

I've just reviewed the new NullableOnMethodReturnType recipe after receiving the latest release locally. To me it seems incomplete:

• It handles Nullable, but not the opposite annotation ("NonNull", "NotNull" or other variants)
• it seems to handle only the rewrite.internal.Nullable annotation, no other Nullable (like checker framework, Eclipse JDT, ...) due to relying on the class reference, not the simple class name literal.

Please be aware I only read the code and didn't run any test. So I might be completely wrong with this analysis.

[timtebeek]

Hi! I'ts been developed to scratch an internal itch, and to that end makes some assumptions indeed. We have an convention of annotating the package with NonNullApi in package-info.java, which is why there's no equivalent handling of NonNull. Similarly, we consistently use our internal Nullable annotation, which is why you don't see other annotations handled.

We could make the recipe more broadly applicable by giving it an @Option of which annotation to match, such that folks can override the default with a declarative recipe. Would that be an acceptable solution for both of the cases you've mentioned?

[Bananeweizen]

Such an option should be sufficient for both cases. Is there a general convention on whether such an option should be multi-valued (which would depend on the recipe author), or whether the recipe should be listed multiple times with single values by the user? Just wondering if that makes a difference for performance...

[timtebeek]

I like simplicity, so I'd lean towards just repeating the recipe with different arguments in a recipeList:, as you can see here. The runtime characteristics would be slightly different, but negligible when compared to other recipes that call out to Maven Central.