SuspiciousListRemove: control flow with "return" is compliant (#4613)

Leaving a loop with `return` is the same as leaving the loop
with `break`.

Co-authored-by: Julian Ladisch <[email protected]>

java-checks-test-sources/default/src/main/java/checks/SuspiciousListRemove.java

@@ -1,5 +1,6 @@
 package checks;
 
+import java.util.ArrayList;
 import java.util.List;
 
 public class SuspiciousListRemove {
@@ -66,6 +67,34 @@ void controlFlow2(List<String> list) {
     }
   }
 
+  void controlFlowReturn(List<String> list) {
+    for (int i = 0; i < list.size(); i++) {
+      if (list.get(i).isEmpty()) {
+        list.remove(i);  // Compliant because control flow with "return"
+        return;
+      }
+    }
+  }
+
+  String controlFlowReturn2(List<String> list) {
+    for (int i = 0; i < list.size(); i++) {
+      if (list.get(i).isEmpty()) {
+        return list.remove(i);  // Compliant because control flow with "return"
+      }
+    }
+    return null;
+  }
+
+  List<String> controlFlowReturn3(List<String> list) {
+    List<String> deleted = new ArrayList<>();
+    for (int i = 0; i < list.size(); i++) {
+      if (list.get(i).isEmpty()) {
+        deleted.add(list.remove(i));  // Noncompliant
+      }
+    }
+    return deleted;  // "return" is outside the loop
+  }
+
   void coverage1(List<String> list, int from) {
     for (this.x = from; this.x < list.size(); this.x++)
       list.remove(this.x); // Consider only local vars

java-checks/src/main/java/org/sonar/java/checks/SuspiciousListRemoveCheck.java

@@ -35,6 +35,7 @@
 import org.sonar.plugins.java.api.tree.ForStatementTree;
 import org.sonar.plugins.java.api.tree.IdentifierTree;
 import org.sonar.plugins.java.api.tree.MethodInvocationTree;
+import org.sonar.plugins.java.api.tree.ReturnStatementTree;
 import org.sonar.plugins.java.api.tree.StatementTree;
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.UnaryExpressionTree;
@@ -97,7 +98,7 @@ private static boolean isIncrementingLoop(ForStatementTree forStatementTree, Sym
   private static class LoopBodyVisitor extends BaseTreeVisitor {
     private final Symbol counter;
     private MethodInvocationTree listRemove;
-    private boolean hasBreakOrContinue;
+    private boolean hasBreakOrContinueOrReturn;
     private boolean isCounterAssigned;
 
     public LoopBodyVisitor(Symbol counter) {
@@ -114,16 +115,22 @@ public void visitMethodInvocation(MethodInvocationTree tree) {
 
     @Override
     public void visitBreakStatement(BreakStatementTree tree) {
-      hasBreakOrContinue = true;
+      hasBreakOrContinueOrReturn = true;
       super.visitBreakStatement(tree);
     }
 
     @Override
     public void visitContinueStatement(ContinueStatementTree tree) {
-      hasBreakOrContinue = true;
+      hasBreakOrContinueOrReturn = true;
       super.visitContinueStatement(tree);
     }
 
+    @Override
+    public void visitReturnStatement(ReturnStatementTree tree) {
+      hasBreakOrContinueOrReturn = true;
+      super.visitReturnStatement(tree);
+    }
+
     @Override
     public void visitAssignmentExpression(AssignmentExpressionTree tree) {
       if (tree.variable().is(Tree.Kind.IDENTIFIER)) {
@@ -141,7 +148,7 @@ public void visitUnaryExpression(UnaryExpressionTree tree) {
     }
 
     boolean hasIssue() {
-      return listRemove != null && !hasBreakOrContinue && !isCounterAssigned;
+      return listRemove != null && !hasBreakOrContinueOrReturn && !isCounterAssigned;
     }
   }