rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

ShortenFullyQualifiedNames don't shorten FQN Annotations

Open MBoegers opened this issue 1 year ago • 6 comments

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?

MBoegers avatar Jun 01 '24 07:06 MBoegers

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?

knutwannheden avatar Jun 01 '24 07:06 knutwannheden

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 😬

MBoegers avatar Jun 01 '24 08:06 MBoegers

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.

knutwannheden avatar Jun 02 '24 14:06 knutwannheden

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

MBoegers avatar Jun 02 '24 16:06 MBoegers

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.

MBoegers avatar Jun 04 '24 20:06 MBoegers

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

timtebeek avatar Jun 06 '24 14:06 timtebeek