From e12a1970e4be3b8529de8fe5202f5ab837d294ae Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Thu, 25 Apr 2019 15:22:44 +0700 Subject: [PATCH] IDEA-209947 Report obviously not-null values; better propagation through assignment GitOrigin-RevId: 2f6cd06b9750e8f080ab543af40b522f4c2bfefa --- .../dataFlow/TrackingDfaMemoryState.java | 10 ++-- .../dataFlow/TrackingRunner.java | 50 +++++++++++++++---- .../ObviousNullCheckInspection.java | 22 ++------ .../dataFlow/tracker/IndexOfPlusOne.java | 19 +++++++ .../tracker/NotNullAssignmentInside.java | 12 +++++ .../dataFlow/tracker/NotNullObvious.java | 12 +++++ .../DataFlowInspectionTrackerTest.java | 3 ++ 7 files changed, 95 insertions(+), 33 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/IndexOfPlusOne.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/NotNullAssignmentInside.java create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/NotNullObvious.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingDfaMemoryState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingDfaMemoryState.java index 9d8860915ad3..c896c5bfc981 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingDfaMemoryState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/TrackingDfaMemoryState.java @@ -241,14 +241,14 @@ public class TrackingDfaMemoryState extends DfaMemoryStateImpl { @Nullable MemoryStateChange findExpressionPush(@Nullable PsiExpression expression) { if (expression == null) return null; - return findChange(change -> change.getExpression() == expression); + return findChange(change -> change.getExpression() == expression, false); } - MemoryStateChange findRelation(DfaVariableValue value, @NotNull Predicate relationPredicate) { + MemoryStateChange findRelation(DfaVariableValue value, @NotNull Predicate relationPredicate, boolean startFromSelf) { return findChange(change -> { Change varChange = change.myChanges.get(value); return varChange != null && varChange.myAddedRelations.stream().anyMatch(relationPredicate); - }); + }, startFromSelf); } @NotNull @@ -272,8 +272,8 @@ public class TrackingDfaMemoryState extends DfaMemoryStateImpl { } @Nullable - private MemoryStateChange findChange(@NotNull Predicate predicate) { - for (MemoryStateChange change = myPrevious; change != null; change = change.myPrevious) { + private MemoryStateChange findChange(@NotNull Predicate predicate, boolean startFromSelf) { + for (MemoryStateChange change = startFromSelf ? this : myPrevious; change != null; change = change.myPrevious) { if (predicate.test(change)) { return change; } 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 df9c818918d4..afe5ebc6722b 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 @@ -10,11 +10,13 @@ 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.editor.Document; +import com.intellij.openapi.progress.ProgressManager; import com.intellij.openapi.util.Pair; 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.util.PsiUtil; import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; import com.intellij.util.containers.ContainerUtil; @@ -219,7 +221,6 @@ public class TrackingRunner extends StandardDataFlowRunner { TODO: 2. Describe causes in more cases: Warning caused by contract Warning caused by CustomMethodHandler - Warning caused by mismatched type constraints TODO: 3. Check how it works with Inliners (notably: Stream API) Ternary operators @@ -254,7 +255,7 @@ public class TrackingRunner extends StandardDataFlowRunner { if (value instanceof DfaVariableValue) { MemoryStateChange change = history.findRelation( (DfaVariableValue)value, rel -> rel.myRelationType == RelationType.EQ && rel.myCounterpart instanceof DfaConstValue && - Objects.equals(expectedValue, ((DfaConstValue)rel.myCounterpart).getValue())); + Objects.equals(expectedValue, ((DfaConstValue)rel.myCounterpart).getValue()), false); if (change != null) { PsiExpression varSourceExpression = change.getExpression(); Instruction instruction = change.myInstruction; @@ -297,8 +298,8 @@ public class TrackingRunner extends StandardDataFlowRunner { if (!value) { relationType = relationType.getNegated(); } - PsiExpression leftOperand = binOp.getLOperand(); - PsiExpression rightOperand = binOp.getROperand(); + PsiExpression leftOperand = PsiUtil.skipParenthesizedExprDown(binOp.getLOperand()); + PsiExpression rightOperand = PsiUtil.skipParenthesizedExprDown(binOp.getROperand()); MemoryStateChange leftChange = history.findExpressionPush(leftOperand); MemoryStateChange rightChange = history.findExpressionPush(rightOperand); if (leftChange != null && rightChange != null) { @@ -372,6 +373,7 @@ public class TrackingRunner extends StandardDataFlowRunner { private static CauseItem[] findRelationCause(RelationType relationType, MemoryStateChange leftChange, MemoryStateChange rightChange) { + ProgressManager.checkCanceled(); DfaValue leftValue = leftChange.myTopOfStack; DfaValue rightValue = rightChange.myTopOfStack; if (leftValue instanceof DfaVariableValue) { @@ -443,7 +445,7 @@ public class TrackingRunner extends StandardDataFlowRunner { } if (instruction instanceof AssignInstruction) { DfaValue target = change.myTopOfStack; - PsiExpression rValue = ((AssignInstruction)instruction).getRExpression(); + PsiExpression rValue = PsiUtil.skipParenthesizedExprDown(((AssignInstruction)instruction).getRExpression()); if (target == value) { CauseItem item = new CauseItem("'" + target + "' was assigned", rValue); MemoryStateChange rValuePush = change.findExpressionPush(rValue); @@ -482,9 +484,7 @@ public class TrackingRunner extends StandardDataFlowRunner { if (factDef != null && expression != null) { PsiExpression defExpression = factDef.getExpression(); if (defExpression != null) { - return new CauseItem( - new CustomDfaProblemType(expression.getText() + " is known to be '" + nullability.getPresentationName() + "' from #ref"), - defExpression); + return new CauseItem(expression.getText() + " is known to be '" + nullability.getPresentationName() + "' from #ref", defExpression); } } if (expression instanceof PsiMethodCallExpression) { @@ -504,6 +504,12 @@ public class TrackingRunner extends StandardDataFlowRunner { return fromMemberNullability(nullability, variable, "Variable", ((PsiReferenceExpression)expression).getReferenceNameElement()); } } + if (nullability == DfaNullability.NOT_NULL) { + String explanation = getObviouslyNonNullExplanation(expression); + if (explanation != null) { + return new CauseItem("Expression cannot be null as it's " + explanation, expression); + } + } return null; } @@ -630,6 +636,19 @@ public class TrackingRunner extends StandardDataFlowRunner { String rangeText = range.getPresentationText(expression != null ? expression.getType() : null); CauseItem item = new CauseItem(String.format(template, rangeText), factUse); if (factDef != null) { + if (factDef.myInstruction instanceof AssignInstruction && factDef.myTopOfStack == value) { + PsiExpression rExpression = PsiUtil.skipParenthesizedExprDown(((AssignInstruction)factDef.myInstruction).getRExpression()); + if (rExpression != null) { + MemoryStateChange rValuePush = factDef.findExpressionPush(rExpression); + if (rValuePush != null) { + CauseItem assignmentItem = new CauseItem("'" + value + "' was assigned", rExpression); + Pair rValueFact = rValuePush.findFact(rValuePush.myTopOfStack, DfaFactType.RANGE); + assignmentItem.addChildren(findRangeCause(rValuePush, rValueFact.first, range, "Value is %s")); + item.addChildren(assignmentItem); + return item; + } + } + } PsiExpression defExpression = factDef.getExpression(); if (defExpression != null) { item.addChildren(new CauseItem("Range is known from #ref", defExpression)); @@ -638,6 +657,19 @@ public class TrackingRunner extends StandardDataFlowRunner { return item; } + @Nullable + public static String getObviouslyNonNullExplanation(PsiExpression arg) { + if (arg == null || ExpressionUtils.isNullLiteral(arg)) return null; + if (arg instanceof PsiNewExpression) return "newly created object"; + if (arg instanceof PsiLiteralExpression) return "literal"; + if (arg.getType() instanceof PsiPrimitiveType) return "a value of primitive type '" + arg.getType().getCanonicalText() + "'"; + if (arg instanceof PsiPolyadicExpression && ((PsiPolyadicExpression)arg).getOperationTokenType() == JavaTokenType.PLUS) { + return "concatenation"; + } + if (arg instanceof PsiThisExpression) return "'this' object"; + return null; + } + private static MemoryStateChange findRelationAddedChange(MemoryStateChange history, DfaVariableValue var, Relation relation) { List subRelations; switch (relation.myRelationType) { @@ -656,6 +688,6 @@ public class TrackingRunner extends StandardDataFlowRunner { default: subRelations = Collections.singletonList(relation); } - return history.findRelation(var, subRelations::contains); + return history.findRelation(var, subRelations::contains, true); } } diff --git a/java/java-impl/src/com/intellij/codeInspection/ObviousNullCheckInspection.java b/java/java-impl/src/com/intellij/codeInspection/ObviousNullCheckInspection.java index 447288d0bd95..79aa635bce48 100644 --- a/java/java-impl/src/com/intellij/codeInspection/ObviousNullCheckInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/ObviousNullCheckInspection.java @@ -2,11 +2,8 @@ package com.intellij.codeInspection; import com.intellij.codeInsight.BlockUtils; -import com.intellij.codeInspection.dataFlow.ContractReturnValue; +import com.intellij.codeInspection.dataFlow.*; import com.intellij.codeInspection.dataFlow.ContractReturnValue.ParameterReturnValue; -import com.intellij.codeInspection.dataFlow.ContractValue; -import com.intellij.codeInspection.dataFlow.JavaMethodContractUtil; -import com.intellij.codeInspection.dataFlow.MethodContract; import com.intellij.openapi.project.Project; import com.intellij.psi.*; import com.intellij.psi.util.PsiTreeUtil; @@ -34,13 +31,13 @@ public class ObviousNullCheckInspection extends AbstractBaseJavaLocalInspectionT PsiExpression[] args = call.getArgumentList().getExpressions(); // Avoid method resolve if no argument is a candidate for obvious non-null warning // (checking this is easier than resolving and calls without arguments are excluded at all) - if (!ContainerUtil.exists(args, arg -> getObviouslyNonNullExplanation(PsiUtil.skipParenthesizedExprDown(arg)) != null)) return; + if (!ContainerUtil.exists(args, arg -> TrackingRunner.getObviouslyNonNullExplanation(PsiUtil.skipParenthesizedExprDown(arg)) != null)) return; NullCheckParameter nullCheckParameter = NullCheckParameter.fromCall(call); if (nullCheckParameter == null) return; if (!ExpressionUtils.isVoidContext(call) && !nullCheckParameter.myReturnsParameter) return; if (args.length <= nullCheckParameter.myIndex) return; PsiExpression nullArg = PsiUtil.skipParenthesizedExprDown(args[nullCheckParameter.myIndex]); - String explanation = getObviouslyNonNullExplanation(nullArg); + String explanation = TrackingRunner.getObviouslyNonNullExplanation(nullArg); if (explanation == null) return; if(nullCheckParameter.myNull) { holder.registerProblem(nullArg, InspectionsBundle.message("inspection.redundant.null.check.always.fail.message", explanation)); @@ -53,19 +50,6 @@ public class ObviousNullCheckInspection extends AbstractBaseJavaLocalInspectionT }; } - @Nullable - private static String getObviouslyNonNullExplanation(PsiExpression arg) { - if (arg == null || ExpressionUtils.isNullLiteral(arg)) return null; - if (arg instanceof PsiNewExpression) return "newly created object"; - if (arg instanceof PsiLiteralExpression) return "literal"; - if (arg.getType() instanceof PsiPrimitiveType) return "a value of primitive type '" + arg.getType().getCanonicalText() + "'"; - if (arg instanceof PsiPolyadicExpression && ((PsiPolyadicExpression)arg).getOperationTokenType() == JavaTokenType.PLUS) { - return "concatenation"; - } - if (arg instanceof PsiThisExpression) return "'this' object"; - return null; - } - static class NullCheckParameter { int myIndex; boolean myNull; diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/IndexOfPlusOne.java b/java/java-tests/testData/inspection/dataFlow/tracker/IndexOfPlusOne.java new file mode 100644 index 000000000000..723134684363 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/IndexOfPlusOne.java @@ -0,0 +1,19 @@ +/* +Value is always true (j >= 0) + Left operand is >= 0 (j) + 'j' was assigned (i + 1) + Result of '+' is >= 0 (i + 1) + Left operand is in {-1..Integer.MAX_VALUE-1} (i) + 'i' was assigned (s.indexOf(' ')) + Value is in {-1..Integer.MAX_VALUE-1} (s.indexOf(' ')) + */ + +class Test { + private static void dosmth(String s) { + int i = s.indexOf(' '); + int j = i + 1; + if (j >= 0) { + + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/NotNullAssignmentInside.java b/java/java-tests/testData/inspection/dataFlow/tracker/NotNullAssignmentInside.java new file mode 100644 index 000000000000..dfd2f8826c6f --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/NotNullAssignmentInside.java @@ -0,0 +1,12 @@ +/* +Value is always true ((s = new Object()) != null) + 's' was assigned (new Object()) + Expression cannot be null as it's newly created object (new Object()) + */ + +class Test { + void test() { + Object s; + if ((s = new Object()) != null) {} + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/NotNullObvious.java b/java/java-tests/testData/inspection/dataFlow/tracker/NotNullObvious.java new file mode 100644 index 000000000000..f04337b8c62b --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/NotNullObvious.java @@ -0,0 +1,12 @@ +/* +Value is always true (s != null) + 's' was assigned (new Object()) + Expression cannot be null as it's newly created object (new Object()) + */ + +class Test { + void test() { + Object s = (new Object()); + if (s != null) {} + } +} \ 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 45538a75ee43..e5a4e436fb82 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java @@ -121,4 +121,7 @@ public class DataFlowInspectionTrackerTest extends LightCodeInsightFixtureTestCa public void testInstanceOfChain2() { doTest(); } public void testNotInstanceOf() { doTest(); } public void testInstanceOfPreviousCast() { doTest(); } + public void testNotNullObvious() { doTest(); } + public void testNotNullAssignmentInside() { doTest(); } + public void testIndexOfPlusOne() { doTest(); } }