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

[java] Use single node for annotations #2282

Merged
merged 4 commits into from
Feb 20, 2020

Conversation

oowekyala
Copy link
Member

Grammar change is described below. This also makes Annotatable use node streams.

Annotations

  • What: Annotations are consolidated into a single node. SingleMemberAnnotation, NormalAnnotation and MarkerAnnotation are removed in favour of Annotation. The Name node is removed.
  • Why: Those different node types implement a syntax-only distinction, that only makes semantically equivalent annotations have different possible representations. For example, @A and @A() are semantically equivalent, yet they were parsed as MarkerAnnotation resp. NormalAnnotation. Similarly, @A("") and @A(value="") were parsed as SingleMemberAnnotation resp. NormalAnnotation. This also makes parsing much simpler.
CodeOld ASTNew AST
@A
+ Annotation
  + MarkerAnnotation
    + Name "A"
+ Annotation "A"
@A()
+ Annotation
  + NormalAnnotation
    + Name "A"
+ Annotation
  + AnnotationMemberList
@A(value="v")
+ Annotation
  + NormalAnnotation
    + Name "A"
    + MemberValuePairs
      + MemberValuePair "value"
        + MemberValue
          + PrimaryExpression
            + PrimaryPrefix
              + Literal '"v"'
+ Annotation
  + AnnotationMemberList
    + MemberValuePair "value" [@Shorthand=false()]
      + StringLiteral '"v"'
@A("v")
+ Annotation
  + SingleMemberAnnotation
    + Name "A"
    + MemberValue
      + PrimaryExpression
        + PrimaryPrefix
          + Literal '"v"'
+ Annotation
  + AnnotationMemberList
    + MemberValuePair "value" [@Shorthand=true()]
      + StringLiteral '"v"'
@A(value="v", on=true)
+ Annotation
  + NormalAnnotation
    + Name "A"
    + MemberValuePairs
      + MemberValuePair "value"
        + MemberValue
          + PrimaryExpression
            + PrimaryPrefix
              + Literal '"v"'
      + MemberValuePair "on"
        + MemberValue
          + PrimaryExpression
            + PrimaryPrefix
              + Literal
                + BooleanLiteral [@True=true()]
+ Annotation
  + AnnotationMemberList
    + MemberValuePair "value" [@Shorthand=false()]
      + StringLiteral '"v"'
    + MemberValuePair "on"
      + BooleanLiteral [@True=true()]

@oowekyala oowekyala added the in:ast About the AST structure or API, the parsing step label Feb 12, 2020
@oowekyala oowekyala added this to the 7.0.0 milestone Feb 12, 2020
@oowekyala oowekyala changed the title Grammar annotations [java] Use single node for annotations Feb 12, 2020
@pmd-test
Copy link

1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

{String name = null;}
{
"@" name=VoidName() {n.setImage(name);}
"@" jjtThis.name=VoidName() [ AnnotationMemberList() ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting - this time without a setter. I've no strong opinion whether we should use jjtThis.setName(VoidName()) or jjtThis.name=VoidName()- whatever we do, we should do it the same in all other AST classes. Using a setter would provide us a bit more flexibility (although I don't know, whether we need/should use that - a setter should not contain too much logic...).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I tried to make the grammar cleaner. But maybe, using a setter would be better, especially if the field is shared (this could be the image field)

@@ -140,3 +140,6 @@ class ASTCastExpressionTest : ParserTestSpec({


})

val Annotatable.declaredAnnotationsList: List<ASTAnnotation>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work, if it is declared here in ASTCastExpressionTest? I could imagine, this test needs to run before the others... This should probably be moved to the DSL in TestExtensions.kt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toplevel functions and properties in kotlin are translated to static members of a class:
https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html#package-level-functions

So this doesn't need to run. But for clarity, since this is used in several tests, it should indeed be put into TestExtensions.kt

@adangel adangel merged commit a67043e into pmd:java-grammar Feb 20, 2020
@adangel
Copy link
Member

adangel commented Feb 20, 2020

@oowekyala oowekyala deleted the grammar-annotations branch February 20, 2020 18:32
@adangel adangel mentioned this pull request Jan 23, 2023
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in:ast About the AST structure or API, the parsing step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants