[java] ClassWithOnlyPrivateConstructorsShouldBeFinal - fix FP with inner private classes

[adangel]

classes.

I found this while trying to run PMD 7 on PMD 7 sources (dogfood).

Rule: ClassWithOnlyPrivateConstructorsShouldBeFinal

E.g.:

pmd/pmd-core/src/main/java/net/sourceforge/pmd/cpd/GUI.java

Line 775 in f36b99f

private abstract class SortingTableModel<E> extends AbstractTableModel {

[pmd-test]

	2 Messages
📖	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 19 violations, 0 errors and 0 configuration errors. Full report
📖	Compared to master: This changeset changes 48816 violations, introduces 26199 new violations, 5 new errors and 0 new configuration errors, removes 154526 violations, 27 errors and 3 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 48816 violations, introduces 26218 new violations, 5 new errors and 0 new configuration errors, removes 154526 violations, 27 errors and 3 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 4 new violations, 0 new errors and 0 new configuration errors, removes 290 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 45901 violations, introduces 25411 new violations, 38 new errors and 0 new configuration errors, removes 182110 violations, 27 errors and 3 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 4 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 45900 violations, introduces 25658 new violations, 38 new errors and 0 new configuration errors, removes 182075 violations, 27 errors and 3 configuration errors. Full report
✅	Compared to pmd/7.0.x: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 36 violations, 0 errors and 0 configuration errors. Full report
✅	Compared to master: This changeset changes 45895 violations, introduces 25631 new violations, 38 new errors and 0 new configuration errors, removes 182080 violations, 27 errors and 3 configuration errors. Full report

Generated by 🚫 Danger

[adangel]

Ok, much better now: Only real false-positive cases are now removed.

[jsotuyod]

I'm unsure excluding abstract classes is the right approach here…

If we had a top level class:

public abstract FooUtils {
    private FooUtils() {
        throw new AssertionError("Do not instantiate utility classes");
    }

…
}

I'd still consider this something to be flagged. Here abstract is being used as "can't instantiate it", which is incorrect. abstract actually means "I expect this to be extended", which would obviously be a contradiction with the private constructor. The right approach would be to have this class be final ("can't extend"), with a private constructor ("can't instantiate") to reinforce the design idea.

I think this false positive can be addressed in a different way, not by waving abstract classes, but by waving nested classes. This is down to the fact that these "private constructors" can actually be invoked within the top level class by way of the auto generated default-visiblity constructor the compiler generate (and hence, do not contain "only private constructors" as the rule flags).

EDIT: Java 11+ doesn't generate the synthetic method, but can still invoke the private members due to nest-based access control

[adangel]

Hm... I'm working on it, but I'm regularly loose what this rule should do... so, I'm trying to describe, what should be correct as far as I understand (please correct me...):

The rule ClassWithOnlyPrivateConstructorsShouldBeFinal should flag classes, that can't be extended, because it has only private constructors. So far so good. This has nothing to do with a constructor, that prevents instantiation by throwing a exception.

This creates the following test cases:
a) public top level class with only private constructors -> class should be made final
b) public abstract top level class with only private constructors -> this should be flagged. It reminds the developer to decided: either abstract and at least protected constructor or final and not abstract.
c) public final top level class with only private constructors -> ok

Now considering inner classes. There is also #2536 . Let's only consider first inner classes that are visible somewhere else (not private inner classes). The same test cases apply here. For inner classes that are public, package-private or protected - if they only contain private constructors -> class should be made final. But unless they are subclassed in the same compilation unit (then making it final would result in compilation error, so the rule shouldn't suggest that).

Now the very special case private inner classes. These can be used inside the same compilation unit without restriction. So, making a constructor private here does not prevent subclassing. We probably indeed should do nothing there. We could suggest to make the class final - but for other reasons (there are no subclasses) - and therefore it would be a different rule.

I think, I was additionally thrown off now by the sentence in the rule description "... unless the private constructor is invoked by a inner class". These are different things... It should probably be "... unless it is a private inner class". Whether the constructors are invoked or not doesn't matter.

I'll try to update the test cases accordingly....