IDEA-188555 Warn on List.remove(i) inside a counted loop with i index

2026-09-27 10:03:11 +07:00 · 2018-03-21 17:21:37 +07:00
parent 4069c3e04a
commit 2444cc9165
8 changed files with 155 additions and 3 deletions
@@ -765,6 +765,11 @@
                     enabledByDefault="true" level="WARNING"
                     key="inspection.redundant.comparator.comparing.display.name" bundle="messages.InspectionsBundle"
                     implementationClass="com.intellij.codeInspection.RedundantComparatorComparingInspection"/>
+    <localInspection groupPath="Java" language="JAVA" shortName="SuspiciousListRemoveInLoop"
+                     groupBundle="messages.InspectionsBundle" groupKey="group.names.probable.bugs"
+                     enabledByDefault="true" level="WARNING"
+                     key="inspection.suspicious.list.remove.display.name" bundle="messages.InspectionsBundle"
+                     implementationClass="com.intellij.codeInspection.SuspiciousListRemoveInLoopInspection"/>
    <localInspection groupPath="Java,Java language level migration aids" language="JAVA" shortName="FoldExpressionIntoStream"
                     groupBundle="messages.InspectionsBundle" groupKey="group.names.language.level.specific.issues.and.migration.aids8"
                     enabledByDefault="true" level="INFORMATION"
@@ -0,0 +1,49 @@
+// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
+package com.intellij.codeInspection;
+
+import com.intellij.psi.*;
+import com.intellij.psi.util.PsiUtil;
+import com.intellij.util.ObjectUtils;
+import com.siyeh.ig.callMatcher.CallMatcher;
+import com.siyeh.ig.psiutils.ControlFlowUtils;
+import com.siyeh.ig.psiutils.CountingLoop;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.Objects;
+
+public class SuspiciousListRemoveInLoopInspection extends AbstractBaseJavaLocalInspectionTool {
+  private static final CallMatcher LIST_REMOVE = CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_LIST, "remove").parameterTypes("int");
+
+  @NotNull
+  @Override
+  public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+    return new JavaElementVisitor() {
+      @Override
+      public void visitMethodCallExpression(PsiMethodCallExpression call) {
+        if (!LIST_REMOVE.test(call)) return;
+        PsiReferenceExpression arg =
+          ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(call.getArgumentList().getExpressions()[0]), PsiReferenceExpression.class);
+        if (arg == null) return;
+        PsiExpressionStatement parentStatement = ObjectUtils.tryCast(call.getParent(), PsiExpressionStatement.class);
+        if (parentStatement == null) return;
+        PsiElement parent = parentStatement.getParent();
+        while (parent instanceof PsiLabeledStatement ||
+               parent instanceof PsiIfStatement ||
+               parent instanceof PsiSwitchLabelStatement ||
+               parent instanceof PsiSwitchStatement ||
+               parent instanceof PsiBlockStatement ||
+               parent instanceof PsiCodeBlock) {
+          parent = parent.getParent();
+        }
+
+        if (!(parent instanceof PsiForStatement)) return;
+        CountingLoop loop = CountingLoop.from((PsiForStatement)parent);
+        if (loop == null) return;
+        if (!arg.isReferenceTo(loop.getCounter())) return;
+        if (ControlFlowUtils.isExecutedOnceInLoop(parentStatement, (PsiLoopStatement)parent)) return;
+        holder.registerProblem(Objects.requireNonNull(call.getMethodExpression().getReferenceNameElement()),
+                               InspectionsBundle.message("inspection.suspicious.list.remove.display.name"));
+      }
+    };
+  }
+}
@@ -0,0 +1,11 @@
+<html>
+<body>
+Reports when <strong>list.remove(index)</strong> is called inside the ascending counted loop. This is suspicious as list becomes
+shorter after that and the element next to removed will not be processed. Simple fix is to decrease the index variable after removal,
+but probably removing via iterator or using <strong>removeIf</strong> method (since Java 8) is a more robust alternative.
+If you don't expect that <strong>remove</strong> will be called more than once in a loop, consider adding a <strong>break</strong> command
+after it.
+<!-- tooltip end -->
+<p><small>Since 2018.2</small></p>
+</body>
+</html>
@@ -0,0 +1,58 @@
+import java.util.*;
+
+class Test {
+  void processList(List<String> list, int start) {
+    for(int i=start; i<list.size(); i++) {
+      if(list.get(i).isEmpty()) {
+        list.<warning descr="Suspicious 'List.remove()' in the loop">remove</warning>(i);
+      }
+    }
+  }
+
+  void processAndCorrect(List<String> list, int start) {
+    for(int i=start; i<list.size(); i++) {
+      if(list.get(i).isEmpty()) {
+        list.remove(i);
+        i--;
+      }
+    }
+  }
+
+  void processSingle(List<String> list, int start) {
+    for(int i=start; i<list.size(); i++) {
+      if(list.get(i).isEmpty()) {
+        list.remove(i);
+        break;
+      }
+    }
+  }
+
+  void processContinue(List<String> list, int start) {
+    for(int i=start; i<list.size(); i++) {
+      if(list.get(i).isEmpty()) {
+        list.<warning descr="Suspicious 'List.remove()' in the loop">remove</warning>(i);
+        continue;
+      }
+      System.out.println(list.get(i));
+    }
+  }
+
+  void processContinueOuter(List<String> list, int[] starts) {
+    OUTER:
+    for(int start : starts) {
+      for (int i = start; i < list.size(); i++) {
+        if (list.get(i).isEmpty()) {
+          list.remove(i);
+          continue OUTER;
+        }
+        System.out.println(list.get(i));
+      }
+    }
+  }
+
+  void deleteTail(List<String> list, int from) {
+    for(int i=from; i<list.size(); i++) {
+      list.<warning descr="Suspicious 'List.remove()' in the loop">remove</warning>(i);
+    }
+  }
+}
@@ -0,0 +1,23 @@
+// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
+package com.intellij.java.codeInspection;
+
+import com.intellij.JavaTestUtil;
+import com.intellij.codeInspection.InspectionProfileEntry;
+import com.intellij.codeInspection.SuspiciousListRemoveInLoopInspection;
+import com.siyeh.ig.LightInspectionTestCase;
+
+public class SuspiciousListRemoveInLoopInspectionTest extends LightInspectionTestCase {
+  public void testListRemoveInLoop() {
+    doTest();
+  }
+
+  @Override
+  protected InspectionProfileEntry getInspection() {
+    return new SuspiciousListRemoveInLoopInspection();
+  }
+
+  @Override
+  protected String getBasePath() {
+    return JavaTestUtil.getRelativeJavaTestDataPath() + "/inspection/listRemoveInLoop/";
+  }
+}
@@ -961,4 +961,6 @@ inspection.fold.expression.into.stream.fix.name=Fold expression into Stream chai
 inspection.charset.object.can.be.used.display.name=Standard Charset object can be used
 inspection.charset.object.can.be.used.message={0} can be used instead
 inspection.charset.object.can.be.used.fix.family.name=Use Charset constant
-inspection.charset.object.can.be.used.fix.name=Replace with ''{0}''
+inspection.charset.object.can.be.used.fix.name=Replace with ''{0}''
+
+inspection.suspicious.list.remove.display.name=Suspicious 'List.remove()' in the loop
@@ -358,7 +358,7 @@ public class ControlFlowUtils {
      return false;
    }
    final PsiStatement body = loopStatement.getBody();
-    return body != null && PsiTreeUtil.isAncestor(body, element, true);
+    return PsiTreeUtil.isAncestor(body, element, true);
  }

  public static boolean isInFinallyBlock(@NotNull PsiElement element) {
@@ -714,7 +714,10 @@ public class ControlFlowUtils {
  public static boolean flowBreaksLoop(PsiStatement statement, PsiLoopStatement loop) {
    if(statement == null || statement == loop) return false;
    for (PsiStatement sibling = statement; sibling != null; sibling = nextExecutedStatement(sibling)) {
-      if(sibling instanceof PsiContinueStatement) return false;
+      if(sibling instanceof PsiContinueStatement) {
+        PsiStatement continueTarget = ((PsiContinueStatement)sibling).findContinuedStatement();
+        return PsiTreeUtil.isAncestor(continueTarget, loop, true);
+      }
      if(sibling instanceof PsiThrowStatement || sibling instanceof PsiReturnStatement) return true;
      if(sibling instanceof PsiBreakStatement) {
        PsiBreakStatement breakStatement = (PsiBreakStatement)sibling;
@@ -126,6 +126,7 @@ public class CountingLoop {
    } else return null;
    if(bound == null || !ExpressionUtils.isReferenceTo(ref, counter)) return null;
    if(!TypeConversionUtil.areTypesAssignmentCompatible(counter.getType(), bound)) return null;
+    if(VariableAccessUtils.variableIsAssigned(counter, forStatement.getBody())) return null;
    return new CountingLoop(forStatement, counter, initializer, bound, closed);
  }
 }