From d255c5b10cc262b5f37037305916e13bddc18b05 Mon Sep 17 00:00:00 2001 From: peter Date: Wed, 18 Dec 2013 14:38:19 +0100 Subject: [PATCH] dfa: make state merging result predictable when removing a!=b and a!=c && b==c, don't restore a!=c --- .../dataFlow/DfaMemoryStateImpl.java | 5 ++++ .../codeInspection/dataFlow/StateMerger.java | 11 ++++---- ...MakeUnrelatedVariableFalseWhenMerging.java | 26 +++++++++++++++++++ .../DataFlowInspectionTest.java | 1 + .../intellij/util/containers/MultiMap.java | 11 ++++++++ 5 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/DontMakeUnrelatedVariableFalseWhenMerging.java 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 35f37f66358a..e18d164e0de3 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 @@ -270,6 +270,11 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return myEqClasses.size() - 1; } + boolean areEquivalent(DfaValue val1, DfaValue val2) { + int index = getEqClassIndex(val1); + return index >= 0 && index == getEqClassIndex(val2); + } + @NotNull private List getEqClassesFor(@NotNull DfaValue dfaValue) { int index = getEqClassIndex(dfaValue); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java index dfdb399a593e..65899e90e2de 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StateMerger.java @@ -36,7 +36,7 @@ class StateMerger { @Nullable List mergeByFacts(List states) { - MultiMap statesByFact = MultiMap.create(); + MultiMap statesByFact = MultiMap.createLinked(); for (DfaMemoryStateImpl state : states) { ProgressManager.checkCanceled(); for (Fact fact : getFacts(state)) { @@ -52,7 +52,7 @@ class StateMerger { ProgressManager.checkCanceled(); - MultiMap, DfaMemoryStateImpl> statesByUnrelatedFacts = MultiMap.create(); + MultiMap, DfaMemoryStateImpl> statesByUnrelatedFacts = MultiMap.createLinked(); for (DfaMemoryStateImpl state : ContainerUtil.concat(statesByFact.get(fact), statesWithNegations)) { statesByUnrelatedFacts.putValue(getUnrelatedFacts(fact, state), state); } @@ -100,7 +100,7 @@ class StateMerger { for (DfaMemoryStateImpl member : mergedGroup) { LinkedHashSet memberFacts = getFacts(member); if (memberFacts.contains(removedFact)) { - Set otherInequalities = getOtherInequalities(removedFact, memberFacts); + Set otherInequalities = getOtherInequalities(removedFact, memberFacts, member); if (inequalitiesToRestore == null) { inequalitiesToRestore = otherInequalities; } else { @@ -116,11 +116,12 @@ class StateMerger { } } - private static Set getOtherInequalities(Fact removedFact, LinkedHashSet memberFacts) { + private static Set getOtherInequalities(Fact removedFact, LinkedHashSet memberFacts, DfaMemoryStateImpl state) { Set otherInequalities = ContainerUtil.newLinkedHashSet(); for (Fact candidate : memberFacts) { if (candidate.myType == FactType.equality && !candidate.myPositive && candidate.myVar == removedFact.myVar && - candidate.myArg != removedFact.myArg && candidate.myArg instanceof DfaConstValue) { + !state.areEquivalent((DfaValue)candidate.myArg, (DfaValue)removedFact.myArg) && + candidate.myArg instanceof DfaConstValue) { otherInequalities.add((DfaConstValue)candidate.myArg); } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/DontMakeUnrelatedVariableFalseWhenMerging.java b/java/java-tests/testData/inspection/dataFlow/fixture/DontMakeUnrelatedVariableFalseWhenMerging.java new file mode 100644 index 000000000000..858cd00c34d2 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/DontMakeUnrelatedVariableFalseWhenMerging.java @@ -0,0 +1,26 @@ +import org.jetbrains.annotations.Nullable; + +import java.io.File; + +class Some { + void foo(String[] args) { + boolean b = false; + + if (hashCode() == 2) { + b = new File("a").exists(); + } + + boolean b2 = hashCode() == 4; + if (!b2) { + return; + } + + if (b) { + + } + + if (b) { + + } + } +} diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java index 3f8a810419eb..3ce3fe86fa6a 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java @@ -298,6 +298,7 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase { public void testDontForgetEqInfoWhenMergingByType() { doTest(); } public void testDontMakeNullableAfterInstanceof() { doTest(); } public void testDontMakeUnrelatedVariableNotNullWhenMerging() { doTest(); } + public void testDontMakeUnrelatedVariableFalseWhenMerging() { doTest(); } public void testDontLoseInequalityInformation() { doTest(); } public void _testNullCheckBeforeInstanceof() { doTest(); } // http://youtrack.jetbrains.com/issue/IDEA-113220 diff --git a/platform/util/src/com/intellij/util/containers/MultiMap.java b/platform/util/src/com/intellij/util/containers/MultiMap.java index b4fb559b1ccc..8dafedfff9b1 100644 --- a/platform/util/src/com/intellij/util/containers/MultiMap.java +++ b/platform/util/src/com/intellij/util/containers/MultiMap.java @@ -17,6 +17,7 @@ package com.intellij.util.containers; import com.intellij.util.SmartList; +import com.intellij.util.containers.hash.LinkedHashMap; import gnu.trove.THashMap; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -242,6 +243,16 @@ public class MultiMap implements Serializable { return new MultiMap(); } + @NotNull + public static MultiMap createLinked() { + return new MultiMap() { + @Override + protected Map> createMap() { + return new LinkedHashMap>(); + } + }; + } + @NotNull public static MultiMap createSmartList() { return new MultiMap() {