From 2d176f73ee935c2938484e4c6a949f7c86f8fdb0 Mon Sep 17 00:00:00 2001
From: Mykhailo Palahuta <michaeller.2012@gmail.com>
Date: Thu, 6 Aug 2020 12:04:01 +0300
Subject: [PATCH] [java] ProperCloneImplementation not valid for final class

---
 .../ProperCloneImplementationRule.java        | 47 ++++++++++---------
 .../xml/ProperCloneImplementation.xml         | 12 +++++
 2 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ProperCloneImplementationRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ProperCloneImplementationRule.java
index 8868ff651a..d55a94c278 100644
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ProperCloneImplementationRule.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ProperCloneImplementationRule.java
@@ -8,7 +8,6 @@ package net.sourceforge.pmd.lang.java.rule.errorprone;
 import java.util.List;
 
 import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
-import net.sourceforge.pmd.lang.java.ast.ASTBlock;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
@@ -21,36 +20,40 @@ public class ProperCloneImplementationRule extends AbstractJavaRule {
     }
 
     @Override
-    public Object visit(ASTMethodDeclaration node, Object data) {
-        if (!"clone".equals(node.getName()) || node.getArity() > 0) {
-            return data;
+    public Object visit(ASTMethodDeclaration method, Object data) {
+        if (isCloneMethod(method) && isNotAbstractMethod(method)) {
+            ASTClassOrInterfaceDeclaration classDecl = method.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class);
+            if (isNotFinal(classDecl) && hasAnyAllocationOfClass(method, classDecl.getSimpleName())) {
+                addViolation(data, method);
+            }
         }
-
-        ASTBlock block = node.getFirstChildOfType(ASTBlock.class);
-        if (block == null) {
-            return data;
-        }
-
-        String enclosingClassName = node.getFirstParentOfType(ASTClassOrInterfaceDeclaration.class).getSimpleName();
-        if (blockHasAllocations(block, enclosingClassName)) {
-            addViolation(data, node);
-        }
-
         return data;
     }
 
-    private boolean blockHasAllocations(ASTBlock block, String enclosingClassName) {
-        List<ASTAllocationExpression> allocations = block.findDescendantsOfType(ASTAllocationExpression.class);
-        for (ASTAllocationExpression alloc : allocations) {
-            ASTClassOrInterfaceType type = alloc.getFirstChildOfType(ASTClassOrInterfaceType.class);
-            if (typeHasImage(type, enclosingClassName)) {
+    private boolean isCloneMethod(ASTMethodDeclaration method) {
+        return "clone".equals(method.getName()) && method.getArity() == 0;
+    }
+
+    private boolean isNotAbstractMethod(ASTMethodDeclaration method) {
+        return !method.isAbstract();
+    }
+
+    private boolean isNotFinal(ASTClassOrInterfaceDeclaration classOrInterfaceDecl) {
+        return !classOrInterfaceDecl.isFinal();
+    }
+
+    private boolean hasAnyAllocationOfClass(ASTMethodDeclaration method, String className) {
+        List<ASTAllocationExpression> allocations = method.findDescendantsOfType(ASTAllocationExpression.class);
+        for (ASTAllocationExpression allocation : allocations) {
+            ASTClassOrInterfaceType allocatedType = allocation.getFirstChildOfType(ASTClassOrInterfaceType.class);
+            if (isSimpleNameOfType(className, allocatedType)) {
                 return true;
             }
         }
         return false;
     }
 
-    private boolean typeHasImage(ASTClassOrInterfaceType type, String image) {
-        return type != null && type.hasImageEqualTo(image);
+    private boolean isSimpleNameOfType(String simpleName, ASTClassOrInterfaceType type) {
+        return type != null && type.hasImageEqualTo(simpleName);
     }
 }
diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ProperCloneImplementation.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ProperCloneImplementation.xml
index 8af9e8f294..6100f76912 100644
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ProperCloneImplementation.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/ProperCloneImplementation.xml
@@ -63,6 +63,18 @@ public class Foo {
         super.clone();
         byte[] array = new byte[6];
     }
+}
+        ]]></code>
+    </test-code>
+    
+    <test-code>
+        <description>ok, final class false-positive</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+final class Foo {
+    Foo clone() {
+        return new Foo();
+    }
 }
         ]]></code>
     </test-code>