Skip to content

Commit

Permalink
Issue checkstyle#4764: correct NeedBracesCheck
Browse files Browse the repository at this point in the history
  • Loading branch information
@strkkk
strkkk committed Jun 11, 2019
1 parent 9b8dea4 commit 3975092
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 88 deletions.
140 changes: 57 additions & 83 deletions src/main/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheck.java
View file
Expand Up @@ -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());
}

/**
Expand All @@ -228,29 +243,13 @@ private boolean isSkipStatement(DetailAST statement) {
}

/**
* Checks if current loop statement does not have body, e.g.:
* <p>
* {@code
* while (value.incrementValue() < 5);
* ...
* for(int i = 0; i < 10; value.incrementValue());
* }
* </p>
* @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();
}

/**
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -399,58 +396,35 @@ 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.:
* <p>
* {@code
* case 1: doSomeStuff(); break;
* case 2: doSomeStuff(); break;
* case 3: ;
* default: doSomeStuff();break;
* }
* </p>
* @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;
}
else {
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.:
* <p>
* {@code
* default: doSomeStuff();
* }
* </p>
* @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;
Expand All @@ -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);
}

}
32 changes: 27 additions & 5 deletions src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/NeedBracesCheckTest.java
View file
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 =
Expand All @@ -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);
Expand All @@ -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);
Expand Down
34 changes: 34 additions & 0 deletions .../com/puppycrawl/tools/checkstyle/checks/blocks/needbraces/InputNeedBracesCaseDefault.java
View file
@@ -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";
}
}
}
}

0 comments on commit 3975092

Please sign in to comment.