openrewrite / rewrite-kotlin

Work-in-progress implementation of Kotlin language support for OpenRewrite.
Apache License 2.0
38 stars 11 forks source link

New LST models to support use-site annotation and deprecate `AnnotationUseSite` marker #549

Closed kunli2 closed 6 months ago

kunli2 commented 6 months ago

After discussion the design doc with the team, we decide to use option 3 to support use-site annotation instead of the AnnotationUseSite marker.

Here are more details of option 3 Two models for single and multiple use-site included annotations, and both of them will be an annotationType field of the J.Annotation.

Schema:

class AnnotationType implements K, NameTree {
    Space prefix;
    @Nullable JRightPadded<Expression> useSite;
    J.Annotation callee;
  }

 class MultiAnnotationType implements K, NameTree {
    Space prefix;
    @Nullable JRightPadded<Expression> useSite;
    JContainer<J.Annotation> annotations;
  }

Use case examples:

  1. Single use-site annotation
@field : Ann1
-----------------------
"@field : Ann1" is a J.Annotation
    annotationType = K.AnnotationType
        arguments = []
"@field : Ann1" is a K.AnnotationType
    useSite = field + rightPadding
        callee = Ann1
  1. Single annotation in bracket

    @field : [ Ann1 ]
    -----------------------
    "@field : [ Ann1 ]": J.Annotation
    annotationType = K.MultiAnnotationType
        arguments = []
    "@field : [ Ann1 ]": K.MultiAnnotationType, 
        useSite = field + rightPadding
        annotations = [Ann1]
  2. Multi annotations

    
    @field : [Ann1 Ann2]
    -----------------------
    "@field : [Ann1 Ann2]": J.Annotation
    annotationType = K.MultiAnnotationType
        arguments = []
    "@field : [Ann1 Ann2]": K.MultiAnnotationType
    useSite = field + rightPadding
        annotations = [Ann1, Ann2]
4. Multi annotations without target

@[ Ann1 Ann2]

"@[ Ann1 Ann2]": J.Annotation annotationType = K.MultiAnnotationType arguments = [] "@[ Ann1 Ann2]": K.MultiAnnotationType: useSite = J.Empty annotations = [Ann1, Ann2]


Notes:
This PR requires a small rewrite code change to `J.Annotation#getSimpleName()`
from
```java
        public String getSimpleName() {
            return annotationType instanceof Identifier ?
                    ((Identifier) annotationType).getSimpleName() :
                    ((J.FieldAccess) annotationType).getSimpleName();
        }

to

        public String getSimpleName() {
            if (annotationType instanceof Identifier) {
                return ((Identifier) annotationType).getSimpleName();
            } else if (annotationType instanceof J.FieldAccess ) {
                return ((J.FieldAccess) annotationType).getSimpleName();
            } else {
                return annotationType.toString();
            }
        }
knutwannheden commented 6 months ago

@kunli2 AFAICT this will still require us to add a getSimpleName() method to NameTree. I will create a PR for this and then ask Jonathan for his input on that.

knutwannheden commented 6 months ago

@kunli2 AFAICT this will still require us to add a getSimpleName() method to NameTree. I will create a PR for this and then ask Jonathan for his input on that.

Turns out we don't need that quite yet after all. We had a call site in KotlinPrinter which called J.Annotation#getSimpleName() for all annotations: https://github.com/openrewrite/rewrite-kotlin/pull/549/commits/12f31a05ab935d5e95b89c905f60d7333d253f23. By removing that the tests now pass.

Since recipes should be allowed to call this method, we should still address this problem. In a first step we could maybe just return the empty string, if the annotation type isn't a J.Identifier or J.FieldAccess.

knutwannheden commented 6 months ago

I've now gone ahead and made the changes to J.Annotation#getSimpleName(): https://github.com/openrewrite/rewrite/commit/f7772889cb6e28427bf1ac78566fd6df45f47ba5