diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java index 7bd6967e1c5c..4b03ac9c8202 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java @@ -420,23 +420,52 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { final PsiParameter parameter = statement.getIterationParameter(); final PsiExpression iteratedValue = statement.getIteratedValue(); + ControlFlow.ControlFlowOffset loopEndOffset = getEndOffset(statement); + boolean hasSizeCheck = false; + if (iteratedValue != null) { iteratedValue.accept(this); addInstruction(new FieldReferenceInstruction(iteratedValue, "Collection iterator or array.length")); + DfaValue qualifier = myFactory.createValue(iteratedValue); + + if (qualifier instanceof DfaVariableValue) { + PsiType type = iteratedValue.getType(); + SpecialField length = null; + if (type instanceof PsiArrayType) { + length = SpecialField.ARRAY_LENGTH; + } + else if (InheritanceUtil.isInheritor(type, JAVA_UTIL_COLLECTION)) { + length = SpecialField.COLLECTION_SIZE; + } + if (length != null) { + addInstruction(new PushInstruction(length.createValue(myFactory, qualifier), null)); + addInstruction(new PushInstruction(myFactory.getConstFactory().createFromValue(0, PsiType.INT, null), null)); + addInstruction(new BinopInstruction(JavaTokenType.EQEQ, iteratedValue, myProject)); + addInstruction(new ConditionalGotoInstruction(loopEndOffset, false, null)); + hasSizeCheck = true; + } + } } ControlFlow.ControlFlowOffset offset = myCurrentFlow.getNextOffset(); DfaVariableValue dfaVariable = myFactory.getVarFactory().createVariableValue(parameter, false); addInstruction(new FlushVariableInstruction(dfaVariable)); - pushUnknown(); - addInstruction(new ConditionalGotoInstruction(getEndOffset(statement), true, null)); + if (!hasSizeCheck) { + pushUnknown(); + addInstruction(new ConditionalGotoInstruction(loopEndOffset, true, null)); + } final PsiStatement body = statement.getBody(); if (body != null) { body.accept(this); } + if (hasSizeCheck) { + pushUnknown(); + addInstruction(new ConditionalGotoInstruction(loopEndOffset, true, null)); + } + addInstruction(new GotoInstruction(offset)); finishElement(statement); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java index d8982b30824f..ea1f142b51c0 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java @@ -669,11 +669,24 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { PsiElement psiAnchor, boolean evaluatesToTrue) { if (!skipReportingConstantCondition(visitor, psiAnchor, evaluatesToTrue)) { - final LocalQuickFix fix = createSimplifyBooleanExpressionFix(psiAnchor, evaluatesToTrue); - String message = InspectionsBundle.message(isAtRHSOfBooleanAnd(psiAnchor) ? - "dataflow.message.constant.condition.when.reached" : - "dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); - holder.registerProblem(psiAnchor, message, fix == null ? null : new LocalQuickFix[]{fix}); + if (psiAnchor.getParent() instanceof PsiForeachStatement) { + // highlighted for-each iterated value means evaluatesToTrue == "collection is always empty" + if (!evaluatesToTrue) { + // loop on always non-empty collection -- nothing to report + return; + } + boolean array = psiAnchor instanceof PsiExpression && ((PsiExpression)psiAnchor).getType() instanceof PsiArrayType; + holder.registerProblem(psiAnchor, array ? + InspectionsBundle.message("dataflow.message.loop.on.empty.array") : + InspectionsBundle.message("dataflow.message.loop.on.empty.collection")); + } + else { + final LocalQuickFix fix = createSimplifyBooleanExpressionFix(psiAnchor, evaluatesToTrue); + String message = InspectionsBundle.message(isAtRHSOfBooleanAnd(psiAnchor) ? + "dataflow.message.constant.condition.when.reached" : + "dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); + holder.registerProblem(psiAnchor, message, fix == null ? null : new LocalQuickFix[]{fix}); + } } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/LiveVariablesAnalyzer.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/LiveVariablesAnalyzer.java index cc79e8aa20c7..ac6b48e03be8 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/LiveVariablesAnalyzer.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/LiveVariablesAnalyzer.java @@ -28,6 +28,7 @@ import com.intellij.util.PairFunction; import com.intellij.util.containers.*; import com.intellij.util.containers.Queue; import one.util.streamex.IntStreamEx; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -42,7 +43,7 @@ public class LiveVariablesAnalyzer { private final MultiMap myForwardMap; private final MultiMap myBackwardMap; @SuppressWarnings("MismatchedQueryAndUpdateOfCollection") private final FactoryMap> myClosureReads = new FactoryMap>() { - @Nullable + @NotNull @Override protected List create(PsiElement closure) { final Set result = ContainerUtil.newLinkedHashSet(); @@ -208,7 +209,11 @@ public class LiveVariablesAnalyzer { if (ok) { for (FinishElementInstruction instruction : toFlush.keySet()) { - instruction.getVarsToFlush().addAll(toFlush.get(instruction)); + Collection values = toFlush.get(instruction); + // Do not flush special values as they could be used implicitly + values.removeIf(var -> var.getQualifier() != null && + StreamEx.of(SpecialField.values()).anyMatch(sf -> sf.isMyAccessor(var.getPsiVariable()))); + instruction.getVarsToFlush().addAll(values); } } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ForEachOverEmptyCollection.java b/java/java-tests/testData/inspection/dataFlow/fixture/ForEachOverEmptyCollection.java new file mode 100644 index 000000000000..d346355acab7 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ForEachOverEmptyCollection.java @@ -0,0 +1,52 @@ +import java.util.Collection; +import java.util.List; + +public class ForEachOverEmptyCollection { + void testArray(int[][] arr) { + if(arr.length != 0) return; + for (int[] ints : arr) { + System.out.println(ints.length); + } + } + + void testCollection(Collection c) { + if(!c.isEmpty()) return; + for (Object o : c) { + System.out.println(o); + } + } + + void testArrayAfter(String[] arr) { + int count = 0; + boolean hasItem = false; + for(String str : arr) { + if(str != null) { + count++; + } + hasItem = true; + } + if(arr.length == 0 && count > 0) { + // count > 0 means we visited the loop -- impossible + System.out.println("Impossible"); + } + if(!hasItem) { + // we never visited the loop: array is empty + System.out.println(arr[1]); + } + } + + void testCollectionAfter(List list) { + boolean hasItem = false; + String max = null; + for (String s : list) { + if(!hasItem || s.compareTo(max) > 0) { + max = s; + } + hasItem = true; + } + if(!hasItem) { + System.out.println( + list.get(max == null ? 0 : 1)); + } + } +} diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java index 6ad10574bbbd..ad654b1d46aa 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java @@ -332,6 +332,7 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { public void testAccessingSameArrayElements() { doTest(); } public void testArrayLength() { doTest(); } + public void testForEachOverEmptyCollection() { doTest(); } public void testMethodParametersCanChangeNullability() { doTest(); } diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 93c1a96ae41d..cbeff4223bef 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -77,6 +77,8 @@ dataflow.message.cce=Casting {0} to #ref #loc may prod dataflow.message.redundant.instanceof=Condition #ref #loc is redundant and can be replaced with != null dataflow.message.constant.condition=Condition #ref #loc is always {0} dataflow.message.constant.condition.when.reached=Condition #ref #loc is always {0} when reached +dataflow.message.loop.on.empty.array=Array #ref is always empty +dataflow.message.loop.on.empty.collection=Collection #ref is always empty dataflow.message.unreachable.switch.label=Switch label #ref #loc is unreachable dataflow.message.pointless.assignment.expression=Condition #ref #loc at the left side of assignment expression is always {0}. Can be simplified dataflow.message.passing.null.argument=Passing null argument to parameter annotated as @NotNull