False Negative of ClassFanOutCheck with "new" Keyword

[Lmh-java]

Spotted at #14740 (comment)

I have read check documentation: https://checkstyle.sourceforge.io/checks/metrics/classfanoutcomplexity.html
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run (10.15.0)
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

Compiling test case:

/var/tmp $ javac Test.java
[No complains]

Config file:

/var/tmp $ cat config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" 
     "http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="ClassFanOutComplexity">
      <property name="max" value="0" />
    </module>
  </module>
</module>

Test Java file:

/var/tmp $ cat Test.java

import java.util.List;
import java.util.Map;
import java.util.ArrayList;
import java.io.File;

public class Test {
    public void testMethod(Map<String, List<String>> entry) {
        List<File> files = new ArrayList<>();
        String string = "";
        entry.values().stream()
                .flatMap(List::stream)
                .map(File::new)
                .forEach(files::add);
    }
}

Audit result:

/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-10.15.0-all.jar -c config.xml Test.java
Starting audit...
Audit done.

Expected result:

Starting audit...
[ERROR] /var/tmp/Test.java:5:1: Class Fan-Out Complexity is 1 (max allowed is 0). [ClassFanOutComplexity]
Audit done.
Checkstyle ends with 1 errors.

Reasoning: Our Java test case relies on Constraint class, so there should be one class dependency counted. This is caused by incorrectly handled new keyword inside lambda expression. In other words, it cannot recognize Constraint::new as creating an instance of the Constraint class.

I will work on this after PR #14740 is done.

[romani]

Ideally this Check description should be updated to explain this metric.
It is not obvious why we need "new" token proper handling if all types are in imports.

[romani]

@Lmh-java , please update test to not use inner class, as inner classes should not be counted, please use some JDK class.

[Lmh-java]

@Lmh-java , please update test to not use inner class, as inner classes should not be counted, please use some JDK class.

Updated to use java.io.File instead. The false negative still presents

[Lmh-java]

Ideally this Check description should be updated to explain this metric.
It is not obvious why we need "new" token proper handling if all types are in imports.

@romani I was looking at the code. Basically, the problem comes from AbstractClassCouplingCheck, so it will also affect the sbling checks of ClassFanOut. Import statements are only used to check whether a reference should be excluded based on excludedClasses. The check actually counts complexity from parsing different types of tokens and handle them to find out the class references.

Sure, I will also update the documentation along with this change, if this issue is approved.

[Prathamesh-007]

I have gone through the issue.
Code:

import java.util.List;
import java.util.ArrayList;
import java.io.File;

public class Test {
    public void testMethod(List<String> list) {
        List<File> files = new ArrayList<>();
    }
}

Config file:

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
     "http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="ClassFanOutComplexity" >
      <property name="max" value="0" />
    </module>
  </module>
</module>

After running java -jar checkstyle-10.14.2-all.jar -c config.xml Test.java there is no error as you have rightly mentioned

But you will get error for:

public void testMethod(List<String> list) {
        File obj;
    }

I think in the first case (no error case) there is a problem with visitType method in AbstractClassCoupling. When a token is IDENT, it is added to referencedClassName if it is a class (import is mapped). But that is not happening here even though "File" is being identified as IDENT.

[Lmh-java]

@Prathamesh-007 Thanks for your analysis. :)

It seems that you are focusing on the type parameter of the List (i.e., List<File>). However, this issue focuses on the the lambda expression with new keyword (File::new), as mentioned in the PR description.

I am not very sure about why the type parameter is not counted as a "complexity". I haven't gone through that part of code yet, but I feel like that is a very obvious case that is too hard to miss by all these users and tests. I would assume this might be designed this way on purpose? I will look into that and get back to this later. But for this issue we might should focus more on the File::new expression. I think I have seen a method that deals with new keyword in the check ircc, and you might want to look into it. :)