diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java index e519963352f5..ea312065b52e 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaInstructionState.java @@ -121,8 +121,7 @@ class StateQueue { StateMerger merger = new StateMerger(); while (true) { - List nextStates = merger.mergeByEquality(group); - if (nextStates == null) nextStates = merger.mergeByType(group); + List nextStates = merger.mergeByFacts(group); if (nextStates == null) nextStates = merger.mergeByNullability(group); if (nextStates == null) nextStates = merger.mergeByUnknowns(group); if (nextStates == null) break; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java index 4894eeafdc04..4f9c27107cd2 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java @@ -172,4 +172,13 @@ public class DfaVariableState { public DfaValue getValue() { return null; } + + public Set getInstanceofValues() { + return myInstanceofValues; + } + + public Set getNotInstanceofValues() { + return myNotInstanceofValues; + } + } 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 0a4308ded015..dfdb399a593e 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 @@ -15,68 +15,118 @@ */ package com.intellij.codeInspection.dataFlow; -import com.intellij.codeInspection.dataFlow.value.DfaConstValue; -import com.intellij.codeInspection.dataFlow.value.DfaPsiType; -import com.intellij.codeInspection.dataFlow.value.DfaValue; -import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; +import com.intellij.codeInspection.dataFlow.value.*; import com.intellij.openapi.progress.ProgressManager; import com.intellij.openapi.util.Condition; -import com.intellij.openapi.util.Pair; import com.intellij.openapi.util.UnorderedPair; +import com.intellij.psi.JavaTokenType; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.MultiMap; import gnu.trove.THashSet; import org.jetbrains.annotations.Nullable; -import java.util.Collection; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; /** * @author peter */ class StateMerger { - private final Map> myVarValues = ContainerUtil.newIdentityHashMap(); - private final Map>> myEqPairs = ContainerUtil.newIdentityHashMap(); + private final Map> myFacts = ContainerUtil.newIdentityHashMap(); private final Map> myCopyCache = ContainerUtil.newIdentityHashMap(); @Nullable - public List mergeByEquality(List states) { - final MultiMap,DfaMemoryStateImpl> statesByEq = new MultiMap, DfaMemoryStateImpl>(); + List mergeByFacts(List states) { + MultiMap statesByFact = MultiMap.create(); for (DfaMemoryStateImpl state : states) { ProgressManager.checkCanceled(); - for (UnorderedPair pair : getEqPairs(state)) { - statesByEq.putValue(pair, state); + for (Fact fact : getFacts(state)) { + statesByFact.putValue(fact, state); } } - for (final DfaMemoryStateImpl state : states) { + for (final Fact fact : statesByFact.keySet()) { + if (statesByFact.get(fact).size() == states.size() || fact.myPositive) continue; + + Collection statesWithNegations = statesByFact.get(fact.getPositiveCounterpart()); + if (statesWithNegations.isEmpty()) continue; + ProgressManager.checkCanceled(); - MultiMap distincts = getDistinctsMap(state); - for (DfaVariableValue var : distincts.keySet()) { - Map> statesByValue = getCompatibleStatesByValue(state, var, distincts, statesByEq); - if (statesByValue == null) { - continue; - } - - final THashSet complementaryStates = findComplementaryStates(var, statesByValue, state); - if (complementaryStates == null) { - continue; - } - - DfaMemoryStateImpl copy = copyWithoutVar(state, var).createCopy(); - - complementaryStates.add(state); - mergeNullableState(var, copy, complementaryStates); - mergeUnknowns(copy, complementaryStates); - return getMergeResult(states, complementaryStates, copy); + + MultiMap, DfaMemoryStateImpl> statesByUnrelatedFacts = MultiMap.create(); + for (DfaMemoryStateImpl state : ContainerUtil.concat(statesByFact.get(fact), statesWithNegations)) { + statesByUnrelatedFacts.putValue(getUnrelatedFacts(fact, state), state); } + Set removedStates = ContainerUtil.newIdentityTroveSet(); + List result = ContainerUtil.newArrayList(); + for (Set key : statesByUnrelatedFacts.keySet()) { + Collection group = statesByUnrelatedFacts.get(key); + if (group.size() > 1) { + DfaMemoryStateImpl copy = group.iterator().next().createCopy(); + fact.removeFromState(copy); + if (fact.myType == FactType.equality) { + restoreOtherInequalities(fact, group, copy); + } + mergeUnknowns(copy, group); + + removedStates.addAll(group); + result.add(copy); + } + } + + if (!result.isEmpty()) { + for (DfaMemoryStateImpl state : states) { + if (!removedStates.contains(state)) { + result.add(state); + } + } + return result; + } } return null; } + private LinkedHashSet getUnrelatedFacts(final Fact fact, DfaMemoryStateImpl state) { + return new LinkedHashSet(ContainerUtil.filter(getFacts(state), new Condition() { + @Override + public boolean value(Fact another) { + return !fact.invalidatesFact(another); + } + })); + } + + private void restoreOtherInequalities(Fact removedFact, Collection mergedGroup, DfaMemoryStateImpl state) { + Set inequalitiesToRestore = null; + for (DfaMemoryStateImpl member : mergedGroup) { + LinkedHashSet memberFacts = getFacts(member); + if (memberFacts.contains(removedFact)) { + Set otherInequalities = getOtherInequalities(removedFact, memberFacts); + if (inequalitiesToRestore == null) { + inequalitiesToRestore = otherInequalities; + } else { + inequalitiesToRestore.retainAll(otherInequalities); + } + } + } + if (inequalitiesToRestore != null) { + DfaRelationValue.Factory relationFactory = state.getFactory().getRelationFactory(); + for (DfaConstValue toRestore : inequalitiesToRestore) { + state.applyCondition(relationFactory.createRelation(removedFact.myVar, toRestore, JavaTokenType.EQEQ, true)); + } + } + } + + private static Set getOtherInequalities(Fact removedFact, LinkedHashSet memberFacts) { + 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) { + otherInequalities.add((DfaConstValue)candidate.myArg); + } + } + return otherInequalities; + } + private static void mergeUnknowns(DfaMemoryStateImpl mergedState, Collection complementaryStates) { for (DfaMemoryStateImpl removedState : complementaryStates) { for (DfaVariableValue unknownVar : removedState.getUnknownVariables()) { @@ -84,15 +134,6 @@ class StateMerger { } } } - private static void mergeNullableState(DfaVariableValue var, - DfaMemoryStateImpl mergedState, - Collection complementaryStates) { - for (DfaMemoryStateImpl removedState : complementaryStates) { - if (removedState.getVariableState(var).isNullable()) { - mergedState.setVariableState(var, mergedState.getVariableState(var).withNullability(Nullness.NULLABLE)); - } - } - } private static List getMergeResult(List statesBeforeMerge, final THashSet mergedStates, @@ -181,64 +222,12 @@ class StateMerger { return null; } - @Nullable - public List mergeByType(List states) { - MultiMap,DfaMemoryStateImpl> byInstanceof = new MultiMap, DfaMemoryStateImpl>(); - for (final DfaMemoryStateImpl state : states) { - ProgressManager.checkCanceled(); - for (DfaVariableValue value : state.getChangedVariables()) { - for (DfaPsiType instanceofValue : state.getVariableState(value).myInstanceofValues) { - byInstanceof.putValue(Pair.create(value, instanceofValue), state); - } - } - } - - for (final DfaMemoryStateImpl state : states) { - ProgressManager.checkCanceled(); - - for (final DfaVariableValue var : state.getChangedVariables()) { - for (final DfaPsiType notInstanceof : state.getVariableState(var).myNotInstanceofValues) { - final DfaVariableState varStateWithoutType = getVarStateWithoutType(state, var, notInstanceof); - List complementaryStates = ContainerUtil.filter( - byInstanceof.get(Pair.create(var, notInstanceof)), - new Condition() { - @Override - public boolean value(DfaMemoryStateImpl another) { - return seemCompatible(state, another, var) && - another.getVariableState(var).myInstanceofValues.contains(notInstanceof) && - varStateWithoutType.equals(getVarStateWithoutType(another, var, notInstanceof)) && - areEquivalentModuloVar(another, state, var) && - !(state.isNull(var) && another.isNotNull(var)); - } - }); - if (complementaryStates.isEmpty()) { - continue; - } - - DfaMemoryStateImpl copy = state.createCopy(); - copy.setVariableState(var, varStateWithoutType); - - complementaryStates.add(state); - mergeNullableState(var, copy, complementaryStates); - mergeUnknowns(copy, complementaryStates); - return getMergeResult(states, ContainerUtil.newIdentityTroveSet(complementaryStates), copy); - } - } - - } - return null; - } - private boolean areEquivalentModuloVar(DfaMemoryStateImpl state1, DfaMemoryStateImpl state2, DfaVariableValue var) { DfaMemoryStateImpl copy1 = copyWithoutVar(state1, var); DfaMemoryStateImpl copy2 = copyWithoutVar(state2, var); return copy2.equalsByRelations(copy1) && copy2.equalsByVariableStates(copy1); } - private static DfaVariableState getVarStateWithoutType(DfaMemoryStateImpl s, DfaVariableValue var, DfaPsiType type) { - return s.getVariableState(var).withoutType(type).withNullability(Nullness.UNKNOWN); - } - private DfaMemoryStateImpl copyWithoutVar(DfaMemoryStateImpl state, DfaVariableValue var) { Map map = myCopyCache.get(state); if (map == null) { @@ -253,129 +242,140 @@ class StateMerger { return copy; } - @Nullable - private THashSet findComplementaryStates(DfaVariableValue var, - Map> statesByValue, - DfaMemoryStateImpl state) { - THashSet removedStates = ContainerUtil.newIdentityTroveSet(); - - eachValue: - for (DfaValue value : statesByValue.keySet()) { - for (DfaMemoryStateImpl originalState : statesByValue.get(value)) { - if (areEquivalentModuloVar(originalState, state, var)) { - removedStates.add(originalState); - continue eachValue; - } - } - return null; - } - return removedStates; - } - - @Nullable - private Map> getCompatibleStatesByValue(final DfaMemoryStateImpl state, - final DfaVariableValue var, - MultiMap distincts, - MultiMap,DfaMemoryStateImpl> statesByEq) { - Map> statesByValue = ContainerUtil.newHashMap(); - for (DfaValue value : distincts.get(var)) { - List compatible = ContainerUtil.filter(statesByEq.get(createPair(var, value)), new Condition() { - @Override - public boolean value(DfaMemoryStateImpl state2) { - return seemCompatible(state, state2, var) && - areVarStatesEqualModuloNullability(state, state2, var); - } - }); - if (compatible.isEmpty()) { - return null; - } - statesByValue.put(value, compatible); - } - return statesByValue; - } - private static boolean areVarStatesEqualModuloNullability(DfaMemoryStateImpl state1, DfaMemoryStateImpl state2, DfaVariableValue var) { return state1.getVariableState(var).withNullability(Nullness.UNKNOWN).equals(state2.getVariableState(var).withNullability(Nullness.UNKNOWN)); } - private boolean seemCompatible(DfaMemoryStateImpl state1, DfaMemoryStateImpl state2, DfaVariableValue differentVar) { - Map varValues1 = getVarValues(state1); - Map varValues2 = getVarValues(state2); + private LinkedHashSet getFacts(DfaMemoryStateImpl state) { + LinkedHashSet result = myFacts.get(state); + if (result != null) { + return result; + } - for (DfaVariableValue var : varValues1.keySet()) { - if (var != differentVar && varValues1.get(var) != varValues2.get(var)) { - return false; - } - } - for (DfaVariableValue var : varValues2.keySet()) { - if (var != differentVar && !varValues1.containsKey(var)) { - return false; - } - } - return true; - } - - private Map getVarValues(DfaMemoryStateImpl state) { - Map map = myVarValues.get(state); - if (map == null) { - map = ContainerUtil.newHashMap(); - for (UnorderedPair pair : getEqPairs(state)) { - if (pair.first instanceof DfaVariableValue && pair.second instanceof DfaConstValue) { - map.put((DfaVariableValue)pair.first, (DfaConstValue)pair.second); + result = ContainerUtil.newLinkedHashSet(); + for (EqClass eqClass : state.getNonTrivialEqClasses()) { + DfaConstValue constant = eqClass.findConstant(true); + List vars = eqClass.getVariables(); + for (DfaVariableValue var : vars) { + if (constant != null) { + result.add(Fact.createEqualityFact(var, constant, true)); + } + for (DfaVariableValue eqVar : vars) { + if (var != eqVar) { + result.add(Fact.createEqualityFact(var, eqVar, true)); + } } } - myVarValues.put(state, map); - } - return map; - } - - private static MultiMap getDistinctsMap(DfaMemoryStateImpl state) { - MultiMap distincts = new MultiMap(); + for (UnorderedPair classPair : state.getDistinctClassPairs()) { - for (DfaValue value1 : classPair.first.getMemberValues()) { - value1 = DfaMemoryStateImpl.unwrap(value1); - for (DfaValue value2 : classPair.second.getMemberValues()) { - value2 = DfaMemoryStateImpl.unwrap(value2); - if (value1 instanceof DfaVariableValue) { - if (value2 instanceof DfaVariableValue || value2 instanceof DfaConstValue) { - distincts.putValue((DfaVariableValue)value1, value2); - } - } - if (value2 instanceof DfaVariableValue) { - if (value1 instanceof DfaVariableValue || value1 instanceof DfaConstValue) { - distincts.putValue((DfaVariableValue)value2, value1); - } - } - } - } - } - return distincts; - } + List vars1 = classPair.first.getVariables(); + List vars2 = classPair.second.getVariables(); + + LinkedHashSet firstSet = new LinkedHashSet(vars1); + ContainerUtil.addIfNotNull(firstSet, classPair.first.findConstant(true)); - private List> getEqPairs(DfaMemoryStateImpl state) { - List> result = myEqPairs.get(state); - if (result == null) { - Set> eqPairs = ContainerUtil.newHashSet(); - for (EqClass eqClass : state.getNonTrivialEqClasses()) { - DfaConstValue constant = eqClass.findConstant(true); - List vars = eqClass.getVariables(); - for (int i = 0; i < vars.size(); i++) { - DfaVariableValue var = vars.get(i); - if (constant != null) { - eqPairs.add(createPair(var, constant)); - } - for (int j = i + 1; j < vars.size(); j++) { - eqPairs.add(createPair(var, vars.get(j))); - } + LinkedHashSet secondSet = new LinkedHashSet(vars2); + ContainerUtil.addIfNotNull(secondSet, classPair.second.findConstant(true)); + + for (DfaVariableValue var : vars1) { + for (DfaValue value : secondSet) { + result.add(new Fact(FactType.equality, var, false, value)); + } + } + for (DfaVariableValue var : vars2) { + for (DfaValue value : firstSet) { + result.add(new Fact(FactType.equality, var, false, value)); } } - myEqPairs.put(state, result = ContainerUtil.newArrayList(eqPairs)); } + + Map states = state.getVariableStates(); + for (DfaVariableValue var : states.keySet()) { + DfaVariableState variableState = states.get(var); + for (DfaPsiType type : variableState.getInstanceofValues()) { + result.add(new Fact(FactType.instanceOf, var, true, type)); + } + for (DfaPsiType type : variableState.getNotInstanceofValues()) { + result.add(new Fact(FactType.instanceOf, var, false, type)); + } + } + + myFacts.put(state, result); return result; } - private static UnorderedPair createPair(DfaVariableValue var, DfaValue val) { - return new UnorderedPair(var, val); + private enum FactType { equality, instanceOf } + + private static class Fact { + final FactType myType; + final DfaVariableValue myVar; + final boolean myPositive; + final Object myArg; // DfaValue for equality fact, DfaPsiType for instanceOf fact + + private Fact(FactType type, DfaVariableValue var, boolean positive, Object arg) { + myType = type; + myVar = var; + myPositive = positive; + myArg = arg; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof Fact)) return false; + + Fact fact = (Fact)o; + + if (myPositive != fact.myPositive) return false; + if (!myArg.equals(fact.myArg)) return false; + if (myType != fact.myType) return false; + if (!myVar.equals(fact.myVar)) return false; + + return true; + } + + @Override + public int hashCode() { + int result = myType.hashCode(); + result = 31 * result + myVar.hashCode(); + result = 31 * result + (myPositive ? 1 : 0); + result = 31 * result + myArg.hashCode(); + return result; + } + + @Override + public String toString() { + return myVar + " " + (myPositive ? "" : "!") + myType + " " + myArg; + } + + static Fact createEqualityFact(DfaVariableValue var, DfaValue val, boolean equal) { + if (val instanceof DfaVariableValue && val.getID() < var.getID()) { + return new Fact(FactType.equality, (DfaVariableValue)val, equal, var); + } + return new Fact(FactType.equality, var, equal, val); + } + + Fact getPositiveCounterpart() { + return new Fact(myType, myVar, true, myArg); + } + + boolean invalidatesFact(Fact another) { + if (another.myType != myType) return false; + if (myType == FactType.equality) { + return myVar == another.myVar || myVar == another.myArg; + } + return myVar == another.myVar && myArg == another.myArg; + } + + void removeFromState(DfaMemoryStateImpl state) { + DfaVariableState varState = state.getVariableState(myVar); + if (myType == FactType.equality) { + state.flushVariable(myVar); + state.setVariableState(myVar, varState); + } else { + state.setVariableState(myVar, varState.withoutType((DfaPsiType)myArg)); + } + } } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/DontLoseInequalityInformation.java b/java/java-tests/testData/inspection/dataFlow/fixture/DontLoseInequalityInformation.java new file mode 100644 index 000000000000..14fcdffb8be0 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/DontLoseInequalityInformation.java @@ -0,0 +1,13 @@ +class Some { + public static void main(int i) { + if (i != 0) { + if (i == 1 || i == 2 || i == 3) { + System.out.println("hello"); + } + if (i == 0) { + System.out.println("wat?"); + } + } + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/DontMakeUnrelatedVariableNotNullWhenMerging.java b/java/java-tests/testData/inspection/dataFlow/fixture/DontMakeUnrelatedVariableNotNullWhenMerging.java new file mode 100644 index 000000000000..8e995dc9c905 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/DontMakeUnrelatedVariableNotNullWhenMerging.java @@ -0,0 +1,14 @@ +import org.jetbrains.annotations.Nullable; + +class Some { + public static void main(String arg, @Nullable StringBuilder sb) { + if (arg != null) { + return; + } + + if (sb != null) { } + + if (sb != null) { } + } + +} diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/LongDisjunctionsNotComplex.java b/java/java-tests/testData/inspection/dataFlow/fixture/LongDisjunctionsNotComplex.java index affd67c7742d..c816a5081407 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/LongDisjunctionsNotComplex.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/LongDisjunctionsNotComplex.java @@ -1,5 +1,8 @@ class Some { void getName(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int i9, int i10) { + if (i1 == 0 || i2 == 0 || i3 == 0 || i4 == 0 || i5 == 0 || i6 == 0 || i7 == 0 || i8 == 0 || i9 == 0 || i10 == 0) { + return; + } if (i1 == 1 || i1 == 2 || i1 == 3 || diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java index 30993c07b4ac..3f8a810419eb 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java @@ -297,6 +297,8 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase { public void testDontForgetInstanceofInfoWhenMerging() { doTest(); } public void testDontForgetEqInfoWhenMergingByType() { doTest(); } public void testDontMakeNullableAfterInstanceof() { doTest(); } + public void testDontMakeUnrelatedVariableNotNullWhenMerging() { doTest(); } + public void testDontLoseInequalityInformation() { doTest(); } public void _testNullCheckBeforeInstanceof() { doTest(); } // http://youtrack.jetbrains.com/issue/IDEA-113220 }