Fixes #1532 [java] CloneMethodMustImplementCloneable: Implemented Interface extends Cloneable

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/typeresolution/rules/CloneMethodMustImplementCloneable.java

@@ -4,13 +4,16 @@
 package net.sourceforge.pmd.lang.java.typeresolution.rules;
 
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTBlock;
 import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
+import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit;
 import net.sourceforge.pmd.lang.java.ast.ASTExtendsList;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters;
 import net.sourceforge.pmd.lang.java.ast.ASTImplementsList;
@@ -31,6 +34,13 @@ public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
 
     @Override
     public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
+        if (extendsOrImplementsCloneable(node)) {
+            return data;
+        }
+        return super.visit(node, data);
+    }
+
+    private boolean extendsOrImplementsCloneable(ASTClassOrInterfaceDeclaration node) {
         ASTImplementsList impl = node.getFirstChildOfType(ASTImplementsList.class);
         if (impl != null && impl.jjtGetParent().equals(node)) {
             for (int ix = 0; ix < impl.jjtGetNumChildren(); ix++) {
@@ -43,14 +53,16 @@ public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
                 ASTClassOrInterfaceType type = (ASTClassOrInterfaceType) child;
                 if (type.getType() == null) {
                     if ("Cloneable".equals(type.getImage())) {
-                        return data;
+                        return true;
                     }
                 } else if (type.getType().equals(Cloneable.class)) {
-                    return data;
+                    return true;
+                } else if (Cloneable.class.isAssignableFrom(type.getType())) {
+                    return true;
                 } else {
                     List<Class<?>> implementors = Arrays.asList(type.getType().getInterfaces());
                     if (implementors.contains(Cloneable.class)) {
-                        return data;
+                        return true;
                     }
                 }
             }
@@ -59,23 +71,21 @@ public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
             ASTClassOrInterfaceType type = (ASTClassOrInterfaceType) node.jjtGetChild(0).jjtGetChild(0);
             Class<?> clazz = type.getType();
             if (clazz != null && clazz.equals(Cloneable.class)) {
-                return data;
+                return true;
             }
             while (clazz != null && !Object.class.equals(clazz)) {
                 if (Arrays.asList(clazz.getInterfaces()).contains(Cloneable.class)) {
-                    return data;
+                    return true;
                 }
                 clazz = clazz.getSuperclass();
             }
         }
-    
+        return false;
-        return super.visit(node, data);
     }
 
     @Override
     public Object visit(ASTMethodDeclaration node, Object data) {
-        ASTClassOrInterfaceDeclaration classOrInterface = node
+        ASTClassOrInterfaceDeclaration classOrInterface = node.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class);
-            .getFirstParentOfType(ASTClassOrInterfaceDeclaration.class);
         if (classOrInterface != null && //Don't analyze enums, which cannot subclass clone()
             (node.isFinal() || classOrInterface.isFinal())) {
             if (node.findDescendantsOfType(ASTBlock.class).size() == 1) {
@@ -93,9 +103,51 @@ public class CloneMethodMustImplementCloneable extends AbstractJavaRule {
                 }
             }
         }
+
+        // Now check other whether implemented or extended classes are defined inside the same file
+        if (classOrInterface != null) {
+            Set<String> classesNames = determineTopLevelCloneableClasses(classOrInterface);
+
+            ASTImplementsList implementsList = classOrInterface.getFirstChildOfType(ASTImplementsList.class);
+            if (implementsList != null) {
+                List<ASTClassOrInterfaceType> types = implementsList.findChildrenOfType(ASTClassOrInterfaceType.class);
+                for (ASTClassOrInterfaceType t : types) {
+                    if (classesNames.contains(t.getImage())) {
+                        return data;
+                    }
+                }
+            }
+
+            ASTExtendsList extendsList = classOrInterface.getFirstChildOfType(ASTExtendsList.class);
+            if (extendsList != null) {
+                ASTClassOrInterfaceType type = extendsList.getFirstChildOfType(ASTClassOrInterfaceType.class);
+                if (classesNames.contains(type.getImage())) {
+                    return data;
+                }
+            }
+        }
+
         return super.visit(node, data);
     }
 
+    /**
+     * Determines all the class/interface declarations inside this compilation unit,
+     * which implement Cloneable
+     * @param currentClass the node of the class, that is currently analyzed (inside this compilation unit)
+     * @return a Set of class/interface names
+     */
+    private Set<String> determineTopLevelCloneableClasses(ASTClassOrInterfaceDeclaration currentClass) {
+        List<ASTClassOrInterfaceDeclaration> classes = currentClass.getFirstParentOfType(ASTCompilationUnit.class)
+                .findDescendantsOfType(ASTClassOrInterfaceDeclaration.class);
+        Set<String> classesNames = new HashSet<String>();
+        for (ASTClassOrInterfaceDeclaration c : classes) {
+            if (c != currentClass && extendsOrImplementsCloneable(c)) {
+                classesNames.add(c.getImage());
+            }
+        }
+        return classesNames;
+    }
+
     @Override
     public Object visit(ASTMethodDeclarator node, Object data) {
         if (!"clone".equals(node.getImage())) {

pmd-java/src/main/resources/rulesets/java/clone.xml

@@ -101,8 +101,8 @@ The method clone() should only be implemented if the class implements the Clonea
                 <value>
                     <![CDATA[
 //ClassOrInterfaceDeclaration
-[not(./ImplementsList/ClassOrInterfaceType
+[not(./ExtendsList/ClassOrInterfaceType[@Image='Cloneable'])]
-[@Image='Cloneable'])]
+[not(./ImplementsList/ClassOrInterfaceType[@Image='Cloneable'])]
 /ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration
 [MethodDeclaration
 [MethodDeclarator[@Image

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/clone/xml/CloneMethodMustImplementCloneable.xml

@@ -91,4 +91,14 @@ throw new CloneNotSupportedException();
 public class UnmodifiableList<T> implements @Readonly List<@Readonly T> {}
         ]]></code>
     </test-code>
+
+    <test-code>
+        <description>#1532 [java] CloneMethodMustImplementCloneable: Implemented Interface extends Cloneable - part 1: interface</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+  interface TestInterface extends Cloneable {
+    TestInterface clone();
+  }
+        ]]></code>
+    </test-code>
 </test-data>

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/typeresolution/xml/CloneMethodMustImplementCloneable.xml

@@ -169,4 +169,31 @@ public enum Foo {
 public class UnmodifiableList<T> implements @Readonly List<@Readonly T> {}
         ]]></code>
     </test-code>
+
+    <test-code>
+        <description>#1532 [java] CloneMethodMustImplementCloneable: Implemented Interface extends Cloneable - part 1: interface</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+  interface TestInterface extends Cloneable {
+    TestInterface clone();
+  }
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>#1532 [java] CloneMethodMustImplementCloneable: Implemented Interface extends Cloneable</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+  interface TestInterface extends Cloneable {
+    TestInterface clone();
+  }
+
+  class CloneableClass implements TestInterface {
+    @Override // creates a warning though CloneableClass is actually implementing Cloneable
+    public CloneableClass clone() {
+      // clone implementation
+    }
+  }
+        ]]></code>
+    </test-code>
 </test-data>

src/site/markdown/overview/changelog.md

@@ -32,6 +32,7 @@
 *   java-imports/UnusedImports
     *   [#1529](https://sourceforge.net/p/pmd/bugs/1529/): \[java] UnusedImports: The created rule violation has no class name
 *   java-typeresolution/CloneMethodMustImplementCloneable
+    *   [#1532](https://sourceforge.net/p/pmd/bugs/1532/): \[java] CloneMethodMustImplementCloneable: Implemented Interface extends Cloneable
     *   [#1534](https://sourceforge.net/p/pmd/bugs/1534/): \[java] CloneMethodMustImplementCloneable: ClassCastException with Annotation (java8)
 *   java-typeresolution/SignatureDeclareThrowsException
     *   [#1535](https://sourceforge.net/p/pmd/bugs/1535/): \[java] SignatureDeclareThrowsException: ClassCastException with Annotation