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

NeedBraces: False Positives for LITERAL_CASE and LITERAL_DEFAULT #4764

Closed
gkeighren opened this issue Jul 19, 2017 · 2 comments

Comments

Projects
None yet
3 participants
@gkeighren
Copy link

commented Jul 19, 2017

https://checkstyle.org/config_blocks.html#NeedBraces

/var/tmp $ javac NeedBracesSwitchTest.java

/var/tmp $ cat NeedBracesSwitchTest.java

public class NeedBracesSwitchTest {

  // NeedBraces should not raise any violations for this code
  // Violation raised by Checkstyle 6.4+

  // Specific cases tested:
  // Raises violation: 8.0, 7.8.2, 7.7, 7.6, 7.5.1, 6.19, 6.7, 6.4.1, 6.4
  // No violations: 5.9, 6.1, 6.2, 6.3

  public String aMethod(int val) {
      switch (val) {
              case 0: {
                      return "zero";
                  }
              case 1: {
                      return "one";
                  }
              default: {
                      return "non-binary";
                  }
          }
  }

}
/var/tmp $ cat config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd">
<module name="Checker">
    <module name="TreeWalker">
        <module name="NeedBraces">
            <property name="tokens" value="LITERAL_CASE, LITERAL_DEFAULT"/>
        </module>
    </module>
</module>
/var/tmp $ java -jar checkstyle-8.0-all.jar -c config.xml NeedBracesSwitchTest.java

Starting audit...
[ERROR] C:\Users\gavin.keighren\Desktop\NeedBracesSwitchTest.java:12: 'case' construct must use '{}'s. [NeedBraces]
[ERROR] C:\Users\gavin.keighren\Desktop\NeedBracesSwitchTest.java:15: 'case' construct must use '{}'s. [NeedBraces]
[ERROR] C:\Users\gavin.keighren\Desktop\NeedBracesSwitchTest.java:18: 'default' construct must use '{}'s. [NeedBraces]
Audit done.
Checkstyle ends with 3 errors.
/var/tmp $ java -jar checkstyle-6.4-all.jar -c config.xml NeedBracesSwitchTest.java
Starting audit...
C:\Users\gavin.keighren\Desktop\NeedBracesSwitchTest.java:12: 'case' construct must use '{}'s.
C:\Users\gavin.keighren\Desktop\NeedBracesSwitchTest.java:15: 'case' construct must use '{}'s.
C:\Users\gavin.keighren\Desktop\NeedBracesSwitchTest.java:18: 'default' construct must use '{}'s.
Audit done.
/var/tmp $ java -jar checkstyle-6.3-all.jar -c config.xml NeedBracesSwitchTest.java
Starting audit...
Audit done.

I expect no violations reported. Based on local testing, the incorrect violations started being reported with Checkstyle 6.4. Please see comment in java test file for list of locally tested versions and the results.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

the incorrect violations started being reported with Checkstyle 6.4

Though we don't specify when acceptable tokens were added in our documentation at http://checkstyle.sourceforge.net/config_blocks.html#NeedBraces , case and default were not added as acceptable tokens until abe2b20#diff-95edbb4da33f2c258c9d24deab6b19ec which was 6.4.
Before this commit, acceptable tokens was same as default tokens which didn't list case and default.
So 6.3 or prior was never intended to support case and default.

I'm not sure why there is no error specifying the tokens being used weren't acceptable when doing regression with 6.3, but it must be a bug of some kind in old code as there will be an error in 6.4 if it was not part of acceptable. It was probably all part of the same commit that fixed this.

@romani romani added the approved label Jul 19, 2017

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 11, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 11, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 12, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 13, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 14, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 15, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 15, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 15, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 17, 2019

strkkk added a commit to strkkk/checkstyle that referenced this issue Jun 18, 2019

romani added a commit that referenced this issue Jun 20, 2019

@romani romani added the bug label Jun 20, 2019

@romani romani added this to the 8.22 milestone Jun 20, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Fix is merged

@romani romani closed this Jun 20, 2019

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.