IllegalInstantiation: make all tokens should be required, so no need to use custom values in tokens property

[rnveach]

http://checkstyle.sourceforge.net/config_coding.html#IllegalInstantiation

tokens to check subset of tokens CLASS_DEF.

Usage of this token was to let users to skip local classes.
But classes allow to specify fully qualified name to exclude, local/nested classes also have fully qualified name. So all can be covered to make CLASS_DEF to be used all times, and let use play with classes only.

There are no examples or explanation from what CLASS_DEF 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 also have no tests showing what turning CLASS_DEF off does differently.

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 IllegalInstantiation. Leaving it alone will also have no side effects as there is no way to override required tokens.

[Sukrityshrivastava]

Can you please specify what exactly I need to do.

[rnveach]

Starting with bb1db49 , CLASS_DEF was added as a default token. Before this change, default and required tokens had the exact same list making it seem like all tokens should be required for the check to function. CLASS_DEF was added to identify if the file being examined defined a class or if the class was imported without an import statement, like java.lang. IE: Ban java.lang.Boolean but allow com.myspace.Class.Boolean.

Also to note in b0c3377 , a comment existed that specifically said "to not allow user to change configuration" for acceptable tokens. Meaning this didn't look to be intended that any customization of tokens was the design at the time. It was changed in 69e5f93 .

Tokens 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 (Example below). This bypasses any checks and uses this instead of the default. Required tokens are always added by default anyways. Empty token property is always ignored. The required tokens for this check are IMPORT, LITERAL_NEW, PACKAGE_DEF.

The CLASS_DEF token in this check ensures we are capturing class names to do the extra validation mention in the 1st paragraph. Without it, class names are ignored, basically undoing the specified commit, and sees java.lang.Boolean and com.myspace.Class.Boolean as the same.

Example.
Current Behavior:

$ cat TestClass.java
public class MyTest {
  public class Boolean {
    boolean a;

    public Boolean (boolean a) { this.a = a; }
  }

  public void myTest (boolean a, int b) {
    Boolean c = new Boolean(a); // OK
    java.lang.Boolean d = new java.lang.Boolean(a); // violation, instantiation of
                                                    // java.lang.Boolean should be avoided

    Integer e = new Integer(b); // violation, instantiation of
                                // java.lang.Integer should be avoided
    Integer f = Integer.valueOf(b); // OK
  }
}

$ 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="IllegalInstantiation">
  <property name="classes" value="java.lang.Boolean" />
</module>
    </module>
</module>

$ java -jar checkstyle-10.3.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:10:27: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 1 errors.

Behavior with CLASS_DEF removed:

$ cat TestClass.java
public class MyTest {
  public class Boolean {
    boolean a;

    public Boolean (boolean a) { this.a = a; }
  }

  public void myTest (boolean a, int b) {
    Boolean c = new Boolean(a); // OK
    java.lang.Boolean d = new java.lang.Boolean(a); // violation, instantiation of
                                                    // java.lang.Boolean should be avoided

    Integer e = new Integer(b); // violation, instantiation of
                                // java.lang.Integer should be avoided
    Integer f = Integer.valueOf(b); // OK
  }
}

$ 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="IllegalInstantiation">
  <property name="classes" value="java.lang.Boolean" />
  <property name="tokens" value="IMPORT, LITERAL_NEW, PACKAGE_DEF" />
</module>
    </module>
</module>

$ java -jar checkstyle-10.3.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:9:17: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
[ERROR] TestClass.java:10:27: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 2 errors.

Notice the validation of the file with the CLASS_DEF removed has an extra violation.

Edit:
An example was added in documentation updates at some point. https://checkstyle.org/config_coding.html#IllegalInstantiation

To configure the check to allow violations for local classes vs classes defined in the check, for example java.lang.Boolean, property tokens must be defined to not mention CLASS_DEF, so its value should be LITERAL_NEW, PACKAGE_DEF, IMPORT:

Discussion started at #9181 (comment)

[rnveach]

IMO, this is a bug and CLASS_DEF should be added to the required list. This would mean this check has no customizable properties.

I did want to also mention that JavadocVariable, and ImportOrder are similar looking checks that will need to be examined as well.

[nrmancuso]

If all approve at #12108, we need to update issue description for release notes, it does not accurately describe changes there.

[romani]

This is not breaking compatibility as all previous variations of config continue to work as before. Behavior changed a bit.

[romani]

Fix is merged