openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.27k stars 340 forks source link

Move type annotations from methods and fields closer to the types #3880

Closed vlsi closed 6 months ago

vlsi commented 11 months ago

What problem are you trying to solve?

I don't like how OpenRewrite moved @Nullable from type to a method.

-  @Nullable String getSlotName();
+  @Nullable
+  @Override
+  String getSlotName();

Describe the solution you'd like

@Nullable is a type annotation, so it applies to String type rather than getSlotName() method.

Since OpenRewrite knows the exact definition of the annotation, it should be able to infer that @Nullable is a type annotation while @Override is not.

So I would like the formatting to be

  @Override
  @Nullable String getSlotName();

The emerging jspecify uses only TYPE_USE, see https://github.com/jspecify/jspecify/blob/ee91db649ce2de81d39b55b52345730b3f0f2531/src/main/java/org/jspecify/annotations/Nullable.java#L142, so users should prefer

    @Nullable String newValue;

rather than

    @Nullable
    String newValue;
knutwannheden commented 11 months ago

At the moment I am not even sure that OpenRewrite knows the full definition of annotation types. While we have type attribution for annotations (like @Nullable) and the type attribution also lists the annotations present on that annotation type (e.g. @Target), we currently unfortunately don't include the annotation parameter values in the type attribution. So we don't know if @Target includes TYPE_USE or not. At parse time of the source this is available, but in the LST (specifically the type attribution) this information is lost.

We have an open issue for this here: #3504. Having a concrete use case for this enhancement is really useful. Should we add that as a comment to the mentioned issue and close this one?

vlsi commented 11 months ago

Deduplicating issues sounds reasonable

timtebeek commented 6 months ago

Thanks! Copied the above there just now