Add a check for redundant continue statements.

I gave it a suitably generic name in case we generalise in the future. I'm curious about redundant trailing `return;`s from methods!

Flume: unknown commit

I didn't spot any obvious _bugs_ looking through, though of course it's hard to tell.

PiperOrigin-RevId: 696892282

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;
+  }
+}

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,

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<String> xs) {
+                for (String x : xs) {
+                  // BUG: Diagnostic contains:
+                  continue;
+                }
+              }
+            }
+            """)
+        .doTest();
+  }
+
+  @Test
+  public void onlyStatementInTerminalIf() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            """
+            class Test {
+              void test(Iterable<String> 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<String> 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<String> 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<String> 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<String> 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<String> xs) {
+                for (String x : xs) {
+                  if (x.equals("foo")) {
+                    if (x.equals("bar")) {
+                      continue;
+                    }
+                  }
+                  return x;
+                }
+                return null;
+              }
+            }
+            """)
+        .doTest();
+  }
+}

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<Group> filterGroupsOfPrimeOrder(Iterable<Group> groups) {
+  ImmutableList.Builder<Group> 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<Group> filterGroupsOfPrimeOrder(Iterable<Group> groups) {
+  ImmutableList.Builder<Group> 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<Group> filterGroupsOfPrimeOrder(Iterable<Group> groups) {
+  ImmutableList.Builder<Group> 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();
+}
+```