From 76b64842153384012b584ea45fed1e2811647ed3 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 8 May 2019 13:32:49 +0700 Subject: [PATCH] IDEA-209947 Better contract support; special field equality support GitOrigin-RevId: 24777182250695b9465c705097b866f7772ed526 --- .../dataFlow/ContractValue.java | 12 ++ .../dataFlow/MethodContract.java | 31 +++- .../codeInspection/dataFlow/SpecialField.java | 2 + .../dataFlow/StandardMethodContract.java | 2 +- .../dataFlow/TrackingRunner.java | 150 +++++++++++------- .../dataFlow/inference/preContracts.kt | 4 +- .../dataFlow/value/DfaConstValue.java | 2 + .../dataFlow/tracker/EqualsContract.java | 18 +++ .../DataFlowInspectionTrackerTest.java | 1 + 9 files changed, 157 insertions(+), 65 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/tracker/EqualsContract.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java index 958c787e542d..6d74bc407f4d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractValue.java @@ -69,6 +69,10 @@ public abstract class ContractValue { return false; } + public ContractValue invert() { + return null; + } + /** * @return true if this contract value represents a bounds-checking condition */ @@ -377,9 +381,17 @@ public abstract class ContractValue { } } } + if (value instanceof Qualifier && call instanceof PsiMethodCallExpression) { + return ((PsiMethodCallExpression)call).getMethodExpression().getQualifierExpression(); + } return null; } + @Override + public ContractValue invert() { + return new Condition(myLeft, myRelationType.getNegated(), myRight); + } + @Override public String toString() { return myLeft + " " + myRelationType + " " + myRight; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java index a933665c067e..51cc547e0b45 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java @@ -16,7 +16,9 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInspection.dataFlow.value.DfaRelationValue.RelationType; +import org.jetbrains.annotations.NotNull; +import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -71,11 +73,16 @@ public abstract class MethodContract { }; } + @NotNull public static MethodContract singleConditionContract(ContractValue left, RelationType relationType, ContractValue right, ContractReturnValue returnValue) { - ContractValue condition = ContractValue.condition(left, relationType, right); + return singleConditionContract(ContractValue.condition(left, relationType, right), returnValue); + } + + @NotNull + private static MethodContract singleConditionContract(ContractValue condition, ContractReturnValue returnValue) { return new MethodContract(returnValue) { @Override String getArgumentsPresentation() { @@ -88,4 +95,26 @@ public abstract class MethodContract { } }; } + + public static List toNonIntersectingContracts(List contracts) { + if (contracts.size() == 1) return contracts; + if (contracts.stream().allMatch(StandardMethodContract.class::isInstance)) { + @SuppressWarnings("unchecked") List standardContracts = (List)contracts; + return StandardMethodContract.toNonIntersectingStandardContracts(standardContracts); + } + if (contracts.size() == 2 && contracts.get(1).isTrivial()) { + List result = new ArrayList<>(); + result.add(contracts.get(0)); + List conditions = contracts.get(0).getConditions(); + for (ContractValue condition : conditions) { + ContractValue inverted = condition.invert(); + if (inverted == null) { + return null; + } + result.add(singleConditionContract(inverted, contracts.get(1).getReturnValue())); + } + return result; + } + return null; + } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/SpecialField.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/SpecialField.java index 8c06878f994e..ee0b7876bec8 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/SpecialField.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/SpecialField.java @@ -354,8 +354,10 @@ public enum SpecialField implements VariableDescriptor { * @param type a qualifier type * @return a special field; null if no special field is available for given type */ + @Contract("null -> null") @Nullable public static SpecialField fromQualifierType(PsiType type) { + if (type == null) return null; for (SpecialField value : VALUES) { if (value.isMyQualifierType(type)) { return value; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardMethodContract.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardMethodContract.java index bccc61a107d4..a863daacc16f 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardMethodContract.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardMethodContract.java @@ -134,7 +134,7 @@ public final class StandardMethodContract extends MethodContract { * (e.g. contracts with different parameter count) */ @Nullable("When result is too big or contracts are erroneous") - public static List toNonIntersectingContracts(List contracts) { + public static List toNonIntersectingStandardContracts(List contracts) { if (contracts.isEmpty()) return contracts; int paramCount = contracts.get(0).getParameterCount(); List result = new ArrayList<>(); 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 fdcab93e4e72..db12be4fc2bf 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 @@ -594,8 +594,8 @@ public class TrackingRunner extends StandardDataFlowRunner { LongRangeSet fromRelation = rightRange.myFact.fromRelation(relationType.getNegated()); if (fromRelation != null && !fromRelation.intersects(leftRange.myFact)) { return new CauseItem[]{ - findRangeCause(leftChange, leftRange.myFact, "left operand is %s"), - findRangeCause(rightChange, rightRange.myFact, "right operand is %s")}; + findRangeCause(leftChange, leftValue, leftRange.myFact, "left operand is %s"), + findRangeCause(rightChange, rightValue, rightRange.myFact, "right operand is %s")}; } } if (leftValue instanceof DfaVariableValue) { @@ -627,6 +627,22 @@ public class TrackingRunner extends StandardDataFlowRunner { return new CauseItem[]{findRelationCause(change, (DfaVariableValue)rightValue, relation, leftChange)}; } } + if (relationType == RelationType.NE) { + SpecialField leftField = SpecialField.fromQualifierType(leftValue.getType()); + SpecialField rightField = SpecialField.fromQualifierType(leftValue.getType()); + if (leftField != null && leftField == rightField) { + DfaValue leftSpecial = leftField.createValue(getFactory(), leftValue); + DfaValue rightSpecial = rightField.createValue(getFactory(), rightValue); + CauseItem[] specialCause = findRelationCause(relationType, leftChange, leftSpecial, rightChange, rightSpecial); + if (specialCause.length > 0) { + CauseItem item = + new CauseItem("Values cannot be equal because " + leftValue + "." + leftField + " != " + rightValue + "." + rightField, + (PsiElement)null); + item.addChildren(specialCause); + return new CauseItem[]{item}; + } + } + } return new CauseItem[0]; } @@ -806,66 +822,75 @@ public class TrackingRunner extends StandardDataFlowRunner { private CauseItem fromCallContract(MemoryStateChange history, PsiCallExpression call, ContractReturnValue contractReturnValue) { PsiMethod method = call.resolveMethod(); if (method == null) return null; - List contracts = - ContainerUtil.filter(JavaMethodContractUtil.getMethodCallContracts(method, call), - mc -> contractReturnValue.isSuperValueOf(mc.getReturnValue())); - boolean explicit = JavaMethodContractUtil.hasExplicitContractAnnotation(method); + List contracts = JavaMethodContractUtil.getMethodCallContracts(method, call); + if (contracts.isEmpty()) return null; + MethodContract contract = contracts.get(0); + String contractType = JavaMethodContractUtil.hasExplicitContractAnnotation(method) ? "" : + contract instanceof StandardMethodContract ? "inferred " : + "hard-coded "; if (call instanceof PsiMethodCallExpression) { - String prefix = "according to " + (explicit ? "contract" : "inferred contract"); PsiReferenceExpression methodExpression = ((PsiMethodCallExpression)call).getMethodExpression(); String name = methodExpression.getReferenceName(); - for (MethodContract contract : contracts) { - if (contract.isTrivial()) { - return new CauseItem(prefix + - ", method '" + name + "' always returns '" + contract.getReturnValue() + "' value", - methodExpression.getReferenceNameElement()); - } + String prefix = "according to " + contractType + "contract, method '" + name + "'"; + if (contracts.size() == 1 && contract.isTrivial() && contractReturnValue.isSuperValueOf(contract.getReturnValue())) { + return new CauseItem(prefix + + " always returns '" + contract.getReturnValue() + "' value", + methodExpression.getReferenceNameElement()); } - if (contracts.size() == 1) { - List conditions = contracts.get(0).getConditions(); - String conditionsText = StringUtil.join(conditions, c -> c.getPresentationText(method), " and "); - CauseItem causeItem = new CauseItem( - prefix + ", method '" + name + "' returns '" + contracts.get(0).getReturnValue() + "' value when " + conditionsText, - methodExpression.getReferenceNameElement()); - for (ContractValue condition : conditions) { - DfaRelationValue relation = ObjectUtils.tryCast(condition.fromCall(getFactory(), call), DfaRelationValue.class); - PsiExpression leftPlace = condition.findLeftPlace(call); - MemoryStateChange leftPush = history.findExpressionPush(leftPlace); - PsiExpression rightPlace = condition.findRightPlace(call); - MemoryStateChange rightPush = history.findExpressionPush(rightPlace); - if (relation != null) { - DfaValue left = relation.getLeftOperand(); - DfaValue right = relation.getRightOperand(); - RelationType type = relation.getRelation(); - MemoryStateChange leftChange = history; - MemoryStateChange rightChange = history; - if (leftPush != null) { - if (leftPush.myTopOfStack == left) { - leftChange = leftPush; - } - else if (leftPush.myTopOfStack == right) { - rightChange = leftPush; - } - } - if (rightPush != null) { - if (rightPush.myTopOfStack == right) { - rightChange = rightPush; - } - else if (rightPush.myTopOfStack == left) { - leftChange = rightPush; - } - } - causeItem.addChildren(findRelationCause(type, leftChange, left, rightChange, right)); - } - } - return causeItem; + List nonIntersecting = MethodContract.toNonIntersectingContracts(contracts); + if (nonIntersecting != null) { + MethodContract onlyContract = ContainerUtil + .getOnlyItem(ContainerUtil.filter(nonIntersecting, mc -> contractReturnValue.isSuperValueOf(mc.getReturnValue()))); + return fromSingleContract(history, (PsiMethodCallExpression)call, method, prefix, onlyContract); } } return null; } - private static CauseItem findRangeCause(MemoryStateChange factUse, LongRangeSet range, String template) { - DfaValue value = factUse.myTopOfStack; + @Nullable + private CauseItem fromSingleContract(MemoryStateChange history, PsiMethodCallExpression call, + PsiMethod method, String prefix, MethodContract contract) { + if (contract == null) return null; + List conditions = contract.getConditions(); + String conditionsText = StringUtil.join(conditions, c -> c.getPresentationText(method), " and "); + CauseItem causeItem = new CauseItem( + prefix + " returns '" + contract.getReturnValue() + "' value when " + conditionsText, + call.getMethodExpression().getReferenceNameElement()); + for (ContractValue condition : conditions) { + DfaRelationValue relation = ObjectUtils.tryCast(condition.fromCall(getFactory(), call), DfaRelationValue.class); + PsiExpression leftPlace = condition.findLeftPlace(call); + MemoryStateChange leftPush = history.findExpressionPush(leftPlace); + PsiExpression rightPlace = condition.findRightPlace(call); + MemoryStateChange rightPush = history.findExpressionPush(rightPlace); + if (relation != null) { + DfaValue left = relation.getLeftOperand(); + DfaValue right = relation.getRightOperand(); + RelationType type = relation.getRelation(); + MemoryStateChange leftChange = history; + MemoryStateChange rightChange = history; + if (leftPush != null) { + if (leftPush.myTopOfStack == left) { + leftChange = leftPush; + } + else if (leftPush.myTopOfStack == right) { + rightChange = leftPush; + } + } + if (rightPush != null) { + if (rightPush.myTopOfStack == right) { + rightChange = rightPush; + } + else if (rightPush.myTopOfStack == left) { + leftChange = rightPush; + } + } + causeItem.addChildren(findRelationCause(type, leftChange, left, rightChange, right)); + } + } + return causeItem; + } + + private static CauseItem findRangeCause(MemoryStateChange factUse, DfaValue value, LongRangeSet range, String template) { if (value instanceof DfaVariableValue) { VariableDescriptor descriptor = ((DfaVariableValue)value).getDescriptor(); if (descriptor instanceof SpecialField && range.equals(LongRangeSet.indexRange())) { @@ -880,7 +905,7 @@ public class TrackingRunner extends StandardDataFlowRunner { } } } - PsiExpression expression = factUse.getExpression(); + PsiExpression expression = factUse.myTopOfStack == value ? factUse.getExpression() : null; if (expression != null) { PsiType type = expression.getType(); if (expression instanceof PsiLiteralExpression) { @@ -901,7 +926,8 @@ public class TrackingRunner extends StandardDataFlowRunner { PsiExpression operand = ((PsiTypeCastExpression)expression).getOperand(); MemoryStateChange operandPush = factUse.findExpressionPush(operand); if (operandPush != null) { - FactDefinition operandInfo = operandPush.findFact(operandPush.myTopOfStack, DfaFactType.RANGE); + DfaValue castedValue = operandPush.myTopOfStack; + FactDefinition operandInfo = operandPush.findFact(castedValue, DfaFactType.RANGE); LongRangeSet operandRange = operandInfo.myFact == null ? LongRangeSet.fromType(type) : operandInfo.myFact; if (operandRange != null) { LongRangeSet result = operandRange.castTo((PsiPrimitiveType)type); @@ -909,7 +935,7 @@ public class TrackingRunner extends StandardDataFlowRunner { CauseItem cause = new CauseItem("result of '(" + type.getCanonicalText() + ")' cast is " + range.getPresentationText(null), expression); if (!operandRange.equals(LongRangeSet.fromType(operand.getType()))) { - cause.addChildren(findRangeCause(operandPush, operandRange, "cast operand is %s")); + cause.addChildren(findRangeCause(operandPush, castedValue, operandRange, "cast operand is %s")); } return cause; } @@ -928,8 +954,10 @@ public class TrackingRunner extends StandardDataFlowRunner { MemoryStateChange leftPush = factUse.findExpressionPush(left); MemoryStateChange rightPush = factUse.findExpressionPush(right); if (leftPush != null && rightPush != null) { - FactDefinition leftSet = leftPush.findFact(leftPush.myTopOfStack, DfaFactType.RANGE); - FactDefinition rightSet = rightPush.findFact(rightPush.myTopOfStack, DfaFactType.RANGE); + DfaValue leftVal = leftPush.myTopOfStack; + FactDefinition leftSet = leftPush.findFact(leftVal, DfaFactType.RANGE); + DfaValue rightVal = rightPush.myTopOfStack; + FactDefinition rightSet = rightPush.findFact(rightVal, DfaFactType.RANGE); LongRangeSet fromType = Objects.requireNonNull(LongRangeSet.fromType(type)); LongRangeSet leftRange = leftSet.getFact(fromType); LongRangeSet rightRange = rightSet.getFact(fromType); @@ -939,10 +967,10 @@ public class TrackingRunner extends StandardDataFlowRunner { "' is " + range.getPresentationText(type), factUse); CauseItem leftCause = null, rightCause = null; if (!leftRange.equals(fromType)) { - leftCause = findRangeCause(leftPush, leftRange, "left operand is %s"); + leftCause = findRangeCause(leftPush, leftVal, leftRange, "left operand is %s"); } if (!rightRange.equals(fromType)) { - rightCause = findRangeCause(rightPush, rightRange, "right operand is %s"); + rightCause = findRangeCause(rightPush, rightVal, rightRange, "right operand is %s"); } cause.addChildren(leftCause, rightCause); return cause; @@ -962,7 +990,7 @@ public class TrackingRunner extends StandardDataFlowRunner { MemoryStateChange rValuePush = factDef.findSubExpressionPush(rExpression); if (rValuePush != null) { CauseItem assignmentItem = createAssignmentCause((AssignInstruction)factDef.myInstruction, value); - assignmentItem.addChildren(findRangeCause(rValuePush, range, "Value is %s")); + assignmentItem.addChildren(findRangeCause(rValuePush, rValuePush.myTopOfStack, range, "Value is %s")); item.addChildren(assignmentItem); return item; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/preContracts.kt b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/preContracts.kt index d4055c9250c5..48615b148ac2 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/preContracts.kt +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inference/preContracts.kt @@ -38,7 +38,7 @@ internal data class DelegationContract(internal val expression: ExpressionRange, val arguments = call.argumentList.expressions val varArgCall = MethodCallInstruction.isVarArgCall(targetMethod, result.substitutor, arguments, parameters) - val methodContracts = StandardMethodContract.toNonIntersectingContracts(JavaMethodContractUtil.getMethodContracts(targetMethod)) + val methodContracts = StandardMethodContract.toNonIntersectingStandardContracts(JavaMethodContractUtil.getMethodContracts(targetMethod)) ?: return emptyList() var fromDelegate = methodContracts.mapNotNull { dc -> convertDelegatedMethodContract(method, parameters, arguments, varArgCall, dc) @@ -47,7 +47,7 @@ internal data class DelegationContract(internal val expression: ExpressionRange, fromDelegate = fromDelegate.map(this::returnNotNull) + listOf( StandardMethodContract(emptyConstraints(method), ContractReturnValue.returnNotNull())) } - return StandardMethodContract.toNonIntersectingContracts(fromDelegate) ?: emptyList() + return StandardMethodContract.toNonIntersectingStandardContracts(fromDelegate) ?: emptyList() } private fun convertDelegatedMethodContract(callerMethod: PsiMethod, diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java index 3ec5622c99ec..f654c67e7e2d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java @@ -3,6 +3,7 @@ package com.intellij.codeInspection.dataFlow.value; import com.intellij.codeInspection.dataFlow.DfaUtil; +import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; import com.intellij.psi.util.PsiTypesUtil; import com.intellij.psi.util.PsiUtil; @@ -152,6 +153,7 @@ public class DfaConstValue extends DfaValue { public String toString() { if (myValue == null) return "null"; + if (myValue instanceof String) return '"' + StringUtil.escapeStringCharacters((String)myValue) + '"'; return myValue.toString(); } diff --git a/java/java-tests/testData/inspection/dataFlow/tracker/EqualsContract.java b/java/java-tests/testData/inspection/dataFlow/tracker/EqualsContract.java new file mode 100644 index 000000000000..78cf4e1ded1d --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/tracker/EqualsContract.java @@ -0,0 +1,18 @@ +/* +Value is always false (s.equals("."); line#14) + According to hard-coded contract, method 'equals' returns 'false' value when this != parameter (equals; line#14) + Values cannot be equal because s.length != ".".length + Left operand is in {2..Integer.MAX_VALUE} (s; line#14) + Range is known from line #14 (s.startsWith("--"); line#14) + and right operand is 1 ("."; line#14) + */ + +import java.util.Objects; + +class Test { + void test(String s) { + if (s.startsWith("--") && s.equals(".")) { + + } + } +} \ 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 85a0863679ad..4ae1409db0c8 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTrackerTest.java @@ -163,4 +163,5 @@ public class DataFlowInspectionTrackerTest extends LightCodeInsightFixtureTestCa public void testNumericWidening() { doTest(); } public void testSimpleContract() { doTest(); } public void testSimpleContract2() { doTest(); } + public void testEqualsContract() { doTest(); } }