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 does not seem to handle multidimensional array types #4425

Closed
kevin-wayne opened this Issue Jun 6, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@kevin-wayne

kevin-wayne commented Jun 6, 2017

/var/tmp $ javac TestClass.java
[no output]

/var/tmp $ cat TestClass.java 
public class TestClass {
    private Boolean x;
    private Boolean[] x1;
    private Boolean[][] x2; // NO violation
}

/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="IllegalTypeCheck">
            <property name="illegalClassNames" value="Boolean, Boolean[], Boolean[][]"/>
        </module>
    </module>
</module>

/var/tmp $ java -jar checkstyle-7.8.1-all.jar -c config.xml TestClass.java 
Starting audit...
[ERROR] /var/tmp/TestClass.java:2:13: Declaring variables, return values or parameters of type 'Boolean' is not allowed. [IllegalType]
[ERROR] /var/tmp/TestClass.java:3:13: Declaring variables, return values or parameters of type 'Boolean' is not allowed. [IllegalType]
Audit done.
Checkstyle ends with 2 errors.


I was expecting it to flag the variable of type Boolean[][].
The code for IllegalTypeCheck.java doesn't seem to handle Multidimensional array types properly.


@Peartes

This comment has been minimized.

Show comment
Hide comment
@Peartes

Peartes Jun 6, 2017

why is that array type illegal :)

Peartes commented Jun 6, 2017

why is that array type illegal :)

@kevin-wayne

This comment has been minimized.

Show comment
Hide comment
@kevin-wayne

kevin-wayne Jun 6, 2017

@Peartes
The array type should be illegal because that's what I am trying to prohibit in the xml file. :)

While I can imagine uses cases for which type Boolean[][] is appropriate, my use case is a programming assignment in an introductory algorithms class where memory usage is important. Some of our novice Java programmers inadvertently use Boolean instead of boolean.

kevin-wayne commented Jun 6, 2017

@Peartes
The array type should be illegal because that's what I am trying to prohibit in the xml file. :)

While I can imagine uses cases for which type Boolean[][] is appropriate, my use case is a programming assignment in an introductory algorithms class where memory usage is important. Some of our novice Java programmers inadvertently use Boolean instead of boolean.

@romani romani changed the title from IllegalType does not seem to handle array types to IllegalType does not seem to handle multidimensional array types Jun 7, 2017

@kevin-wayne

This comment has been minimized.

Show comment
Hide comment
@kevin-wayne

kevin-wayne Jun 7, 2017

@romani Thanks for the quick follow up.

I believe IllegalType doesn't really handle 1D array types either. The Boolean[] x1 declaration is actually flagged by the "Boolean" part of illegalClassNames (not by the "Boolean[]" part). Another use case for me is to flag the type int[] (but not int). So, it's not desirable to have int[] flagged by the illegalClassName "int".

kevin-wayne commented Jun 7, 2017

@romani Thanks for the quick follow up.

I believe IllegalType doesn't really handle 1D array types either. The Boolean[] x1 declaration is actually flagged by the "Boolean" part of illegalClassNames (not by the "Boolean[]" part). Another use case for me is to flag the type int[] (but not int). So, it's not desirable to have int[] flagged by the illegalClassName "int".

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 7, 2017

Member

@kevin-wayne ,

IllegalTypeCheck now do validation only on very basic name of type, so Integer and Integer[] are the same.

your second issue is moved out to separate issue - #4429 , as it is new feature request , a bit more complicated to do and test.

You are welcome to provide PRs, I could guide on where to fix, logic is simple.

Member

romani commented Jun 7, 2017

@kevin-wayne ,

IllegalTypeCheck now do validation only on very basic name of type, so Integer and Integer[] are the same.

your second issue is moved out to separate issue - #4429 , as it is new feature request , a bit more complicated to do and test.

You are welcome to provide PRs, I could guide on where to fix, logic is simple.

@kevin-wayne

This comment has been minimized.

Show comment
Hide comment
@kevin-wayne

kevin-wayne Jun 8, 2017

Thanks, just uploaded a PR that, I think, addresses both #4425 and #4429. I don't think I followed the right protocol with the format of the PR; sorry about that.

kevin-wayne commented Jun 8, 2017

Thanks, just uploaded a PR that, I think, addresses both #4425 and #4429. I don't think I followed the right protocol with the format of the PR; sorry about that.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 8, 2017

Member

@kevin-wayne , thanks for desire to help, I highly recommend to do fix only for #4425 first.
#4429 is not approved yet, and it will demand bigger testing.

Member

romani commented Jun 8, 2017

@kevin-wayne , thanks for desire to help, I highly recommend to do fix only for #4425 first.
#4429 is not approved yet, and it will demand bigger testing.

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 5, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 5, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 5, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 6, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 6, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 8, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 12, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 12, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 12, 2017

sergileon added a commit to sergileon/checkstyle that referenced this issue Sep 12, 2017

sergileon added a commit to epam/checkstyle that referenced this issue Sep 14, 2017

sergileon added a commit to epam/checkstyle that referenced this issue Sep 18, 2017

sergileon added a commit to epam/checkstyle that referenced this issue Sep 18, 2017

sergileon added a commit to epam/checkstyle that referenced this issue Sep 19, 2017

sergileon added a commit to epam/checkstyle that referenced this issue Sep 19, 2017

@rnveach rnveach closed this in #5109 Sep 22, 2017

rnveach added a commit that referenced this issue Sep 22, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 22, 2017

Member

Fix was merged

Member

rnveach commented Sep 22, 2017

Fix was merged

@rnveach rnveach added this to the 8.3 milestone Sep 22, 2017

@rnveach rnveach added the bug label Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment