From 0eacb8621a4c42bd05a1eee379e6127911cda110 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 26 Apr 2019 13:12:03 +0700 Subject: [PATCH] IDEA-209947 Explain &&/||-chains, minor fixes GitOrigin-RevId: 074ea99bdc28c9f026e097ee5630c3a8c0614dd6 --- .../dataFlow/TrackingRunner.java | 41 +++++++++++++++++-- .../ConditionalGotoInstruction.java | 4 ++ .../dataFlow/tracker/AndChainCause.java | 13 ++++++ .../tracker/AndChainDependentCause.java | 17 ++++++++ .../dataFlow/tracker/AssignTrue.java | 11 +++++ .../dataFlow/tracker/OrChainCause.java | 24 +++++++++++ .../DataFlowInspectionTrackerTest.java | 4 ++ 7 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/AndChainCause.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/AndChainDependentCause.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/AssignTrue.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/OrChainCause.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingRunner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingRunner.java index d9ba0f5ae04a..132bd41d2210 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingRunner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingRunner.java @@ -16,6 +16,7 @@ import com.intellij.openapi.util.Segment; import com.intellij.openapi.util.TextRange; import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; +import com.intellij.psi.tree.IElementType; import com.intellij.psi.util.PsiUtil; import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; @@ -212,7 +213,11 @@ public class TrackingRunner extends StandardDataFlowRunner { if (children.isEmpty()) { ((PossibleExecutionDfaProblemType)mergePoint.myProblem).myComplete = false; } - mergePoint.myChildren.addAll(children); + for (CauseItem child : children) { + if (!mergePoint.myChildren.contains(child)) { + mergePoint.myChildren.add(child); + } + } return true; } @@ -275,6 +280,9 @@ public class TrackingRunner extends StandardDataFlowRunner { TODO: 2. Describe causes in more cases: Warning caused by contract Warning caused by CustomMethodHandler + Warning caused by polyadic math + Warning caused by narrowing conversion + Warning caused by unary minus TODO: 3. Check how it works with Inliners (notably: Stream API) Ternary operators @@ -302,6 +310,7 @@ public class TrackingRunner extends StandardDataFlowRunner { @NotNull private static CauseItem[] findConstantValueCause(PsiExpression expression, MemoryStateChange history, Object expectedValue) { + if (expression instanceof PsiLiteralExpression) return new CauseItem[0]; Object constantExpressionValue = ExpressionUtils.computeConstantExpression(expression); DfaValue value = history.myTopOfStack; if (constantExpressionValue != null && constantExpressionValue.equals(expectedValue)) { @@ -351,6 +360,27 @@ public class TrackingRunner extends StandardDataFlowRunner { } } } + if (expression instanceof PsiPolyadicExpression) { + IElementType tokenType = ((PsiPolyadicExpression)expression).getOperationTokenType(); + boolean and = tokenType.equals(JavaTokenType.ANDAND); + if ((and || tokenType.equals(JavaTokenType.OROR)) && value != and) { + PsiExpression[] operands = ((PsiPolyadicExpression)expression).getOperands(); + for (int i = 0; i < operands.length; i++) { + PsiExpression operand = operands[i]; + operand = PsiUtil.skipParenthesizedExprDown(operand); + MemoryStateChange push = history.findExpressionPush(operand); + if (push != null && + ((push.myInstruction instanceof ConditionalGotoInstruction && + ((ConditionalGotoInstruction)push.myInstruction).isTarget(value, history.myInstruction)) || + (push.myTopOfStack instanceof DfaConstValue && + Boolean.valueOf(value).equals(((DfaConstValue)push.myTopOfStack).getValue())))) { + CauseItem cause = new CauseItem("Operand #" + (i + 1) + " of " + (and ? "&&" : "||") + "-chain is " + value, operand); + cause.addChildren(findBooleanResultCauses(operand, push, value)); + return new CauseItem[]{cause}; + } + } + } + } if (expression instanceof PsiBinaryExpression) { PsiBinaryExpression binOp = (PsiBinaryExpression)expression; RelationType relationType = @@ -366,6 +396,10 @@ public class TrackingRunner extends StandardDataFlowRunner { if (leftChange != null && rightChange != null) { DfaValue leftValue = leftChange.myTopOfStack; DfaValue rightValue = rightChange.myTopOfStack; + CauseItem[] causes = findRelationCause(relationType, leftChange, rightChange); + if (causes.length > 0) { + return causes; + } if (leftValue == rightValue && (leftValue instanceof DfaVariableValue || leftValue instanceof DfaConstValue)) { return new CauseItem[]{new CauseItem("Comparison arguments are the same", binOp.getOperationSign())}; @@ -375,7 +409,6 @@ public class TrackingRunner extends StandardDataFlowRunner { return new CauseItem[]{ new CauseItem("Comparison arguments are different constants", binOp.getOperationSign())}; } - return findRelationCause(relationType, leftChange, rightChange); } } } @@ -661,8 +694,8 @@ public class TrackingRunner extends StandardDataFlowRunner { (PsiType.LONG.equals(expression.getType()) || PsiType.INT.equals(expression.getType()))) { boolean isLong = PsiType.LONG.equals(expression.getType()); PsiBinaryExpression binOp = (PsiBinaryExpression)expression; - PsiExpression left = binOp.getLOperand(); - PsiExpression right = binOp.getROperand(); + PsiExpression left = PsiUtil.skipParenthesizedExprDown(binOp.getLOperand()); + PsiExpression right = PsiUtil.skipParenthesizedExprDown(binOp.getROperand()); MemoryStateChange leftPush = factUse.findExpressionPush(left); MemoryStateChange rightPush = factUse.findExpressionPush(right); if (leftPush != null && rightPush != null) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ConditionalGotoInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ConditionalGotoInstruction.java index 89fbdc01534d..6db46121328e 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ConditionalGotoInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ConditionalGotoInstruction.java @@ -43,6 +43,10 @@ public class ConditionalGotoInstruction extends BranchingInstruction implements return "IF_" + (isNegated() ? "NE" : "EQ") + " " + getOffset(); } + public boolean isTarget(boolean whenTrueOnStack, Instruction target) { + return target.getIndex() == (whenTrueOnStack == myIsNegated ? getIndex() + 1 : getOffset()); + } + @Override public int getOffset() { return myOffset.getInstructionOffset(); diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/AndChainCause.java b/java/java-tests/testData/inspection/dataFlow/tracker/AndChainCause.java new file mode 100644 index 000000000000..049802081148 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/AndChainCause.java @@ -0,0 +1,13 @@ +/* +Value is always false (x > 6 && y > 10) + Operand #1 of &&-chain is false (x > 6) + Left operand is 5 (x) + 'x' was assigned (5) + */ + +class Test { + void test(int y) { + int x = 5; + if(x > 6 && y > 10) {} + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/AndChainDependentCause.java b/java/java-tests/testData/inspection/dataFlow/tracker/AndChainDependentCause.java new file mode 100644 index 000000000000..0a7f9e9674f5 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/AndChainDependentCause.java @@ -0,0 +1,17 @@ +/* +Value is always false (a > 5 && b < 0 && b > a) + One of the following happens: + Operand #1 of &&-chain is false (a > 5) + Operand #2 of &&-chain is false (b < 0) + Operand #3 of &&-chain is false (b > a) + Left operand is <= -1 (b) + Range is known from line #15 (b < 0) + Right operand is >= 6 (a) + Range is known from line #15 (a > 5) + */ + +class Test { + void test(int a, int b) { + if(a > 5 && b < 0 && b > a) {} + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/AssignTrue.java b/java/java-tests/testData/inspection/dataFlow/tracker/AssignTrue.java new file mode 100644 index 000000000000..aeda8421d734 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/AssignTrue.java @@ -0,0 +1,11 @@ +/* +Value is always true (b) + 'b' was assigned (true) + */ + +class Test { + void test() { + boolean b = true; + if(b) {} + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/OrChainCause.java b/java/java-tests/testData/inspection/dataFlow/tracker/OrChainCause.java new file mode 100644 index 000000000000..be2e8428a7ce --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/OrChainCause.java @@ -0,0 +1,24 @@ +/* +Value is always true (x || (a+b)+(c+d)==10) + One of the following happens: + Operand #1 of ||-chain is true (x) + Operand #2 of ||-chain is true ((a+b)+(c+d)==10) + Result of '+' is 10 ((a+b)+(c+d)) + Result of '+' is 3 (a+b) + Left operand is 1 (a) + 'a' was assigned (1) + Right operand is 2 (b) + 'b' was assigned (2) + Result of '+' is 7 (c+d) + Left operand is 3 (c) + 'c' was assigned (3) + Right operand is 4 (d) + 'd' was assigned (4) + */ + +class Test { + void test(boolean x) { + int a = 1, b = 2, c = 3, d = 4; + if(x || (a+b)+(c+d)==10) {} + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java index 01663143d636..7354cee13933 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java @@ -137,4 +137,8 @@ public class DataFlowInspectionTrackerTest extends LightCodeInsightFixtureTestCa public void testWrongCastThreeStates() { doTest(); } public void testIfBothNotNull() { doTest(); } public void testIfBothNotNullReassign() { doTest(); } + public void testAssignTrue() { doTest(); } + public void testAndChainCause() { doTest(); } + public void testAndChainDependentCause() { doTest(); } + public void testOrChainCause() { doTest(); } }