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

IllegalType: rename "format" property to "illegalAbstractClassNameFormat" #5900

Closed
kevin-wayne opened this Issue Jun 10, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@kevin-wayne
Copy link

kevin-wayne commented Jun 10, 2018

/var/tmp $ javac TestIllegalType.java

/var/tmp $ cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
    "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
    "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="IllegalType">
            <property name="format" value="^java\.io\.File$"/>
        </module>
    </module>
</module>

/var/tmp $ cat TestIllegalType.java
import java.io.File;

public class TestIllegalType {
     File file1;
     java.io.File file2;
}

/var/tmp $ RUN_LOCALE="-Duser.language=en -Duser.country=US"
/var/tmp $ java $RUN_LOCALE -jar checkstyle-8.5-all.jar -c config.xml TestIllegalTypejava
Starting audit...
Audit done.

It should flag both occurrences of the type File (but flags neither).

I believe the error is in the isMatchingClassName() method in https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java. In particular, the format.matcher() expression should be OR'd with the first few expressions instead of AND'd with the last one.

    private boolean isMatchingClassName(String className) {
        final String shortName = className.substring(className.lastIndexOf('.') + 1);
        return illegalClassNames.contains(className)
                || illegalShortClassNames.contains(shortName)
                || validateAbstractClassNames
                    && !legalAbstractClassNames.contains(className)
                    && format.matcher(className).find();
    }

@romani romani added the approved label Jun 10, 2018

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Nov 4, 2018

Fixing this seems to present another issue that I'm not sure what to do with.

When code is changed to:

        return illegalClassNames.contains(className)
                || illegalShortClassNames.contains(shortName)
                || format.matcher(className).find()
                || validateAbstractClassNames
                    && !legalAbstractClassNames.contains(className);

It now seems when validateAbstractClassNames is turned on, it prints violations on all types as it doesn't check if the currently examined class name is an abstract class or not. For example I got a violation on void return type of a method. And even still, there are some times we can't know if it is abstract or not like if it is in another file.

I think format was supposed to be used to identify those abstract classes. @kevin-wayne should be using illegalClassNames to create violations on java.io.File.

@romani Please confirm.
I think we just need to update the documentation for format. It's probably too late to change the name of this property to avoid confusion.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Nov 4, 2018

Here is what I believe is the correct way to configure the check:

$ cat TestClass.java
import java.io.File;

public class TestIllegalType {
     File file1;
     java.io.File file2;
}

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

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

    <module name="TreeWalker">
        <module name="IllegalType">
            <property name="illegalClassNames" value="java.io.File"/>
        </module>
    </module>
</module>

$ java -jar checkstyle-8.14-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:4:6: Usage of type 'File' is not allowed. [IllegalType]
[ERROR] TestClass.java:5:6: Usage of type 'java.io.File' is not allowed. [IllegalType]
Audit done.
Checkstyle ends with 2 errors.
@kevin-wayne

This comment has been minimized.

Copy link
Author

kevin-wayne commented Nov 4, 2018

@rnveach Agree that illegalClassNames is perfectly fine for specifying a small number of classes (and that is how I use it in such cases). But how would one specify a large number of classes (e.g., all classes in an entire package) without format? Here are some real examples (where we use IllegalType to limit our student's use of Java to a restricted subset).

     <!-- all classes in these packages -->
     <module name="IllegalType">
         <property name="illegalClassNames" value=""/>
         <property name="format" value="^(java\.lang\.)(annotation|invoke|module|ref|reflect)\."/>

     <!-- all classes in java.awt except java.awt.Color —>
     <module name="IllegalType">
         <property name="illegalClassNames" value=""/>
         <property name="format" value="^java\.awt\.(?!Color$).*$"/>

     <!-- all classes in java.io that write output -->
     <module name="IllegalType">
         <property name="illegalClassNames" value=""/>
         <property name="format" value="^(java\.io\.)?(.*Writer|.*OutputStream)$"/>
@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Nov 4, 2018

how would one specify a large number of classes
all classes in these packages (java.lang....)
all classes in java.awt except java.awt.Color
all classes in java.io that write output

Excluding java.lang, you can use IllegalImport or ImportControl to forbid these packages as those checks do allow regular expressions for packages/classes. But it will only print violations if the class/package is imported. They won't work if the class is in the same package as that class.

Right now for IllegalType, you would have to specify all classes separated by commas like the default value shows at http://checkstyle.sourceforge.net/config_coding.html#IllegalType_Properties for illegalClassNames. It is a non-regular expression field unfortunately.

We would have to expand IllegalType to allow regular expressions in illegalClassNames to be able to do what you want. I'm not sure how easy this would be as we currently split illegalClassNames into full path and just class name for identification.
@romani What is your opinion on this expansion?

@kevin-wayne

This comment has been minimized.

Copy link
Author

kevin-wayne commented Nov 4, 2018

@rnveach Right, thanks. Yes, we do also use IllegalImport but have observed students bypassing that check by using a fully qualified name, e.g., java.io.FileWriter x = …. So, extending IllegalType to support regexp would still be a useful feature for us. But, I do understand now that format was designed with some other purpose in mind.

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 19, 2019

14 years ago format is used for matching to regular classes https://github.com/checkstyle/checkstyle/blame/78e9ce870170ca3ef88fd78f8e0af0681c958eb3/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java#L176 and looks like this Check did not like abstract classes by default https://github.com/checkstyle/checkstyle/blame/78e9ce870170ca3ef88fd78f8e0af0681c958eb3/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/IllegalTypeCheck.java#L47 .

In this commit, 13 years ago it was slightly changed in meaning ea4085c#diff-5c9bd2e5d93e3a00e5e1cb8623ef425bR184

So this property was misused for most amount of time, so it should keep this meaning, as regexp on name is the only way for Checkstyle to distinguish Abstract type vs other types.

We should definitely update documentation to explain role of property , in addition we should rename it to abstractTypeFormat.

@rnveach and @kevin-wayne , do you agree with my proposal ?

Extending to have regexp in list of classes, let's move to separate issue.

@kevin-wayne

This comment has been minimized.

Copy link
Author

kevin-wayne commented Feb 19, 2019

Thanks @romani . Renaming to abstractTypeFormat sounds reasonable to me.

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Feb 19, 2019

I am fine with renaming it abstract...Format

abstractTypeFormat

Why Type over Class? Only classes can be abstract.

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 20, 2019

Let's name it as abstractClassNameFormat.

@romani romani changed the title IllegalType ignores "format" option IllegalType: rename "format" property to "abstractClassNameFormat" Feb 20, 2019

@rnveach

This comment has been minimized.

Copy link
Member

rnveach commented Feb 20, 2019

@romani While working on this, I noticed we have another property named legalAbstractClassNames. It might not be clear from abstractClassNameFormat that this is only for illegal names. Maybe a better name would be illegalAbstractClassNameFormat? Otherwise it may not be clear how this rename is to be used differently then that other similarly named one.

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 21, 2019

Maybe a better name would be illegalAbstractClassNameFormat

Yes.

@romani romani changed the title IllegalType: rename "format" property to "abstractClassNameFormat" IllegalType: rename "format" property to "illegalAbstractClassNameFormat" Feb 21, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 21, 2019

romani added a commit that referenced this issue Feb 22, 2019

@romani romani added this to the 8.18 milestone Feb 22, 2019

@romani

This comment has been minimized.

Copy link
Member

romani commented Feb 22, 2019

Fix is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.