openrewrite / rewrite-kotlin

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

Introduce `K.UseSite` and deprecate `AnnotationUseSite` marker #542

Closed kunli2 closed 6 months ago

kunli2 commented 6 months ago
  1. Introduce K.UseSite which is a NameTree, and will be the annotationType in a J.Annotation.
  2. Deprecate AnnotationUseSite marker
  3. For empty name use-site annotation like this @[ Anno1 Anno2 ], the target will be an J.Empty

Notes: This PR requires a small rewrite code change to J.Annotation#getSimpleName() from

        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();
            }
        }
kunli2 commented 6 months ago

@kunli2 I think it would really help if we could draw object diagrams showing what the LST model looks like for the various cases like:

  • @Ann
  • @field:Ann
  • @[Ann1 Ann2]
  • @field:[Ann1 Ann2]

Sure, here are all the cases, 1 regular annotation 4 use-site annotations, and the LST structure. K.UseSite will be the annotationType of the J.Annotation, and the following annotations will be parameters


// case 0, Non use-site, regular annotation
@Ann1

\---J.Annotation | "@Ann1"
    \---J.Identifier | "Ann1"

// case 1, use-site, implicit bracket
@field : Ann1

\---J.Annotation
    |----J.Annotation | "@Ann1"    // Parameter
    |  \---J.Identifier | "Ann1"
    \---K.UseSite | isImplicitBracket = true 
       \-------J.Identifier | "field"

// case 2, use-site, explicit bracket
@field : [ Ann1 ]

\---J.Annotation
    |---J.Annotation | "@Ann1"      // Parameter
    |   \---J.Identifier | "Ann1"
    \---K.UseSite | isImplicitBracket = false 
        \-------J.Identifier | "field"

// case 3, use-site, multi annotations with explicit bracket
@field : [Ann1 Ann2]

\---J.Annotation
    |-----------J.Annotation | "@Ann1"      // Parameter 1
    |   |       \---J.Identifier | "Ann1"
    |   \-------J.Annotation | "@Ann2"      // Parameter 2
    |           \---J.Identifier | "Ann2"
    \---K.UseSite | isImplicitBracket = false
        \-------J.Identifier | "field"

// case 4, use-site, empty target
@[ Ann1 Ann2]

\---J.Annotation
    |-----------J.Annotation | "@Ann1"      // Parameter 1
    |   |       \---J.Identifier | "Ann1"
    |   \-------J.Annotation | "@Ann2"      // Parameter 2
    |           \---J.Identifier | "Ann2"
    \---K.UseSite | isImplicitBracket = false
        \-------J.Empty
knutwannheden commented 6 months ago

Case 1 seems weird to me. Can we come up with a scheme where UseSite owns both field and Ann1 and there is only one J.Annotation? Having Ann1 as a parameter to some dummy annotation seems very strange.

kunli2 commented 6 months ago

Case 1 seems weird to me. Can we come up with a scheme where UseSite owns both field and Ann1 and there is only one J.Annotation? Having Ann1 as a parameter to some dummy annotation seems very strange.

If I understand this correctly, you do want to put both field and Ann1 to K.UseSite, and K.UseSite is still an annotationType field in a J.Annotation, and the J.Annotation will have empty parameters, if so, I think it's doable, and seems this makes sense too.

please confirm so I can come up with a new PR with this approach.

knutwannheden commented 6 months ago

please confirm so I can come up with a new PR with this approach.

Yes, that is what I meant. For the multi-annotation case we will probably still require a "dummy" annotation.

Possibly something along these lines (pseudocode):

public interface K extends J {

  final class AnnotationType implements K, NameTree, Expression {
    Space prefix;
    JRightPadded<J.Identifier> useSite;
    TypeTree typeTree;

    public JavaType getType() {
      return typeTree.getType();
    }
  }

  final class MultiAnnotationType implements K, NameTree, Expression {
    Space prefix;
    @Nullable
    JRightPadded<J.Identifier> useSite;

    public JavaType getType() {
      return null;
    }
  }
}

@kunli2 What do you think?

kunli2 commented 6 months ago

we composed this doc for LST model proposals. discussed with the team and decided to go option 3. so closing out this PR and I will have a new PR with option 3