Merge pull request #3668 from adangel:pmd7-ClassWithOnlyPrivateConstructorsShouldBeFinal-abstract

[java] ClassWithOnlyPrivateConstructorsShouldBeFinal - fix FP with inner private classes #3668

* pr-3668:
  Compare subtypes without generics
  [java] ClassWithOnlyPrivateConstructorsShouldBeFinal - add test case with generics
  [java] ClassWithOnlyPrivateConstructorsShouldBeFinal - simplify
  [java] ClassWithOnlyPrivateConstructorsShouldBeFinal - consider nested
  [java] ClassWithOnlyPrivateConstructorsShouldBeFinal - allow non-abstract abstract classes
  [java] ClassWithOnlyPrivateConstructorsShouldBeFinal - exclude abstract classes

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/ClassWithOnlyPrivateConstructorsShouldBeFinalRule.java

@@ -9,6 +9,7 @@ import static net.sourceforge.pmd.lang.java.ast.AccessNode.Visibility.V_PRIVATE;
 import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
+import net.sourceforge.pmd.lang.java.ast.JModifier;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 
@@ -21,7 +22,7 @@ public class ClassWithOnlyPrivateConstructorsShouldBeFinalRule extends AbstractJ
     @Override
     public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
         if (node.isRegularClass()
-            && !node.isFinal()
+            && !node.hasModifiers(JModifier.FINAL)
             && hasOnlyPrivateCtors(node)
             && hasNoSubclasses(node)) {
             addViolation(data, node);
@@ -37,7 +38,7 @@ public class ClassWithOnlyPrivateConstructorsShouldBeFinalRule extends AbstractJ
     }
 
     private boolean doesExtend(ASTAnyTypeDeclaration sub, ASTClassOrInterfaceDeclaration superClass) {
-        return sub != superClass && TypeTestUtil.isA(superClass.getTypeMirror(), sub);
+        return sub != superClass && TypeTestUtil.isA(superClass.getTypeMirror().getErasure(), sub);
     }
 
     private boolean hasOnlyPrivateCtors(ASTClassOrInterfaceDeclaration node) {

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/ClassWithOnlyPrivateConstructorsShouldBeFinal.xml

@@ -7,6 +7,18 @@
     <test-code>
         <description>Simple violation</description>
         <expected-problems>1</expected-problems>
+        <expected-linenumbers>1</expected-linenumbers>
+        <code><![CDATA[
+public class Foo {
+    private Foo() { }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>violation for abstract class</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>1</expected-linenumbers>
         <code><![CDATA[
 public class Foo {
     private Foo() { }
@@ -35,6 +47,17 @@ public class Foo {
         ]]></code>
     </test-code>
 
+    <test-code>
+        <description>abstract class, one public constructor, not required to be final</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public abstract class Foo {
+    private Foo() { }
+    public Foo(String param) { }
+}
+        ]]></code>
+    </test-code>
+
     <test-code>
         <description>Ok, subclass using the private constructor</description>
         <expected-problems>0</expected-problems>
@@ -74,7 +97,7 @@ public class Foo {
 public class Foo {
     private Foo() { }
 }
-public class Bar extends Foo {
+class Bar extends Foo {
     public Bar() { }
 }
         ]]></code>
@@ -140,27 +163,55 @@ class ClassWithOnlyPrivateConstructorsShouldBeFinal {
         <expected-problems>1</expected-problems>
         <expected-linenumbers>2</expected-linenumbers>
         <code><![CDATA[
-            public class Main209 {
-                private static class InputBits {
-                    private InputBits(int number) { }
-                }
-            }
-            ]]></code>
+public class Main209 {
+    private static class InputBits {
+        private InputBits(int number) { }
+    }
+}
+        ]]></code>
     </test-code>
     <test-code>
-        <description>Private inner class with no ctor</description>
+        <description>Inner class with only private constructor extended within compilation unit (#2536)</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Main209 {
+    private static class InputBits {
+        private InputBits(int number) { }
+    }
+    public static final class Sub extends InputBits {
+        private Sub() {
+            super(1);
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>Private inner class with no ctor and no usage</description>
         <expected-problems>1</expected-problems>
         <expected-linenumbers>2</expected-linenumbers>
         <code><![CDATA[
-            public class Main209 {
-                private static class InputBits {
-                    // default ctor is implicitly private
-                }
-            }
-            ]]></code>
+public class Main209 {
+    private static class InputBits {
+        // default ctor is implicitly private
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>Private inner class with no ctor and usage</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Main209 {
+    private static class InputBits {
+        // default ctor is implicitly private
+    }
+    public static final class Sub extends InputBits {}
+}
+        ]]></code>
     </test-code>
 
-    <test-code regressionTest="false">
+    <test-code>
         <description>Private abstract classes with abstract methods should be ignored #3668</description>
         <expected-problems>0</expected-problems>
         <code><![CDATA[
@@ -168,6 +219,32 @@ public class Outer {
     private abstract class Base {
         abstract void run();
     }
+    public void someMethod() {
+        Base b = new Base() {
+            void run() {}
+        };
+        b.run();
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Private abstract generic classes with abstract methods should be ignored #3668</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.util.Comparator;
+public class Outer {
+    private abstract class Base<E> {
+        abstract void run();
+        abstract void sort(Comparator<E> comparator);
+    }
+    public void someMethod() {
+        Base b = new Base<Object>() {
+            void run() {}
+            void sort(Comparator<Object> comparator) {}
+        };
+    }
 }
         ]]></code>
     </test-code>
@@ -185,7 +262,7 @@ public class Outer {
         ]]></code>
     </test-code>
 
-    <test-code regressionTest="false">
+    <test-code>
         <description>Private abstract classes without abstract methods and subclasses #3668</description>
         <expected-problems>1</expected-problems>
         <expected-linenumbers>2</expected-linenumbers>
@@ -194,6 +271,22 @@ public class Outer {
     private abstract class Base {
         private void run() {}
     }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Abstract outer classes should be flagged #3668</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>1</expected-linenumbers>
+        <code><![CDATA[
+public abstract class FooUtils {
+    private FooUtils() {
+        // private constructor can't be called from sub classes,
+        // so this class can't be extended
+    }
+    // note: this "abstract" class does not contain abstract methods
+    // it was only declared as abstract to prevent instantiation
 }
         ]]></code>
     </test-code>