From 9c1be97480d7fbb3a43e2c0b1a7ac0a1d2ab8863 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 11 Sep 2019 13:57:59 +0700 Subject: [PATCH] DFA: support unary ++/-- (IDEA-221657) Also: define binop widening based on CFG, not on PSI (so loops generated via inliners are also supported) Better squashing GitOrigin-RevId: e1e15652b0363357f6d8dd40c6048e09ae436d09 --- .../dataFlow/ControlFlowAnalyzer.java | 83 +++++++++---------- .../dataFlow/DataFlowRunner.java | 11 +++ .../dataFlow/DfaInstructionState.java | 23 +++-- .../dataFlow/DfaMemoryStateImpl.java | 3 + .../codeInspection/dataFlow/StateMerger.java | 1 + .../instructions/BinopInstruction.java | 56 ++++++++++++- .../dataFlow/fixture/CellsComplex.java | 42 ++++++++++ ...nstantConditionsWithAssignmentsInside.java | 4 +- .../dataFlow/fixture/StreamInlining.java | 6 ++ .../dataFlow/fixture/UnaryPlusMinus.java | 69 +++++++++++++++ .../DataFlowInspectionTest.java | 1 + .../DataFlowRangeAnalysisTest.java | 1 + 12 files changed, 242 insertions(+), 58 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/CellsComplex.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/UnaryPlusMinus.java 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 c4081fecbd55..630273bcbafe 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 @@ -14,7 +14,6 @@ import com.intellij.codeInspection.dataFlow.inliner.*; import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.rangeSet.LongRangeSet; import com.intellij.codeInspection.dataFlow.value.*; -import com.intellij.codeInspection.dataFlow.value.DfaRelationValue.RelationType; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; import com.intellij.psi.*; @@ -221,8 +220,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { lExpr.accept(this); addInstruction(new DupInstruction()); rExpr.accept(this); - addInstruction(new BinopInstruction( - isAcceptableContextForMathOperation(expression) ? JavaTokenType.PLUS : BinopInstruction.STRING_CONCAT_IN_LOOP, null, type)); + addInstruction(new BinopInstruction(BinopInstruction.STRING_CONCAT_IN_LOOP, null, type)); } else { IElementType sign = TypeConversionUtil.convertEQtoOperation(op); @@ -232,7 +230,6 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { generateBoxingUnboxingInstructionFor(lExpr, resType); rExpr.accept(this); generateBoxingUnboxingInstructionFor(rExpr, resType); - sign = substituteBinaryOperation(rExpr, sign); if (isAssignmentDivision(op) && resType != null && PsiType.LONG.isAssignableFrom(resType)) { checkZeroDivisor(); } @@ -679,7 +676,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new PushInstruction(loopVar, null, true)); addInstruction(new PushInstruction(loopVar, null)); addInstruction(new PushInstruction(myFactory.getConstFactory().createFromValue(1, PsiType.INT), null)); - addInstruction(new BinopInstruction(JavaTokenType.PLUS, null, loopVar.getType())); + addInstruction(new BinopInstruction(JavaTokenType.PLUS, null, loopVar.getType(), -1, true)); addInstruction(new AssignInstruction(null, null)); addInstruction(new PopInstruction()); } @@ -1400,8 +1397,6 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } private void generateOther(PsiPolyadicExpression expression, IElementType op, PsiExpression[] operands, PsiType type) { - op = substituteBinaryOperation(expression, op); - PsiExpression lExpr = operands[0]; lExpr.accept(this); PsiType lType = lExpr.getType(); @@ -1418,38 +1413,6 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } } - @Nullable - private IElementType substituteBinaryOperation(PsiExpression expression, IElementType op) { - if (JavaTokenType.PLUS == op) { - if (isAcceptableContextForMathOperation(expression)) return op; - if (TypeUtils.isJavaLangString(expression.getType())) return BinopInstruction.STRING_CONCAT_IN_LOOP; - return null; - } - if ((JavaTokenType.MINUS == op || JavaTokenType.ASTERISK == op) && !isAcceptableContextForMathOperation(expression)) return null; - return op; - } - - private boolean isAcceptableContextForMathOperation(PsiExpression expression) { - PsiElement parent = expression.getParent(); - while (parent != null && parent != myCodeFragment) { - if ((parent instanceof PsiExpressionList && parent.getParent() instanceof PsiCallExpression) || - parent instanceof PsiArrayInitializerExpression || - parent instanceof PsiArrayAccessExpression) { - return true; - } - if (parent instanceof PsiBinaryExpression && RelationType.fromElementType(((PsiBinaryExpression)parent).getOperationTokenType()) != null) { - return true; - } - if (parent instanceof PsiLoopStatement && - !(parent instanceof PsiForStatement && - PsiTreeUtil.isAncestor(((PsiForStatement)parent).getInitialization(), expression, false))) { - return false; - } - parent = parent.getParent(); - } - return true; - } - private void acceptBinaryRightOperand(@Nullable IElementType op, PsiType type, PsiExpression lExpr, @Nullable PsiType lType, PsiExpression rExpr, @Nullable PsiType rType) { @@ -1949,16 +1912,44 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { PsiExpression operand = PsiUtil.skipParenthesizedExprDown(expression.getOperand()); if (operand != null) { operand.accept(this); - generateBoxingUnboxingInstructionFor(operand, PsiType.INT); - pushUnknown(); - addInstruction(new AssignInstruction(operand, null, myFactory.createValue(operand))); + addInstruction(new DupInstruction()); + processIncrementDecrement(expression, operand); addInstruction(new PopInstruction()); + } else { + pushUnknown(); } - pushUnknown(); finishElement(expression); } + private boolean processIncrementDecrement(PsiUnaryExpression expression, PsiExpression operand) { + IElementType token; + if (expression.getOperationTokenType().equals(JavaTokenType.MINUSMINUS)) { + token = JavaTokenType.MINUS; + } + else if (expression.getOperationTokenType().equals(JavaTokenType.PLUSPLUS)) { + token = JavaTokenType.PLUS; + } + else { + return false; + } + PsiPrimitiveType unboxedType = PsiPrimitiveType.getOptionallyUnboxedType(operand.getType()); + if (unboxedType == null) return false; + addInstruction(new DupInstruction()); + generateBoxingUnboxingInstructionFor(operand, unboxedType); + PsiType resultType = TypeConversionUtil.binaryNumericPromotion(unboxedType, PsiType.INT); + addInstruction(new PushInstruction(myFactory.getConstFactory().createFromValue(1, PsiType.INT), null)); + addInstruction(new BinopInstruction(token, null, resultType)); + if (!unboxedType.equals(resultType)) { + addInstruction(new PrimitiveConversionInstruction(unboxedType, null)); + } + if (!(operand.getType() instanceof PsiPrimitiveType)) { + addInstruction(new BoxingInstruction(operand.getType())); + } + addInstruction(new AssignInstruction(operand, null, myFactory.createValue(operand))); + return true; + } + @Override public void visitPrefixExpression(PsiPrefixExpression expression) { startElement(expression); @@ -1979,8 +1970,10 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { PsiPrimitiveType unboxed = PsiPrimitiveType.getUnboxedType(type); generateBoxingUnboxingInstructionFor(operand, unboxed == null ? type : unboxed); if (PsiUtil.isIncrementDecrementOperation(expression)) { - pushUnknown(); - addInstruction(new AssignInstruction(operand, null, myFactory.createValue(operand))); + if (!processIncrementDecrement(expression, operand)) { + pushUnknown(); + addInstruction(new AssignInstruction(operand, null, myFactory.createValue(operand))); + } } else if (expression.getOperationTokenType() == JavaTokenType.EXCL) { addInstruction(new NotInstruction(expression)); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java index 666c60997d94..dec6475d0272 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowRunner.java @@ -168,6 +168,13 @@ public class DataFlowRunner { int endOffset = flow.getInstructionCount(); myInstructions = flow.getInstructions(); + + for (int i = 0; i < endOffset; i++) { + if (loopNumber[i] > 0 && myInstructions[i] instanceof BinopInstruction) { + ((BinopInstruction)myInstructions[i]).widenOperationInLoop(); + } + } + myNestedClosures.clear(); myWasForciblyMerged = false; @@ -337,6 +344,10 @@ public class DataFlowRunner { } else if (instruction instanceof MethodCallInstruction && !((MethodCallInstruction)instruction).getContracts().isEmpty()) { joinInstructions.add(myInstructions[index + 1]); } + else if (instruction instanceof FinishElementInstruction && !((FinishElementInstruction)instruction).getVarsToFlush().isEmpty()) { + // Good chances to squash something after some vars are flushed + joinInstructions.add(myInstructions[index + 1]); + } } return joinInstructions; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java index 1b0d931d03e1..0398ee7a238b 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java @@ -88,14 +88,23 @@ class StateQueue { } if (memoryStates.size() > 1 && joinInstructions.contains(instruction)) { - MultiMap groups = MultiMap.create(); - for (DfaMemoryStateImpl memoryState : memoryStates) { - groups.putValue(memoryState.getSuperficialKey(), memoryState); - } + while (true) { + int beforeSize = memoryStates.size(); + MultiMap groups = MultiMap.create(); + for (DfaMemoryStateImpl memoryState : memoryStates) { + groups.putValue(memoryState.getSuperficialKey(), memoryState); + } - memoryStates = new ArrayList<>(); - for (Map.Entry> entry : groups.entrySet()) { - memoryStates.addAll(mergeGroup((List)entry.getValue())); + memoryStates = new ArrayList<>(); + for (Map.Entry> entry : groups.entrySet()) { + memoryStates.addAll(mergeGroup((List)entry.getValue())); + } + if (memoryStates.size() == beforeSize) break; + beforeSize = memoryStates.size(); + if (beforeSize == 1) break; + // If some states were merged it's possible that they could be further squashed + memoryStates = squash(memoryStates); + if (memoryStates.size() == beforeSize || memoryStates.size() == 1) break; } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java index 251484c2d97c..fdc672e93298 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java @@ -259,6 +259,9 @@ public class DfaMemoryStateImpl implements DfaMemoryState { DfaVariableValue target = replaceQualifier((DfaVariableValue)value, flushed, replacement); if (target != value) return target; } + if (value.getType() instanceof PsiPrimitiveType) { + return myFactory.getFactValue(DfaFactType.RANGE, getValueFact(value, DfaFactType.RANGE)); + } DfaNullability dfaNullability = isNotNull(value) ? DfaNullability.NOT_NULL : getValueFact(value, DfaFactType.NULLABILITY); if (dfaNullability == null) { dfaNullability = DfaNullability.fromNullability(((DfaVariableValue)value).getInherentNullability()); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java index 902a6cb7540e..524599ca0cfd 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java @@ -254,6 +254,7 @@ class StateMerger { DfaMemoryStateImpl copy = map.get(var); if (copy == null) { copy = state.createCopy(); + copy.setVariableState(var, copy.createVariableState(var)); copy.flushVariable(var); map.put(var, copy); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java index 4e71cccc5277..3a1bf78f1296 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java @@ -20,12 +20,13 @@ import com.intellij.codeInspection.dataFlow.DataFlowRunner; import com.intellij.codeInspection.dataFlow.DfaInstructionState; import com.intellij.codeInspection.dataFlow.DfaMemoryState; import com.intellij.codeInspection.dataFlow.InstructionVisitor; +import com.intellij.codeInspection.dataFlow.value.DfaRelationValue; import com.intellij.openapi.util.TextRange; -import com.intellij.psi.PsiExpression; -import com.intellij.psi.PsiPolyadicExpression; -import com.intellij.psi.PsiType; +import com.intellij.psi.*; import com.intellij.psi.tree.IElementType; import com.intellij.psi.tree.TokenSet; +import com.intellij.psi.util.PsiUtil; +import com.siyeh.ig.psiutils.TypeUtils; import org.jetbrains.annotations.Nullable; import static com.intellij.psi.JavaTokenType.*; @@ -47,19 +48,66 @@ public class BinopInstruction extends BranchingInstruction implements Expression */ public static final IElementType STRING_EQUALITY_BY_CONTENT = EQ; - private final IElementType myOperationSign; + private IElementType myOperationSign; private final @Nullable PsiType myResultType; private final int myLastOperand; + private final boolean myUnrolledLoop; public BinopInstruction(IElementType opSign, @Nullable PsiExpression psiAnchor, @Nullable PsiType resultType) { this(opSign, psiAnchor, resultType, -1); } public BinopInstruction(IElementType opSign, @Nullable PsiExpression psiAnchor, @Nullable PsiType resultType, int lastOperand) { + this(opSign, psiAnchor, resultType, lastOperand, false); + } + + /** + * @param opSign sign of the operation + * @param psiAnchor PSI element to bind the instruction to + * @param resultType result of the operation + * @param lastOperand number of last operand if anchor is a {@link PsiPolyadicExpression} and this instruction is the result of + * part of that expression; -1 if not applicable + * @param unrolledLoop true means that this instruction is executed inside an unrolled loop; in this case it will never be widened + */ + public BinopInstruction(IElementType opSign, + @Nullable PsiExpression psiAnchor, + @Nullable PsiType resultType, + int lastOperand, + boolean unrolledLoop) { super(psiAnchor); myResultType = resultType; myOperationSign = ourSignificantOperations.contains(opSign) ? opSign : null; myLastOperand = lastOperand; + myUnrolledLoop = unrolledLoop; + } + + /** + * Make operation wide (less precise) if necessary (called for the operations inside loops only) + */ + public void widenOperationInLoop() { + // these operations usually produce non-converging states + if (!myUnrolledLoop && (myOperationSign == PLUS || myOperationSign == MINUS || myOperationSign == ASTERISK) && + mayProduceDivergedState()) { + myOperationSign = TypeUtils.isJavaLangString(myResultType) ? STRING_CONCAT_IN_LOOP : null; + } + } + + private boolean mayProduceDivergedState() { + PsiElement anchor = getExpression(); + if (anchor instanceof PsiUnaryExpression) { + return PsiUtil.isIncrementDecrementOperation(anchor); + } + while (anchor != null && !(anchor instanceof PsiAssignmentExpression) && !(anchor instanceof PsiVariable)) { + if (anchor instanceof PsiStatement || + anchor instanceof PsiExpressionList && anchor.getParent() instanceof PsiCallExpression || + anchor instanceof PsiArrayInitializerExpression || anchor instanceof PsiArrayAccessExpression || + anchor instanceof PsiBinaryExpression && + DfaRelationValue.RelationType.fromElementType(((PsiBinaryExpression)anchor).getOperationTokenType()) != null) { + return false; + } + anchor = anchor.getParent(); + } + return true; } /** diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/CellsComplex.java b/java/java-tests/testData/inspection/dataFlow/fixture/CellsComplex.java new file mode 100644 index 000000000000..6f87b1a2d0f4 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/CellsComplex.java @@ -0,0 +1,42 @@ +class Foo { + // TODO: make not complex + public static int[] cells(int[] start, int[] end) { + int overlap = 0; + int gaps = 0; + for (int i = 0, j = 0; j < end.length; ) { + if (i < start.length && start[i] < end[j]) { + overlap++; + i++; + } else { + j++; + overlap--; + } + if (overlap == 0) { + gaps++; + } + } + int[] cells = new int[gaps * 2]; + overlap = 0; + gaps = 0; + int previousOverlap = 0; + for (int i = 0, j = 0; j < end.length; ) { + if (i < start.length && start[i] < end[j]) { + overlap++; + if (previousOverlap == 0) { + cells[gaps++] = start[i]; + } + i++; + } else { + overlap--; + if (overlap == 0) { + cells[gaps++] = end[j]; + } + j++; + } + previousOverlap = overlap; + } + + return cells; + } + +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ConstantConditionsWithAssignmentsInside.java b/java/java-tests/testData/inspection/dataFlow/fixture/ConstantConditionsWithAssignmentsInside.java index d9fc4c43f751..a985feb8c3e3 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ConstantConditionsWithAssignmentsInside.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ConstantConditionsWithAssignmentsInside.java @@ -4,8 +4,8 @@ class Contracts { if (flag == (flag = true)) System.out.println(); int x = 1; - boolean y = x == (x +=1); // returns false - if (y) System.out.println(); + boolean y = x == (x +=1); // returns false + if (y) System.out.println(); int k = 1; boolean z = (k +=1) == k; // returns true diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/StreamInlining.java b/java/java-tests/testData/inspection/dataFlow/fixture/StreamInlining.java index 691c7ee972d4..4ed9d1f87b44 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/StreamInlining.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/StreamInlining.java @@ -301,4 +301,10 @@ public class StreamInlining { bb, cc)); } + + void testNotTooComplexForEach(List list) { + int[] count = {0}; + list.stream().forEach(l -> count[0]++); + System.out.println(count[0]); + } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/UnaryPlusMinus.java b/java/java-tests/testData/inspection/dataFlow/fixture/UnaryPlusMinus.java new file mode 100644 index 000000000000..374a359cc3dc --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/UnaryPlusMinus.java @@ -0,0 +1,69 @@ +import java.util.*; + +public class UnaryPlusMinus { + void test() { + int x = 0; + if (x == 0) { } + + x += 1; + if (x == 1) { } + + x++; + if (x == 3) { } + + ++x; + if (x == 3) {} + + if (--x == 2) {} + x--; + if (x == 1) {} + } + + void testChar() { + char c = 0; + if (--c == '\uFFFF') {} + } + + void testLong() { + long l = Integer.MAX_VALUE; + l++; + if (l == Integer.MAX_VALUE+1L) {} + } + + void testDouble() { + // Not supported + double x = 0; + x++; + if (x == 1) {} + x = 1e15; + x++; + if (x == 1e15) {} + } + + void testArray() { + int[] x = new int[3]; + int index = 0; + x[index++] = 1; + x[index++] = 2; + x[index++] = 3; + x[index++] = 4; + } + + int testInForCondition(int[] _data, int _pos) { + int max = _data[_pos - 1]; + for (int i = _pos - 1; i-- > 0;) { + max = Math.max(max, _data[_pos]); + } + return max; + } + + void testNotComplexInLoop() { + int x = 0; + while(true) { + int y = x + 1; + if (y > 10000) break; + x = y; + } + System.out.println(x); + } +} diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java index 9e8ebdd5f5d2..de16e584333b 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -668,4 +668,5 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { public void testClassCastExceptionDispatch() { doTest(); } public void testInstanceQualifiedStaticMember() { doTest(); } public void testClassEqualityCornerCase() { doTest(); } + public void testCellsComplex() { doTest(); } } diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowRangeAnalysisTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowRangeAnalysisTest.java index 9d7a217ba4f3..51abdeac62cf 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowRangeAnalysisTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowRangeAnalysisTest.java @@ -62,4 +62,5 @@ public class DataFlowRangeAnalysisTest extends DataFlowInspectionTestCase { public void testBackPropagationMod() { doTest(); } public void testArithmeticNoOp() { doTest(); } public void testStringConcat() { doTest(); } + public void testUnaryPlusMinus() { doTest(); } }