dfa, Optional.ofNullable: suggest improvements only if in all states the arg was null or non-null (IDEA-141843)

java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java

@@ -270,7 +270,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
       reportNullableArgumentsPassedToNonAnnotated(visitor, holder, reportedAnchors);
     }
 
-    reportOptionalOfNullableImprovements(holder, visitor, reportedAnchors);
+    reportOptionalOfNullableImprovements(holder, reportedAnchors, runner.getInstructions());
 
 
     if (REPORT_CONSTANT_REFERENCE_VALUES) {
@@ -278,18 +278,26 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
     }
   }
 
-  private static void reportOptionalOfNullableImprovements(ProblemsHolder holder,
-                                                           DataFlowInstructionVisitor visitor,
-                                                           HashSet<PsiElement> reportedAnchors) {
-    for (PsiElement expr : visitor.getProblems(NullabilityProblem.passingNullToOptional)) {
-      if (!reportedAnchors.add(expr)) continue;
-      holder.registerProblem(expr, "Passing <code>null</code> argument to Optional",
-                             DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(expr));
-    }
-    for (PsiElement expr : visitor.getProblems(NullabilityProblem.passingNotNullToOptional)) {
-      if (!reportedAnchors.add(expr)) continue;
-      holder.registerProblem(expr, "Passing a non-null argument to Optional",
-                             DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix());
+  private static void reportOptionalOfNullableImprovements(ProblemsHolder holder, Set<PsiElement> reportedAnchors, Instruction[] instructions) {
+    for (Instruction instruction : instructions) {
+      if (instruction instanceof MethodCallInstruction) {
+        final PsiExpression[] args = ((MethodCallInstruction)instruction).getArgs();
+        if (args.length != 1) continue;
+
+        final PsiExpression expr = args[0];
+
+        if (((MethodCallInstruction)instruction).isOptionalAlwaysNullProblem()) {
+          if (!reportedAnchors.add(expr)) continue;
+          holder.registerProblem(expr, "Passing <code>null</code> argument to <code>Optional</code>",
+                                 DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(expr));
+        }
+        else if (((MethodCallInstruction)instruction).isOptionalAlwaysNotNullProblem()) {
+          if (!reportedAnchors.add(expr)) continue;
+          holder.registerProblem(expr, "Passing a non-null argument to <code>Optional</code>",
+                                 DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix());
+        }
+        
+      }
     }
   }
 

java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java

@@ -29,7 +29,7 @@ import org.jetbrains.annotations.Nullable;
 /**
  * @author anet, peter
  */
-class DfaOptionalSupport {
+public class DfaOptionalSupport {
   private static final Logger LOG = Logger.getInstance("#com.intellij.codeInspection.dataFlow.DfaOptionalSupport");
   private static final String GUAVA_OPTIONAL = "com.google.common.base.Optional";
 
@@ -77,8 +77,8 @@ class DfaOptionalSupport {
   }
 
   @Nullable
-  static PsiMethod resolveOfNullable(PsiCallExpression expression) {
-    String name = ((PsiMethodCallExpression)expression).getMethodExpression().getReferenceName();
+  public static PsiMethod resolveOfNullable(@NotNull PsiMethodCallExpression expression) {
+    String name = expression.getMethodExpression().getReferenceName();
     if ("ofNullable".equals(name) || "fromNullable".equals(name)) {
       PsiMethod method = expression.resolveMethod();
       PsiClass psiClass = method == null ? null : method.getContainingClass();

java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullabilityProblem.java

@@ -10,7 +10,5 @@ public enum NullabilityProblem {
   assigningToNotNull,
   nullableReturn,
   passingNullableToNotNullParameter,
-  passingNullableArgumentToNonAnnotatedParameter,
-  passingNullToOptional,
-  passingNotNullToOptional
+  passingNullableArgumentToNonAnnotatedParameter
 }

java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java

@@ -57,15 +57,6 @@ public class StandardInstructionVisitor extends InstructionVisitor {
       return callExpression != null ? DfaPsiUtil.getElementNullability(key.getResultType(), callExpression.resolveMethod()) : null;
     }
   };
-  @SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
-  private final FactoryMap<MethodCallInstruction, Boolean> myOptionOfNullable = new FactoryMap<MethodCallInstruction, Boolean>() {
-    @Nullable
-    @Override
-    protected Boolean create(MethodCallInstruction key) {
-      PsiCallExpression expression = key.getCallExpression();
-      return expression instanceof PsiMethodCallExpression && DfaOptionalSupport.resolveOfNullable(expression) != null;
-    }
-  };
 
   @Override
   public DfaInstructionState[] visitAssign(AssignInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) {
@@ -240,11 +231,7 @@ public class StandardInstructionVisitor extends InstructionVisitor {
           forceNotNull(runner, memState, arg);
         }
       }
-      else if (myOptionOfNullable.get(instruction)) {
-        checkNotNullable(memState, arg, NullabilityProblem.passingNotNullToOptional, expr);
-        checkNotNullable(memState, arg, NullabilityProblem.passingNullToOptional, expr);
-      }
-      else if (requiredNullability == Nullness.UNKNOWN) {
+      else if (!instruction.updateOfNullable(memState, arg) && requiredNullability == Nullness.UNKNOWN) {
         checkNotNullable(memState, arg, NullabilityProblem.passingNullableArgumentToNonAnnotatedParameter, expr);
       }
     }
@@ -384,14 +371,9 @@ public class StandardInstructionVisitor extends InstructionVisitor {
   protected boolean checkNotNullable(DfaMemoryState state,
                                      DfaValue value, NullabilityProblem problem,
                                      PsiElement anchor) {
-    if (problem == NullabilityProblem.passingNotNullToOptional) {
-      return !state.isNotNull(value);
-    }
-
     boolean notNullable = state.checkNotNullable(value);
     if (notNullable &&
-        problem != NullabilityProblem.passingNullableArgumentToNonAnnotatedParameter &&
-        problem != NullabilityProblem.passingNullToOptional) {
+        problem != NullabilityProblem.passingNullableArgumentToNonAnnotatedParameter) {
       DfaValueFactory factory = ((DfaMemoryStateImpl)state).getFactory();
       state.applyCondition(factory.getRelationFactory().createRelation(value, factory.getConstFactory().getNull(), NE, false));
     }

java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java

@@ -46,8 +46,11 @@ public class MethodCallInstruction extends Instruction {
   private final List<MethodContract> myContracts;
   private final MethodType myMethodType;
   @Nullable private final DfaValue myPrecalculatedReturnValue;
+  private final boolean myOfNullable;
   private final boolean myVarArgCall;
   private final Map<PsiExpression, Nullness> myArgRequiredNullability;
+  private boolean myOnlyNullArgs = true;
+  private boolean myOnlyNotNullArgs = true;
 
   public enum MethodType {
     BOXING, UNBOXING, REGULAR_METHOD_CALL, CAST
@@ -64,6 +67,7 @@ public class MethodCallInstruction extends Instruction {
     myPrecalculatedReturnValue = null;
     myTargetMethod = null;
     myVarArgCall = false;
+    myOfNullable = false;
     myArgRequiredNullability = Collections.emptyMap();
   }
 
@@ -91,6 +95,7 @@ public class MethodCallInstruction extends Instruction {
 
     myShouldFlushFields = !(call instanceof PsiNewExpression && myType != null && myType.getArrayDimensions() > 0) && !isPureCall();
     myPrecalculatedReturnValue = precalculatedReturnValue;
+    myOfNullable = call instanceof PsiMethodCallExpression && DfaOptionalSupport.resolveOfNullable((PsiMethodCallExpression)call) != null;
   }
 
   private Map<PsiExpression, Nullness> calcArgRequiredNullability(PsiSubstitutor substitutor, PsiParameter[] parameters) {
@@ -191,4 +196,25 @@ public class MethodCallInstruction extends Instruction {
              ? "BOX" :
              "CALL_METHOD: " + (myCall == null ? "null" : myCall.getText());
   }
+
+  public boolean updateOfNullable(DfaMemoryState memState, DfaValue arg) {
+    if (!myOfNullable) return false;
+    
+    if (!memState.isNotNull(arg)) {
+      myOnlyNotNullArgs = false;
+    } 
+    if (!memState.isNull(arg)) {
+      myOnlyNullArgs = false;
+    }
+    return true;
+  }
+
+  public boolean isOptionalAlwaysNullProblem() {
+    return myOfNullable && myOnlyNullArgs;
+  }
+
+  public boolean isOptionalAlwaysNotNullProblem() {
+    return myOfNullable && myOnlyNotNullArgs;
+  }
+
 }

java/java-tests/testData/inspection/dataFlow/fixture/OptionalOfNullable.java

@@ -0,0 +1,24 @@
+import java.util.List;
+import java.util.Optional;
+
+class Test {
+  Optional<String> getName(List<String> numbers) {
+    return Optional.ofNullable(
+      numbers.isEmpty() ?
+      null :
+      numbers.get(0));
+  }
+  
+  Optional<String> getName2(List<String> numbers) {
+    return Optional.ofNullable(
+      numbers.isEmpty() ?
+      "2" :
+      numbers.get(0));
+  }
+
+  Optional<String> getName3() {
+    return Optional.ofNullable(<warning descr="Passing 'null' argument to 'Optional'">null</warning>);
+  }
+
+}
+

java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspection8Test.java

@@ -18,14 +18,20 @@ package com.intellij.codeInspection;
 import com.intellij.JavaTestUtil;
 import com.intellij.codeInsight.NullableNotNullManager;
 import com.intellij.codeInspection.dataFlow.DataFlowInspection;
-import com.intellij.openapi.Disposable;
 import com.intellij.openapi.util.Disposer;
+import com.intellij.testFramework.LightProjectDescriptor;
 import com.intellij.testFramework.fixtures.LightCodeInsightFixtureTestCase;
+import org.jetbrains.annotations.NotNull;
 
 /**
  * @author peter
  */
 public class DataFlowInspection8Test extends LightCodeInsightFixtureTestCase {
+  @NotNull
+  @Override
+  protected LightProjectDescriptor getProjectDescriptor() {
+    return JAVA_8;
+  }
 
   @Override
   protected String getTestDataPath() {
@@ -67,12 +73,9 @@ public class DataFlowInspection8Test extends LightCodeInsightFixtureTestCase {
     final NullableNotNullManager nnnManager = NullableNotNullManager.getInstance(getProject());
     nnnManager.setNotNulls("foo.NotNull");
     nnnManager.setNullables("foo.Nullable");
-    Disposer.register(myTestRootDisposable, new Disposable() {
-      @Override
-      public void dispose() {
-        nnnManager.setNotNulls();
-        nnnManager.setNullables();
-      }
+    Disposer.register(myTestRootDisposable, () -> {
+      nnnManager.setNotNulls();
+      nnnManager.setNullables();
     });
   }
 
@@ -91,4 +94,8 @@ public class DataFlowInspection8Test extends LightCodeInsightFixtureTestCase {
     myFixture.enableInspections(inspection);
     myFixture.testHighlighting(true, false, true, getTestName(false) + ".java");
   }
+
+
+  public void testOptionalOfNullable() { doTest(); }
+
 }