EmptyBlock: can't get violation from case token #3839

Closed
rnveach opened this Issue Feb 17, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Feb 17, 2017

Taken from #3748 (comment)

$ cat TestClass.java
public class TestClass {
    void method(int a) {
                switch (a) {}
                switch (a) {case 1:}
                switch (a) {case 1:{}}
                switch (a) {
                    case 1:
                }
                switch (a) {
                    case 1:
                    {}
                }
                switch (a) {
                    case 1:
{
}
                }
    }
}

$ 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="EmptyBlock">
            <property name="tokens" value="LITERAL_CASE"/>
        </module>
    </module>
</module>

$ java -jar checkstyle-7.5.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.

http://checkstyle.sourceforge.net/config_blocks.html#EmptyBlock
LITERAL_CASE is an allowed token, but I can NOT produce a violation for it.

Our tests for the check makes no reference to cases. https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/EmptyBlockCheckTest.java
Even though blocks are mostly considered curlies, it makes sense that we treat case and default as blocks since the case is grouping code under it.
I assume a case that just contains break should still be considered empty.

We need to find the issue with the check and expand all our tests to every acceptable token for the check to verify we have no issues with any other tokens.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 17, 2017

Member

I assume a case that just contains break should still be considered empty.

no , it is not a empty block. The same as catch (exception ex) { throw ex; } - it is not empty catch.

Member

romani commented Feb 17, 2017

I assume a case that just contains break should still be considered empty.

no , it is not a empty block. The same as catch (exception ex) { throw ex; } - it is not empty catch.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 17, 2017

Member

no , it is not a empty block. The same as catch (exception ex) { throw ex; } - it is not empty catch.

I was thinking along the lines of auto-generated code from IDEs. Generated catch usually isn't throw, but generated case are given a break automatically (Eclipse). If there is no break, it falls through to the next and so is similar to not having an end curly for catch.

Should we change the token from LITERAL_CASE to CASE_GROUP then? 2+ cases right after each other shouldn't be considered empty, right? Just like we don't consider them for FallThroughCheck.

Examples if we are only talking cases:

switch(a1) { case 0: } // 1 violation
switch(a2) { case 0: break;} // no violations
switch(a3) { case 0: case 1: } // 1 violation
switch(a4) { case 0: case 1: break;} // no violation
switch(a5) { case 0: case 1: default: } // 1 violation???
switch(a6) { case 0: default: case 1: } // 1 violation???
switch(a7) { case 0: case 1: test(); } // no violations
Member

rnveach commented Feb 17, 2017

no , it is not a empty block. The same as catch (exception ex) { throw ex; } - it is not empty catch.

I was thinking along the lines of auto-generated code from IDEs. Generated catch usually isn't throw, but generated case are given a break automatically (Eclipse). If there is no break, it falls through to the next and so is similar to not having an end curly for catch.

Should we change the token from LITERAL_CASE to CASE_GROUP then? 2+ cases right after each other shouldn't be considered empty, right? Just like we don't consider them for FallThroughCheck.

Examples if we are only talking cases:

switch(a1) { case 0: } // 1 violation
switch(a2) { case 0: break;} // no violations
switch(a3) { case 0: case 1: } // 1 violation
switch(a4) { case 0: case 1: break;} // no violation
switch(a5) { case 0: case 1: default: } // 1 violation???
switch(a6) { case 0: default: case 1: } // 1 violation???
switch(a7) { case 0: case 1: test(); } // no violations
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 17, 2017

Member

Should we change the token from LITERAL_CASE to CASE_GROUP then?

http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#CASE_GROUP
I do not think so, I do not see a reason.

switch(a3) { case 0: case 1: } // 1 violation
switch(a5) { case 0: case 1: default: } // 1 violation???
switch(a6) { case 0: default: case 1: } // 1 violation???

There are no blocks.
There should be no violations, it is not this Check business and validation points.
Check should stay on validation of Blocks.

Member

romani commented Feb 17, 2017

Should we change the token from LITERAL_CASE to CASE_GROUP then?

http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#CASE_GROUP
I do not think so, I do not see a reason.

switch(a3) { case 0: case 1: } // 1 violation
switch(a5) { case 0: case 1: default: } // 1 violation???
switch(a6) { case 0: default: case 1: } // 1 violation???

There are no blocks.
There should be no violations, it is not this Check business and validation points.
Check should stay on validation of Blocks.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 17, 2017

Member

I was under the impression we would handle the entire case as if it was a block, not require users to actually add the blocks.

@romani Are these examples correct then? If not, could you show some?

switch(a1) { case 0: } // no violation
switch(a2) { case 0: {} } // 1 violation
switch(a3) { case 0: test(); {} } // no violation?
switch(a4) { case 0: case 1: {} } // 1 violation
switch(a5) { case 0: {} case 1: {} } // 2 violations
Member

rnveach commented Feb 17, 2017

I was under the impression we would handle the entire case as if it was a block, not require users to actually add the blocks.

@romani Are these examples correct then? If not, could you show some?

switch(a1) { case 0: } // no violation
switch(a2) { case 0: {} } // 1 violation
switch(a3) { case 0: test(); {} } // no violation?
switch(a4) { case 0: case 1: {} } // 1 violation
switch(a5) { case 0: {} case 1: {} } // 2 violations
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 18, 2017

Member

This behavior will be valid as for me.

Member

romani commented Feb 18, 2017

This behavior will be valid as for me.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Mar 5, 2017

Collaborator

@romani @rnveach

switch(a3) { case 0: test(); {} } // no violation

Why are we not to have a violation here ? There's an empty block in case ! Wouldn't it be better to acknowledge every empty block ?

edit: ok it makes sense if we are just considering if case is empty or not :)

Collaborator

ps-sp commented Mar 5, 2017

@romani @rnveach

switch(a3) { case 0: test(); {} } // no violation

Why are we not to have a violation here ? There's an empty block in case ! Wouldn't it be better to acknowledge every empty block ?

edit: ok it makes sense if we are just considering if case is empty or not :)

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 6, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 7, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 7, 2017

Member

Why are we not to have a violation here ? There's an empty block in case ! Wouldn't it be better to acknowledge every empty block ?

Check is doing search of empty blocks that belong to Token, in this case to CASE.
test(); {} is just a general empty block that you can put any place in code not only a CASE.
This Check care only about blocks that are scope of CASE.

Member

romani commented Mar 7, 2017

Why are we not to have a violation here ? There's an empty block in case ! Wouldn't it be better to acknowledge every empty block ?

Check is doing search of empty blocks that belong to Token, in this case to CASE.
test(); {} is just a general empty block that you can put any place in code not only a CASE.
This Check care only about blocks that are scope of CASE.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 7, 2017

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Mar 7, 2017

Collaborator

@romani ! Duly acknowledged. Thanks.

Collaborator

ps-sp commented Mar 7, 2017

@romani ! Duly acknowledged. Thanks.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 8, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 11, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 19, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 22, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 23, 2017

Issue #3839: One new method and a few other changes to help detect em…
…pty LITERAL_CASE. Added corresponding UTs. Another new method for design optimisation

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 23, 2017

Issue #3839: One new method to optimise design and detect empty LITER…
…AL_CASE. Added corresponding UTs. Updated xdoc

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 23, 2017

Issue #3839: One new method to optimise design and detect empty LITER…
…AL_CASE. Added corresponding UTs. Updated xdoc

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 23, 2017

Issue #3839: One new method to optimise design and detect empty LITER…
…AL_CASE. Added corresponding UTs. Updated xdoc

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Mar 25, 2017

Issue #3839: One new method to optimise design and detect empty LITER…
…AL_CASE. Added corresponding UTs. Updated documentation

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 4, 2017

Issue #3839: One new method to optimise design and detect empty LITER…
…AL_CASE. Added corresponding UTs. Updated documentation

rnveach added a commit that referenced this issue Apr 5, 2017

Issue #3839: One new method to optimise design and detect empty LITER…
…AL_CASE. Added corresponding UTs. Updated documentation
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 5, 2017

Member

Fix is merged

Member

rnveach commented Apr 5, 2017

Fix is merged

@rnveach rnveach closed this Apr 5, 2017

@rnveach rnveach added this to the 7.7 milestone Apr 5, 2017

@romani romani added the medium label Apr 21, 2017

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