PR proofreading

docs/pages/next_major_development.md

@@ -15,6 +15,29 @@ TODO
 
 {% include note.html content="Current plans are listed [here](https://github.com/pmd/pmd/labels/in%3Aast) and in particular [here](https://github.com/pmd/pmd/issues/1019)" %}
 
+### Type grammar changes
+
+{% jdoc_nspace :jast java::lang.java.ast %}
+
+* {% jdoc jast::ASTType %} and {% jdoc jast::ASTReferenceType %} have been turned into
+interfaces, implemented by {% jdoc jast::ASTPrimitiveType %}, {% jdoc jast::ASTClassOrInterfaceType %},
+and the new node {% jdoc jast::ASTArrayType %}. This reduces the depth of the relevant
+subtrees, and allows to explore them more easily and consistently.
+
+* {% jdoc jast::ASTClassOrInterfaceType %} appears to be left recursive now.
+TODO document that when we're done discussing the semantic rewrite phase.
+
+* **Migrating**:
+  * `Type/ReferenceType/ClassOrInterfaceType` -> `ClassOrInterfaceType`
+  * `Type/PrimitiveType` -> `PrimitiveType`.
+  * `Type/ReferenceType[@ArrayDepth>1]/ClassOrInterfaceType` -> `ArrayType/ClassOrInterfaceType`.
+  * `Type/ReferenceType/PrimitiveType` -> `ArrayType/PrimitiveType`.
+  * Note that in most cases you should check the type of a variable with
+  `VariableDeclaratorId[pmd-java:typeIs("java.lang.String[]")]` because it
+  considers the additional dimensions on declarations like `String foo[];`.
+  The Java equivalent is `TypeHelper.isA(id, String[].class);`
+
+
 
 
 ## New API support guidelines

pmd-java/etc/grammar/Java.jjt

@@ -1,4 +1,8 @@
 /**
+ * Change parsing of types to flatten their AST and preserve their structure.
+ * Refs #1150 [java] ClassOrInterfaceType AST improvements
+ * Clément Fournier 02/2019
+ *====================================================================
  * Add support for Java 12 switch expressions and switch rules.
  * Andreas Dangel, Clément Fournier 03/2019
  *====================================================================
@@ -1962,8 +1966,8 @@ void Type() #void:
 void Dims() #ArrayTypeDims:
 {}
 {
-  // the list of dimensions is flat which is not cool for annotation
-  // placement, but realistically that's a corner case.
+  // the list of dimensions is flat, but annotations are
+  // preserved within each specific dim.
   (ArrayTypeDim())+
 }
 
@@ -2104,14 +2108,16 @@ FloatingPointType:
 void PrimitiveType() :
 {}
 {
-  "boolean" {jjtThis.setImage("boolean");}
-| "char" {jjtThis.setImage("char");}
-| "byte" {jjtThis.setImage("byte");}
-| "short" {jjtThis.setImage("short");}
-| "int" {jjtThis.setImage("int");}
-| "long" {jjtThis.setImage("long");}
-| "float" {jjtThis.setImage("float");}
-| "double" {jjtThis.setImage("double");}
+    ( "boolean"
+    | "char"
+    | "byte"
+    | "short"
+    | "int"
+    | "long"
+    | "float"
+    | "double"
+    )
+    {jjtThis.setImage(getToken(0).getImage());}
 }
 
 

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

@@ -6,7 +6,34 @@
 package net.sourceforge.pmd.lang.java.ast;
 
 /**
- * TODO should we merge with {@link ASTArrayTypeDims}?
+ * TODO get rid of Dimensionable here. Ideally reuse {@link ASTArrayTypeDim},
+ *     or use a special node for array creation, bc {@link ASTAllocationExpression}
+ *     is too broad.
+ *
+ * Current grammar:
+ * <pre>
+ *
+ * ArrayDimsAndInit ::= TypeAnnotation* "[" Expression "]" ( "[" "]" )*
+ *                    | ( "[" "]" )+ ArrayInitializer()
+ *
+ * </pre>
+ *
+ * Actual JLS (https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-DimExprs):
+ * <pre>
+ *
+ * (: This production doesn't exist in the JLS, they have a special production for Array creation expressions :)
+ * (: Ideally we'd do the same. :)
+ *
+ * ArrayDimsAndInit ::= TypeAnnotation* DimExpr* Dim*
+ *                    | Dim* {@link ASTArrayInitializer ArrayInitializer}
+ *
+ * (: Notice that annotations are allowed before any Dim or DimExpr :)
+ *
+ * DimExpr          ::= TypeAnnotation* [ Expression ]
+ * Dim              ::= {@link ASTArrayTypeDim ArrayTypeDim}
+ *
+ * </pre>
+ *
  */
 public class ASTArrayDimsAndInits extends AbstractJavaNode implements Dimensionable {
     private int arrayDepth;

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

@@ -15,8 +15,11 @@ import net.sourceforge.pmd.annotation.Experimental;
  * Represents a class or interface type, possibly parameterised with type arguments.
  * This node comes in two productions (see below):
  *
- * <p>The first is a left-recursive variant, allowing to parse references to type members
- * unambiguously. The resulting node's {@linkplain #getLeftHandSide() left-hand-side type}
+ * <p>The first is a simple, flat variant. The image of the node corresponds to the
+ * chain of identifiers. May have type arguments.
+ *
+ * <p>The second is a left-recursive variant, allowing to preserve the position of
+ * type parameters and annotations. The resulting node's {@linkplain #getLeftHandSide() left-hand-side type}
  * addresses the type parent of the type. The position of type arguments and annotations are
  * preserved.
  *
@@ -26,13 +29,15 @@ import net.sourceforge.pmd.annotation.Experimental;
  * qualified type name (e.g. {@code java.util.List}, where the full sequence refers to a unique
  * type, but individual segments don't).
  *
- * <p>TODO We could uniformise those with an AST visit provided we have a classloader
- * Could be preliminary to type resolution
+ * <p>TODO I expect to fix that before 7.0.0
+ *      We can add an analysis phase that rewrites nodes using semantic info from the symbol table.
+ *      That could be useful for many things (eg disambiguate obscured expression names in the
+ *      expression grammar).
  *
  * <pre>
  *
- * ClassOrInterfaceType ::= ClassOrInterfaceType ( "." {@link ASTAnnotation Annotation}* &lt;IDENTIFIER&gt; {@link ASTTypeArguments TypeArguments}? )+
- *                        | &lt;IDENTIFIER&gt; ( "." &lt;IDENTIFIER&gt; ) *  {@link ASTTypeArguments TypeArguments}?
+ * ClassOrInterfaceType ::= &lt;IDENTIFIER&gt; ( "." &lt;IDENTIFIER&gt; ) *  {@link ASTTypeArguments TypeArguments}?
+ *                        | ClassOrInterfaceType ( "." {@link ASTAnnotation Annotation}* &lt;IDENTIFIER&gt; {@link ASTTypeArguments TypeArguments}? )+
  *
  * </pre>
  */

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

@@ -6,8 +6,14 @@
 package net.sourceforge.pmd.lang.java.ast;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Optional;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import net.sourceforge.pmd.annotation.Experimental;
 
 
 /**
@@ -63,9 +69,17 @@ public class ASTPrimitiveType extends AbstractJavaTypeNode implements ASTType {
     }
 
 
+    public PrimitiveType getModelConstant() {
+        return PrimitiveType.fromToken(getImage()).get();
+    }
+
+
     /**
-     * Model constants to work with when the node's position is not important.
+     * Constants to symbolise a primitive type when the tree context is
+     * not important. I expect this may be fleshed out to be used by type
+     * resolution or something.
      */
+    @Experimental
     public enum PrimitiveType {
         BOOLEAN,
         CHAR,
@@ -76,6 +90,14 @@ public class ASTPrimitiveType extends AbstractJavaTypeNode implements ASTType {
         DOUBLE,
         FLOAT;
 
+        private static final Map<String, PrimitiveType> LOOKUP =
+            Collections.unmodifiableMap(
+                Arrays.stream(values()).collect(Collectors.toMap(
+                    PrimitiveType::getToken,
+                    Function.identity()
+                ))
+            );
+
 
         /**
          * Returns the token used to represent the type in source,
@@ -95,9 +117,8 @@ public class ASTPrimitiveType extends AbstractJavaTypeNode implements ASTType {
          * @return A constant, or the empty optional if the token doesn't correspond to a constant
          */
         public static Optional<PrimitiveType> fromToken(String token) {
-            return Arrays.stream(values()).filter(v -> v.getToken().equals(token)).findFirst();
+            return Optional.ofNullable(LOOKUP.get(token));
         }
-
     }
 
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java

@@ -31,6 +31,10 @@ public final class TypeHelper {
             return isA(n, clazz);
         }
 
+        // FIXME checking against the image is for the most part meaningless.
+        //   Many type nodes don't have an image or have one that has no relation
+        //   to their type. The TypeNode interface should have a method getTypeImage,
+        //   or better, we could use symbols instead.
         return clazzName.equals(n.getImage()) || clazzName.endsWith("." + n.getImage());
     }
 

pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTypeTest.kt

@@ -7,9 +7,9 @@ import io.kotlintest.specs.FunSpec
  * @author Clément Fournier
  * @since 7.0.0
  */
-class ASTClassOrInterfaceTypeTest : FunSpec({
+class ASTTypeTest : FunSpec({
 
-    testGroup("Test non-recursive COITs") {
+    testGroup("Test non-recursive ClassOrInterfaceTypes") {
 
         "java.util.List" should matchType<ASTClassOrInterfaceType> {
             it.typeImage shouldBe "java.util.List"
@@ -31,7 +31,7 @@ class ASTClassOrInterfaceTypeTest : FunSpec({
         }
     }
 
-    testGroup("Test recursive COITs") {
+    testGroup("Test recursive ClassOrInterfaceTypes") {
 
         "java.util.Map.@Foo Entry<K, V>" should matchType<ASTClassOrInterfaceType> {
             it.typeImage shouldBe "java.util.Map.Entry"