From c11388a5348af05b0f1a19ddab6f66b724ad3dca Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 26 Apr 2019 09:54:00 +0700 Subject: [PATCH] IDEA-209947 Explain CCE warnings; merge explanations from different states GitOrigin-RevId: b83e85b4712257fd536bca806790e2878bbf89a8 --- .../dataFlow/DataFlowInspectionBase.java | 5 +- .../dataFlow/TrackingRunner.java | 79 ++++++++++++++++--- .../dataFlow/tracker/IfBothNotNull.java | 22 ++++++ .../tracker/IfBothNotNullReassign.java | 24 ++++++ .../dataFlow/tracker/WrongCastSimple.java | 13 +++ .../tracker/WrongCastThreeStates.java | 18 +++++ .../dataFlow/tracker/WrongCastTwoStates.java | 14 ++++ .../DataFlowInspectionTrackerTest.java | 27 +++++-- 8 files changed, 184 insertions(+), 18 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNull.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNullReassign.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/WrongCastSimple.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/WrongCastThreeStates.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/WrongCastTwoStates.java 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 2c21a3b6e65c..d88a2184802a 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 @@ -682,14 +682,15 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool reporter.registerProblem(toHighlight, message, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); } - private static void reportFailingCasts(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { + private void reportFailingCasts(ProblemReporter reporter, DataFlowInstructionVisitor visitor) { for (TypeCastInstruction instruction : visitor.getClassCastExceptionInstructions()) { PsiTypeCastExpression typeCast = instruction.getExpression(); PsiExpression operand = typeCast.getOperand(); PsiTypeElement castType = typeCast.getCastType(); assert castType != null; assert operand != null; - reporter.registerProblem(castType, InspectionsBundle.message("dataflow.message.cce", operand.getText())); + reporter.registerProblem(castType, InspectionsBundle.message("dataflow.message.cce", operand.getText()), + createExplainFix(typeCast, new TrackingRunner.CastDfaProblemType())); } } 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 afe5ebc6722b..d9ba0f5ae04a 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 @@ -75,14 +75,17 @@ public class TrackingRunner extends StandardDataFlowRunner { StandardInstructionVisitor visitor = new StandardInstructionVisitor(); RunnerResult result = runner.analyzeMethodRecursively(body, visitor, ignoreAssertions); if (result != RunnerResult.OK) return Collections.emptyList(); - List results = new ArrayList<>(); + CauseItem cause = null; for (MemoryStateChange history : runner.myHistoryForContext) { CauseItem root = findCauseChain(expression, history, type); - if (!results.contains(root)) { - results.add(root); + if (cause == null) { + cause = root; + } else { + cause = cause.merge(root); + if (cause == null) return Collections.emptyList(); } } - return results; + return Collections.singletonList(cause); } public abstract static class DfaProblemType { @@ -182,7 +185,59 @@ public class TrackingRunner extends StandardDataFlowRunner { public String toString() { return myProblem.toString().replaceFirst("#ref$", "here"); } + + public CauseItem merge(CauseItem other) { + if (this.equals(other)) return this; + if (Objects.equals(this.myTarget, other.myTarget) && this.myProblem.toString().equals(other.myProblem.toString())) { + if(tryMergeChildren(other.myChildren)) return this; + if(other.tryMergeChildren(this.myChildren)) return other; + } + return null; + } + + private boolean tryMergeChildren(List children) { + if (myChildren.isEmpty()) return false; + if (myChildren.size() != 1 || !(myChildren.get(0).myProblem instanceof PossibleExecutionDfaProblemType)) { + if (children.size() == myChildren.size()) { + List merged = StreamEx.zip(myChildren, children, CauseItem::merge).toList(); + if (!merged.contains(null)) { + myChildren.clear(); + myChildren.addAll(merged); + return true; + } + } + insertIntoHierarchy(new CauseItem(new PossibleExecutionDfaProblemType(), (PsiElement)null)); + } + CauseItem mergePoint = myChildren.get(0); + if (children.isEmpty()) { + ((PossibleExecutionDfaProblemType)mergePoint.myProblem).myComplete = false; + } + mergePoint.myChildren.addAll(children); + return true; + } + + private void insertIntoHierarchy(CauseItem intermediate) { + intermediate.myChildren.addAll(myChildren); + myChildren.clear(); + myChildren.add(intermediate); + } } + + public static class CastDfaProblemType extends DfaProblemType { + public String toString() { + return "Cast may fail"; + } + } + + static class PossibleExecutionDfaProblemType extends DfaProblemType { + boolean myComplete = true; + + @Override + public String toString() { + return myComplete ? "One of the following happens:" : "An execution might exist where..."; + } + } + public static class ValueDfaProblemType extends DfaProblemType { final Object myValue; @@ -215,7 +270,6 @@ public class TrackingRunner extends StandardDataFlowRunner { Cause for possible NPE Cause for AIOOBE Cause for "Contract always fails" - Cause for possible CCE Cause for "modifying an immutable collection" Cause for "Collection is always empty" (separate inspection now) TODO: 2. Describe causes in more cases: @@ -236,6 +290,13 @@ public class TrackingRunner extends StandardDataFlowRunner { CauseItem[] causes = findConstantValueCause(expression, history, expectedValue); root.addChildren(causes); } + if (type instanceof CastDfaProblemType && expression instanceof PsiTypeCastExpression) { + PsiType expressionType = expression.getType(); + MemoryStateChange operandPush = history.findExpressionPush(((PsiTypeCastExpression)expression).getOperand()); + if (operandPush != null) { + root.addChildren(findTypeCause(operandPush, expressionType, false)); + } + } return root; } @@ -335,8 +396,8 @@ public class TrackingRunner extends StandardDataFlowRunner { PsiTypeElement typeElement = instanceOfExpression.getCheckType(); if (typeElement != null) { PsiType type = typeElement.getType(); - CauseItem[] causeItem = findTypeCause(operandHistory, type, value); - if (causeItem != null) return causeItem; + CauseItem causeItem = findTypeCause(operandHistory, type, value); + if (causeItem != null) return new CauseItem[]{causeItem}; } } } @@ -344,7 +405,7 @@ public class TrackingRunner extends StandardDataFlowRunner { } @Nullable - private static CauseItem[] findTypeCause(MemoryStateChange operandHistory, PsiType type, boolean isInstance) { + private static CauseItem findTypeCause(MemoryStateChange operandHistory, PsiType type, boolean isInstance) { PsiExpression operand = Objects.requireNonNull(operandHistory.getExpression()); DfaValue operandValue = operandHistory.myTopOfStack; DfaPsiType wanted = operandValue.getFactory().createDfaType(type); @@ -362,7 +423,7 @@ public class TrackingRunner extends StandardDataFlowRunner { if (prevExplanation == null) { CauseItem causeItem = new CauseItem(explanation, operand); causeItem.addChildren(new CauseItem("Type of '" + operand.getText() + "' is known from #ref", causeLocation)); - return new CauseItem[]{causeItem}; + return causeItem; } explanation = prevExplanation; } diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNull.java b/java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNull.java new file mode 100644 index 000000000000..e3731ec6e279 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNull.java @@ -0,0 +1,22 @@ +/* +Value is always false (x == null) + One of the following happens: + 'x' was assigned (new Object()) + Expression cannot be null as it's newly created object (new Object()) + 'x' was assigned ("foo") + Expression cannot be null as it's literal ("foo") + */ + +class Test { + void test(boolean b) { + Object x; + if (b) { + x = new Object(); + } else { + x = "foo"; + } + if (x == null) { + + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNullReassign.java b/java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNullReassign.java new file mode 100644 index 000000000000..92230f58d3a2 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/IfBothNotNullReassign.java @@ -0,0 +1,24 @@ +/* +Value is always false (y == null) + 'y' was assigned (x) + One of the following happens: + 'x' was assigned (new Object()) + Expression cannot be null as it's newly created object (new Object()) + 'x' was assigned ("foo") + Expression cannot be null as it's literal ("foo") + */ + +class Test { + void test(boolean b) { + Object x; + if (b) { + x = new Object(); + } else { + x = "foo"; + } + Object y = x; + if (y == null) { + + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastSimple.java b/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastSimple.java new file mode 100644 index 000000000000..6a1a25bc04da --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastSimple.java @@ -0,0 +1,13 @@ +/* +Cast may fail ((Integer)x) + An object type is exactly String which is not a subtype of Integer (x) + Type of 'x' is known from line #9 (x instanceof String) + */ + +class Test { + void test(Object x) { + if (x instanceof String) { + System.out.println((Integer)x); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastThreeStates.java b/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastThreeStates.java new file mode 100644 index 000000000000..011a034add28 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastThreeStates.java @@ -0,0 +1,18 @@ +/* +Cast may fail ((Integer)x) + An execution might exist where... + An object type is exactly Double which is not a subtype of Integer (x) + Type of 'x' is known from line #14 (x instanceof Double) + An object type is exactly String which is not a subtype of Integer (x) + Type of 'x' is known from line #12 (x instanceof String) + */ + +class Test { + void test(Object x) { + if (x instanceof String) { + } + if (x instanceof Double) { + } + System.out.println((Integer)x); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastTwoStates.java b/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastTwoStates.java new file mode 100644 index 000000000000..9a2acf2bebe7 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/WrongCastTwoStates.java @@ -0,0 +1,14 @@ +/* +Cast may fail ((Integer)x) + An execution might exist where... + An object type is exactly String which is not a subtype of Integer (x) + Type of 'x' is known from line #10 (x instanceof String) + */ + +class Test { + void test(Object x) { + if (x instanceof String) { + } + System.out.println((Integer)x); + } +} \ 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 e5a4e436fb82..01663143d636 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java @@ -54,13 +54,8 @@ public class DataFlowInspectionTrackerTest extends LightCodeInsightFixtureTestCa assertNotNull("Failed to find element at selection: " + selectedText, element); assertTrue("Selected element is not an expression: " + selectedText, element instanceof PsiExpression); PsiExpression expression = (PsiExpression)element; - CommonDataflow.DataflowResult result = CommonDataflow.getDataflowResult(expression); - assertNotNull("No common dataflow result for expression: " + selectedText, result); - Set values = result.getExpressionValues(expression); - assertEquals("No single value for expression: "+selectedText, 1, values.size()); - Object singleValue = values.iterator().next(); - List items = TrackingRunner.findProblemCause( - true, false, expression, new TrackingRunner.ValueDfaProblemType(singleValue)); + TrackingRunner.DfaProblemType problemType = getProblemType(selectedText, expression); + List items = TrackingRunner.findProblemCause(true, false, expression, problemType); String dump = StreamEx.of(items).map(item -> item.dump(getEditor().getDocument())).joining("\n----\n"); PsiComment firstComment = PsiTreeUtil.findChildOfType(file, PsiComment.class); if (firstComment == null) { @@ -94,6 +89,19 @@ public class DataFlowInspectionTrackerTest extends LightCodeInsightFixtureTestCa assertEquals(text, actual); } + @NotNull + private static TrackingRunner.DfaProblemType getProblemType(String selectedText, PsiExpression expression) { + if (expression instanceof PsiTypeCastExpression) { + return new TrackingRunner.CastDfaProblemType(); + } + CommonDataflow.DataflowResult result = CommonDataflow.getDataflowResult(expression); + assertNotNull("No common dataflow result for expression: " + selectedText, result); + Set values = result.getExpressionValues(expression); + assertEquals("No single value for expression: "+selectedText, 1, values.size()); + Object singleValue = values.iterator().next(); + return new TrackingRunner.ValueDfaProblemType(singleValue); + } + public void testDitto() { doTest(); } public void testConstants() { doTest(); } public void testSimpleDeref() { doTest(); } @@ -124,4 +132,9 @@ public class DataFlowInspectionTrackerTest extends LightCodeInsightFixtureTestCa public void testNotNullObvious() { doTest(); } public void testNotNullAssignmentInside() { doTest(); } public void testIndexOfPlusOne() { doTest(); } + public void testWrongCastSimple() { doTest(); } + public void testWrongCastTwoStates() { doTest(); } + public void testWrongCastThreeStates() { doTest(); } + public void testIfBothNotNull() { doTest(); } + public void testIfBothNotNullReassign() { doTest(); } }