ImportOrder: tokens property should be not customizable, move STATIC_IMPORT to required tokens

[rnveach]

Identified at #3955 ,

https://checkstyle.org/config_imports.html#ImportOrder_Properties

There are no examples or explanation from what STATIC_IMPORT gives you in this check.
It is enabled by default, but what happens if you disable it, if its possible. It is also not clear how it can be disabled in configuration, which I assume involves setting the property to an empty string.

We should examine what this token does, if it should remain optional, and update the xdoc based on the findings.

User Migration

As all tokens were made required, users can either remove the tokens property for the check. Leaving it alone will also have no side effects as there is no way to override required tokens.

[rnveach]

This is looking similar to #3955 (comment) ,

STATIC_IMPORT token was added at 6125bef as a default token. Acceptable tokens was clarified at abe2b20 that both should be acceptable. It wasn't until d56a2a3 that required was defined, but only 1 token was added as required. There was no discussion around this change.

kens like this, where a single token is listed in the acceptable and default required documentation tables can be "removed". The only way to do this is to define properties as the list of required tokens. The required tokens for this check is IMPORT.

Basically not adding STATIC_IMPORT makes the check act like the static imports don't exist. This can probably lead to weird side-effects, depending on the make up of the file, as we still look at line numbers when doing some validations. Properties like separatedStaticGroups, staticGroups, sortStaticImportsAlphabetically, useContainerOrderingForStatic should become meaningless if static imports are ignored.

Example.
Current Behavior:

$ cat TestClass.java
import static java.io.File.createTempFile;
import static java.lang.Math.abs; // OK, alphabetical case-sensitive ASCII order, 'i' < 'l'
import java.lang.Math.sqrt; // OK, follows property 'Option' value 'above'
import java.io.File; // violation, alphabetical case-sensitive ASCII order, 'i' < 'l'

import java.io.IOException; // violation, extra separation in 'java' import group

import org.albedo.*;

import static javax.WindowConstants.*; // violation, wrong order, 'javax' comes before 'org'
import javax.swing.JComponent;
import org.apache.http.ClientConnectionManager; // violation, must separate from previous import
import org.linux.apache.server.SoapServer; // OK

import com.neurologic.http.HttpClient; // OK
import com.neurologic.http.impl.ApacheHttpClient; // OK

public class TestClass { }

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
<module name="ImportOrder">
  <property name="groups" value="/^java\./,javax,org,com"/>
  <property name="ordered" value="true"/>
  <property name="separated" value="true"/>
  <property name="option" value="above"/>
  <property name="sortStaticImportsAlphabetically" value="true"/>
</module>
    </module>
</module>

$ java -jar checkstyle-10.3.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:4:1: Wrong order for 'java.io.File' import. [ImportOrder]
[ERROR] TestClass.java:6:1: Extra separation in import group before 'java.io.IOException' [ImportOrder]
[ERROR] TestClass.java:10:1: Wrong order for 'javax.WindowConstants.*' import. [ImportOrder]
[ERROR] TestClass.java:12:1: 'org.apache.http.ClientConnectionManager' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 4 errors.

Behavior with STATIC_IMPORT removed:

$ cat TestClass.java
import static java.io.File.createTempFile;
import static java.lang.Math.abs; // OK, alphabetical case-sensitive ASCII order, 'i' < 'l'
import java.lang.Math.sqrt; // OK, follows property 'Option' value 'above'
import java.io.File; // violation, alphabetical case-sensitive ASCII order, 'i' < 'l'

import java.io.IOException; // violation, extra separation in 'java' import group

import org.albedo.*;

import static javax.WindowConstants.*; // violation, wrong order, 'javax' comes before 'org'
import javax.swing.JComponent;
import org.apache.http.ClientConnectionManager; // violation, must separate from previous import
import org.linux.apache.server.SoapServer; // OK

import com.neurologic.http.HttpClient; // OK
import com.neurologic.http.impl.ApacheHttpClient; // OK

public class TestClass { }

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
<module name="ImportOrder">
  <property name="groups" value="/^java\./,javax,org,com"/>
  <property name="ordered" value="true"/>
  <property name="separated" value="true"/>
  <property name="option" value="above"/>
  <property name="sortStaticImportsAlphabetically" value="true"/>
  <property name="tokens" value="IMPORT"/>
</module>
    </module>
</module>

$ java -jar checkstyle-10.3.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:4:1: Wrong order for 'java.io.File' import. [ImportOrder]
[ERROR] TestClass.java:6:1: Extra separation in import group before 'java.io.IOException' [ImportOrder]
[ERROR] TestClass.java:11:1: Wrong order for 'javax.swing.JComponent' import. [ImportOrder]
[ERROR] TestClass.java:12:1: 'org.apache.http.ClientConnectionManager' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 4 errors.

Notice the validation of the file with the STATIC_IMPORT changed one of the violations.

However, if I remove statics imports completely, which one would assume should be the same as removing the token, then the violations that come up are not the same as the one with the token removed.

$ cat TestClass.java
import java.lang.Math.sqrt; // OK, follows property 'Option' value 'above'
import java.io.File; // violation, alphabetical case-sensitive ASCII order, 'i' < 'l'

import java.io.IOException; // violation, extra separation in 'java' import group

import org.albedo.*;

import javax.swing.JComponent;
import org.apache.http.ClientConnectionManager; // violation, must separate from previous import
import org.linux.apache.server.SoapServer; // OK

import com.neurologic.http.HttpClient; // OK
import com.neurologic.http.impl.ApacheHttpClient; // OK

public class TestClass { }

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
<module name="ImportOrder">
  <property name="groups" value="/^java\./,javax,org,com"/>
  <property name="ordered" value="true"/>
  <property name="separated" value="true"/>
  <property name="option" value="above"/>
  <property name="sortStaticImportsAlphabetically" value="true"/>
  <property name="tokens" value="IMPORT"/>
</module>
    </module>
</module>

$ java -jar checkstyle-10.3.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:2:1: Wrong order for 'java.io.File' import. [ImportOrder]
[ERROR] TestClass.java:4:1: Extra separation in import group before 'java.io.IOException' [ImportOrder]
[ERROR] TestClass.java:8:1: Wrong order for 'javax.swing.JComponent' import. [ImportOrder]
[ERROR] TestClass.java:9:1: 'org.apache.http.ClientConnectionManager' should be separated from previous imports. [ImportOrder]
Audit done.
Checkstyle ends with 4 errors.

[rnveach]

Similar to #3955 , I also feel allowing STATIC_IMPORT to be optional is a bug and the check does not really handle the situation appropriately. I could see some arguments asking to not validate statics, but I think that should probably be a separate option to avoid the weird way we have to "remove" single optional tokens like this.

[nrmancuso]

Behavior without STATIC_IMPORT token is strange, I agree. Let's approve this and try to make all similar breaking changes in the same release.

[romani]

fix is merged.

I changed label to be bug, as user can continue to use his config.
the only change is behavior.