diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/RedundantControlFlow.java b/core/src/main/java/com/google/errorprone/bugpatterns/RedundantControlFlow.java new file mode 100644 index 00000000000..9bbe0573889 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/RedundantControlFlow.java @@ -0,0 +1,66 @@ +/* + * 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.errorprone.matchers.Description.NO_MATCH; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ContinueTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ContinueTree; +import com.sun.source.tree.Tree; + +/** A bug checker; see the summary. */ +@BugPattern( + severity = SeverityLevel.WARNING, + summary = "This continue statement is redundant and can be removed. It may be misleading.") +public final class RedundantControlFlow extends BugChecker implements ContinueTreeMatcher { + @Override + public Description matchContinue(ContinueTree tree, VisitorState state) { + if (tree.getLabel() != null) { + return NO_MATCH; + } + Tree last = tree; + for (Tree parent : state.getPath()) { + switch (parent.getKind()) { + case FOR_LOOP: + case ENHANCED_FOR_LOOP: + case WHILE_LOOP: + case DO_WHILE_LOOP: + return describeMatch(tree, SuggestedFix.delete(tree)); + case CASE: + // It's arguable that "break" is clearer than "continue" from a switch if they both have + // the same effect, but it's not what we want to flag here. + return NO_MATCH; + case BLOCK: + var statements = ((BlockTree) parent).getStatements(); + if (statements.indexOf(last) != statements.size() - 1) { + return NO_MATCH; + } + // fall through + default: + // fall out + } + last = parent; + } + return NO_MATCH; + } +} 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 49c549b9d59..7ddf1061ce9 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -325,6 +325,7 @@ import com.google.errorprone.bugpatterns.RandomCast; import com.google.errorprone.bugpatterns.RandomModInteger; import com.google.errorprone.bugpatterns.ReachabilityFenceUsage; +import com.google.errorprone.bugpatterns.RedundantControlFlow; import com.google.errorprone.bugpatterns.RedundantOverride; import com.google.errorprone.bugpatterns.RedundantSetterCall; import com.google.errorprone.bugpatterns.RedundantThrows; @@ -1061,6 +1062,7 @@ public static ScannerSupplier warningChecks() { ProtoTimestampGetSecondsGetNano.class, QualifierOrScopeOnInjectMethod.class, ReachabilityFenceUsage.class, + RedundantControlFlow.class, ReferenceEquality.class, RethrowReflectiveOperationExceptionAsLinkageError.class, ReturnAtTheEndOfVoidFunction.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/RedundantControlFlowTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/RedundantControlFlowTest.java new file mode 100644 index 00000000000..5df2cac2adf --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/RedundantControlFlowTest.java @@ -0,0 +1,176 @@ +/* + * 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 com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class RedundantControlFlowTest { + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(RedundantControlFlow.class, getClass()); + + @Test + public void onlyStatementInForLoop() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + void test(Iterable xs) { + for (String x : xs) { + // BUG: Diagnostic contains: + continue; + } + } + } + """) + .doTest(); + } + + @Test + public void onlyStatementInTerminalIf() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + void test(Iterable xs) { + for (String x : xs) { + if (x.equals("foo")) { + // BUG: Diagnostic contains: + continue; + } + } + } + } + """) + .doTest(); + } + + @Test + public void onlyStatementInNestedIf() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + void test(Iterable xs) { + for (String x : xs) { + if (x.equals("foo")) { + if (x.equals("bar")) { + // BUG: Diagnostic contains: + continue; + } + } + } + } + } + """) + .doTest(); + } + + @Test + public void onlyStatementInElse() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + void test(Iterable xs) { + for (String x : xs) { + if (x.equals("foo")) { + } else { + // BUG: Diagnostic contains: + continue; + } + } + } + } + """) + .doTest(); + } + + @Test + public void negative() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + String test(Iterable xs) { + for (String x : xs) { + if (x.equals("foo")) { + continue; + } + return x; + } + return null; + } + } + """) + .doTest(); + } + + @Test + public void labelledContinue_noFinding() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + String test(Iterable xs) { + outer: + for (String x : xs) { + for (int i = 0; i < x.length(); i++) { + if (x.charAt(i) == 'a') { + continue outer; + } + } + } + return null; + } + } + """) + .doTest(); + } + + @Test + public void withinNestedIfs_statementsAfter() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + String test(Iterable xs) { + for (String x : xs) { + if (x.equals("foo")) { + if (x.equals("bar")) { + continue; + } + } + return x; + } + return null; + } + } + """) + .doTest(); + } +} diff --git a/docs/bugpattern/RedundantControlFlow.md b/docs/bugpattern/RedundantControlFlow.md new file mode 100644 index 00000000000..8d8343a1bd7 --- /dev/null +++ b/docs/bugpattern/RedundantControlFlow.md @@ -0,0 +1,54 @@ +Unnecessary control flow statements may be misleading in some contexts. + +For instance, this method to find groups of prime order has a bug: + +```java +static ImmutableList filterGroupsOfPrimeOrder(Iterable groups) { + ImmutableList.Builder filtered = ImmutableList.builder(); + for (Group group : groups) { + for (int i = 2; i < group.order(); i++) { + if (group.order() % i == 0) { + continue; + } + } + filtered.add(group); + } + return filtered.build(); +} +``` + +The `continue` statement only breaks out of the innermost loop, so the input is +returned unchanged. + +The most readable alternative is probably to avoid a nested loop entirely: + +```java +static ImmutableList filterGroupsOfPrimeOrder(Iterable groups) { + ImmutableList.Builder filtered = ImmutableList.builder(); + for (Group group : groups) { + if (!isPrime(group.order())) { + continue; + } + filtered.add(group); + } + return filtered.build(); +} +``` + +A labelled break statement would also be correct, but is quite uncommon: + +```java +static ImmutableList filterGroupsOfPrimeOrder(Iterable groups) { + ImmutableList.Builder filtered = ImmutableList.builder(); + outer: + for (Group group : groups) { + for (int i = 2; i < group.order(); i++) { + if (group.order() % i == 0) { + continue outer; + } + } + filtered.add(group); + } + return filtered.build(); +} +```