Issue #10859: FinalClass now exempts private-only constructor classes that are extended by another class in the same compilation unit

[pbludov]

Resolves #10859: FinalClass now exempts private-only constructor classes that are extended by another class in the same compilation unit

Original PR: #10887

Diff Regression projects: https://gist.githubusercontent.com/Vyom-Yadav/91807e6244cff60698d9e33e32ba2d51/raw/eb228fa41edadc10eea264a914ea1411f0e52122/projects-to-test-on.properties
Diff Regression config: https://gist.githubusercontent.com/Vyom-Yadav/98dceb63a79f4833e85fff9b2e1464a6/raw/2e9e29be66f99b22505651aa211886535402b6cf/my_checks.xml

[Vyom-Yadav]

@pbludov Looks good to me, report is also fine, just remove

Doesn't check for classes nested in interfaces or annotations, as they are always final there.

and examples related to it.
https://checkstyle.sourceforge.io/config_design.html#FinalClass

Edit-
Also, why are we using TreeMap, I don't see any benefit of sorting, can't we use HashMap instead

[pbludov]

Also, why are we using TreeMap, I don't see any benefit of sorting, can't we use HashMap instead

Originally, it was a HashMap. But pitest say "hey! you null-check is useles! Remove it". For this code:

        final String currentAstSuperClassName = getSuperClassName(classAst);
        if (currentAstSuperClassName != null) {                                                                // Pitest violation
            final List<ClassDesc> sameNamedClassList = classesWithSameName(
                    currentAstSuperClassName);
            if (sameNamedClassList.isEmpty()) {
                final ClassDesc classDesc = innerClasses.get(currentAstSuperClassName);
                if (classDesc != null) {
                    classDesc.registerNestedSubclass();
                }
            }
            else {
                registerNearestAsSuperClass(qualifiedClassName, sameNamedClassList);
            }
        }

and this is true: classesWithSameName(null) works fine, it basically look for ".null" prefix, innerClasses.get(null) also works.

UPD: String::concat will throw NPE here.

[pbludov]

String.concat used here to fix a pitest issue about redundant null check.
String.concat throws NPE, but PACKAGE_SEPARATOR + className doesn't.

[pbludov]

Github, generate report

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/8cc50b3_2021104834/reports/diff/index.html

[romani]

@nmancus1 , please finish review first.

[nrmancuso]

Few minor items:

[nrmancuso]

Github, generate site

Link to check documentation: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c986f11_2021155035/config_design.html#FinalClass

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c986f11_2021155035/index.html

[pbludov]

Github, generate report

[romani]

@pbludov , please fix conflict

[pbludov]

@pbludov , please fix conflict

done

[romani]

Github, rebase

[romani]

Github, generate report

[romani]

both cases in https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4887dca_2021170400/reports/diff/elasticsearch/index.html#A1 looks likefalse postive

UPDATE: it is valid violatoin as there is no extensions of this class.

[github-actions]

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/fe655b7_2022000435/reports/diff/index.html

[romani]

I unresolved #10968 (comment)

item:

[romani]

ok to merge

[rnveach]

Maybe this connects too much to my other comment, but why not a Set (or TreeSet) ? Can we have multiples that we can't drop?

[pbludov]

There's no reason to return a set here. This method returns a list that is iterated once.

[rnveach]

It is not clear why not, especially since the code was changed to return a Collection which a Set is also subset of.

Collections.min(Collection<? extends T> coll, Comparator<? super T> comp), requires a Comparator in the 2nd argument.
Also using sorted method with the stream in classesWithSameName also requires a Comparator.

A sorted list will make finding the min the easiest as well since it will always be the first entry if it is not empty.

It is not going to gain us any compare time by combining classesWithSameName and registerNearestAsSuperClass to just find and return the nearest and keep the collection/list internal? Seems to me this way as it travels through the stream, it will already be tracking for the closest one in addition. registerNestedSubclassToOuterSuperClasses just really cares if we find a nearest or not, which in a sorted list should be the first entry.

[pbludov]

A sorted list will make finding the min the easiest as well since it will always be the first entry if it is not empty.

We need just closest class. For example,

class A {
  class B {}
}

class C {
   class A {
   }
   class E {
     class F extends A {}
   }
 }

For class F, the closest A is C.A, not the first class in the list sorted in any way.

[rnveach]

@pbludov Why won't this work? All the tests written continue to pass on my local.

diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java
index edc0e68..26af1aa 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/design/FinalClassCheck.java
@@ -20,13 +20,10 @@
 package com.puppycrawl.tools.checkstyle.checks.design;
 
 import java.util.ArrayDeque;
-import java.util.Collection;
-import java.util.Collections;
 import java.util.Deque;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
-import java.util.stream.Collectors;
 
 import com.puppycrawl.tools.checkstyle.FileStatefulCheck;
 import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
@@ -271,13 +268,13 @@
                                                            ClassDesc currentClass) {
         final String superClassName = getSuperClassName(currentClass.getClassAst());
         if (superClassName != null) {
-            final Collection<ClassDesc> homonyms = classesWithSameName(superClassName);
-            if (homonyms.isEmpty()) {
+            final ClassDesc nearest = getNearestClassWithSameName(superClassName, qualifiedClassName);
+            if (nearest == null) {
                 Optional.ofNullable(innerClasses.get(superClassName))
                         .ifPresent(ClassDesc::registerNestedSubclass);
             }
             else {
-                registerNearestAsSuperClass(qualifiedClassName, homonyms);
+                nearest.registerNestedSubclass();
             }
         }
     }
@@ -289,35 +286,22 @@
      * @return true if there is another class with same name.
      * @noinspection CallToStringConcatCanBeReplacedByOperator
      */
-    private Collection<ClassDesc> classesWithSameName(String className) {
+    private ClassDesc getNearestClassWithSameName(String className, String superClassName) {
         final String dotAndClassName = PACKAGE_SEPARATOR.concat(className);
         return innerClasses.entrySet().stream()
                 .filter(entry -> entry.getKey().endsWith(dotAndClassName))
                 .map(Map.Entry::getValue)
-                .collect(Collectors.toUnmodifiableList());
-    }
-
-    /**
-     * For all classes with the same name as the superclass, finds the nearest one
-     * and registers a subclass for it. The nearest class is the class that have
-     * the longest common prefix with the parameter {@code superClassName}.
-     * If more than one class matches, the winner is the most inner class.
-     *
-     * @param superClassName current class name
-     * @param classesWithSameName classes which same name as super class
-     */
-    private static void registerNearestAsSuperClass(String superClassName,
-                                                    Collection<ClassDesc> classesWithSameName) {
-        final ClassDesc nearest = Collections.min(classesWithSameName, (first, second) -> {
-            int diff = Integer.compare(
-                    classNameMatchingCount(superClassName, second.getQualifiedName()),
-                    classNameMatchingCount(superClassName, first.getQualifiedName()));
-            if (diff == 0) {
-                diff = Integer.compare(first.getDepth(), second.getDepth());
-            }
-            return diff;
-        });
-        nearest.registerNestedSubclass();
+                .sorted((first, second) -> {
+                    int diff = Integer.compare(
+                            classNameMatchingCount(superClassName, second.getQualifiedName()),
+                            classNameMatchingCount(superClassName, first.getQualifiedName()));
+                    if (diff == 0) {
+                        diff = Integer.compare(first.getDepth(), second.getDepth());
+                    }
+                    return diff;
+                })
+                .findFirst()
+                .orElse(null);
     }
 
     /**

This is what I was talking about basically combining, the 2 into one. Sorting by nearest and selecting the first should virtually be the same as comparing each one to find nearest and looking for the min of the group. Sorting is just about organizing from min to max.

Sorting and comparing is all the things a comparator is used for and can be interchangeable.

[pbludov]

Done. Your patch has been applied. With one improvement suggested by IDEA:

this change makes sense, as sorted has the complexity O(N*N) (in the worst case) while min is O(N) only.

[rnveach]

I did not realize the stream had a min as well. Your right it makes more sense as we don't need to sort everything, just find the min.

sorted has the complexity O(N*N)

Why is sorted N^2 complexity when sorting a collection is N log N ? Is this just because of the extra, underlying work needed to be done with streams?

Edit:
Looking at my JDK's implementation of SortedOps, I see a few usages of Arrays.sort which is also N log N.

[rnveach]

https://stackoverflow.com/questions/49798129/what-is-more-efficient-sorted-stream-or-sorting-a-list

the sorted() method adds a component to a stream pipeline that captures all of the stream elements into either an array or an ArrayList.
If an ArrayList is used, you incur the extra overhead of building / growing the list.

as the list gets larger, the O(NlogN) sorting will tend to dominate the O(N) overheads of copying. That will mean that the relative difference between the two versions will get smaller.

Results show that Collections.sort is faster, but not by a big margin

[rnveach]

Maybe I am missing something from the requirements, but why are completely empty classes not checked?

[pbludov]

Good point. From the check description

Checks that a class which has only private constructors is declared as final.

And the specs says that

The default constructor has the same access modifier as the class.

Thus, if a private class has no ctor, it should be final.

[pbludov]

Started new issue #11338