diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheck.java index 306ecc2ed4c4..00e1ca4eb641 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheck.java @@ -189,33 +189,48 @@ public int[] getRequiredTokens() { @Override public void visitToken(DetailAST ast) { - final DetailAST slistAST = ast.findFirstToken(TokenTypes.SLIST); - boolean isElseIf = false; - if (ast.getType() == TokenTypes.LITERAL_ELSE - && ast.findFirstToken(TokenTypes.LITERAL_IF) != null) { - isElseIf = true; + final int type = ast.getType(); + final boolean hasSlist = ast.findFirstToken(TokenTypes.SLIST) == null; + boolean hasViolation = hasSlist && !isSkipStatement(ast); + switch (type) { + case TokenTypes.LITERAL_FOR: + case TokenTypes.LITERAL_WHILE: + hasViolation = hasViolation && !isEmptyLoopBodyAllowed(ast); + break; + case TokenTypes.LITERAL_CASE: + case TokenTypes.LITERAL_DEFAULT: + hasViolation = hasViolation && hasUnbracedStatements(ast); + break; + case TokenTypes.LITERAL_ELSE: + hasViolation = hasViolation && ast.findFirstToken(TokenTypes.LITERAL_IF) == null; + break; + default: + break; } - final boolean isInAnnotationField = isInAnnotationField(ast); - final boolean skipStatement = isSkipStatement(ast); - final boolean skipEmptyLoopBody = allowEmptyLoopBody && isEmptyLoopBody(ast); - - if (slistAST == null && !isElseIf && !isInAnnotationField - && !skipStatement && !skipEmptyLoopBody) { + if (hasViolation) { log(ast.getLineNo(), MSG_KEY_NEED_BRACES, ast.getText()); } } /** - * Checks if ast is in an annotation field. - * @param ast ast to test. - * @return true if current ast is part of an annotation field. + * Checks if current loop has empty body and can be skipped by this check. + * @param ast for, while statements. + * @return true if current loop can be skipped by check. */ - private static boolean isInAnnotationField(DetailAST ast) { - boolean isDefaultInAnnotation = false; - if (ast.getParent().getType() == TokenTypes.ANNOTATION_FIELD_DEF) { - isDefaultInAnnotation = true; - } - return isDefaultInAnnotation; + private boolean isEmptyLoopBodyAllowed(DetailAST ast) { + return allowEmptyLoopBody && ast.findFirstToken(TokenTypes.EMPTY_STAT) != null; + } + + /** + * Checks if switch member (case, default statements) has statements without curly braces. + * @param ast case, default statements. + * @return true if switch member has unbraced statements, false otherwise. + */ + private static boolean hasUnbracedStatements(DetailAST ast) { + final DetailAST nextSibling = ast.getNextSibling(); + return nextSibling != null + && nextSibling.getType() == TokenTypes.SLIST + && !"{".equals(nextSibling.getFirstChild().getText()); } /** @@ -228,29 +243,13 @@ private boolean isSkipStatement(DetailAST statement) { } /** - * Checks if current loop statement does not have body, e.g.: - *

- * {@code - * while (value.incrementValue() < 5); - * ... - * for(int i = 0; i < 10; value.incrementValue()); - * } - *

- * @param ast ast token. - * @return true if current loop statement does not have body. + * Checks if two ast nodes are on the same line. + * @param first ast to check + * @param second ast to check + * @return true if elements on same line, false otherwise */ - private static boolean isEmptyLoopBody(DetailAST ast) { - boolean noBodyLoop = false; - - if (ast.getType() == TokenTypes.LITERAL_FOR - || ast.getType() == TokenTypes.LITERAL_WHILE) { - DetailAST currentToken = ast.getFirstChild(); - while (currentToken.getNextSibling() != null) { - currentToken = currentToken.getNextSibling(); - } - noBodyLoop = currentToken.getType() == TokenTypes.EMPTY_STAT; - } - return noBodyLoop; + private static boolean isOnSameLine(DetailAST first, DetailAST second) { + return first.getLineNo() == second.getLineNo(); } /** @@ -288,10 +287,8 @@ private static boolean isSingleLineStatement(DetailAST statement) { result = isSingleLineLambda(statement); break; case TokenTypes.LITERAL_CASE: - result = isSingleLineCase(statement); - break; case TokenTypes.LITERAL_DEFAULT: - result = isSingleLineDefault(statement); + result = isSingleLineSwitchMember(statement); break; default: result = isSingleLineElse(statement); @@ -315,7 +312,7 @@ private static boolean isSingleLineWhile(DetailAST literalWhile) { boolean result = false; if (literalWhile.getParent().getType() == TokenTypes.SLIST) { final DetailAST block = literalWhile.getLastChild().getPreviousSibling(); - result = literalWhile.getLineNo() == block.getLineNo(); + result = isOnSameLine(literalWhile, block); } return result; } @@ -334,7 +331,7 @@ private static boolean isSingleLineDoWhile(DetailAST literalDo) { boolean result = false; if (literalDo.getParent().getType() == TokenTypes.SLIST) { final DetailAST block = literalDo.getFirstChild(); - result = block.getLineNo() == literalDo.getLineNo(); + result = isOnSameLine(block, literalDo); } return result; } @@ -355,7 +352,7 @@ private static boolean isSingleLineFor(DetailAST literalFor) { result = true; } else if (literalFor.getParent().getType() == TokenTypes.SLIST) { - result = literalFor.getLineNo() == literalFor.getLastChild().getLineNo(); + result = isOnSameLine(literalFor, literalFor.getLastChild()); } return result; } @@ -382,7 +379,7 @@ private static boolean isSingleLineIf(DetailAST literalIf) { block = literalIfLastChild; } final DetailAST ifCondition = literalIf.findFirstToken(TokenTypes.EXPR); - result = ifCondition.getLineNo() == block.getLineNo(); + result = isOnSameLine(ifCondition, block); } return result; } @@ -399,24 +396,26 @@ private static boolean isSingleLineIf(DetailAST literalIf) { */ private static boolean isSingleLineLambda(DetailAST lambda) { final DetailAST block = lambda.getLastChild(); - return lambda.getLineNo() == block.getLineNo(); + return isOnSameLine(lambda, block); } /** - * Checks if current case statement is single-line statement, e.g.: + * Checks if switch member (case or default statement) is single-line statement, e.g.: *

* {@code * case 1: doSomeStuff(); break; * case 2: doSomeStuff(); break; * case 3: ; + * default: doSomeStuff();break; * } *

- * @param literalCase {@link TokenTypes#LITERAL_CASE case statement}. - * @return true if current case statement is single-line statement. + * @param ast {@link TokenTypes#LITERAL_CASE case statement} or + * {@link TokenTypes#LITERAL_DEFAULT default statement}. + * @return true if current switch member is single-line statement. */ - private static boolean isSingleLineCase(DetailAST literalCase) { + private static boolean isSingleLineSwitchMember(DetailAST ast) { boolean result = false; - final DetailAST slist = literalCase.getNextSibling(); + final DetailAST slist = ast.getNextSibling(); if (slist == null) { result = true; } @@ -424,33 +423,8 @@ private static boolean isSingleLineCase(DetailAST literalCase) { final DetailAST caseBreak = slist.findFirstToken(TokenTypes.LITERAL_BREAK); if (caseBreak != null) { final DetailAST block = slist.getFirstChild(); - final boolean atOneLine = literalCase.getLineNo() == block.getLineNo(); - result = atOneLine && block.getLineNo() == caseBreak.getLineNo(); - } - } - return result; - } - - /** - * Checks if current default statement is single-line statement, e.g.: - *

- * {@code - * default: doSomeStuff(); - * } - *

- * @param literalDefault {@link TokenTypes#LITERAL_DEFAULT default statement}. - * @return true if current default statement is single-line statement. - */ - private static boolean isSingleLineDefault(DetailAST literalDefault) { - boolean result = false; - final DetailAST slist = literalDefault.getNextSibling(); - if (slist == null) { - result = true; - } - else { - final DetailAST block = slist.getFirstChild(); - if (block != null && block.getType() != TokenTypes.SLIST) { - result = literalDefault.getLineNo() == block.getLineNo(); + final boolean atOneLine = isOnSameLine(ast, block); + result = atOneLine && isOnSameLine(block, caseBreak); } } return result; @@ -468,7 +442,7 @@ private static boolean isSingleLineDefault(DetailAST literalDefault) { */ private static boolean isSingleLineElse(DetailAST literalElse) { final DetailAST block = literalElse.getFirstChild(); - return literalElse.getLineNo() == block.getLineNo(); + return isOnSameLine(literalElse, block); } } diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheckTest.java index 1eb5e05abc5a..b8df4c3ae966 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheckTest.java @@ -93,7 +93,6 @@ public void testItWithAllowsOn() throws Exception { "106: " + getCheckMessage(MSG_KEY_NEED_BRACES, "do"), "107: " + getCheckMessage(MSG_KEY_NEED_BRACES, "if"), "108: " + getCheckMessage(MSG_KEY_NEED_BRACES, "for"), - "110: " + getCheckMessage(MSG_KEY_NEED_BRACES, "default"), }; verify(checkConfig, getPath("InputNeedBraces.java"), expected); } @@ -131,6 +130,19 @@ public void testSingleLineLambda() throws Exception { verify(checkConfig, getPath("InputNeedBracesSingleLineLambda.java"), expected); } + @Test + public void testDoNotAllowSingleLineLambda() throws Exception { + final DefaultConfiguration checkConfig = + createModuleConfig(NeedBracesCheck.class); + checkConfig.addAttribute("tokens", "LAMBDA"); + final String[] expected = { + "5: " + getCheckMessage(MSG_KEY_NEED_BRACES, "->"), + "6: " + getCheckMessage(MSG_KEY_NEED_BRACES, "->"), + "7: " + getCheckMessage(MSG_KEY_NEED_BRACES, "->"), + }; + verify(checkConfig, getPath("InputNeedBracesSingleLineLambda.java"), expected); + } + @Test public void testSingleLineCaseDefault() throws Exception { final DefaultConfiguration checkConfig = @@ -154,6 +166,20 @@ public void testSingleLineCaseDefault2() throws Exception { verify(checkConfig, getPath("InputNeedBracesEmptyDefault.java"), expected); } + @Test + public void testSingleLineCaseDefaultNoSingleLine() throws Exception { + final DefaultConfiguration checkConfig = + createModuleConfig(NeedBracesCheck.class); + checkConfig.addAttribute("tokens", "LITERAL_CASE, LITERAL_DEFAULT"); + final String[] expected = { + "9: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), + "10: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), + "13: " + getCheckMessage(MSG_KEY_NEED_BRACES, "default"), + "16: " + getCheckMessage(MSG_KEY_NEED_BRACES, "default"), + }; + verify(checkConfig, getPath("InputNeedBracesCaseDefault.java"), expected); + } + @Test public void testCycles() throws Exception { final DefaultConfiguration checkConfig = createModuleConfig(NeedBracesCheck.class); @@ -169,13 +195,9 @@ public void testConditions() throws Exception { checkConfig.addAttribute("tokens", "LITERAL_ELSE, LITERAL_CASE, LITERAL_DEFAULT"); checkConfig.addAttribute("allowSingleLineStatement", "true"); final String[] expected = { - "29: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), "35: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), - "36: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), - "38: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), "41: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), "44: " + getCheckMessage(MSG_KEY_NEED_BRACES, "case"), - "49: " + getCheckMessage(MSG_KEY_NEED_BRACES, "default"), "56: " + getCheckMessage(MSG_KEY_NEED_BRACES, "default"), }; verify(checkConfig, getPath("InputNeedBracesConditional.java"), expected); diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/checks/blocks/needbraces/InputNeedBracesCaseDefault.java b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/blocks/needbraces/InputNeedBracesCaseDefault.java new file mode 100644 index 000000000000..b2117867e6d9 --- /dev/null +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/checks/blocks/needbraces/InputNeedBracesCaseDefault.java @@ -0,0 +1,34 @@ +package com.puppycrawl.tools.checkstyle.checks.blocks.needbraces; + +public class InputNeedBracesCaseDefault { + + public String aMethod(int val) { + switch (val){ + default: + case 0: + case -1: break; + case -2: Math.random(); + } + switch (val){ + default: break; + } + switch (val){ + default: Math.random(); + } + switch (val){ + case 1: {} + default: + } + switch (val) { + case 0: { + return "zero"; + } + case 1: { + return "one"; + } + default: { + return "non-binary"; + } + } + } +}