Remove name node in import & package decl

Make ImportDeclaration not a TypeNode

pmd-java/etc/grammar/Java.jjt

@@ -1709,15 +1709,17 @@ private void ModuleDeclLahead() #void:
 
 
 void PackageDeclaration() :
-{}
+{String image;}
 {
-  ( Annotation() )* "package" Name() ";"
+  ( Annotation() )* "package" image=VoidName() { jjtThis.setImage(image); } ";"
 }
 
 void ImportDeclaration() :
-{}
+{String image;}
 {
-  "import" [ "static" {checkForBadStaticImportUsage();jjtThis.setStatic();} ] Name() [ "." "*" {jjtThis.setImportOnDemand();} ] ";"
+  "import" [ "static" {checkForBadStaticImportUsage();jjtThis.setStatic();} ]
+  image=ImportName() { jjtThis.setImage(image); }
+  [ "." "*" {jjtThis.setImportOnDemand();} ] ";"
 }
 
 /*
@@ -3148,10 +3150,10 @@ void Annotation() #void:
 {}
 {
  (
-   LOOKAHEAD( "@" Name() "(" ( <IDENTIFIER> "=" | ")" ))
+   LOOKAHEAD( "@" VoidName() "(" ( <IDENTIFIER> "=" | ")" ))
    NormalAnnotation()
  |
-   LOOKAHEAD( "@" Name() "(" )
+   LOOKAHEAD( "@" VoidName() "(" )
    SingleMemberAnnotation()
  |
    MarkerAnnotation()
@@ -3339,6 +3341,23 @@ void AmbiguousName():
 
 
 String VoidName() #void:
+/* This is identical to ImportName except we only need 1 token lookahead. */
+{
+  StringBuilder s = new StringBuilder();
+  Token t;
+}
+{
+  t=<IDENTIFIER>
+  {
+    s.append(t.image);
+  }
+  ( "." t=<IDENTIFIER>
+    {s.append('.').append(t.image);}
+  )*
+  {return s.toString();}
+}
+
+String ImportName() #void:
 /*
  * A lookahead of 2 is required below since "Name" can be followed
  * by a ".*" when used in the context of an "ImportDeclaration".

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

@@ -4,31 +4,21 @@
 
 package net.sourceforge.pmd.lang.java.ast;
 
-import net.sourceforge.pmd.annotation.InternalApi;
-
 /**
  * Represents an import declaration in a Java file.
  *
  * <pre class="grammar">
  *
- * ImportDeclaration ::= "import" "static"? {@linkplain ASTName Name} ( "." "*" )? ";"
+ * ImportDeclaration ::= "import" "static"? Name ( "." "*" )? ";"
  *
  * </pre>
  *
  * @see <a href="https://docs.oracle.com/javase/specs/jls/se9/html/jls-7.html#jls-7.5">JLS 7.5</a>
  */
-// TODO should this really be a type node?
-// E.g. for on-demand imports, what's the type of this node? There's no type name, just a package name
-// for on-demand static imports?
-// for static imports of a field? the type of the field or the type of the enclosing type?
-// for static imports of a method?
-// I don't think we can work out a spec without surprising corner cases, and #1207 will abstract
-// things away anyway, so I think we should make it a regular node
-public final class ASTImportDeclaration extends AbstractJavaTypeNode {
+public final class ASTImportDeclaration extends AbstractJavaNode {
 
     private boolean isImportOnDemand;
     private boolean isStatic;
-    private Package pkg;
 
     ASTImportDeclaration(int id) {
         super(id);
@@ -75,19 +65,13 @@ public final class ASTImportDeclaration extends AbstractJavaTypeNode {
         return isStatic;
     }
 
-    // TODO - this should go away, but the DuplicateImports rule still uses it
-    // (in a clunky way)
-    public ASTName getImportedNameNode() {
-        return (ASTName) jjtGetChild(0);
-    }
-
 
     /**
      * Returns the full name of the import. For on-demand imports, this is the name without
      * the final dot and asterisk.
      */
     public String getImportedName() {
-        return jjtGetChild(0).getImage();
+        return getImage();
     }
 
 
@@ -132,22 +116,5 @@ public final class ASTImportDeclaration extends AbstractJavaTypeNode {
         visitor.visit(this, data);
     }
 
-    @InternalApi
-    @Deprecated
-    public void setPackage(Package packge) {
-        this.pkg = packge;
-    }
 
-
-    /**
-     * Returns the {@link Package} instance representing the package of the
-     * type or method imported by this declaration. This may be null if the
-     * auxclasspath is not correctly set, as this method depends on correct
-     * type resolution.
-     */
-    // TODO deprecate? This is only used in a test. I don't think it's really
-    // useful and it gives work to ClassTypeResolver.
-    public Package getPackage() {
-        return this.pkg;
-    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/AbstractLombokAwareRule.java

@@ -56,8 +56,7 @@ public class AbstractLombokAwareRule extends AbstractIgnoredAnnotationRule {
 
     @Override
     public Object visit(ASTImportDeclaration node, Object data) {
-        ASTName name = node.getFirstChildOfType(ASTName.class);
-        if (!lombokImported && name != null && name.getImage() != null & name.getImage().startsWith(LOMBOK_PACKAGE)) {
+        if (!lombokImported && node.getImage() != null & node.getImage().startsWith(LOMBOK_PACKAGE)) {
             lombokImported = true;
         }
         return super.visit(node, data);

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/DuplicateImportsRule.java

@@ -87,19 +87,19 @@ public class DuplicateImportsRule extends AbstractJavaRule {
     @Override
     public Object visit(ASTImportDeclaration node, Object data) {
         ImportWrapper wrapper = new ImportWrapper(node.getImportedName(), node.getImportedName(),
-                node.getImportedNameNode(), node.isStatic() && node.isImportOnDemand());
+                node, node.isStatic() && node.isImportOnDemand());
 
         // blahhhh... this really wants to be ASTImportDeclaration to be
         // polymorphic...
         if (node.isImportOnDemand()) {
             if (importOnDemandImports.contains(wrapper)) {
-                addViolation(data, node.getImportedNameNode(), node.getImportedNameNode().getImage());
+                addViolation(data, node, node.getImportedName());
             } else {
                 importOnDemandImports.add(wrapper);
             }
         } else {
             if (singleTypeImports.contains(wrapper)) {
-                addViolation(data, node.getImportedNameNode(), node.getImportedNameNode().getImage());
+                addViolation(data, node, node.getImportedName());
             } else {
                 singleTypeImports.add(wrapper);
             }

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

@@ -220,23 +220,6 @@ public class ClassTypeResolver extends JavaParserVisitorAdapter {
         return data;
     }
 
-    @Override
-    public Object visit(ASTImportDeclaration node, Object data) {
-        ASTName importedType = (ASTName) node.jjtGetChild(0);
-
-        if (importedType.getType() != null) {
-            setTypeDefinition(node, JavaTypeDefinition.forClass(importedType.getType()));
-        } else {
-            populateType(node, importedType.getImage());
-        }
-
-        if (node.getType() != null) {
-            node.setPackage(node.getType().getPackage());
-        }
-
-        // no need to visit children, the only child, ASTName, will have no type
-        return data;
-    }
 
     @Override
     public Object visit(ASTTypeDeclaration node, Object data) {

pmd-java/src/test/java/net/sourceforge/pmd/typeresolution/ClassTypeResolverTest.java

@@ -158,8 +158,6 @@ public class ClassTypeResolverTest {
         assertEquals(ArrayListFound.class,
                      acu.getFirstDescendantOfType(ASTClassOrInterfaceDeclaration.class).getType());
         ASTImportDeclaration id = acu.getFirstDescendantOfType(ASTImportDeclaration.class);
-        assertEquals("java.util", id.getPackage().getName());
-        assertEquals(ArrayList.class, id.getType());
         assertEquals(ArrayList.class, acu.getFirstDescendantOfType(ASTClassOrInterfaceType.class).getType());
         assertEquals(ArrayList.class, acu.getFirstDescendantOfType(ASTReferenceType.class).getType());
         assertEquals(ArrayList.class, acu.getFirstDescendantOfType(ASTType.class).getType());