Skip to content

[java] Use single node for annotations#2282

Merged
adangel merged 4 commits into
pmd:java-grammarfrom
oowekyala:grammar-annotations
Feb 20, 2020
Merged

[java] Use single node for annotations#2282
adangel merged 4 commits into
pmd:java-grammarfrom
oowekyala:grammar-annotations

Conversation

@oowekyala

Copy link
Copy Markdown
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
@ghost

ghost commented Feb 14, 2020

Copy link
Copy Markdown
1 Message
📖 No java rules are changed!

Generated by 🚫 Danger

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

Copy link
Copy Markdown
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
Copy Markdown
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)


})

val Annotatable.declaredAnnotationsList: List<ASTAnnotation>

Copy link
Copy Markdown
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
Copy Markdown
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

adangel commented Feb 20, 2020

Copy link
Copy Markdown
Member

@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.

2 participants