Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #7643: Update doc for IllegalInstantiation #9181

Merged
merged 1 commit into from Feb 7, 2021
Merged

Issue #7643: Update doc for IllegalInstantiation #9181

merged 1 commit into from Feb 7, 2021

Conversation

himanshurijal
Copy link

@himanshurijal himanshurijal commented Jan 13, 2021

Issue #7643:
I closed the previous PR #9154

Add code example

Screen Shot 2021-01-31 at 5 45 21 PM

Output of default example:

$ cat my_checks.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">
  <module name="TreeWalker">
      <module name="IllegalInstantiation">
      </module>
  </module>
</module>

$ cat MyTest.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); // OK

    Integer e = new Integer(b); // OK
    Integer f = Integer.valueOf(b); // OK
  }
}

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/default_properties/my_checks.xml IllegalInstantiation/default_properties/MyTest.java
Starting audit...
Audit done.

Screen Shot 2021-01-31 at 5 45 34 PM

Output of non-default example:

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

$ cat MyTest.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
  }
}

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/classes_Boolean,Integer,Double/my_checks.xml  IllegalInstantiation/classes_Boolean,Integer,Double/MyTest.java

Starting audit...
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/classes_Boolean,Integer,Double/MyTest.java:10:27: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/classes_Boolean,Integer,Double/MyTest.java:13:17: Instantiation of java.lang.Integer should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 2 errors.

Screen Shot 2021-01-31 at 5 45 46 PM

Output of non-default example:

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

$ cat MyTest.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); // violation, instantiation of
                                // java.lang.Boolean should be avoided
    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
  }
}

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/src/tokens/my_checks.xml  IllegalInstantiation/src/tokens/MyTest.java

Starting audit...
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:9:17: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:11:27: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:14:17: Instantiation of java.lang.Integer should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 3 errors.

Screen Shot 2021-01-31 at 5 45 55 PM

Output of non-default example:

$ cat my_checks.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">
  <module name="TreeWalker">
      <module name="IllegalInstantiation">
        <property name="classes" value="java.lang.Boolean[], Boolean[],
          java.lang.Integer[], Integer[]"/>
      </module>
  </module>
</module>

$ cat MyTest.java

public class MyTest {
  public void myTest () {
    Boolean[] newBoolArray = new Boolean[]{true,true,false}; // OK
    Integer[] newIntArray = new Integer[]{1,2,3}; // OK
  }
}

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/array_classes/my_checks.xml  IllegalInstantiation/array_classes/tokens/MyTest.java

Starting audit...
Audit done.

@himanshurijal
Copy link
Author

himanshurijal commented Jan 13, 2021

I closed the previous PR #9154 trying to fix the same issue due to problems with branch naming, which is fixed now in this PR.

Additional examples were also added at the request of reviewer @romani.

I did not add an example for property tokens in the documentation because it was not clear what extra validation the property provided (as compared to declaring it explicitly in the config file).

$ cat my_checks.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">
  <module name="TreeWalker">
      <module name="IllegalInstantiation">
          <property name="classes" value="java.lang.Boolean, java.lang.Integer,java.lang.Double" />
      </module>
  </module>
</module>

$ cat MyTest.java

 public class MyTest {
    public void myTest (boolean a, int b, double c) {
      Boolean d = new Boolean(a); 
      Integer e = new Integer(b); 
      Double f = new Double(c); 
    }
 }

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/tokens_null/my_checks.xml  IllegalInstantiation/tokens_null/MyTest.java

Starting audit...
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:3:19: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:4:19: Instantiation of java.lang.Integer should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:5:18: Instantiation of java.lang.Double should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 3 errors.

Upon testing property tokens with the value set to, IMPORT, LITERAL_NEW, PACKAGE_DEF, CLASS_DEF, "" (list of acceptable tokens for this check), all of them yielded the same result compared to excluding property tokens from the config file. (only one attached here for brevity):

$ cat my_checks.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">
  <module name="TreeWalker">
      <module name="IllegalInstantiation">
          <property name="classes" value="java.lang.Boolean, java.lang.Integer,java.lang.Double" />
          <property name = "tokens" value = "CLASS_DEF" />
      </module>
  </module>
</module>

$ cat MyTest.java

 public class MyTest {
    public void myTest (boolean a, int b, double c) {
      Boolean d = new Boolean(a); 
      Integer e = new Integer(b); 
      Double f = new Double(c); 
    }
 }

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/tokens_null/my_checks.xml  IllegalInstantiation/tokens_null/MyTest.java

Starting audit...
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:3:19: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:4:19: Instantiation of java.lang.Integer should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:5:18: Instantiation of java.lang.Double should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 3 errors.

Interestingly, the following configuration yields the result:

$ cat my_checks.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">
  <module name="TreeWalker">
      <module name="IllegalInstantiation">
          <property name="classes" value="java.lang.Boolean, java.lang.Integer,java.lang.Double" />
          <property name = "tokens" value = "CLASS_DEF" />
      </module>
  </module>
</module>

$ cat MyTest.java

public interface MyTest {
    static final Boolean d = new Boolean(true);
    static final Integer e = new Integer(true);
    static final Double f = new Double(true);
}

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/tokens_null/my_checks.xml  IllegalInstantiation/tokens_null/MyTest.java

Starting audit...
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:2:30: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:3:30: Instantiation of java.lang.Integer should be avoided. [IllegalInstantiation]
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/tokens_null/MyTest.java:4:29: Instantiation of java.lang.Double should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 3 errors.

I don't know if the check is also supposed to show errors for when the user uses an interface instead of a class. I was expecting using tokens with CLASS_DEF to only check for classes and not interfaces (parsed as token INTERFACE_DEF).

@nrmancuso
Copy link
Member

nrmancuso commented Jan 18, 2021

@himanshurijal I missed a lot of things in my original review; I apologize. I did not look deeply into the logic for the check and trusted existing documentation too much. You have done well to question the check implementation/ documentation above. After evaluating the code in the check, and reviewing your notes above, there are a few things that I notice:

Inclusion of the CLASS_DEF token
This token is included in the check so that classes defined in the check do not cause violation. We should make this obvious in the documentation. However, the CLASS_DEF token is not required; it would be good to have an example config with CLASS_DEF token and one without. In the code example for each, define a class, and show that the CLASS_DEF token is necessary if the user wants to avoid violations for classes defined in the same file. This is also a good way to check that this actually works as intended.

The table might be confusing, since it does not list the actual tokens that we are checking
IMPORT, LITERAL_NEW, and PACKAGE_DEF. I think it would be good to mention that this check looks at imports and the package definition to determine if an instantiation is of a class from another package (in the "Description"), but it would be misleading to include IMPORT and PACKAGE_DEF in the properties table, since that is implementation logic. I also believe it would be good to add the LITERAL_NEW token to the table, since that is the token we are actually checking.

This check should only show violations on the new operator, regardless if it occurs in an interface, class, etc. The other tokens help determine if there should be a violation on each instance of new.

@romani
Copy link
Member

romani commented Jan 19, 2021

I think it would be good to mention that this check looks at imports and the package definition to determine if an instantiation is of a class from another package

no need, such tokens are required, user can not modify them. It is internal details that we process such tokens. We never do this.

it would be good to have an example config with CLASS_DEF token and one without.

yes!

@himanshurijal
Copy link
Author

Thank you for the detailed explanation @nmancus1 . I now understand what the property tokens is trying to do.

However, before I add more examples I wanted to clarify a few things first.

it would be good to have an example config with CLASS_DEF token and one without

  1. How do you disable CLASS_DEF in tokens? Using the following configuration did not work as expected.
$ cat my_checks.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">
  <module name="TreeWalker">
      <module name="IllegalInstantiation">
          <property name="classes" value="java.lang.Boolean" />
          <property name = "tokens" value = "" />
      </module>
  </module>
</module>

$ cat MyTest.java

public class MyTest {

    public class Boolean {
        boolean a;

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

    public void myTest (boolean b) {
        Boolean c = new Boolean(b); // Expected violation here with tokens set to empty string
    }
}

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/src/tokens/my_checks.xml  IllegalInstantiation/src/tokens/MyTest.java
Starting audit...
Audit done.

Checkstyle ends without any errors. I am guessing using an empty string leads tokens to default to the list of acceptable tokens (CLASS_DEF included).
However, using tokens with any other values from the list of acceptable tokens, for example <property name = "tokens" value = "LITERAL_NEW" />, leads to a violation at the expected line.

  1. Does this check also consider classes defined in the same package? For example:
$ cat my_checks.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">
  <module name="TreeWalker">
      <module name="IllegalInstantiation">
          <property name="classes" value="java.lang.Boolean" />
          <property name = "tokens" value = "CLASS_DEF" />
      </module>
  </module>
</module>

$ cat Boolean.java  // inside same package as MyTest.java

package tokens;
public class Boolean {
    boolean b;

    public Boolean (boolean b) {
        this.b = b;
    }
    
    public void printLocalBool () { System.out.println("My Boolean!"); }
}

$ cat MyTest.java 

package tokens;
public class MyTest {
    public void myTest (boolean b) {
        Boolean c = new Boolean(b); // Expected no violation with tokens set to CLASS_DEF
        c.printLocalBool();
    }
}

$ java -jar checkstyle-8.39-all.jar -c IllegalInstantiation/src/tokens/my_checks.xml  IllegalInstantiation/src/tokens/MyTest.java
Starting audit...
[ERROR] /Users/himanshurijal/Desktop/CheckStyle/IllegalInstantiation/src/tokens/MyTest.java:4:21: Instantiation of java.lang.Boolean should be avoided. [IllegalInstantiation]
Audit done.
Checkstyle ends with 1 errors.

Sorry for the lengthy examples. But, I feel like they add more clarity to what I am trying to ask.

@nrmancuso
Copy link
Member

nrmancuso commented Jan 19, 2021

I am guessing using an empty string leads tokens to default to the list of acceptable tokens (CLASS_DEF included).

This is correct. After some more digging, I have found this to be true for all checks that are children of TreeWalker. See

final Set<String> checkTokens = check.getTokenNames();
if (checkTokens.isEmpty()) {
tokens = check.getDefaultTokens();
}
. FYI @romani

Using your above examples with contrasting configs: https://gist.github.com/nmancus1/7aa17723f1af911f8a7dec3ff39fde81

Does this check also consider classes defined in the same package...

Here is a good example of package checking:

https://gist.github.com/nmancus1/6c52ad7dbd504f99991a59af72566939

Sorry for the lengthy examples. But, I feel like they add more clarity to what I am trying to ask.

No need for apologies, your examples helped me to help you. Checkstyle has a lot of code, there is no way for anyone to know all of it. Open source is about working together :)

I would include some form of all above examples to help users to understand how exactly this check works. Show how package name is used, show how token config affects violations, etc. Any question that you have had while working on this is probably a question that users will have.

@himanshurijal
Copy link
Author

@nmancus1 I updated doc with new examples from our discussion. If you could review when you have time, that'd be great.

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please demonstrate package name checking (where check is aware of the package name and illegal class, so it causes violation since we are in that package) as in my second example (please just use another class though, not boolean). You can add it to one of the existing examples. One other item:

@romani romani requested review from romani and rnveach January 20, 2021 17:50
@romani romani self-assigned this Jan 20, 2021
@romani romani removed the request for review from rnveach January 20, 2021 17:50
@nrmancuso
Copy link
Member

Github, generate site

@romani
Copy link
Member

romani commented Jan 22, 2021

nice progress ....
@himanshurijal, unfortunately this Check is not obvious, so your work will be highly valuable for users.

unfortunately I found example hard to understand ... main reason is that example java code block is different all the time.
We need single code block that used for all configs, so user will easily see how Check behavior is changing by simply manipulation in config.
Because user do not want to change his code, he want to adjust Check to his preference and his expectations.

Please use:

public class MyTest {
  public class Boolean {
    boolean a;

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

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

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

it might be good to be for all configs.


from doc: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/7643-update-doc-IllegalInstantiationCheck_2021114644/config_coding.html#IllegalInstantiation

There is a limitation that it is currently not possible to specify array classes.

Please add case in example to make it visual or user.


To configure the check to avoid violations for local classes vs classes defined in the check, for example, java.lang.Boolean:

and

To configure the check to find instantiations of java.lang.Boolean, java.lang.Integer, or java.lang.Double

are almost identical by configuration, please merge it to To configure the check to find instantiations of java.lang.Boolean, java.lang.Integer, or java.lang.Double


<module name="IllegalInstantiation">
  <property name="classes" value="java.lang.Boolean"/>
  <property name="tokens" value="LITERAL_NEW, PACKAGE_DEF,
    IMPORT"/>
</module>

It is magic for user why "tokens" value="LITERAL_NEW, PACKAGE_DEF,IMPORT" is allowed.
and it is big conceptual problem in Checkstyle :(.
To resolve this confusion please use wording like:
To configure the check to allow violations for local classes vs classes defined in the check you need to define token value to not mention CLASS_DEF, so value should be IMPORT, LITERAL_NEW,PACKAGE_DEF (list of required tokens).

@himanshurijal
Copy link
Author

Thank you for reviewing @romani

Please use:

public class MyTest {
  public class Boolean {
    boolean a;

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

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

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

it might be good to be for all configs.

Just to be clear. Since you recommended me to use that example for all configs, should I remove examples of java.lang.Double in all configs? I wanted to confirm since in last review you wanted me to include examples for java.lang.Double.

@romani
Copy link
Member

romani commented Jan 22, 2021

in all configs?

yes , you can, it is not critical, all we need is to show how to define few items in property, so 2 is enough.

@himanshurijal
Copy link
Author

Understood. I'll make the changes and get back to you.

@romani
Copy link
Member

romani commented Jan 29, 2021

@himanshurijal , ping.

@romani romani removed their assignment Jan 29, 2021
@himanshurijal
Copy link
Author

I apologize for the delay. I was really busy with my coursework. I'll be pushing my changes today.

@himanshurijal
Copy link
Author

Some points to note:

please merge it to To configure the check to find instantiations of java.lang.Boolean, java.lang.Integer, or java.lang.Double

  1. The second example is the merged example. To avoid the case of misunderstanding where user might believe property tokens is also required in check configuration to find instantiations of specific java.lang classes, I added a NOTE clarifying why the property is used in the example but not necessary to produce the same results for violation.

  2. Also, I am not sure if my example for limitations on specifying array classes is correct (or totally understandable). Please let me know if I need to make changes @romani. Again, sorry for the delay.

@romani
Copy link
Member

romani commented Feb 7, 2021

Github, generate site

@romani romani merged commit adb2c92 into checkstyle:master Feb 7, 2021
@romani
Copy link
Member

romani commented Feb 7, 2021

@himanshurijal , thanks a lot for fix.
Looking forward for more contributions from you.

@himanshurijal
Copy link
Author

Absolutely! Thank you both for guiding me through the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants