From f2d023b78a367105b588842df31e9430d61999e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Sat, 16 Feb 2019 16:47:42 +0100 Subject: [PATCH] PR proofreading --- docs/pages/next_major_development.md | 23 +++++++++++++++ pmd-java/etc/grammar/Java.jjt | 26 ++++++++++------- .../lang/java/ast/ASTArrayDimsAndInits.java | 29 ++++++++++++++++++- .../java/ast/ASTClassOrInterfaceType.java | 17 +++++++---- .../pmd/lang/java/ast/ASTPrimitiveType.java | 27 +++++++++++++++-- .../lang/java/typeresolution/TypeHelper.java | 4 +++ ...sOrInterfaceTypeTest.kt => ASTTypeTest.kt} | 6 ++-- 7 files changed, 109 insertions(+), 23 deletions(-) rename pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/{ASTClassOrInterfaceTypeTest.kt => ASTTypeTest.kt} (95%) diff --git a/docs/pages/next_major_development.md b/docs/pages/next_major_development.md index 2586d1d946..6963c8e4bb 100644 --- a/docs/pages/next_major_development.md +++ b/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 diff --git a/pmd-java/etc/grammar/Java.jjt b/pmd-java/etc/grammar/Java.jjt index a4350ba67b..84f1aae9c7 100644 --- a/pmd-java/etc/grammar/Java.jjt +++ b/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());} } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTArrayDimsAndInits.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTArrayDimsAndInits.java index f858976922..3c11580d3f 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTArrayDimsAndInits.java +++ b/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: + * 
+ *
+ * ArrayDimsAndInit ::= TypeAnnotation* "[" Expression "]" ( "[" "]" )*
+ *                    | ( "[" "]" )+ ArrayInitializer()
+ *
+ *
+ * + * Actual JLS (https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-DimExprs): + * 
+ *
+ * (: 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}
+ *
+ *
+ * */ public class ASTArrayDimsAndInits extends AbstractJavaNode implements Dimensionable { private int arrayDepth; diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceType.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceType.java index ef4bb804a0..bee539e2e2 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceType.java +++ b/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): * - *

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} + *

The first is a simple, flat variant. The image of the node corresponds to the + * chain of identifiers. May have type arguments. + * + *

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). * - *

TODO We could uniformise those with an AST visit provided we have a classloader - * Could be preliminary to type resolution + *

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). * * 

  *
- * ClassOrInterfaceType ::= ClassOrInterfaceType ( "." {@link ASTAnnotation Annotation}* <IDENTIFIER> {@link ASTTypeArguments TypeArguments}? )+
- *                        | <IDENTIFIER> ( "." <IDENTIFIER> ) *  {@link ASTTypeArguments TypeArguments}?
+ * ClassOrInterfaceType ::= <IDENTIFIER> ( "." <IDENTIFIER> ) *  {@link ASTTypeArguments TypeArguments}?
+ *                        | ClassOrInterfaceType ( "." {@link ASTAnnotation Annotation}* <IDENTIFIER> {@link ASTTypeArguments TypeArguments}? )+
  *
  *
*/ diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTPrimitiveType.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTPrimitiveType.java index 548b2d96ae..0c20836379 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTPrimitiveType.java +++ b/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 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 fromToken(String token) { - return Arrays.stream(values()).filter(v -> v.getToken().equals(token)).findFirst(); + return Optional.ofNullable(LOOKUP.get(token)); } - } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java index 2e7bfde05a..8e8737a026 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/TypeHelper.java +++ b/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()); } diff --git a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceTypeTest.kt b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTypeTest.kt similarity index 95% rename from pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceTypeTest.kt rename to pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTTypeTest.kt index 8b94450d8d..8d09e1c7ec 100644 --- a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceTypeTest.kt +++ b/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 { 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" should matchType { it.typeImage shouldBe "java.util.Map.Entry"