[java] Move annotations inside the node they apply to #1875

oowekyala · 2019-06-19T16:03:26Z

Annotations are now part of the node they apply to, which fixes a lot of inconsistencies, where sometimes the annotations are inside the node, and sometimes just somewhere in the parent, with no real structure.

Implementation is another jjtree tweak, which allows avoiding looking ahead past the annotations. The annotations are just marked as "pending", and then adopted by the next node to be opened (be it a class or method declaration, whatever).

This makes some nodes completely redundant, which can later be turned into interfaces or removed: TypeDeclaration, ClassOrInterfaceBodyDeclaration, AnnotationTypeBodyDeclaration, and BlockStatement (which for now contains the annotations of a local class for example)

Some examples:

Code	Old AST	Current 7.0 AST	New AST after this PR
Top-level type declaration @A class C {}	+TypeDeclaration +MarkerAnnotation "A" +ClassOrInterfaceDeclaration +ClassOrInterfaceBody	Same	+TypeDeclaration - +MarkerAnnotation "A" +ClassOrInterfaceDeclaration + +MarkerAnnotation "A" +ClassOrInterfaceBody
Cast expression (@A T.@B S) expr	N/A (Parse error)	+CastExpression +MarkerAnnotation "A" +ClassOrInterfaceType "S" +MarkerAnnotation "B" +AmbiguousName "T" +(Expression `expr`)	+CastExpression - +MarkerAnnotation "A" +ClassOrInterfaceType "S" +MarkerAnnotation "B" - +AmbiguousName "T" + +ClassOrInterfaceType "T" + +MarkerAnnotation "A" +(Expression `expr`) Notice how `T` is not ambiguous anymore.
Cast expression with intersection (@A T & S) expr	+CastExpression +MarkerAnnotation "A" +ClassOrInterfaceType "T" +ClassOrInterfaceType "S" +(Expression `expr`)	+CastExpression +MarkerAnnotation "A" + +IntersectionType +ClassOrInterfaceType "T" +ClassOrInterfaceType "S" +(Expression `expr`)	+CastExpression - +MarkerAnnotation "A" +IntersectionType +ClassOrInterfaceType "T" + +MarkerAnnotation "A" +ClassOrInterfaceType "S" +(Expression `expr`) Notice `@A` binds to `T`, not `T & S`
Constructor call new @A T()	+AllocationExpression +MarkerAnnotation "A" +Type +ReferenceType +ClassOrInterfaceType "T" +Arguments	+ConstructorCall +MarkerAnnotation "A" +ClassOrInterfaceType "T" +ArgumentsList	+ConstructorCall +ClassOrInterfaceType "T" +MarkerAnnotation "A" +ArgumentsList
Array allocation new @A int[0]	+AllocationExpression +MarkerAnnotation "A" +Type +PrimitiveType "int" +ArrayDimsAndInits +Expression +PrimaryExpression +Literal "0"	+ArrayAllocation +MarkerAnnotation "A" +PrimitiveType "int" +ArrayAllocationDims +ArrayDimExpr +NumericLiteral "0"	+ArrayAllocation - +MarkerAnnotation "A" +PrimitiveType "int" + +MarkerAnnotation "A" +ArrayAllocationDims +ArrayDimExpr +NumericLiteral "0"
Array type @A int @B[]	N/A (parse error)	+MarkerAnnotation "A" +ArrayType +PrimitiveType "int" +ArrayTypeDims +ArrayTypeDim +MarkerAnnotation "B"	-+MarkerAnnotation "A" +ArrayType +PrimitiveType "int" + +MarkerAnnotation "A" +ArrayTypeDims +ArrayTypeDim +MarkerAnnotation "B" Notice `@A` binds to `int`, not `int[]`
Type parameters <@A T, @B S extends @C Object>	+TypeParameters +MarkerAnnotation "A" +TypeParameter "T" +MarkerAnnotation "B" +TypeParameter "S" +MarkerAnnotation "C" +TypeBound +ReferenceType +ClassOrInterfaceType "Object"	+TypeParameters +MarkerAnnotation "A" +TypeParameter "T" +MarkerAnnotation "B" +TypeParameter "S" +MarkerAnnotation "C" +TypeBound - +ReferenceType +ClassOrInterfaceType "Object"	+TypeParameters - +MarkerAnnotation "A" +TypeParameter "T" + +MarkerAnnotation "A" - +MarkerAnnotation "B" +TypeParameter "S" + +MarkerAnnotation "B" - +MarkerAnnotation "C" - +TypeBound +ClassOrInterfaceType "Object" + +MarkerAnnotation "C" TypeParameters now only can have TypeParameter as a child Annotations that apply to the param are in the param Annotations that apply to the bound are in the type This removes the need for TypeBound, because annotations are cleanly placed.
Enum constants enum { @A E1, @B E2 }	+EnumBody +MarkerAnnotation "A" +EnumConstant "E1" +MarkerAnnotation "B" +EnumConstant "E2"	+EnumBody +MarkerAnnotation "A" +EnumConstant "E1" + +VariableDeclaratorId "E1" +MarkerAnnotation "B" +EnumConstant "E2" + +VariableDeclaratorId "E1"	+EnumBody - +MarkerAnnotation "A" +EnumConstant "E1" + +MarkerAnnotation "A" +VariableDeclaratorId "E1" - +MarkerAnnotation "B" +EnumConstant "E2" + +MarkerAnnotation "B" +VariableDeclaratorId "E1" Annotations are not just randomly in the enum body anymore

oowekyala · 2019-06-26T15:35:47Z

I added a lengthy test case for type annotations that includes all contexts where type annotations may appear for exhaustiveness. It's based off the spec of type annotations.

There are a few cases where type annotations are legal that this PR fixes

On method references: @Nonnull String::length
On instanceof expressions: o instanceof @Readonly String
On all bounds of an intersection type: @A U1 & @B U2. I originally misread the spec and thought only the first type could be annotated.

There are two cases which we currently don't handle:

On the types of a throws clause: void foo() throws @A Exception, java.io.@B IOException
- This is easy to fix and there's already a TODO on the wiki, about replacing NameList with ThrowsList, which would be a list of ClassOrInterfaceType
On the varargs ellipsis ...: void varargs(String @A ... last);
- In that case, the ... is treated just like array dimensions and annotations apply exactly as if the param was declared as String @A [] last
- The sensible way to parse that would probably be to use ArrayTypeDims in some way... I'm not sure yet.
- Eg maybe we should parse String... just as if it was String[], so use an ArrayType.
  - This causes problems because ArrayType is not fully recursive, so that the component type of an ArrayType is not always represented by a node.
    - Eg in String[], the component type is represented by the ClassOrInterfaceType
    - But in String[][], there is no node representing the component type String[] , since [][] are flat.
    - So maybe we should represent array types like the following:

Code

Old AST

Current 7.0 AST

Proposed representation

String[][]

 +Type
  +ReferenceType[@ArrayDepth=2]
   +ClassOrInterfaceType "String"

 +ArrayType
    +ClassOrInterfaceType "String"
    +ArrayTypeDims
       +ArrayTypeDim
       +ArrayTypeDim

 +ArrayType
    +ArrayType
      +ClassOrInterfaceType "String"

 String @A [] @B []

N/A (Parse error)

 +ArrayType
    +ClassOrInterfaceType
    +ArrayTypeDims
       +ArrayTypeDim
          +Annotation "A"
       +ArrayTypeDim
          +Annotation "B"

 +ArrayType
    +ArrayType
      +ClassOrInterfaceType
      +Annotation "A"
    +Annotation "B"

With varargs:

 String @A [] @B ...

N/A (Parse error)

 +ArrayType[@Varargs = true()]
    +ArrayType
      +ClassOrInterfaceType
      +Annotation "A"
    +Annotation "B"

Note that this representation is both more economic in number of nodes (especially in the most common case where there are no annotations on the array dimensions), and also preserves every intermediary array type, which is nice.

Fixes some failing tests

adangel · 2019-07-15T18:06:29Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JSingleChildNode.java

+ *
+ * @author Clément Fournier
+ */
+interface JSingleChildNode<T extends JavaNode> extends JavaNode {


Is it important, that the interface starts with the prefix "J"? We don't usually do this...

I only did this, because this interface is equivalent to one that is introduced in #1622, and I want to avoid the name clash. When that PR is merged we can remove this one.

adangel · 2019-07-15T18:12:14Z

pmd-java/src/test/resources/net/sourceforge/pmd/ast/FullTypeAnnotations.java

+ * https://checkerframework.org/jsr308/java-annotation-design.html#type-names
+ * is particularly interesting
+ */
+public class ParserCornerCases18 {


We should probably rename this class to avoid confusion 😄

Do we have annotated method declarations covered? Like

@Override public void run() { }

adangel · 2019-07-15T18:22:35Z

@oowekyala
Sorry for the delay. I've merged it now into java-grammar. I've updated https://github.com/pmd/pmd/wiki/PMD-7.0.0-Java and https://github.com/pmd/pmd/wiki/Java_clean_changes#annotation-nesting

oowekyala added 16 commits June 19, 2019 00:37

Extract annotation changes

a709ce5

Fix tests

a9bcb00

Update grammar hints

5b91d2f

Remove test to enable later

d84eb67

Fix comment

69f7227

Fix wildcard type annotations

93ceb24

Add test for enum constants

eec53bc

Test other annotation positions

0351b59

remove wrapLeft

8262e38

Remove annotation list, parse annotations flatly

388c952

Generalise annotatable

154b376

Array type annotations

4030f0f

Add local class annotation tests

176eb23

Fix tests

cbc336a

Add changelog notice

2a40d49

Fix instanceof with annotated type

926c008

oowekyala added the in:ast About the AST structure or API, the parsing step label Jun 19, 2019

oowekyala added this to the 7.0.0 milestone Jun 19, 2019

oowekyala added 4 commits June 20, 2019 16:27

Make type param implement annotatable

4248c44

Merge branch 'java-grammar' into grammar-annotation-floating

9356d12

Fix text bounds on declarations

b47aa62

Fix some cases of type annotations failure

a64c467

oowekyala added 2 commits July 12, 2019 02:53

Fix type annot on method ref in cast ctx

d664b17

Fix choice amibiguity with annotated local class

0d6e2f3

Fixes some failing tests

adangel self-assigned this Jul 15, 2019

adangel reviewed Jul 15, 2019

View reviewed changes

adangel merged commit 0d6e2f3 into pmd:java-grammar Jul 15, 2019

oowekyala deleted the grammar-annotation-floating branch July 15, 2019 22:38

oowekyala mentioned this pull request Feb 19, 2020

[java] Flatten body declarations #2300

Merged

adangel mentioned this pull request Jan 23, 2023

PMD 7 Tracking Issue #3898

Closed

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java] Move annotations inside the node they apply to #1875

[java] Move annotations inside the node they apply to #1875

oowekyala commented Jun 19, 2019 •

edited

Loading

oowekyala commented Jun 26, 2019

adangel Jul 15, 2019

oowekyala Jul 15, 2019

adangel Jul 15, 2019

adangel commented Jul 15, 2019

[java] Move annotations inside the node they apply to #1875

[java] Move annotations inside the node they apply to #1875

Conversation

oowekyala commented Jun 19, 2019 • edited Loading

oowekyala commented Jun 26, 2019

adangel Jul 15, 2019

Choose a reason for hiding this comment

oowekyala Jul 15, 2019

Choose a reason for hiding this comment

adangel Jul 15, 2019

Choose a reason for hiding this comment

adangel commented Jul 15, 2019

oowekyala commented Jun 19, 2019 •

edited

Loading