From b4d16557b379ea9fd790b6aa53558a5c3a08d7ee Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 22 Jun 2018 17:06:06 +0700 Subject: [PATCH] DFA: better equals handling --- .../dataFlow/DataFlowInspectionBase.java | 13 ++++ .../dataFlow/HardcodedContracts.java | 61 ++++++++++++------- .../fixture/EqualsInLoopNotTooComplex.java | 29 +++++++++ .../dataFlow/fixture/EqualsWithItself.java | 18 ++++++ .../DataFlowInspectionTest.java | 2 + .../ig/bugs/EqualsWithItselfInspection.java | 48 +++++++-------- 6 files changed, 126 insertions(+), 45 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.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 43df8c7853ee..b12206f6af66 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 @@ -30,6 +30,7 @@ import com.intellij.psi.util.PsiUtil; import com.intellij.psi.util.TypeConversionUtil; import com.intellij.util.*; import com.intellij.util.containers.ContainerUtil; +import com.siyeh.ig.bugs.EqualsWithItselfInspection; import com.siyeh.ig.fixes.EqualsToEqualityFix; import com.siyeh.ig.psiutils.*; import one.util.streamex.IntStreamEx; @@ -690,6 +691,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } private void reportConstantBoolean(ProblemsHolder holder, PsiElement psiAnchor, boolean evaluatesToTrue) { + if (shouldBeSuppressed(psiAnchor)) return; boolean isAssertion = isAssertionEffectively(psiAnchor, evaluatesToTrue); if (!DONT_REPORT_TRUE_ASSERT_STATEMENTS || !isAssertion) { List fixes = new ArrayList<>(); @@ -706,6 +708,17 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } } + private static boolean shouldBeSuppressed(PsiElement anchor) { + if (!(anchor instanceof PsiExpression)) return false; + PsiExpression expression = (PsiExpression)anchor; + while (expression != null && BoolUtils.isNegation(expression)) { + expression = BoolUtils.getNegated(expression); + } + PsiMethodCallExpression call = ObjectUtils.tryCast(expression, PsiMethodCallExpression.class); + // Reported by "Equals with itself" inspection; avoid double reporting + return call != null && EqualsWithItselfInspection.isEqualsWithItself(call); + } + private static LocalQuickFix createReplaceWithNullCheckFix(PsiElement psiAnchor, boolean evaluatesToTrue) { if (evaluatesToTrue) return null; if (!(psiAnchor instanceof PsiMethodCallExpression) || !MethodCallUtils.isEqualsCall((PsiMethodCallExpression)psiAnchor)) return null; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java index 7c69a0b4b1f6..41a9af388f2d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java @@ -16,6 +16,7 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInspection.dataFlow.StandardMethodContract.ValueConstraint; +import com.intellij.codeInspection.dataFlow.value.DfaRelationValue; import com.intellij.codeInspection.dataFlow.value.DfaRelationValue.RelationType; import com.intellij.lang.injection.InjectedLanguageManager; import com.intellij.psi.*; @@ -35,6 +36,7 @@ import java.util.List; import java.util.function.Supplier; import static com.intellij.codeInspection.dataFlow.ContractReturnValue.*; +import static com.intellij.codeInspection.dataFlow.MethodContract.singleConditionContract; import static com.intellij.codeInspection.dataFlow.MethodContract.trivialContract; import static com.intellij.codeInspection.dataFlow.StandardMethodContract.ValueConstraint.*; import static com.intellij.codeInspection.dataFlow.StandardMethodContract.createConstraintArray; @@ -48,11 +50,11 @@ public class HardcodedContracts { private static final List ARRAY_RANGE_CONTRACTS = ContainerUtil.immutableList( nonnegativeArgumentContract(1), nonnegativeArgumentContract(2), - MethodContract.singleConditionContract(ContractValue.argument(1), RelationType.GT, - ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), fail()), - MethodContract.singleConditionContract(ContractValue.argument(2), RelationType.GT, - ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), fail()), - MethodContract.singleConditionContract(ContractValue.argument(1), RelationType.GT, ContractValue.argument(2), fail()) + singleConditionContract(ContractValue.argument(1), RelationType.GT, ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), + fail()), + singleConditionContract(ContractValue.argument(2), RelationType.GT, ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), + fail()), + singleConditionContract(ContractValue.argument(1), RelationType.GT, ContractValue.argument(2), fail()) ); private static final CallMatcher QUEUE_POLL = instanceCall("java.util.Queue", "poll").parameterCount(0); @@ -103,17 +105,17 @@ public class HardcodedContracts { .register(instanceCall(JAVA_UTIL_MAP, "equals").parameterTypes(JAVA_LANG_OBJECT), ContractProvider.list(SpecialField.MAP_SIZE::getEqualsContracts)) .register(instanceCall(JAVA_UTIL_COLLECTION, "contains").parameterCount(1), - ContractProvider.single(() -> MethodContract.singleConditionContract( + ContractProvider.single(() -> singleConditionContract( ContractValue.qualifier().specialField(SpecialField.COLLECTION_SIZE), RelationType.EQ, ContractValue.zero(), returnFalse()))) .register(instanceCall(JAVA_UTIL_MAP, "containsKey", "containsValue").parameterCount(1), - ContractProvider.single(() -> MethodContract.singleConditionContract( + ContractProvider.single(() -> singleConditionContract( ContractValue.qualifier().specialField(SpecialField.MAP_SIZE), RelationType.EQ, ContractValue.zero(), returnFalse()))) .register(instanceCall(JAVA_UTIL_LIST, "get").parameterTypes("int"), ContractProvider.list(() -> Arrays.asList(nonnegativeArgumentContract(0), specialFieldRangeContract(0, RelationType.LT, SpecialField.COLLECTION_SIZE)))) .register(instanceCall("java.util.SortedSet", "first", "last").parameterCount(0), - ContractProvider.single(() -> MethodContract.singleConditionContract( + ContractProvider.single(() -> singleConditionContract( ContractValue.qualifier().specialField(SpecialField.COLLECTION_SIZE), RelationType.EQ, ContractValue.zero(), fail()))) // All these methods take array as 1st parameter, from index as 2nd and to index as 3rd @@ -123,7 +125,7 @@ public class HardcodedContracts { .register(staticCall("org.mockito.ArgumentMatchers", "argThat").parameterCount(1), ContractProvider.single(() -> new StandardMethodContract(new ValueConstraint[]{ANY_VALUE}, returnAny()))) .register(instanceCall("java.util.Queue", "peek", "poll").parameterCount(0), - (call, paramCount) -> Arrays.asList(MethodContract.singleConditionContract( + (call, paramCount) -> Arrays.asList(singleConditionContract( ContractValue.qualifier().specialField(SpecialField.COLLECTION_SIZE), RelationType.EQ, ContractValue.zero(), returnNull()), trivialContract(returnAny()))) .register(anyOf(staticCall(JAVA_LANG_MATH, "max").parameterTypes("int", "int"), @@ -137,9 +139,11 @@ public class HardcodedContracts { staticCall(JAVA_LANG_LONG, "min").parameterTypes("long", "long")), (call, paramCount) -> mathMinMax(false)) .register(instanceCall(JAVA_LANG_STRING, "startsWith", "endsWith", "contains"), - ContractProvider.single(() -> MethodContract.singleConditionContract( + ContractProvider.single(() -> singleConditionContract( ContractValue.qualifier().specialField(SpecialField.STRING_LENGTH), RelationType.LT, - ContractValue.argument(0).specialField(SpecialField.STRING_LENGTH), returnFalse()))); + ContractValue.argument(0).specialField(SpecialField.STRING_LENGTH), returnFalse()))) + .register(instanceCall(JAVA_LANG_OBJECT, "equals").parameterTypes(JAVA_LANG_OBJECT), + (call, paramCount) -> equalsContracts(call)); public static List getHardcodedContracts(@NotNull PsiMethod method, @Nullable PsiMethodCallExpression call) { PsiClass owner = method.getContainingClass(); @@ -204,34 +208,49 @@ public class HardcodedContracts { if (endLimited) { contracts.add(nonnegativeArgumentContract(1)); contracts.add(specialFieldRangeContract(1, RelationType.LE, SpecialField.STRING_LENGTH)); - contracts.add(MethodContract - .singleConditionContract(ContractValue.argument(0), RelationType.LE.getNegated(), - ContractValue.argument(1), fail())); + contracts.add(singleConditionContract(ContractValue.argument(0), RelationType.LE.getNegated(), ContractValue.argument(1), fail())); } return contracts; } static MethodContract optionalAbsentContract(ContractReturnValue returnValue) { - return MethodContract - .singleConditionContract(ContractValue.qualifier(), RelationType.IS, ContractValue.optionalValue(false), returnValue); + return singleConditionContract(ContractValue.qualifier(), RelationType.IS, ContractValue.optionalValue(false), returnValue); } static MethodContract nonnegativeArgumentContract(int argNumber) { - return MethodContract - .singleConditionContract(ContractValue.argument(argNumber), RelationType.LT, ContractValue.zero(), fail()); + return singleConditionContract(ContractValue.argument(argNumber), RelationType.LT, ContractValue.zero(), fail()); } static MethodContract specialFieldRangeContract(int index, RelationType type, SpecialField specialField) { - return MethodContract.singleConditionContract(ContractValue.argument(index), type.getNegated(), - ContractValue.qualifier().specialField(specialField), fail()); + return singleConditionContract(ContractValue.argument(index), type.getNegated(), ContractValue.qualifier().specialField(specialField), + fail()); } static List mathMinMax(boolean isMax) { - return Arrays.asList(MethodContract.singleConditionContract( + return Arrays.asList(singleConditionContract( ContractValue.argument(0), isMax ? RelationType.GT : RelationType.LT, ContractValue.argument(1), returnParameter(0)), trivialContract(returnParameter(1))); } + private static List equalsContracts(PsiMethodCallExpression call) { + PsiExpression qualifier = call == null ? null : call.getMethodExpression().getQualifierExpression(); + if (qualifier != null && knownAsEqualByReference(qualifier.getType())) { + return Arrays.asList( + singleConditionContract(ContractValue.qualifier(), RelationType.EQ, ContractValue.argument(0), returnTrue()), + trivialContract(returnFalse()) + ); + } + return Arrays.asList(new StandardMethodContract(new StandardMethodContract.ValueConstraint[]{NULL_VALUE}, returnFalse()), + singleConditionContract(ContractValue.qualifier(), DfaRelationValue.RelationType.EQ, + ContractValue.argument(0), returnTrue())); + } + + private static boolean knownAsEqualByReference(PsiType type) { + if (type instanceof PsiArrayType) return true; + PsiClass psiClass = PsiUtil.resolveClassInClassTypeOnly(type); + return psiClass != null && (psiClass.isEnum() || JAVA_LANG_CLASS.equals(psiClass.getQualifiedName())); + } + private static boolean isJunit(String className) { return className.startsWith("junit.framework.") || className.startsWith("org.junit.") || className.equals("org.testng.AssertJUnit"); diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java new file mode 100644 index 000000000000..6ed439c2022b --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java @@ -0,0 +1,29 @@ +import java.util.List; + +class Test { + enum A {X, Y, Z}; + List list1, list2; + + void test(List tokens) { + String t = "int"; + A s = A.X; + A l = A.X; + for (String token : tokens) { + if ("unsigned".equals(token)) { + s = A.Z; + } + else if ("signed".equals(token)) { + s = A.Y; + } + else if ("short".equals(token)) { + l = A.Y; + } + else if ("long".equals(token)) { + l = A.Z; + } + else if (list1.contains(token) || list2.contains(token)) { + t = token; + } + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java new file mode 100644 index 000000000000..ff72840bb442 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java @@ -0,0 +1,18 @@ +import java.util.List; + +class Test { + void test(Object x, Object y) { + if(x.equals(x)) { + // do not report here; reported by EqualsWithItselfInspection + System.out.println("always"); + } + if(!x.equals(x)) { + // do not report here; reported by EqualsWithItselfInspection + System.out.println("never"); + } + y = x; + if(x.equals(y)) { + System.out.println("always"); + } + } +} \ No newline at end of file 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 0a9665af2521..465992118725 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -619,4 +619,6 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { public void testNanComparisonWrong() { doTest(); } public void testConstantMethods() { doTest(); } public void testPolyadicEquality() { doTest(); } + public void testEqualsInLoopNotTooComplex() { doTest(); } + public void testEqualsWithItself() { doTest(); } } diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java index 53c00cbcdc98..7ea9d1a8a19e 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java @@ -53,31 +53,31 @@ public class EqualsWithItselfInspection extends BaseInspection { @Override public void visitMethodCallExpression(PsiMethodCallExpression expression) { super.visitMethodCallExpression(expression); - if (!MethodCallUtils.isEqualsCall(expression) && - !MethodCallUtils.isEqualsIgnoreCaseCall(expression) && - !MethodCallUtils.isCompareToCall(expression) && - !MethodCallUtils.isCompareToIgnoreCaseCall(expression)) { - return; + if (isEqualsWithItself(expression)) { + registerMethodCallError(expression); } - final PsiReferenceExpression methodExpression = expression.getMethodExpression(); - final PsiExpressionList argumentList = expression.getArgumentList(); - final PsiExpression[] arguments = argumentList.getExpressions(); - if (arguments.length != 1) { - return; - } - final PsiExpression argument = ParenthesesUtils.stripParentheses(arguments[0]); - final PsiExpression qualifier = methodExpression.getQualifierExpression(); - if (qualifier == null) { - if (!(argument instanceof PsiThisExpression)) { - return; - } - } else { - if (!EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(qualifier, argument) || - SideEffectChecker.mayHaveSideEffects(qualifier)) { - return; - } - } - registerMethodCallError(expression); } } + + public static boolean isEqualsWithItself(PsiMethodCallExpression expression) { + if (!MethodCallUtils.isEqualsCall(expression) && + !MethodCallUtils.isEqualsIgnoreCaseCall(expression) && + !MethodCallUtils.isCompareToCall(expression) && + !MethodCallUtils.isCompareToIgnoreCaseCall(expression)) { + return false; + } + final PsiReferenceExpression methodExpression = expression.getMethodExpression(); + final PsiExpressionList argumentList = expression.getArgumentList(); + final PsiExpression[] arguments = argumentList.getExpressions(); + if (arguments.length != 1) { + return false; + } + final PsiExpression argument = ParenthesesUtils.stripParentheses(arguments[0]); + final PsiExpression qualifier = methodExpression.getQualifierExpression(); + if (qualifier != null) { + return EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(qualifier, argument) && + !SideEffectChecker.mayHaveSideEffects(qualifier); + } + return argument instanceof PsiThisExpression; + } }