Merge pull request #3404 from oowekyala:update-ClassWithOnlyPrivateConstructorsShouldBeFinal

[java] Update ClassWithOnlyPrivateConstructorsShouldBeFinal #3404

.ci/files/all-java.xml

@@ -130,7 +130,7 @@
     <rule ref="category/java/design.xml/AvoidThrowingNullPointerException"/>
     <!-- <rule ref="category/java/design.xml/AvoidThrowingRawExceptionTypes"/> -->
     <rule ref="category/java/design.xml/AvoidUncheckedExceptionsInSignatures"/>
-    <!-- <rule ref="category/java/design.xml/ClassWithOnlyPrivateConstructorsShouldBeFinal"/> -->
+    <rule ref="category/java/design.xml/ClassWithOnlyPrivateConstructorsShouldBeFinal"/>
     <rule ref="category/java/design.xml/CognitiveComplexity" />
     <rule ref="category/java/design.xml/CollapsibleIfStatements"/>
     <!-- <rule ref="category/java/design.xml/CouplingBetweenObjects"/> -->

docs/pages/7_0_0_release_notes.md

@@ -178,6 +178,8 @@ The following previously deprecated rules have been finally removed:
     * [#3218](https://github.com/pmd/pmd/pull/3218): \[java] Generalize UnnecessaryCast to flag all unnecessary casts
     * [#3221](https://github.com/pmd/pmd/issues/3221): \[java] PrematureDeclaration false positive for unused variables
     * [#3238](https://github.com/pmd/pmd/issues/3238): \[java] Improve ExprContext, fix FNs of UnnecessaryCast
+* java-design
+    * [#2536](https://github.com/pmd/pmd/issues/2536): \[java] ClassWithOnlyPrivateConstructorsShouldBeFinal can't detect inner class
 * java-errorprone
     * [#659](https://github.com/pmd/pmd/issues/659): \[java] MissingBreakInSwitch - last default case does not contain a break
     * [#1005](https://github.com/pmd/pmd/issues/1005): \[java] CloneMethodMustImplementCloneable triggers for interfaces

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

@@ -0,0 +1,49 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.design;
+
+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.rule.AbstractJavaRulechainRule;
+import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
+
+public class ClassWithOnlyPrivateConstructorsShouldBeFinalRule extends AbstractJavaRulechainRule {
+
+    public ClassWithOnlyPrivateConstructorsShouldBeFinalRule() {
+        super(ASTClassOrInterfaceDeclaration.class);
+    }
+
+    @Override
+    public Object visit(ASTClassOrInterfaceDeclaration node, Object data) {
+        if (node.isRegularClass()
+            && !node.isFinal()
+            && hasOnlyPrivateCtors(node)
+            && hasNoSubclasses(node)) {
+            addViolation(data, node);
+        }
+        return null;
+    }
+
+    private boolean hasNoSubclasses(ASTClassOrInterfaceDeclaration klass) {
+        return klass.getRoot()
+                    .descendants(ASTAnyTypeDeclaration.class)
+                    .crossFindBoundaries()
+                    .none(it -> doesExtend(it, klass));
+    }
+
+    private boolean doesExtend(ASTAnyTypeDeclaration sub, ASTClassOrInterfaceDeclaration superClass) {
+        return sub != superClass && TypeTestUtil.isA(superClass.getTypeMirror(), sub);
+    }
+
+    private boolean hasOnlyPrivateCtors(ASTClassOrInterfaceDeclaration node) {
+        return node.getDeclarations(ASTConstructorDeclaration.class).all(it -> it.getVisibility() == V_PRIVATE)
+            && (node.getVisibility() == V_PRIVATE // then the default ctor is private
+            || node.getDeclarations(ASTConstructorDeclaration.class).nonEmpty());
+    }
+
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeTestUtil.java

@@ -95,7 +95,7 @@ public final class TypeTestUtil {
             return isExactlyA(clazz, type.getSymbol());
         }
 
-        return isA(type, otherType);
+        return isA(otherType, type);
     }
 
 
@@ -134,26 +134,35 @@ public final class TypeTestUtil {
         return isA(canonicalName, thisType, null);
     }
 
+    public static boolean isA(@NonNull JTypeMirror t1, @Nullable TypeNode t2) {
+        return t2 != null && isA(t1, t2.getTypeMirror());
+    }
+
     /**
-     * This is the subtyping routine we use, which prunes some behavior
-     * of isSubtypeOf that we don't want (eg, that unresolved types are
-     * subtypes of everything).
+     * Checks whether the first type is a subtype of the second. This
+     * removes some behavior of isSubtypeOf that we don't want (eg, that
+     * unresolved types are subtypes of everything).
+     *
+     * @param t1 A supertype
+     * @param t2 A type
+     *
+     * @return Whether t1 is a subtype of t2
      */
-    private static boolean isA(JTypeMirror t1, JTypeMirror t2) {
-        if (t1 == null || t2 == null) {
+    private static boolean isA(@Nullable JTypeMirror t1, @NonNull JTypeMirror t2) {
+        if (t1 == null) {
             return false;
-        } else if (t1.isPrimitive() || t2.isPrimitive()) {
-            return t1.equals(t2); // isSubtypeOf considers primitive widening like subtyping
-        } else if (TypeOps.isUnresolved(t1)) {
+        } else if (t2.isPrimitive() || t1.isPrimitive()) {
+            return t2.equals(t1); // isSubtypeOf considers primitive widening like subtyping
+        } else if (TypeOps.isUnresolved(t2)) {
             // we can't get any useful info from this, isSubtypeOf would return true
             return false;
-        } else if (t2.isClassOrInterface() && ((JClassType) t2).getSymbol().isAnonymousClass()) {
+        } else if (t1.isClassOrInterface() && ((JClassType) t1).getSymbol().isAnonymousClass()) {
             return false; // conventionally
-        } else if (t1 instanceof JTypeVar) {
-            return t2.isTop() || isA(((JTypeVar) t1).getUpperBound(), t2);
+        } else if (t2 instanceof JTypeVar) {
+            return t1.isTop() || isA(t1, ((JTypeVar) t2).getUpperBound());
         }
 
-        return t1.isSubtypeOf(t2);
+        return t2.isSubtypeOf(t1);
     }
 
     private static boolean isA(@NonNull String canonicalName, @NonNull JTypeMirror thisType, @Nullable UnresolvedClassStore unresolvedStore) {
@@ -173,7 +182,7 @@ public final class TypeTestUtil {
         TypeSystem ts = thisType.getTypeSystem();
         @Nullable JTypeMirror otherType = TypesFromReflection.loadType(ts, canonicalName, unresolvedStore);
 
-        return isA(thisType, otherType);
+        return isA(otherType, thisType);
     }
 
     /**

pmd-java/src/main/resources/category/java/design.xml

@@ -312,27 +312,15 @@ public void foo() throws RuntimeException {
     <rule name="ClassWithOnlyPrivateConstructorsShouldBeFinal"
           language="java"
           since="4.1"
-          class="net.sourceforge.pmd.lang.rule.XPathRule"
-          message="A class which only has private constructors should be final"
+          class="net.sourceforge.pmd.lang.java.rule.design.ClassWithOnlyPrivateConstructorsShouldBeFinalRule"
+          message="This class has only private constructors and may be final"
           externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_design.html#classwithonlyprivateconstructorsshouldbefinal">
         <description>
-A class with only private constructors should be final, unless the private constructor
-is invoked by a inner class.
+Reports classes that may be made final because they cannot be extended from outside
+their compilation unit anyway. This is because all their constructors are private,
+so a subclass could not call the super constructor.
         </description>
         <priority>1</priority>
-        <properties>
-            <property name="xpath">
-                <value>
-<![CDATA[
-//TypeDeclaration[count(../TypeDeclaration) = 1]/ClassOrInterfaceDeclaration
-[@Final = false()]
-[ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration[@Private = true()]]
-[not(./ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/ConstructorDeclaration[(@Public = true()) or (@Protected = true()) or (@PackagePrivate = true())])]
-[not(.//ClassOrInterfaceDeclaration)]
-]]>
-                </value>
-            </property>
-        </properties>
         <example>
 <![CDATA[
 public class Foo {  //Should be final

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/ClassWithOnlyPrivateConstructorsShouldBeFinalTest.java

@@ -6,7 +6,6 @@ package net.sourceforge.pmd.lang.java.rule.design;
 
 import net.sourceforge.pmd.testframework.PmdRuleTst;
 
-@org.junit.Ignore("Rule has not been updated yet")
 public class ClassWithOnlyPrivateConstructorsShouldBeFinalTest extends PmdRuleTst {
     // no additional unit tests
 }

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

@@ -135,4 +135,28 @@ class ClassWithOnlyPrivateConstructorsShouldBeFinal {
 }
         ]]></code>
     </test-code>
+    <test-code>
+        <description>#2536 [java] ClassWithOnlyPrivateConstructorsShouldBeFinal can't detect inner class with only private constructor</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>2</expected-linenumbers>
+        <code><![CDATA[
+            public class Main209 {
+                private static class InputBits {
+                    private InputBits(int number) { }
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>Private inner class with no ctor</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>
+    </test-code>
 </test-data>