Skip to content

Commit

Permalink
Issue checkstyle#11220: False positive in MissingSwitchDefault with p…
Browse files Browse the repository at this point in the history
…attern in switch label
  • Loading branch information
nrmancuso committed Jan 27, 2022
1 parent 1d51d50 commit 2e8759e
Show file tree
Hide file tree
Showing 6 changed files with 336 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.utils.TokenUtil;

/**
* <p>
Expand All @@ -35,10 +36,23 @@
* cases are covered, this should be expressed in the
* default branch, e.g. by using an assertion. This way
* the code is protected against later changes, e.g.
* introduction of new types in an enumeration type. Note that
* the compiler requires switch expressions to be exhaustive,
* so this check does not enforce default branches on
* such expressions.
* introduction of new types in an enumeration type.
* </p>
* <p>
* This check does not validate any switch expressions. Rationale:
* The compiler requires switch expressions to be exhaustive. This means
* that all possible inputs must be covered.
* </p>
* <p>
* This check does not validate switch statements that use pattern or null
* labels. Rationale: Switch statements that use pattern or null labels are
* checked by the compiler for exhaustiveness. This means that all possible
* inputs must be covered.
* </p>
* <p>
* See the <a href="https://docs.oracle.com/javase/specs/jls/se17/html/jls-15.html#jls-15.28">
* Java Language Specification</a> for more information about switch statements
* and expressions.
* </p>
* <p>
* To configure the check:
Expand All @@ -65,15 +79,81 @@
* default: // OK
* break;
* }
* return switch (option) { // OK, the compiler requires switch expression to be exhaustive
* case ONE:
* yield 1;
* case TWO:
* yield 2;
* case THREE:
* yield 3;
* switch (o) {
* case String s: // type pattern
* System.out.println(s);
* break;
* case Integer i: // type pattern
* System.out.println("Integer");
* break;
* default: // will not compile without default label, thanks to type pattern label usage
* break;
* }
* </pre>
* <p>Example of correct code which does not require default labels:</p>
* <pre>
* sealed interface S permits A, B, C {}
* final class A implements S {}
* final class B implements S {}
* record C(int i) implements S {} // Implicitly final
*
* /**
* * The completeness of a switch statement can be
* * determined by the contents of the permits clause
* * of interface S. No default label or default case
* * label is allowed by the compiler in this situation, so
* * this check does not enforce a default label in such
* * statements.
* *&#47;
* static void showSealedCompleteness(S s) {
* switch (s) {
* case A a: System.out.println("A"); break;
* case B b: System.out.println("B"); break;
* case C c: System.out.println("C"); break;
* }
* }
*
* /**
* * A total type pattern matches all possible inputs,
* * including null. A default label or
* * default case is not allowed by the compiler in this
* * situation. Accordingly, check does not enforce a
* * default label in this case.
* *&#47;
* static void showTotality(String s) {
* switch (s) {
* case Object o: // total type pattern
* System.out.println("o!");
* }
* }
*
* enum Color { RED, GREEN, BLUE }
*
* static int showSwitchExpressionExhaustiveness(Color color) {
* switch (color) {
* case RED: System.out.println("RED"); break;
* case BLUE: System.out.println("BLUE"); break;
* case GREEN: System.out.println("GREEN"); break;
* // Check will require default label below, compiler
* // does not enforce a switch statement with constants
* // to be complete.
* default: System.out.println("Something else");
* }
*
* // Check will not require default label in switch
* // expression below, because code will not compile
* // if all possible values are not handled. If the
* // 'Color' enum is extended, code will fail to compile.
* return switch (color) {
* case RED:
* yield 1;
* case GREEN:
* yield 2;
* case BLUE:
* yield 3;
* };
* }
* </pre>
* <p>
* Parent is {@code com.puppycrawl.tools.checkstyle.TreeWalker}
* </p>
Expand Down Expand Up @@ -114,47 +194,52 @@ public int[] getRequiredTokens() {

@Override
public void visitToken(DetailAST ast) {
final DetailAST firstSwitchMemberAst = findFirstSwitchMember(ast);

if (!containsDefaultSwitch(firstSwitchMemberAst) && !isSwitchExpression(ast)) {
if (!containsDefaultLabel(ast)
&& !containsPatternCaseLabelElement(ast)
&& !containsDefaultCaseLabelElement(ast)
&& !isSwitchExpression(ast)) {
log(ast, MSG_KEY);
}
}

/**
* Checks if the case group or its sibling contain the 'default' switch.
*
* @param caseGroupAst first case group to check.
* @param detailAst first case group to check.
* @return true if 'default' switch found.
*/
private static boolean containsDefaultSwitch(DetailAST caseGroupAst) {
DetailAST nextAst = caseGroupAst;
boolean found = false;

while (nextAst != null) {
if (nextAst.findFirstToken(TokenTypes.LITERAL_DEFAULT) != null) {
found = true;
break;
}

nextAst = nextAst.getNextSibling();
}
private static boolean containsDefaultLabel(DetailAST detailAst) {
return TokenUtil.findFirstTokenByPredicate(detailAst,
ast -> ast.findFirstToken(TokenTypes.LITERAL_DEFAULT) != null
).isPresent();
}

return found;
/**
* Checks if a switch block contains a case label with a pattern variable definition.
* In this situation, the compiler enforces the given switch block to cover
* all possible inputs, and we do not need a default label.
*
* @param detailAst first case group to check.
* @return true if switch block contains a pattern case label element
*/
private static boolean containsPatternCaseLabelElement(DetailAST detailAst) {
return TokenUtil.findFirstTokenByPredicate(detailAst, ast -> {
return ast.getFirstChild() != null
&& ast.getFirstChild().findFirstToken(TokenTypes.PATTERN_VARIABLE_DEF) != null;
}).isPresent();
}

/**
* Returns first CASE_GROUP or SWITCH_RULE ast.
* Checks if a switch block contains a default case label.
*
* @param parent the switch statement we are checking
* @return ast of first switch member.
* @param detailAst first case group to check.
* @return true if switch block contains default case label
*/
private static DetailAST findFirstSwitchMember(DetailAST parent) {
DetailAST switchMember = parent.findFirstToken(TokenTypes.CASE_GROUP);
if (switchMember == null) {
switchMember = parent.findFirstToken(TokenTypes.SWITCH_RULE);
}
return switchMember;
private static boolean containsDefaultCaseLabelElement(DetailAST detailAst) {
return TokenUtil.findFirstTokenByPredicate(detailAst, ast -> {
return ast.getFirstChild() != null
&& ast.getFirstChild().findFirstToken(TokenTypes.LITERAL_DEFAULT) != null;
}).isPresent();
}

/**
Expand All @@ -167,5 +252,4 @@ private static boolean isSwitchExpression(DetailAST ast) {
return ast.getParent().getType() == TokenTypes.EXPR
|| ast.getParent().getParent().getType() == TokenTypes.EXPR;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,23 @@
cases are covered, this should be expressed in the
default branch, e.g. by using an assertion. This way
the code is protected against later changes, e.g.
introduction of new types in an enumeration type. Note that
the compiler requires switch expressions to be exhaustive,
so this check does not enforce default branches on
such expressions.
introduction of new types in an enumeration type.
&lt;/p&gt;
&lt;p&gt;
This check does not validate any switch expressions. Rationale:
The compiler requires switch expressions to be exhaustive. This means
that all possible inputs must be covered.
&lt;/p&gt;
&lt;p&gt;
This check does not validate switch statements that use pattern or null
labels. Rationale: Switch statements that use pattern or null labels are
checked by the compiler for exhaustiveness. This means that all possible
inputs must be covered.
&lt;/p&gt;
&lt;p&gt;
See the &lt;a href="https://docs.oracle.com/javase/specs/jls/se17/html/jls-15.html#jls-15.28"&gt;
Java Language Specification&lt;/a&gt; for more information about switch statements
and expressions.
&lt;/p&gt;</description>
<message-keys>
<message-key key="missing.switch.default"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public void testMissingSwitchDefault() throws Exception {
"23:9: " + getCheckMessage(MSG_KEY, "default"),
"35:17: " + getCheckMessage(MSG_KEY, "default"),
"46:17: " + getCheckMessage(MSG_KEY, "default"),
"53:9: " + getCheckMessage(MSG_KEY, "default"),
};
verifyWithInlineConfigParser(
getPath("InputMissingSwitchDefault.java"),
Expand Down Expand Up @@ -90,4 +91,12 @@ public void testMissingSwitchDefaultSwitchExpressionsThree() throws Exception {
expected);
}

@Test
public void testMissingSwitchDefaultCaseLabelElements() throws Exception {
final String[] expected = CommonUtil.EMPTY_STRING_ARRAY;
verifyWithInlineConfigParser(
getNonCompilablePath("InputMissingSwitchDefaultCaseLabelElements.java"),
expected);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
MissingSwitchDefault
*/

//non-compiled with javac: Compilable with Java17
package com.puppycrawl.tools.checkstyle.checks.coding.missingswitchdefault;

public class InputMissingSwitchDefaultCaseLabelElements {
static void m(Object o) {
switch (o) { // ok
case null, String s -> System.out.println("String, including null");
default -> System.out.println("something else");
}

switch (o) { // ok
case null, String s: System.out.println("String, including null"); break;
default: System.out.println("something else");
}

switch(o) {
case null: default: // ok
System.out.println("The rest (including null)");
}

switch(o) { // ok, pattern totality enforced by compiler
case Object o1:
System.out.println("object");
}

switch(o) { // ok, pattern totality enforced by compiler
case Object o1 && o1.toString().length() > 2:
System.out.println("object");
break;
case Object o1 && o1.toString().length() <= 2:
System.out.println("object");
break;
case null, Object o1:
break;
}

switch(o) { // ok
case String str, null ->
System.out.println("null");
default -> System.out.println("default");
}

switch(o) { // ok
case null, default ->
System.out.println("The rest (including null)");
}

switch(o) { // ok
case String s && s.length() > 2 ->
System.out.println("The string longer than 2 chars");
default -> System.out.println("default!");
}

switch(o) { // ok
case default, null ->
System.out.println("The rest (including null)");
}

switch(o) { // ok
case default, null:
System.out.println("The rest (including null)");
}

switch(o) { // ok
case null, default:
System.out.println("The rest (including null)");
}

switch(o) { // ok
case null, default:
throw new UnsupportedOperationException("not supported!");
}
}

void m2(String s) {
switch (s) { // ok
case "a": throw new AssertionError("Wrong branch.");
case default: break;
}
switch (s) { // ok
case "a" -> throw new AssertionError("Wrong branch.");
case default -> {}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,9 @@ public void nestedSwitch2() {
}
default: break;
}

switch(i) { // violation

}
}
}

0 comments on commit 2e8759e

Please sign in to comment.