From e623b2ed38507175d1948e5495da8d2d0495c698 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 17 Aug 2018 12:40:49 +0700 Subject: [PATCH] String equality: fix some corner cases; check dependent variables on equality --- .../dataFlow/DataFlowInstructionVisitor.java | 5 +- .../dataFlow/DfaMemoryStateImpl.java | 47 +++++++++++++++---- .../dataFlow/StandardInstructionVisitor.java | 2 +- .../dataFlow/fixture/AdvancedArrayAccess.java | 14 +++--- .../dataFlow/fixture/FieldEquality.java | 10 ++++ .../dataFlow/fixture/StringEquality.java | 24 ++++++++++ .../DataFlowInspectionTest.java | 1 + 7 files changed, 86 insertions(+), 17 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/FieldEquality.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java index b4cc50d099c7..672c7df72ff5 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java @@ -11,6 +11,7 @@ import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiTypesUtil; import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; +import com.siyeh.ig.psiutils.TypeUtils; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -39,7 +40,9 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { @Override public DfaInstructionState[] visitAssign(AssignInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { PsiExpression left = instruction.getLExpression(); - if (left != null && !Boolean.FALSE.equals(mySameValueAssigned.get(left))) { + if (left != null && !Boolean.FALSE.equals(mySameValueAssigned.get(left)) && !TypeUtils.isJavaLangString(left.getType())) { + // Reporting strings is skipped because string reassignment might be intentionally used to deduplicate the heap objects + // (we compare strings by contents) if (!left.isPhysical()) { if (LOG.isDebugEnabled()) { LOG.debug("Non-physical element in assignment instruction: " + left.getParent().getText(), new Throwable()); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java index 4670e589a8a8..fdc7d0fc264d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java @@ -35,6 +35,7 @@ import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.Stack; import gnu.trove.TIntObjectHashMap; import gnu.trove.TIntObjectProcedure; +import one.util.streamex.EntryStream; import one.util.streamex.StreamEx; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NotNull; @@ -859,7 +860,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { } if (isEffectivelyNaN(dfaLeft) || isEffectivelyNaN(dfaRight)) { - applyEquivalenceRelation(dfaRelation, dfaLeft, dfaRight); + applyEquivalenceRelation(relationType, dfaLeft, dfaRight); return relationType == RelationType.NE; } if ((canBeNaN(dfaLeft) && !isNull(dfaRight)) || (canBeNaN(dfaRight) && !isNull(dfaLeft))) { @@ -869,11 +870,11 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return !dfaRelation.isNonEquality(); } - applyEquivalenceRelation(dfaRelation, dfaLeft, dfaRight); + applyEquivalenceRelation(relationType, dfaLeft, dfaRight); return true; } - return applyEquivalenceRelation(dfaRelation, dfaLeft, dfaRight); + return applyEquivalenceRelation(relationType, dfaLeft, dfaRight); } private void updateVarStateOnComparison(@NotNull DfaVariableValue dfaVar, DfaValue value) { @@ -897,9 +898,9 @@ public class DfaMemoryStateImpl implements DfaMemoryState { } } - private boolean applyEquivalenceRelation(@NotNull DfaRelationValue dfaRelation, DfaValue dfaLeft, DfaValue dfaRight) { - boolean isNegated = dfaRelation.isNonEquality(); - if (!isNegated && !dfaRelation.isEquality()) { + private boolean applyEquivalenceRelation(RelationType type, DfaValue dfaLeft, DfaValue dfaRight) { + boolean isNegated = type == RelationType.NE || type == RelationType.GT || type == RelationType.LT; + if (!isNegated && type != RelationType.EQ) { return true; } @@ -920,11 +921,14 @@ public class DfaMemoryStateImpl implements DfaMemoryState { } } - if (dfaRelation.getRelation() == RelationType.LT) { + if (type == RelationType.LT) { if (!applyLessThanRelation(dfaLeft, dfaRight)) return false; - } else if (dfaRelation.getRelation() == RelationType.GT) { + } else if (type == RelationType.GT) { if (!applyLessThanRelation(dfaRight, dfaLeft)) return false; } else { + if (!isNegated && !applyDependentFieldsEquivalence(dfaLeft, dfaRight)) { + return false; + } if (!applyRelation(dfaLeft, dfaRight, isNegated)) return false; } if (!checkCompareWithBooleanLiteral(dfaLeft, dfaRight, isNegated)) { @@ -938,6 +942,33 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return true; } + @NotNull + private EntryStream getDependentPairs(DfaValue left, DfaValue right) { + if (left instanceof DfaVariableValue) { + List leftVars = ((DfaVariableValue)left).getDependentVariables(); + if (right instanceof DfaVariableValue) { + List rightVars = ((DfaVariableValue)right).getDependentVariables(); + return StreamEx.of(leftVars).mapToEntry(leftVar -> StreamEx.of(rightVars) + .findFirst(rightVar -> leftVar.getSource().equals(rightVar.getSource())).orElse(null)) + .nonNullValues() + .filterKeyValue((leftVar, rightVar) -> getEqClassIndex(leftVar) != -1 || getEqClassIndex(rightVar) != -1); + } + if (right instanceof DfaConstValue) { + return StreamEx.of(leftVars).filter(leftVar -> leftVar.getSource() instanceof SpecialField) + .mapToEntry(leftVar -> ((SpecialField)leftVar.getSource()).createValue(myFactory, right)); + } + } + if (left instanceof DfaConstValue && right instanceof DfaVariableValue) { + return getDependentPairs(right, left); + } + return EntryStream.empty(); + } + + private boolean applyDependentFieldsEquivalence(@NotNull DfaValue left, @NotNull DfaValue right) { + return getDependentPairs(left, right) + .allMatch(pair -> applyCondition(myFactory.createCondition(pair.getKey(), RelationType.EQ, pair.getValue()))); + } + private boolean applyBoxedRelation(@NotNull DfaVariableValue dfaLeft, DfaValue dfaRight, boolean negated) { if (!TypeConversionUtil.isPrimitiveAndNotNull(dfaLeft.getVariableType())) return true; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java index c2ead5812587..f27ff927723d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java @@ -693,7 +693,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { if (expression instanceof PsiBinaryExpression) { PsiExpression left = ((PsiBinaryExpression)expression).getLOperand(); PsiExpression right = ((PsiBinaryExpression)expression).getROperand(); - return right != null && (TypeUtils.isJavaLangString(left.getType()) || TypeUtils.isJavaLangString(right.getType())); + return right != null && (TypeUtils.isJavaLangString(left.getType()) && TypeUtils.isJavaLangString(right.getType())); } return false; } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/AdvancedArrayAccess.java b/java/java-tests/testData/inspection/dataFlow/fixture/AdvancedArrayAccess.java index 3b7cf01eb810..118e6f989e18 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/AdvancedArrayAccess.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/AdvancedArrayAccess.java @@ -128,27 +128,27 @@ class AdvancedArrayAccess { void testLocalRewritten() { String[] arr = {"foo", "bar", "baz"}; arr[0] = "qux"; - String result = ""; + int result = 0; if(arr[1].equals("bar")) { - result = "yes"; + result = 1; } if(arr[2].equals("bar")) { - result = "yes"; + result = 1; } if(arr[0].equals("foo")) { - result = "no"; + result = 2; } System.out.println(result); arr = new String[] {"bar", "baz", "foo"}; arr[2] = "qux"; if(arr[1].equals("bar")) { - result = "no"; + result = 2; } if(arr[2].equals("qux")) { - result = "yes"; + result = 1; } if(arr[0].equals("bar")) { - result = "no"; + result = 2; } System.out.println(result); } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/FieldEquality.java b/java/java-tests/testData/inspection/dataFlow/fixture/FieldEquality.java new file mode 100644 index 000000000000..3444c95d4c04 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/FieldEquality.java @@ -0,0 +1,10 @@ +class Point { + int x, y; + + void check(Point other) { + if(x != other.x && this == other) { + System.out.println("Impossible"); + } + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/StringEquality.java b/java/java-tests/testData/inspection/dataFlow/fixture/StringEquality.java index 09de2cc8d23b..68eb41b78cb5 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/StringEquality.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/StringEquality.java @@ -53,4 +53,28 @@ class StringEquality { } return false; } + + static final String SENTINEL = "foo"; + + void test(Object o) { + if(o == SENTINEL) { + System.out.println("oops"); + } else { + System.out.println(((Number)o).longValue()); + } + } + + String internFoo(String s) { + if (s.equals("foo")) { + // "foo" is often used, intern it + s = "foo"; + } + return s; + } + + void length(String s) { + if(!s.startsWith("--") || s.equals(".")) { + System.out.println("invalid parameter"); + } + } } \ 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 f5ff0dc1c482..50d66745387a 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -638,4 +638,5 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { public void testBooleanMergeInLoop() { doTest(); } public void testVoidIsAlwaysNull() { doTest(); } public void testStringEquality() { doTest(); } + public void testFieldEquality() { doTest(); } }