IllegalImport with class pattern fails on static import

[Bananeweizen]

https://checkstyle.sourceforge.io/checks/imports/illegalimport.html#IllegalImport

IllegalImport with property illegalClasses configured doesn't detect illegal static imports, because it just does a text comparison of the import statement (which contains the method names for static imports):

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/IllegalImportCheck.java

Line 245 in c525c67

if (importText.equals(element)) {

/var/tmp $ cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="severity" value="error"/>
  <property name="fileExtensions" value="java, properties, xml"/>
  <module name="TreeWalker">
    <module name="IllegalImport">
      <property name="illegalClasses" value="org.junit.jupiter.api.Assertions"/>
    </module>
  </module>
</module>

/var/tmp $ cat TestStatic.java
import static org.junit.jupiter.api.Assertions.fail;

public class TestStatic {

  void useTheImport() {
    fail();
  }
}

/var/tmp $ cat Test.java
import org.junit.jupiter.api.Assertions;

public class Test {

  void useTheImport() {
    Assertions.fail();
  }
}

❯ java %RUN_LOCALE% -jar checkstyle-10.12.3-all.jar -c config.xml Test.java
Starting audit...
[ERROR] C:\dev\eclipse-cs\ws\Reproducer\Test.java:1:1: 
     Illegal import - org.junit.jupiter.api.Assertions. [IllegalImport]
Audit done.
Checkstyle ends with 1 errors.

❯ java %RUN_LOCALE% -jar checkstyle-10.12.3-all.jar -c config.xml TestStatic.java
Starting audit...
Audit done.

Both files should report the same issue.

Causes checkstyle/eclipse-cs#523

[nrmancuso]

Link to check documentation: https://checkstyle.sourceforge.io/checks/imports/illegalimport.html#IllegalImport

[AayushSaini101]

I am on it

[nrmancuso]

I am removing 'approved' label, since this seems like the expected check behavior.

From https://checkstyle.sourceforge.io/checks/imports/illegalimport.html#IllegalImport:

illegalClasses... checks if import equals class name.

This import equals the given class name: org.junit.jupiter.api.Assertions
This import does not equal the given class name: org.junit.jupiter.api.Assertions.fail

Checkstyle is not type aware, and operates on one file at a time. It has no idea if fail is a class or method. Furthermore, this check is aware of static imports, and we can prove that the check behavior is the same, regardless of static import or not (we can ignore the ability to compile this file for the sake of this example):

➜  src cat Test.java 
import static org.junit.jupiter.api.Assertions;

public class Test {

}

➜  src cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="severity" value="error"/>
  <property name="fileExtensions" value="java, properties, xml"/>
  <module name="TreeWalker">
    <module name="IllegalImport">
      <property name="illegalClasses" value="org.junit.jupiter.api.Assertions"/>
    </module>
  </module>
</module>
➜  src java -jar checkstyle-10.12.1-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /home/nick/IdeaProjects/tester/src/Test.java:1:1: Illegal import - org.junit.jupiter.api.Assertions. [IllegalImport]
Audit done.
Checkstyle ends with 1 errors.
➜  src cat Test.java                                               
import org.junit.jupiter.api.Assertions;

public class Test {

}

➜  src java -jar checkstyle-10.12.1-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /home/nick/IdeaProjects/tester/src/Test.java:1:1: Illegal import - org.junit.jupiter.api.Assertions. [IllegalImport]
Audit done.
Checkstyle ends with 1 errors.

Imports are treated the same.

@Bananeweizen you might want to try the following config to achieve the check behavior that you expected in the issue description:

➜  src cat Test.java 
import static org.junit.jupiter.api.Assertions.fail;

public class Test {

}

➜  src cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="severity" value="error"/>
  <property name="fileExtensions" value="java, properties, xml"/>
  <module name="TreeWalker">
    <module name="IllegalImport">
      <property name="regexp" value="true"/>
      <property name="illegalClasses" value="org.junit.jupiter.api.Assertions.*"/>
    </module>
  </module>
</module>
➜  src java -jar checkstyle-10.12.1-all.jar -c config.xml Test.java

Starting audit...
[ERROR] /home/nick/IdeaProjects/tester/src/Test.java:1:1: Illegal import - org.junit.jupiter.api.Assertions.fail. [IllegalImport]
Audit done.
Checkstyle ends with 1 errors.

[romani]

confusion point is fact that we did not explain in doc how we match. We are not type aware, yes, but it is same for packages.
We have such properties for humans to easily understand what they want. But we simply apply regexp(or strict match) to line of import from illegalPkgs and illegalClasses .

Example for illegalPkgs that affects

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="severity" value="error"/>
  <module name="TreeWalker">
    <module name="IllegalImport">
      <property name="illegalPkgs" value="org.junit.jupiter.api"/>
    </module>
  </module>
</module>

$ cat Test.java 
import static org.junit.jupiter.api.Assertions.fail;

public class Test {

}

$ java -jar checkstyle-10.12.4-all.jar -c config.xml Test.java
Starting audit...
[ERROR] /var/tmp/Test.java:1:1: Illegal import - org.junit.jupiter.api.Assertions.fail. [IllegalImport]
Audit done.
Checkstyle ends with 1 errors.

I do believe we just need to relax equals to be startWith as we do for packages

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/IllegalImportCheck.java

Line 238 in c525c67

if (importText.startsWith(element + ".")) {

There is always posibility to have inner class for illegal import
import static org.junit.jupiter.api.Assertions.SomeInnerClass.fail;

I still believe issue should be approved, but implementation (of referenced PR) should different and we should expand documentation.

[nrmancuso]

confusion point is fact that we did not explain in doc how we match

checks if import equals class name

[romani]

specify class names to reject

As you pointed, we are not type aware, so we simply match string. And we have this two options just convenience for users to define what they want. Ideally there should be should be single property regexp that simply define string to find in import line.
But we end up with two properties of same functionality.

[rnveach]

But we end up with two properties of same functionality

They are not the same, just similar. Package assumes it is not a complete match. Class assumes it is a complete match. Package has some logic to basically check if a period comes after the user supplied text.

be single property regexp that simply define string to find in import line

2 properties. 1 of what text to match, and the other to specify if it is regexp or not.

I would still think some users would want to possibly differentiate between static and non-static imports, which may be another option.

=============

A static import is always guaranteed to be import static <package and/or Class>.<Class>.<method or field>. This could give us part of the class, but as the "and/or" shows, it isn't guaranteed and this fails on regular import as it is still import <package and/or Class>.<Class>. To identify what "and/or" is requires type aware.

It seems to me we either need to fully clarify documentation and live with the check as it is, or change it and break compatibility.

[romani]

Let's not break compatibility.

I still see that user assumption that

<module name="IllegalImport">
   <property name="illegalClasses" 
 
   value="org.junit.jupiter.api.Assertions"/>
</module>

Will violate:
import static org.junit.jupiter.api.Assertions.fail;

Is valid assumption.

So we just need to update implementation to handle it. We trust user in config and he say that org.junit.jupiter.api.Assertions is class name we need to rely on this.
All code like: import xxxxxx org.junit.jupiter.api.Assertions.xxxxxx.xxxxx; should be violated.

regexp just convenience for users to define it value="org.junit..*.Assertions"/>

[nrmancuso]

@Bananeweizen please share your opinion

[romani]

@rnveach , please share your opinion on relying on user input, we such strategy in other Checks.
So it can help us: to update check a bit to identify class asis , as it is defined by user. No special properties for static. We need extend doc to be clear with user how we match.

[rnveach]

I don't understand what you want my opinion on.

I gave a opinion at #13772 (comment) .

[romani]

do you propose ?

<module name="IllegalImport">
   <property name="illegalStaticImports" 
       value="org.junit.jupiter.api.Assertions,org.junit.jupiter.api.Something"/>
</module>

[rnveach]

#13772 (comment)

2 properties. 1 of what text to match, and the other to specify if it is regexp or not.

[romani]

we have 3 properties
https://checkstyle.org/checks/imports/illegalimport.html#Properties

illegalPkgs was added at 3.0 , if will be very noticeable breaking compatibility.
And there are starts imports , where package is defined.
https://www.baeldung.com/java-wildcard-imports

So it is reasonable to have it,
illegalClasses also reasonable.

redoing whole Check, 2 property deplrecation, for such small update is not very reasoable to my mind.

[rnveach]

Then why did #13772 (comment) mention property illegalStaticImports, which also doesn't exist. I am not understanding where this conversation is going.

[romani]

I tried to understand you and wrote a bit more exact proposal to guess what you mean in case you have no time to write more extended, but you can do brief responses like yes/no.

What I see is we can not agree on how to fix it, so issue stays unapproved, may be in future someone change his mind

Meanwhile users need to use workaround with usage of regexp property.

[rnveach]

You can approve issue for how you want to do it. I was asked for my opinion and with no clear understanding of on what, I kept pointing to my post that had my opinion. If we don't want to break compatibility, then we should update documentation for users. Since changing the property type is breaking, and I don't see adding another property will make things clearer, I can only see right now documentation change.

[Bananeweizen]

@Bananeweizen please share your opinion

While updating the documentation might be one way out of this, I'd expect more users to either not read those details or to not recognize the consequences for static imports, thereby having their configuration work for some, but not all cases. I personally also realized this only after several years of active use. You as the developers of the tool have a very precise mental model of what's happening. Users have a totally different and way more simple mental model of the same thing. Therefore if this gets fixed with documentation only, it should have at least a complete example of how to match both static and non static imports.

Regarding an actual fix via code: Can't we use a rule of thumb, that good Java developers use uppercase class names and lowercase method names? Therefore I would remove all segments from an import, that start with a lowercase character, and are behind the last segment starting with an uppercase character. And I would use this "normalized" import with the same code as before. That way the import "org.junit.jupiter.api.Assertions.fail" would be normalized to "org.junit.jupiter.api.Assertions", and that would then fit again to the current implementation, right?

[romani]

Can't we use a rule of thumb, that good Java developers use uppercase class names and lowercase method names?

we do not need to do this.
User define: <property name="illegalClasses" value="org.junit.jupiter.api.Assertions"/> we already knows and trust user that org.junit.jupiter.api.Assertions is Class with its package, we just need to use this string value as-is to match, no understanding of package name or short class name is required.