Add a check to ban use of specified methods and constructors

[raghavgautam]

/var/tmp $ javac -cp ~/.m2/repository/junit/junit/4.12/junit-4.12.jar BannedConstructor.java InputBanishedMethods.java

/var/tmp $ cat BannedConstructor.java InputBanishedMethods.java

package com.puppycrawl.tools.checkstyle.checks.misc.banishedmethods;

public class BannedConstructor {
  public BannedConstructor(String str) {

  }
}
////////////////////////////////////////////////////////////////////////////////
// Test case file for checkstyle.
// Created: 2017
////////////////////////////////////////////////////////////////////////////////
package com.puppycrawl.tools.checkstyle.checks.misc.banishedmethods;

import org.junit.Assert;

/**
 * Test case for detecting usage of banished methods & constructors.
 * @author Raghav Kumar Gautam
 **/
class InputBanishedMethods
{
    /**
     * no param constructor
     */
    InputBanishedMethods() {
        System.exit(1);
        BannedConstructor bannedConstructor = new BannedConstructor("oneArgument");
    }

    /**
     * non final param method
     */
    void method(String s) {
        Assert.assertTrue(1 != 2);
        Assert.assertTrue("Good assert with some reason.", true);
    }

---

With the about java file. I want to be able to run checkstyle and get error that:

1. System.exit() should not be called.
2. Assert.asserTrue with 1 argument should not be called.

I could not find a simple and robust way to do this. So, I have written a check that can do this. I want to contribute it back. Others have discussed this on stackoverflow as well:
https://stackoverflow.com/questions/8416870/checkstyle-rule-to-prevent-invocation-of-some-methods-and-constructors

A sample output for above example is:

/var/tmp $ cat config.xml

<module name="TreeWalker">
    <module name="MethodCount">
        <property name="maxTotal" value="1"/>
    </module>
    <module name="com.puppycrawl.tools.checkstyle.checks.BanishedMethodsCheck">
        <property name="file" value="src/test/resources/com/puppycrawl/tools/checkstyle/checks/misc/banishedmethods/banished_methods.xml"/>
    </module>
</module>

/var/tmp $ cat banished_methods.xml

/var/tmp $ java -jar checkstyle-X.XX-all.jar -c config.xml InputBanishedMethods.java

Starting audit...
[ERROR] /Users/raghav/play/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/misc/banishedmethods/InputBanishedMethods.java:19:20: Call to exit with 1 arguments is banished. [BanishedMethods]
[ERROR] /Users/raghav/play/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/misc/banishedmethods/InputBanishedMethods.java:20:47: Call to BannedConstructor with 1 arguments is banished. [BanishedMethods]
[ERROR] /Users/raghav/play/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/misc/banishedmethods/InputBanishedMethods.java:27:26: Call to assertTrue with 1 arguments is banished. [BanishedMethods]
Audit done.
Checkstyle ends with 3 errors.

[romani]

@raghavgautam , please share banished_methods.xml .

This Check will not be robust, and most likely I would suggest you to contribute it to our experimental Checks project - https://github.com/sevntu-checkstyle/sevntu.checkstyle

[raghavgautam]

Here is the banished_methods.xml.

<?xml version="1.0"?>

<!DOCTYPE BanishedMethods SYSTEM "../../../../../../../../../../src/main/resources/com/puppycrawl/tools/checkstyle/banished_methods_1_0.dtd">

<BanishedMethods>
    <BanishedMethod methodName="System\.exit" argCount=".*"/>
    <BanishedMethod methodName=".*assertTrue" argCount="1"/>
    <BanishedMethod methodName="BannedConstructor" argCount="1"/>
</BanishedMethods>

And banished_methods dtd:

<?xml version="1.0" encoding="UTF-8"?>

<!-- Add the following to any file that is to be validated against this DTD:

<!DOCTYPE BanishedMethods PUBLIC
    "-//Puppy Crawl//DTD Banished Methods 1.0//EN"
    "http://checkstyle.sourceforge.net/dtds/banished_methods_1_0.dtd">
-->

<!ELEMENT BanishedMethods (BanishedMethod*)>

<!ELEMENT BanishedMethod EMPTY>
<!ATTLIST BanishedMethod methodName CDATA #REQUIRED
                         argCount CDATA #REQUIRED>

[raghavgautam]

@romani Do you have any input on the feature ?

I have a question. I see that the project has moved away from sourceforge. What would be the url for the banished_methods_1_0.dtd ?

[rnveach]

What would be the url for the banished_methods_1_0.dtd ?

We still use the sourceforge URL.
https://github.com/checkstyle/checkstyle/blob/master/config/checkstyle_checks.xml#L4

The only one we don't use anymore is http://www.puppycrawl.com.

Do you have any input on the feature ?

My comments are,

<property name="file" value="banished_methods.xml"

I don't think we should rely on an external XML file.
Most people like everything embedded into the configuration file. We have one open issue to embed the suppressions into the configuration without the need for an external file.

methodName="System.exit"

Technically System isn't the method, and how will you know System is the class instead of a variable? There is no restriction that you can't name your variables System. If I make the banished method exit or System will it still pick up System.exit?

Unless you make your check complex and follow types defined in the file, which still leaves out types from external classes, you may have to rely mostly on the method name alone.

[romani]

@raghavgautam , Check that you propose is very unstable and uncertain and imply a lot of false positives.
It is partially covered by http://checkstyle.sourceforge.net/config_regexp.html#RegexpSinglelineJava .
Your idea of catching forbidden methods is ok, but we will not host it in main project at least for now.

We could host it in https://github.com/sevntu-checkstyle/sevntu.checkstyle/tree/master/sevntu-checks but have to be changed to avoid external config file, and pass code review process and stress testing, ... . We will guide on the process.

If no good ideas come up during 2 weeks, I will close this issue.

[raghavgautam]

@rnveach Thanks for the feedback !!

• I can embed the banished_methods.xml file.
• I could not find a way to ensure that System is a class and not a variable. Also, since you can have a nested blocks and fields in the code, I don't think that we can do this without tracking all the variable that are currently in scope. Any pointers on how to do this will be very helpful.
• The matching of both method as well as argument count is based on regex. So, the user has flexibility to cover common variants by using wildcards or be more specific as per his needs.
  • If he wants to ban only System.exit then he can say ^System\.exit$
  • If he wants to ban all the exit methods he can say \.exit$

[raghavgautam]

@romani Thanks for the feedback!!
The problem with RegexpSingleLine is that it will not cover following cases

1. anonymous class
2. multi-line scenario
3. it's too coarse grained for scenarios like assertTrue. For assertTrue it's recommended to use 2 arg variant instead of 1 argument variant. With a line RegexpSingleLine it's hard ensure this.

Thanks for suggesting https://github.com/sevntu-checkstyle/sevntu.checkstyle/tree/master/sevntu-checks, I am looking at it's code.

[romani]

See you in sevntu project