Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add modifiers for ParenthesizedTypeTree #3862

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add modifiers for ParenthesizedTypeTree #3862

wants to merge 3 commits into from

Conversation

kunli2
Copy link
Contributor

@kunli2 kunli2 commented Dec 29, 2023

This is to support cases like openrewrite/rewrite-kotlin#571 which has modifiers before redundant parens, and we present redundant paren to a ParenthesizedTypeTree, so adding modifiers to ParenthesizedTypeTree in this PR.

An example

              class SomeReceiver
              suspend inline fun SomeReceiver  .   method(
                  body  : suspend  (    SomeReceiver .  () -> Unit    )
              ) {}

@knutwannheden
Copy link
Contributor

These parentheses are annoying. I am here also wondering if Kotlin requires a specific order between annotations and modifiers. As far as I can see there is no such order, so if we now add modifiers here, we will still have that problem left to solve. See https://kotlinlang.org/docs/reference/grammar.html#type.

I get the impression we might need something like an AnnotationOrModifier union type (interface)...

@traceyyoshima
Copy link
Contributor

These parentheses are annoying. I am here also wondering if Kotlin requires a specific order between annotations and modifiers. As far as I can see there is no such order, so if we now add modifiers here, we will still have that problem left to solve. See https://kotlinlang.org/docs/reference/grammar.html#type.

I get the impression we might need something like an AnnotationOrModifier union type (interface)...

Adding a code example:

class SomeReceiver
suspend inline fun SomeReceiver  .   method(
    body  : @A1 suspend @A2 (    SomeReceiver .  () -> Unit    )
) {}

@Target(AnnotationTarget.TYPE)
annotation class A1
@Target(AnnotationTarget.TYPE)
annotation class A2

@kunli2
Copy link
Contributor Author

kunli2 commented Dec 29, 2023

Since in PSI, those parentheses are just flattened to tokens only and the nested parentheses structures are not preserved, so building nested structures is challenging for us.

I think having List<J.Modifier> modifiers and List<J.Annotation> leadingAnnotations are a common way now we have used in many models. (this is an example)

I think the mixed modifier and annotation in random order can be supported in the same way we used for others.

For the example provided by Tracey above,

class SomeReceiver
suspend inline fun SomeReceiver  .   method(
    body  : @A1 suspend @A2 (    SomeReceiver .  () -> Unit    )
) {}

Models

  1. @A1 suspend @A2 ( SomeReceiver . () -> Unit ) is a J.ParenthesizedTypeTree
  2. @A1 suspend is a J.Modifier and @A1 is stored in J.Modifier#annotations.
  3. @A2 is stored in J.ParenthesizedTypeTree#annotations

@kunli2 kunli2 marked this pull request as draft December 29, 2023 18:50
@knutwannheden
Copy link
Contributor

Yes, I forgot that our modifiers can also have annotations. But the annotations on modifiers are leading annotations (despite the field being declared as the last field) and typically the pattern we have is:

        List<Annotation> leadingAnnotations;
        List<Modifier> modifiers;
        OtherTypeWithAnnotations other;

While our field is called annotations (and not leadingAnnotations), I still think we should make that clearer by moving the modifiers field so it comes first. Because I assume the idea here is indeed that any annotations after the modifiers would be stored in the annotations field.

@kunli2
Copy link
Contributor Author

kunli2 commented Jan 2, 2024

Yes, I forgot that our modifiers can also have annotations. But the annotations on modifiers are leading annotations (despite the field being declared as the last field) and typically the pattern we have is:

        List<Annotation> leadingAnnotations;
        List<Modifier> modifiers;
        OtherTypeWithAnnotations other;

While our field is called annotations (and not leadingAnnotations), I still think we should make that clearer by moving the modifiers field so it comes first. Because I assume the idea here is indeed that any annotations after the modifiers would be stored in the annotations field.

Yes, annotations on modifiers are leading annotations, and they are supposed to be stored in modifiers, agree that moving modifiers to in front of annotations makes it more clear to reflect the order in the code. revised the code a bit and move modifiers before annotations

@traceyyoshima
Copy link
Contributor

Yes, annotations on modifiers are leading annotations, and they are supposed to be stored in modifiers, agree that moving modifiers to in front of annotations makes it more clear to reflect the order in the code. revised the code a bit and move modifiers before annotations

To clarify, Yes, annotations on modifiers are leading annotation is not true.

class Foo {
    List<Annotation> leadingAnnotations;
    List<Modifier> modifiers;
    OtherTypeWithAnnotations other;
}

The modeling that Knut linked shows that we represent leading annotations as a separate field before modifiers. Then have a list of modifiers, since annotations may exist between modifiers.

Example:

@LeadingAnnotation private @AnnotationOnModifier static foo() {}` ;

In terms of modeling, fields are declared in the order they are parsed, which is what lead to the confusion.

This case is odd, since there may be annotations following the modifiers without a container in the model to add the annotations to:

body  : @A1 suspend @A2 (    SomeReceiver .  () -> Unit    )`.

In this case, we would either:

List<Annotation> leadingAnnotations
List<Modifier> modifiers
List<Annotation> otherAnnotations

or

List<Modifier> modifiers
List<Annotation> annotations

@kunli2
Copy link
Contributor Author

kunli2 commented Jan 3, 2024

List<Modifier> modifiers

I didn't see any contradiction here, we are referring to the 2nd option,

List<Modifier> modifiers
List<Annotation> annotations

so the annotations on modifiers are leading annotations is true for this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants