pmd · adangel · May 26, 2018 · May 23, 2018 · May 23, 2018 · May 23, 2018
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
@@ -49,6 +49,7 @@ This is a bug fixing release.
 *   java-codestyle
     *   [#720](https://github.com/pmd/pmd/issues/720): \[java] ShortVariable should whitelist lambdas
     *   [#955](https://github.com/pmd/pmd/issues/955): \[java] Detect identical catch statements
+    *   [#1114](https://github.com/pmd/pmd/issues/1114): \[java] Star import overwritten by explicit import is not correctly handled
     *   [#1064](https://github.com/pmd/pmd/issues/1064): \[java] ClassNamingConventions suggests to add Util suffix for simple exception wrappers
     *   [#1065](https://github.com/pmd/pmd/issues/1065): \[java] ClassNamingConventions shouldn't prohibit numbers in class names
     *   [#1067](https://github.com/pmd/pmd/issues/1067): \[java] [6.3.0] PrematureDeclaration false-positive

diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTImportDeclaration.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTImportDeclaration.java
@@ -5,6 +5,14 @@
 
 package net.sourceforge.pmd.lang.java.ast;
 
+/**
+ * Represents an import declaration in a Java file.
+ *
+ * <pre>
+ *   ImportDeclaration ::= "import" [ "static" ] Name [ "." "*" ] ";"
+ * </pre>
+ *
+ */
 public class ASTImportDeclaration extends AbstractJavaTypeNode {
 
     private boolean isImportOnDemand;
@@ -41,10 +49,35 @@ 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 asterisk.
+     */
     public String getImportedName() {
-        return ((ASTName) jjtGetChild(0)).getImage();
+        return jjtGetChild(0).getImage();
+    }
+
+
+    /**
+     * Returns the simple name of the type or method imported by this declaration.
+     * For on-demand imports, returns {@code null}.
+     */
+    public String getImportedSimpleName() {
+        if (isImportOnDemand) {
+            return null;
+        }
+
+        String importName = getImportedName();
+        return importName.substring(importName.lastIndexOf('.') + 1);
     }
 
+
+    /**
+     * Returns the "package" prefix of the imported name. For type imports, including on-demand
+     * imports, this is really the package name of the imported type(s). For static imports,
+     * this is actually the qualified name of the enclosing type, including the type name.
+     */
     public String getPackageName() {
         String importName = getImportedName();
         if (isImportOnDemand) {
@@ -57,9 +90,6 @@ public String getPackageName() {
         return importName.substring(0, lastDot);
     }
 
-    /**
-     * Accept the visitor. *
-     */
     @Override
     public Object jjtAccept(JavaParserVisitor visitor, Object data) {
         return visitor.visit(this, data);
@@ -69,6 +99,13 @@ 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.
+     */
     public Package getPackage() {
         return this.pkg;
     }

diff --git a/.../java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java b/.../java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryFullyQualifiedNameRule.java
@@ -22,7 +22,6 @@
 public class UnnecessaryFullyQualifiedNameRule extends AbstractJavaRule {
 
     private List<ASTImportDeclaration> imports = new ArrayList<>();
-    private List<ASTImportDeclaration> matches = new ArrayList<>();
 
     public UnnecessaryFullyQualifiedNameRule() {
         super.addRuleChainVisit(ASTCompilationUnit.class);
@@ -45,6 +44,10 @@ public Object visit(ASTImportDeclaration node, Object data) {
 
     @Override
     public Object visit(ASTClassOrInterfaceType node, Object data) {
+        // This name has no qualification, it can't be unnecessarily qualified
+        if (node.getImage().indexOf('.') < 0) {
+            return data;
+        }
         checkImports(node, data);
         return data;
     }
@@ -53,38 +56,44 @@ public Object visit(ASTClassOrInterfaceType node, Object data) {
     public Object visit(ASTName node, Object data) {
         if (!(node.jjtGetParent() instanceof ASTImportDeclaration)
                 && !(node.jjtGetParent() instanceof ASTPackageDeclaration)) {
+            // This name has no qualification, it can't be unnecessarily qualified
+            if (node.getImage().indexOf('.') < 0) {
+                return data;
+            }
             checkImports(node, data);
         }
         return data;
     }
 
+
+    /**
+     * Returns true if the name could be imported by this declaration.
+     * The name must be fully qualified, the import is either on-demand
+     * or static, that is its {@link ASTImportDeclaration#getImportedName()}
+     * is the enclosing package or type name of the imported type or static member.
+     */
+    private boolean declarationMatches(ASTImportDeclaration decl, String name) {
+        return name.startsWith(decl.getImportedName())
+                && name.lastIndexOf('.') == decl.getImportedName().length();
+    }
+
     private void checkImports(JavaNode node, Object data) {
         String name = node.getImage();
-        matches.clear();
+        List<ASTImportDeclaration> matches = new ArrayList<>();
 
         // Find all "matching" import declarations
         for (ASTImportDeclaration importDeclaration : imports) {
-            if (importDeclaration.isImportOnDemand()) {
-                // On demand import exactly matches the package of the type
-                if (name.startsWith(importDeclaration.getImportedName())) {
-                    if (name.lastIndexOf('.') == importDeclaration.getImportedName().length()) {
-                        matches.add(importDeclaration);
-                        continue;
-                    }
-                }
-            } else {
+            if (!importDeclaration.isImportOnDemand()) {
                 // Exact match of imported class
                 if (name.equals(importDeclaration.getImportedName())) {
                     matches.add(importDeclaration);
                     continue;
                 }
-                // Match of static method call on imported class
-                if (name.startsWith(importDeclaration.getImportedName())) {
-                    if (name.lastIndexOf('.') == importDeclaration.getImportedName().length()) {
-                        matches.add(importDeclaration);
-                        continue;
-                    }
-                }
+            }
+            // On demand import exactly matches the package of the type
+            // Or match of static method call on imported class
+            if (declarationMatches(importDeclaration, name)) {
+                matches.add(importDeclaration);
             }
         }
 
@@ -98,7 +107,7 @@ private void checkImports(JavaNode node, Object data) {
         // List list1 = Arrays.asList("foo"); // Array class name not needed!
         // List list2 = asList("foo"); // Preferred, used static import
         // }
-        if (matches.isEmpty() && name.indexOf('.') >= 0) {
+        if (matches.isEmpty()) {
             for (ASTImportDeclaration importDeclaration : imports) {
                 if (importDeclaration.isStatic()) {
                     String[] importParts = importDeclaration.getImportedName().split("\\.");
@@ -130,22 +139,18 @@ private void checkImports(JavaNode node, Object data) {
                 addViolation(data, node, new Object[] { node.getImage(), importStr, type });
             }
         }
-
-        matches.clear();
     }
 
     private boolean isAvoidingConflict(final JavaNode node, final String name,
             final ASTImportDeclaration firstMatch) {
         // is it a conflict between different imports?
-        if (firstMatch.isImportOnDemand() && firstMatch.isStatic() && name.indexOf('.') != -1) {
+        if (firstMatch.isImportOnDemand() && firstMatch.isStatic()) {
             final String methodCalled = name.substring(name.indexOf('.') + 1);
 
             // Is there any other static import conflictive?
             for (final ASTImportDeclaration importDeclaration : imports) {
                 if (!Objects.equals(importDeclaration, firstMatch) && importDeclaration.isStatic()) {
-                    if (importDeclaration.getImportedName().startsWith(firstMatch.getImportedName())
-                            && importDeclaration.getImportedName().lastIndexOf('.') == firstMatch.getImportedName()
-                                    .length()) {
+                    if (declarationMatches(firstMatch, importDeclaration.getImportedName())) {
                         // A conflict against the same class is not an excuse,
                         // ie:
                         // import java.util.Arrays;
@@ -170,11 +175,31 @@ private boolean isAvoidingConflict(final JavaNode node, final String name,
             }
         }
 
-        // Is it a conflict with a class in the same file?
         final String unqualifiedName = name.substring(name.lastIndexOf('.') + 1);
         final int unqualifiedNameLength = unqualifiedName.length();
+
+        // There could be a conflict between an import on demand and another import, e.g.
+        // import One.*;
+        // import Two.Problem;
+        // Where One.Problem is a legitimate qualification
+        if (firstMatch.isImportOnDemand() && !firstMatch.isStatic()) {
+            for (ASTImportDeclaration importDeclaration : imports) {
+                if (importDeclaration != firstMatch     // NOPMD
+                        && !importDeclaration.isStatic()
+                        && !importDeclaration.isImportOnDemand()) {
+
+                    // Duplicate imports are legal
+                    if (!importDeclaration.getPackageName().equals(firstMatch.getPackageName())
+                            && importDeclaration.getImportedSimpleName().equals(unqualifiedName)) {
+                        return true;
+                    }
+                }
+            }
+        }
+
+        // Is it a conflict with a class in the same file?
         final Set<String> qualifiedTypes = node.getScope().getEnclosingScope(SourceFileScope.class)
-                .getQualifiedTypeNames().keySet();
+                                               .getQualifiedTypeNames().keySet();
         for (final String qualified : qualifiedTypes) {
             int fullLength = qualified.length();
             if (qualified.endsWith(unqualifiedName)