openrewrite / rewrite

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

ShortenFullyQualifiedNames don't shorten FQN Annotations #4222

Open MBoegers opened 1 month ago

MBoegers commented 1 month ago

While working on PR 4217 I discovered that ShortenFullyQualifiedNames does not work as expected for Annotations with Full Qualified Name. As Java Language Spec 9.7.1 states @packages.typename is the FQN form for an Annotation, therefore @java.lang.Overwrite should be transformed to @Overwrite.

What version of OpenRewrite are you using?

I am using the current main branch and unit tests.

How are you running OpenRewrite?

Running ShortenFullyQualifiedNamesTest with reproducing test.

What is the smallest, simplest way to reproduce the problem?

See Gist https://gist.github.com/MBoegers/9c805b9caa35144466d9f380ddbb40bd

class A {
    @java.lang.Overwrite
    void foo() {}
}

What did you expect to see?

See expectations in Gist https://gist.github.com/MBoegers/9c805b9caa35144466d9f380ddbb40bd

class A {
    @Overwrite
    void foo() {}
}

What did you see instead?

The recipe makes no changes.

Are you interested in contributing a fix to OpenRewrite?

knutwannheden commented 1 month ago

The java.lang types are unfortunately a bit tricky. The reason being that if a type with the same simple name exists in the current package, then that takes precedence over the java.lang type in the absence of any import (that is IIRC). So the visitor in question would have to know if a type by that name exists in the current package.

Is the issue also reproducible for other annotation types?

MBoegers commented 1 month ago

Yes thought about that edge case as well. If you see the gist the same problem shows up if you use a different annotation like @org.jetbrains.annotations.NotNull. Sorry the gist like was broken 😬

knutwannheden commented 1 month ago

The diff that I am getting for that second test when running it is this:

diff --git a/TypeAnnotationTest.java b/TypeAnnotationTest.java
index 1da8705..aa3a024 100644
--- a/TypeAnnotationTest.java
+++ b/TypeAnnotationTest.java
@@ -1,8 +1,10 @@ 
-import org.jetbrains.annotations.NotNull;
 import java.sql.DatabaseMetaData;
 import java.util.List;
+
 import java.lang.annotation.*;

+import org.jetbrains.annotations.NotNull;
+
 class TypeAnnotationTest {
     @NotNull
     String test() {}

So it looks like the test expection's import order is "wrong". When I align it, the test succeeds. So unless I am missing something, the only issue is with java.lang, but I don't see how that can easily be fixed in the short term and if you can live with the slight risk that the code where you apply a JavaTemplate contains a type in the current package with the same name as an unqualified java.lang type in the template, then that should be a valid workaround. I.e. not fully qualify java.lang types in the template.

MBoegers commented 1 month ago

I'll analyse and give you an answer by tomorrow evening CEST

MBoegers commented 1 month ago

I can confirm that the recipe works as expected, with the known exception to java.lang.* annotations. @knutwannheden and @timtebeek this is fine for me.

timtebeek commented 1 month ago

So do I understand correctly then that we can close this issue as a known limitation?