Issue checkstyle#4044: Fix false negative in ALONE while checking sin…

…gle line if blocks
subkrish · Mar 20, 2017 · e763212 · e763212
1 parent acca138
commit e763212
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 5 deletions.
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyCheck.java
@@ -189,16 +189,14 @@ public void visitToken(DetailAST ast) {
      */
     private String validate(Details details) {
         final DetailAST rcurly = details.rcurly;
-        final DetailAST lcurly = details.lcurly;
         final DetailAST nextToken = details.nextToken;
         final boolean shouldCheckLastRcurly = details.shouldCheckLastRcurly;
         String violation = "";
-        if (option == RightCurlyOption.SAME
-                && !hasLineBreakBefore(rcurly)
-                && lcurly.getLineNo() != rcurly.getLineNo()) {
+        if (shouldHaveLineBreakBefore(option, details)) {
             violation = MSG_KEY_LINE_BREAK_BEFORE;
         }
-        else if (shouldCheckLastRcurly) {
+        else if (shouldCheckLastRcurly
+                 && option != RightCurlyOption.ALONE) {
             if (rcurly.getLineNo() == nextToken.getLineNo()) {
                 violation = MSG_KEY_LINE_ALONE;
             }
@@ -218,6 +216,19 @@ else if (shouldStartLine) {
         return violation;
     }
 
+    /**
+     * Checks whether a right curly should have a line break before.
+     * @param bracePolicy option for placing the right curly brace.
+     * @param details details for validation.
+     * @return true if a right curly should have a line break before.
+     */
+    private static boolean shouldHaveLineBreakBefore(RightCurlyOption bracePolicy,
+                                                     Details details) {
+        return bracePolicy == RightCurlyOption.SAME
+                && !hasLineBreakBefore(details.rcurly)
+                && details.lcurly.getLineNo() != details.rcurly.getLineNo();
+    }
+
     /**
      * Checks that a right curly should be on the same line as the next statement.
      * @param bracePolicy option for placing the right curly brace

diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyCheckTest.java
@@ -99,6 +99,7 @@ public void testAlone() throws Exception {
         checkConfig.addAttribute("option", RightCurlyOption.ALONE.toString());
         final String[] expected = {
             "93:27: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 27),
+            "97:72: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 72),
         };
         verify(checkConfig, getPath("InputLeftCurlyOther.java"), expected);
     }
@@ -124,6 +125,7 @@ public void testShouldStartLine() throws Exception {
         checkConfig.addAttribute("shouldStartLine", "false");
         final String[] expected = {
             "93:27: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 27),
+            "97:72: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 72),
         };
         verify(checkConfig, getPath("InputLeftCurlyOther.java"), expected);
     }
@@ -134,6 +136,7 @@ public void testMethodCtorNamedClassClosingBrace() throws Exception {
         checkConfig.addAttribute("shouldStartLine", "false");
         final String[] expected = {
             "93:27: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 27),
+            "97:72: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 72),
         };
         verify(checkConfig, getPath("InputLeftCurlyOther.java"), expected);
     }
@@ -203,6 +206,7 @@ public void testWithAnnotations() throws Exception {
             "127:77: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 77),
             "136:9: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 9),
             "138:9: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 9),
+            "138:33: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 33),
             "148:9: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 9),
             "150:75: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 75),
             "151:77: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 77),
@@ -322,6 +326,9 @@ public void testTryWithResourceAlone() throws Exception {
         final String[] expected = {
             "19:9: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 9),
             "24:67: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 67),
+            "25:35: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 35),
+            "27:92: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 92),
+            "33:67: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 67),
         };
         verify(checkConfig, getPath("InputRightCurlyTryResource.java"), expected);
     }
@@ -334,4 +341,15 @@ public void testTryWithResourceAloneSingle() throws Exception {
         };
         verify(checkConfig, getPath("InputRightCurlyTryResource.java"), expected);
     }
+
+    @Test
+    public void testBracePolicyAloneAndSinglelineIfBlocks() throws Exception {
+        checkConfig.addAttribute("option", RightCurlyOption.ALONE.toString());
+        final String[] expected = {
+            "5:32: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 32),
+            "7:45: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 45),
+            "7:47: " + getCheckMessage(MSG_KEY_LINE_ALONE, "}", 47),
+        };
+        verify(checkConfig, getPath("InputRightCurlySingelineIfBlocks.java"), expected);
+    }
 }
diff --git a/...urces/com/puppycrawl/tools/checkstyle/checks/blocks/InputRightCurlySingelineIfBlocks.java b/...urces/com/puppycrawl/tools/checkstyle/checks/blocks/InputRightCurlySingelineIfBlocks.java
@@ -0,0 +1,9 @@
+package com.puppycrawl.tools.checkstyle.checks.blocks;
+
+public class InputRightCurlySingelineIfBlocks {
+    void foo1() {
+        if (true) { int a = 5; } // violation
+
+        if (true) { if (false) { int b = 6; } } // violation
+    }
+}