Handle switch block consisting of switch rules in Reachability.

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.
    *
    * <p>An exception is made for {@code System.exit}, which cannot complete normally in practice.
    */
-  public static boolean canCompleteNormally(CaseTree caseTree) {
-    List<? extends StatementTree> 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<? extends StatementTree> 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<Boolean, Void> {
@@ -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,23 +233,44 @@ 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());
       }
       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)

check_api/src/test/java/com/google/errorprone/util/ReachabilityTest.java

@@ -320,6 +320,41 @@ public static List<Object[]> 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());
   }

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);

core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryDefaultInEnumSwitch.java

@@ -24,6 +24,7 @@
 import static com.google.errorprone.util.ASTHelpers.getStartPosition;
 import static com.google.errorprone.util.ASTHelpers.isSwitchDefault;
 import static com.google.errorprone.util.Reachability.canCompleteNormally;
+import static com.google.errorprone.util.Reachability.canFallThrough;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
@@ -165,8 +166,8 @@ private Description fixDefault(
     if (!SuggestedFixes.compilesWithFix(SuggestedFix.delete(defaultCase), state)) {
       return NO_MATCH; // case (3)
     }
-    if (!canCompleteNormally(caseBeforeDefault)) {
-      // case (2) -- If the case before the default can't complete normally,
+    if (!canFallThrough(caseBeforeDefault)) {
+      // case (2) -- If the case before the default can't fall through,
       // it's OK to to delete the default.
       return buildDescription(defaultCase)
           .setMessage(DESCRIPTION_REMOVED_DEFAULT)

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<? extends StatementTree> statements = caseTree.getStatements();
-      if (statements != null
-          && !statements.isEmpty()
-          && !canCompleteNormally(getLast(statements))) {
+      if (!canFallThrough(caseTree)) {
         allowableGetters.clear();
       }
     }