Disallow unnecessary break statements at the end of the body of a `…

…->` switch statement Startblock: * unknown commit is submitted PiperOrigin-RevId: 655602239
google · Jul 24, 2024 · fc339e0 · fc339e0
1 parent 0ec88c0
commit fc339e0
Show file tree

Hide file tree

Showing 5 changed files with 218 additions and 0 deletions.
diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
@@ -2836,6 +2836,33 @@ private static Method getGetCaseKindMethod() {
     }
   }
 
+  /**
+   * Returns the statement or expression after the arrow for a {@link CaseTree} of the form: {@code
+   * case <expression> -> <body>}.
+   */
+  @Nullable
+  public static Tree getCaseTreeBody(CaseTree caseTree) {
+    if (GET_CASE_BODY_METHOD == null) {
+      return null;
+    }
+    try {
+      return (Tree) GET_CASE_BODY_METHOD.invoke(caseTree);
+    } catch (ReflectiveOperationException e) {
+      throw new LinkageError(e.getMessage(), e);
+    }
+  }
+
+  @Nullable private static final Method GET_CASE_BODY_METHOD = getGetCaseBodyMethod();
+
+  @Nullable
+  private static Method getGetCaseBodyMethod() {
+    try {
+      return CaseTree.class.getMethod("getBody");
+    } catch (NoSuchMethodException e) {
+      return null;
+    }
+  }
+
   /**
    * Retrieves a stream containing all case expressions, in order, for a given {@code CaseTree}.
    * This method acts as a facade to the {@code CaseTree.getExpressions()} API, falling back to

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBreakInSwitch.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBreakInSwitch.java
@@ -0,0 +1,63 @@
+/*
+ * Copyright 2024 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static com.google.common.collect.Iterables.getLast;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.isRuleKind;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.BlockTree;
+import com.sun.source.tree.BreakTree;
+import com.sun.source.tree.CaseTree;
+import com.sun.source.tree.StatementTree;
+import com.sun.source.tree.Tree;
+
+/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
+@BugPattern(
+    summary = "This break is unnecessary, fallthrough does not occur in -> switches",
+    severity = WARNING)
+public class UnnecessaryBreakInSwitch extends BugChecker implements BugChecker.CaseTreeMatcher {
+  @Override
+  public Description matchCase(CaseTree tree, VisitorState state) {
+    if (!isRuleKind(tree)) {
+      return NO_MATCH;
+    }
+    Tree body = ASTHelpers.getCaseTreeBody(tree);
+    if (!(body instanceof BlockTree)) {
+      return NO_MATCH;
+    }
+    BlockTree blockTree = (BlockTree) body;
+    if (blockTree.getStatements().isEmpty()) {
+      return NO_MATCH;
+    }
+    StatementTree last = getLast(blockTree.getStatements());
+    if (!(last instanceof BreakTree)) {
+      return NO_MATCH;
+    }
+    BreakTree breakTree = (BreakTree) last;
+    if (breakTree.getLabel() != null) {
+      return NO_MATCH;
+    }
+    return describeMatch(last, SuggestedFix.delete(last));
+  }
+}
diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
@@ -410,6 +410,7 @@
 import com.google.errorprone.bugpatterns.UnnecessaryAsync;
 import com.google.errorprone.bugpatterns.UnnecessaryBoxedAssignment;
 import com.google.errorprone.bugpatterns.UnnecessaryBoxedVariable;
+import com.google.errorprone.bugpatterns.UnnecessaryBreakInSwitch;
 import com.google.errorprone.bugpatterns.UnnecessaryDefaultInEnumSwitch;
 import com.google.errorprone.bugpatterns.UnnecessaryFinal;
 import com.google.errorprone.bugpatterns.UnnecessaryLambda;
@@ -1091,6 +1092,7 @@ public static ScannerSupplier warningChecks() {
           UnicodeEscape.class,
           UnnecessaryAssignment.class,
           UnnecessaryAsync.class,
+          UnnecessaryBreakInSwitch.class,
           UnnecessaryLambda.class,
           UnnecessaryLongToIntConversion.class,
           UnnecessaryMethodInvocationMatcher.class,

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBreakInSwitchTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBreakInSwitchTest.java
@@ -0,0 +1,123 @@
+/*
+ * Copyright 2024 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static org.junit.Assume.assumeTrue;
+
+import com.google.errorprone.CompilationTestHelper;
+import com.google.errorprone.util.RuntimeVersion;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class UnnecessaryBreakInSwitchTest {
+
+  private final CompilationTestHelper testHelper =
+      CompilationTestHelper.newInstance(UnnecessaryBreakInSwitch.class, getClass());
+
+  @Test
+  public void positive() {
+    assumeTrue(RuntimeVersion.isAtLeast14());
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  void f(int i) {",
+            "    switch (i) {",
+            "      default -> {",
+            "        // BUG: Diagnostic contains: break is unnecessary",
+            "        break;",
+            "      }",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negativeTraditional() {
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  void f(int i) {",
+            "    switch (i) {",
+            "      default:",
+            "        break;",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negativeEmpty() {
+    assumeTrue(RuntimeVersion.isAtLeast14());
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  void f(int i) {",
+            "    switch (i) {",
+            "      default -> {",
+            "      }",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negativeNotLast() {
+    assumeTrue(RuntimeVersion.isAtLeast14());
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  void f(int i) {",
+            "    switch (i) {",
+            "      default -> {",
+            "        if (true) break;",
+            "      }",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negativeLabelledBreak() {
+    assumeTrue(RuntimeVersion.isAtLeast14());
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  void f(int i) {",
+            "    outer:",
+            "    while (true) {",
+            "      switch (i) {",
+            "        default -> {",
+            "          break outer;",
+            "        }",
+            "      }",
+            "    }",
+            "  }",
+            "}")
+        .doTest();
+  }
+}
diff --git a/docs/bugpattern/UnnecessaryBreakInSwitch.md b/docs/bugpattern/UnnecessaryBreakInSwitch.md
@@ -0,0 +1,3 @@
+The newer arrow (`->`) syntax for switches does not permit fallthrough between
+cases. A `break` statement is allowed to break out of the switch, but including
+a `break` as the last statement in a case body is unnecessary.