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 index 79bf0896a8f..84c99e6a853 100644 --- 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 -> }. + */ + @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 new file mode 100644 index 00000000000..8deb44f59fc --- /dev/null +++ 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 index e020cace23f..e02825e352a 100644 --- 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 new file mode 100644 index 00000000000..9e3ed40d0af --- /dev/null +++ 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 new file mode 100644 index 00000000000..dcf29f6d306 --- /dev/null +++ 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.