diff --git a/check_api/src/main/java/com/google/errorprone/util/Reachability.java b/check_api/src/main/java/com/google/errorprone/util/Reachability.java index f48ad5abf27..b21a72c578d 100644 --- a/check_api/src/main/java/com/google/errorprone/util/Reachability.java +++ b/check_api/src/main/java/com/google/errorprone/util/Reachability.java @@ -27,6 +27,7 @@ import com.sun.source.tree.BlockTree; import com.sun.source.tree.BreakTree; import com.sun.source.tree.CaseTree; +import com.sun.source.tree.CaseTree.CaseKind; import com.sun.source.tree.CatchTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.ContinueTree; @@ -69,20 +70,26 @@ public static boolean canCompleteNormally(StatementTree statement) { } /** - * Returns true if the given case tree can complete normally, as defined by JLS 14.21. + * Returns true if the given case tree can fall through to the next case tree. * *

An exception is made for {@code System.exit}, which cannot complete normally in practice. */ - public static boolean canCompleteNormally(CaseTree caseTree) { - List statements = caseTree.getStatements(); - if (statements.isEmpty()) { - return true; - } - // We only care whether the last statement completes; javac would have already - // reported an error if that statement wasn't reachable, and the answer is - // independent of any preceding statements. - // TODO(cushon): This isn't really making an exception for System.exit in the prior statements. - return canCompleteNormally(getLast(statements)); + public static boolean canFallThrough(CaseTree caseTree) { + return switch (caseTree.getCaseKind()) { + case STATEMENT -> { + List statements = caseTree.getStatements(); + if (statements.isEmpty()) { + yield true; + } + // We only care whether the last statement completes; javac would have already + // reported an error if that statement wasn't reachable, and the answer is + // independent of any preceding statements. + // TODO(cushon): This isn't really making an exception for System.exit in the prior + // statements. + yield canCompleteNormally(getLast(statements)); + } + case RULE -> false; + }; } private static class CanCompleteNormallyVisitor extends SimpleTreeVisitor { @@ -203,14 +210,14 @@ public Boolean visitAssert(AssertTree tree, Void unused) { } /* - * A switch statement can complete normally iff at least one of the + * A non-arrow switch statement can complete normally iff at least one of the * following is true: * - * 1) The switch block is empty or contains only switch labels. - * 2) The last statement in the switch block can complete normally. - * 3) There is at least one switch label after the last switch block + * 1) The switch block does not contain a default label. + * 2) The switch block is empty or contains only switch labels. + * 3) The last statement in the switch block can complete normally. + * 4) There is at least one switch label after the last switch block * statement group. - * 4) The switch block does not contain a default label. * 5) There is a reachable break statement that exits the switch statement. * * A switch block is reachable iff its switch statement is reachable. @@ -226,10 +233,35 @@ public Boolean visitAssert(AssertTree tree, Void unused) { @Override public Boolean visitSwitch(SwitchTree tree, Void unused) { // (1) - if (tree.getCases().stream().allMatch(c -> c.getStatements().isEmpty())) { + if (tree.getCases().stream().noneMatch(c -> isSwitchDefault(c))) { return true; } + // A switch statement whose switch block consists of switch rules can complete normally iff at + // least one of the following is true: + // (1 above) The switch statement is not enhanced (ยง14.11.2) and its switch block does not + // contain a default label. + // (2) One of the switch rules introduces a switch rule expression (which is necessarily a + // statement expression). + // (3) One of the switch rules introduces a switch rule block that can complete normally. + // (4) One of the switch rules introduces a switch rule block that contains a reachable + // break statement which exits the switch statement. + if (tree.getCases().stream().anyMatch(c -> c.getCaseKind().equals(CaseKind.RULE))) { + // (2) and (3) + if (tree.getCases().stream().anyMatch(c -> scan(c.getBody()))) { + return true; + } + // (4) + if (breaks.contains(tree)) { + return true; + } + return false; + } + // Past this point, we know each case is a statement. // (2) + if (tree.getCases().stream().allMatch(c -> c.getStatements().isEmpty())) { + return true; + } + // (3) boolean lastCompletes = true; for (CaseTree c : tree.getCases()) { lastCompletes = scan(c.getStatements()); @@ -237,12 +269,8 @@ public Boolean visitSwitch(SwitchTree tree, Void unused) { if (lastCompletes) { return true; } - // (3) - if (getLast(tree.getCases()).getStatements().isEmpty()) { - return true; - } // (4) - if (tree.getCases().stream().noneMatch(c -> isSwitchDefault(c))) { + if (getLast(tree.getCases()).getStatements().isEmpty()) { return true; } // (5) diff --git a/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java b/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java index 3189312ab1a..5c8302159fb 100644 --- a/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java +++ b/check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java @@ -320,6 +320,41 @@ public static List parameters() { "System.err.println();", "// BUG: Diagnostic contains:", }, + { + "switch (42) {", // + " case 1 -> {}", + "}", + "// BUG: Diagnostic contains:", + }, + { + "switch (42) {", // + " case 1 -> throw new AssertionError();", + "}", + "// BUG: Diagnostic contains:", + }, + { + "switch (42) {", // + " case 1 -> throw new AssertionError();", + " case 2 -> throw new AssertionError();", + " default -> {}", + "}", + "// BUG: Diagnostic contains:", + }, + { + "switch (42) {", // + " case 1 -> throw new AssertionError();", + " case 2 -> throw new AssertionError();", + " default -> throw new AssertionError();", + "}", + }, + { + "switch (42) {", // + " case 1 -> { break; }", + " case 2 -> throw new AssertionError();", + " default -> throw new AssertionError();", + "}", + "// BUG: Diagnostic contains:", + }, }; return Arrays.stream(parameters).map(x -> new Object[] {x}).collect(toList()); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SwitchDefault.java b/core/src/main/java/com/google/errorprone/bugpatterns/SwitchDefault.java index a59e834257d..8fb22b8db2b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/SwitchDefault.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/SwitchDefault.java @@ -74,7 +74,7 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) { if (it.hasNext()) { // If there are trailing cases after the default statement group, move the default to the end. // Only emit a fix if the default doesn't fall through. - if (!Reachability.canCompleteNormally(getLast(defaultStatementGroup))) { + if (!Reachability.canFallThrough(getLast(defaultStatementGroup))) { int start = getStartPosition(defaultStatementGroup.get(0)); int end = state.getEndPosition(getLast(defaultStatementGroup)); String replacement; @@ -97,7 +97,7 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) { // If the last statement group falls out of the switch, add a `break;` before moving // the default to the end. CaseTree last = getLast(tree.getCases()); - if (isSwitchDefault(last) || Reachability.canCompleteNormally(last)) { + if (isSwitchDefault(last) || Reachability.canFallThrough(last)) { replacement = "break;\n" + replacement; } fix.replace(start, end, "").postfixWith(getLast(tree.getCases()), replacement); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/WrongOneof.java b/core/src/main/java/com/google/errorprone/bugpatterns/WrongOneof.java index 4df02a65efb..7af060d450e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/WrongOneof.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/WrongOneof.java @@ -17,7 +17,6 @@ package com.google.errorprone.bugpatterns; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static com.google.common.collect.Iterables.getLast; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.predicates.TypePredicates.isDescendantOf; @@ -26,7 +25,7 @@ import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.google.errorprone.util.ASTHelpers.getType; import static com.google.errorprone.util.ASTHelpers.stripParentheses; -import static com.google.errorprone.util.Reachability.canCompleteNormally; +import static com.google.errorprone.util.Reachability.canFallThrough; import com.google.common.base.CaseFormat; import com.google.common.collect.ImmutableSet; @@ -41,11 +40,9 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.StatementTree; import com.sun.source.tree.SwitchTree; import com.sun.source.util.TreePath; import java.util.HashSet; -import java.util.List; import java.util.Set; import javax.inject.Inject; @@ -101,10 +98,7 @@ private void processSwitch( scanForInvalidGetters(getters, allowableGetters, caseTree, constantReceiver, state); - List statements = caseTree.getStatements(); - if (statements != null - && !statements.isEmpty() - && !canCompleteNormally(getLast(statements))) { + if (!canFallThrough(caseTree)) { allowableGetters.clear(); } }