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 27c8971bd80c..b462090126a2 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 @@ -36,6 +36,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaConstValue; import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; +import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.Pair; import com.intellij.openapi.util.WriteExternalException; import com.intellij.openapi.util.text.StringUtil; @@ -48,6 +49,7 @@ import com.intellij.util.ArrayUtil; import com.intellij.util.ArrayUtilRt; import com.intellij.util.IncorrectOperationException; import com.intellij.util.SmartList; +import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.MultiMap; import org.jdom.Element; import org.jetbrains.annotations.NonNls; @@ -696,6 +698,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { private static class DataFlowInstructionVisitor extends StandardInstructionVisitor { private final StandardDataFlowRunner myRunner; private final MultiMap myProblems = new MultiMap(); + private final Map, StateInfo> myStateInfos = ContainerUtil.newHashMap(); private DataFlowInstructionVisitor(StandardDataFlowRunner runner) { myRunner = runner; @@ -706,8 +709,17 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { myRunner.onInstructionProducesCCE(instruction); } - Collection getProblems(NullabilityProblem kind) { - return myProblems.get(kind); + Collection getProblems(final NullabilityProblem kind) { + return ContainerUtil.filter(myProblems.get(kind), new Condition() { + @Override + public boolean value(PsiElement psiElement) { + StateInfo info = myStateInfos.get(Pair.create(kind, psiElement)); + // non-ephemeral NPE should be reported + // ephemeral NPE should also be reported if only ephemeral states have reached a particular problematic instruction + // (e.g. if it's inside "if (var == null)" check after contract method invocation + return info.normalNpe || info.ephemeralNpe && !info.normalOk; + } + }); } @Override @@ -716,7 +728,24 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { if (!ok && anchor != null) { myProblems.putValue(problem, anchor); } + Pair key = Pair.create(problem, anchor); + StateInfo info = myStateInfos.get(key); + if (info == null) { + myStateInfos.put(key, info = new StateInfo()); + } + if (state.isEphemeral() && !ok) { + info.ephemeralNpe = true; + } else if (!state.isEphemeral()) { + if (ok) info.normalOk = true; + else info.normalNpe = true; + } return ok; } + + private static class StateInfo { + boolean ephemeralNpe; + boolean normalNpe; + boolean normalOk; + } } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java index 8309670020ab..106267affda4 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java @@ -59,4 +59,13 @@ public interface DfaMemoryState { @Nullable DfaConstValue getConstantValue(DfaVariableValue value); + + /** + * Ephemeral means a state that was created when considering a method contract and checking if one of its arguments is null. + * With explicit null check, that would result in any non-annotated variable being treated as nullable and producing possible NPE warnings later. + * With contracts, we don't want this. So the state where this variable is null is marked ephemeral and no NPE warnings are issued for such states. + */ + void markEphemeral(); + + boolean isEphemeral(); } 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 5405d36c0637..269f7255e92e 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 @@ -47,6 +47,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { private final TLongHashSet myDistinctClasses = new TLongHashSet(); private final THashMap myVariableStates = new THashMap(); private final THashSet myUnknownVariables = new THashSet(); + private boolean myEphemeral; public DfaMemoryStateImpl(final DfaValueFactory factory) { myFactory = factory; @@ -85,6 +86,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { if (!(obj instanceof DfaMemoryStateImpl)) return false; DfaMemoryStateImpl that = (DfaMemoryStateImpl)obj; + if (myEphemeral != that.myEphemeral) return false; if (myDistinctClasses.size() != that.myDistinctClasses.size()) return false; if (myStack.size() != that.myStack.size()) return false; if (myOffsetStack.size() != that.myOffsetStack.size()) return false; @@ -506,6 +508,16 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return result; } + @Override + public void markEphemeral() { + myEphemeral = true; + } + + @Override + public boolean isEphemeral() { + return myEphemeral; + } + @Override public boolean applyInstanceofOrNull(DfaRelationValue dfaCond) { DfaValue left = dfaCond.getLeftOperand(); 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 e782f5049194..8fcf2ef28614 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 @@ -135,6 +135,10 @@ public class DfaVariableState implements Cloneable { return buf.toString(); } + public Nullness getNullability() { + return myNullability; + } + public boolean isNotNull() { return myNullability == Nullness.NOT_NULL; } 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 ba3b4efe731b..90a64901c424 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 @@ -344,6 +344,8 @@ public class StandardInstructionVisitor extends InstructionVisitor { myCanBeNullInInstanceof.add(instruction); + boolean specialContractTreatment = isUnknownComparisonWithNullInContract(instruction, dfaLeft, dfaRight, factory, memState); + ArrayList states = new ArrayList(); final DfaMemoryState trueCopy = memState.createCopy(); @@ -351,6 +353,9 @@ public class StandardInstructionVisitor extends InstructionVisitor { if (!dfaRelation.isNegated()) { checkOneOperandNotNull(dfaRight, dfaLeft, factory, trueCopy); } + if (specialContractTreatment && !dfaRelation.isNegated()) { + trueCopy.markEphemeral(); + } trueCopy.push(factory.getConstFactory().getTrue()); instruction.setTrueReachable(); states.add(new DfaInstructionState(next, trueCopy)); @@ -362,6 +367,9 @@ public class StandardInstructionVisitor extends InstructionVisitor { if (dfaRelation.isNegated()) { checkOneOperandNotNull(dfaRight, dfaLeft, factory, falseCopy); } + if (specialContractTreatment && dfaRelation.isNegated()) { + falseCopy.markEphemeral(); + } falseCopy.push(factory.getConstFactory().getFalse()); instruction.setFalseReachable(); states.add(new DfaInstructionState(next, falseCopy)); @@ -373,6 +381,23 @@ public class StandardInstructionVisitor extends InstructionVisitor { return states.toArray(new DfaInstructionState[states.size()]); } + private static boolean isUnknownComparisonWithNullInContract(BinopInstruction instruction, + DfaValue dfaLeft, + DfaValue dfaRight, + DfaValueFactory factory, + DfaMemoryState memoryState) { + if (instruction.getPsiAnchor() != null || dfaRight != factory.getConstFactory().getNull()) { + return false; + } + if (dfaLeft instanceof DfaVariableValue) { + return ((DfaMemoryStateImpl)memoryState).getVariableState((DfaVariableValue)dfaLeft).getNullability() == Nullness.UNKNOWN; + } + if (dfaLeft instanceof DfaTypeValue) { + return ((DfaTypeValue)dfaLeft).getNullness() == Nullness.UNKNOWN; + } + return false; + } + public void skipConstantConditionReporting(@Nullable PsiElement anchor) { ContainerUtil.addIfNotNull(myNotToReportReachability, anchor); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaTypeValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaTypeValue.java index 373c913afb57..193d3426ea29 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaTypeValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaTypeValue.java @@ -81,6 +81,10 @@ public class DfaTypeValue extends DfaValue { return myNullness == Nullness.NOT_NULL; } + public Nullness getNullness() { + return myNullness; + } + @NonNls public String toString() { return myType + ", nullable=" + myNullness; diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ContractPreservesUnknownNullability.java b/java/java-tests/testData/inspection/dataFlow/fixture/ContractPreservesUnknownNullability.java new file mode 100644 index 000000000000..d6957fb46035 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ContractPreservesUnknownNullability.java @@ -0,0 +1,19 @@ +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; + +public class Foo { + + @Contract("null->null") + String foo(String s){ + return s; + } + + void bar(String s, String s2) { + foo(s); + s.hashCode(); + goo(foo(s2)); + } + + void goo(@NotNull String s) {} + +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java index b0df0fb12b85..108e2b2e7953 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspectionTest.java @@ -286,6 +286,7 @@ public class DataFlowInspectionTest extends LightCodeInsightFixtureTestCase { public void testContractAnnotation() { doTest(); } public void testContractInLoopNotTooComplex() { doTest(); } public void testContractWithNullable() { doTest(); } + public void testContractPreservesUnknownNullability() { doTest(); } public void testBoxingImpliesNotNull() { doTest(); } public void testLargeIntegersAreNotEqualWhenBoxed() { doTest(); }