diff --git a/java/java-impl/src/META-INF/JavaPlugin.xml b/java/java-impl/src/META-INF/JavaPlugin.xml index 72b30bc467fb..4c735cf39d3b 100644 --- a/java/java-impl/src/META-INF/JavaPlugin.xml +++ b/java/java-impl/src/META-INF/JavaPlugin.xml @@ -765,6 +765,11 @@ enabledByDefault="true" level="WARNING" key="inspection.redundant.comparator.comparing.display.name" bundle="messages.InspectionsBundle" implementationClass="com.intellij.codeInspection.RedundantComparatorComparingInspection"/> + + +Reports when list.remove(index) 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 removeIf method (since Java 8) is a more robust alternative. +If you don't expect that remove will be called more than once in a loop, consider adding a break command +after it. + +

Since 2018.2

+ + \ No newline at end of file diff --git a/java/java-tests/testData/inspection/listRemoveInLoop/ListRemoveInLoop.java b/java/java-tests/testData/inspection/listRemoveInLoop/ListRemoveInLoop.java new file mode 100644 index 000000000000..2803be1be75d --- /dev/null +++ b/java/java-tests/testData/inspection/listRemoveInLoop/ListRemoveInLoop.java @@ -0,0 +1,58 @@ +import java.util.*; + +class Test { + void processList(List list, int start) { + for(int i=start; iremove(i); + } + } + } + + void processAndCorrect(List list, int start) { + for(int i=start; i list, int start) { + for(int i=start; i list, int start) { + for(int i=start; iremove(i); + continue; + } + System.out.println(list.get(i)); + } + } + + void processContinueOuter(List 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 list, int from) { + for(int i=from; iremove(i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/SuspiciousListRemoveInLoopInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/SuspiciousListRemoveInLoopInspectionTest.java new file mode 100644 index 000000000000..a9cbd8311289 --- /dev/null +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/SuspiciousListRemoveInLoopInspectionTest.java @@ -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/"; + } +} diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 0df5c276974b..d0afd18f9fa2 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -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}'' \ No newline at end of file +inspection.charset.object.can.be.used.fix.name=Replace with ''{0}'' + +inspection.suspicious.list.remove.display.name=Suspicious 'List.remove()' in the loop diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java index 45c9c84ca160..131cac9ee687 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/ControlFlowUtils.java @@ -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; diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/CountingLoop.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/CountingLoop.java index 87eb545f2d31..2313100179f0 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/CountingLoop.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/CountingLoop.java @@ -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); } }