IDEA-176397 Warn about pointless rewrapping of Optional value

Also fixed CommonDataflow: facts must be united, not intersected

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

@@ -37,10 +37,11 @@ public class CommonDataflow {
 
     void add(PsiExpression expression, DfaMemoryStateImpl memState) {
       DfaFactMap existing = myFacts.get(expression);
-      if(existing == null && myFacts.containsKey(expression)) return; // bottom
-      DfaValue value = memState.peek();
-      DfaFactMap newMap = memState.getFactMap(value);
-      myFacts.put(expression, DfaFactMap.intersect(existing == null ? DfaFactMap.EMPTY : existing, newMap));
+      if(existing != DfaFactMap.EMPTY) {
+        DfaValue value = memState.peek();
+        DfaFactMap newMap = memState.getFactMap(value);
+        myFacts.put(expression, existing == null ? newMap : existing.union(newMap));
+      }
     }
   }
 

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

@@ -22,7 +22,6 @@ import one.util.streamex.StreamEx;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
-import java.util.List;
 import java.util.Objects;
 
 /**
@@ -112,19 +111,31 @@ public final class DfaFactMap {
     return newFact == null ? null : with(type, newFact);
   }
 
-  private <TT> DfaFactMap intersect(DfaFactMap otherMap, @NotNull DfaFactType<TT> type) {
-    return intersect(type, otherMap.get(type));
+  /**
+   * Returns a fact map which additionally allows having supplied value for the supplied fact
+   *
+   * @param type  a type of a new fact
+   * @param value an additional fact value which should be allowed. Passing null means that fact may have any value
+   * @param <T>   a fact value type
+   * @return a new fact map. May return itself if it's known that new fact does not actually change this map.
+   */
+  @NotNull
+  public <T> DfaFactMap union(@NotNull DfaFactType<T> type, @Nullable T value) {
+    if (value == null) return with(type, null);
+    T curFact = get(type);
+    if (curFact == null) return this;
+    T newFact = type.unionFacts(curFact, value);
+    return with(type, newFact);
   }
 
-  @Nullable
-  public static DfaFactMap intersect(DfaFactMap map1, DfaFactMap map2) {
-    if(map1 == null || map2 == null) return null;
-    List<DfaFactType<?>> types = DfaFactType.getTypes();
-    for (DfaFactType<?> type : types) {
-      map1 = map1.intersect(map2, type);
-      if (map1 == null) return null;
-    }
-    return map1;
+  @NotNull
+  private <TT> DfaFactMap union(DfaFactMap otherMap, @NotNull DfaFactType<TT> type) {
+    return union(type, otherMap.get(type));
+  }
+
+  @NotNull
+  public DfaFactMap union(@NotNull DfaFactMap other) {
+    return StreamEx.of(DfaFactType.getTypes()).foldLeft(this, (map, type) -> map.union(other, type));
   }
 
   @Override

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

@@ -114,6 +114,12 @@ public abstract class DfaFactType<T> extends Key<T> {
       return LongRangeSet.fromType(var.getVariableType());
     }
 
+    @Nullable
+    @Override
+    LongRangeSet unionFacts(@NotNull LongRangeSet left, @NotNull LongRangeSet right) {
+      return left.union(right);
+    }
+
     @Nullable
     @Override
     LongRangeSet intersectFacts(@NotNull LongRangeSet left, @NotNull LongRangeSet right) {
@@ -160,6 +166,18 @@ public abstract class DfaFactType<T> extends Key<T> {
     return left.equals(right) ? left : null;
   }
 
+  /**
+   * Unites two facts of this type.
+   *
+   * @param left left fact
+   * @param right right fact
+   * @return union fact (null means that the fact can have any value)
+   */
+  @Nullable
+  T unionFacts(@NotNull T left, @NotNull T right) {
+    return left.equals(right) ? left : null;
+  }
+
   String toString(T fact) {
     return fact.toString();
   }

java/java-impl/src/com/intellij/codeInspection/SimplifyOptionalCallChainsInspection.java

@@ -15,6 +15,8 @@
  */
 package com.intellij.codeInspection;
 
+import com.intellij.codeInspection.dataFlow.CommonDataflow;
+import com.intellij.codeInspection.dataFlow.DfaFactType;
 import com.intellij.codeInspection.util.LambdaGenerationUtil;
 import com.intellij.codeInspection.util.OptionalUtil;
 import com.intellij.openapi.project.Project;
@@ -32,7 +34,7 @@ import org.jetbrains.annotations.Nls;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
-import java.util.*;
+import java.util.Objects;
 import java.util.regex.Pattern;
 
 import static com.intellij.util.ObjectUtils.tryCast;
@@ -44,12 +46,16 @@ import static com.intellij.util.ObjectUtils.tryCast;
 public class SimplifyOptionalCallChainsInspection extends BaseJavaBatchLocalInspectionTool {
   private static final CallMatcher OPTIONAL_OR_ELSE =
     CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "orElse").parameterCount(1);
+  private static final CallMatcher OPTIONAL_GET =
+    CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "get").parameterCount(0);
   private static final CallMatcher OPTIONAL_OR_ELSE_GET =
     CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "orElseGet").parameterCount(1);
   private static final CallMatcher OPTIONAL_MAP =
     CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "map").parameterCount(1);
   private static final CallMatcher OPTIONAL_OF_NULLABLE =
     CallMatcher.staticCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "ofNullable").parameterCount(1);
+  private static final CallMatcher OPTIONAL_OF_OF_NULLABLE =
+    CallMatcher.staticCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "ofNullable", "of").parameterCount(1);
 
 
   @NotNull
@@ -70,6 +76,10 @@ public class SimplifyOptionalCallChainsInspection extends BaseJavaBatchLocalInsp
   private static abstract class OptionalChainVisitor extends JavaElementVisitor {
     @Override
     public void visitMethodCallExpression(PsiMethodCallExpression call) {
+      if (OPTIONAL_GET.test(call)) {
+        handleRewrapping(call, OPTIONAL_OF_OF_NULLABLE);
+        return;
+      }
       PsiExpression falseArg = null;
       boolean useOrElseGet = false;
       if (OPTIONAL_OR_ELSE.test(call)) {
@@ -83,22 +93,27 @@ public class SimplifyOptionalCallChainsInspection extends BaseJavaBatchLocalInsp
       }
       if (falseArg == null) return;
       handleMapOrElse(call, useOrElseGet, falseArg);
-      handleOfNullableOrElse(call, falseArg);
+      if (ExpressionUtils.isNullLiteral(falseArg)) {
+        handleRewrapping(call, OPTIONAL_OF_NULLABLE);
+      }
       handleOrElseNullConditionalReturn(call, falseArg);
       handleOrElseNullConditionalAction(call, falseArg);
     }
 
-    private void handleOfNullableOrElse(PsiMethodCallExpression call, PsiExpression falseArg) {
-      if (!ExpressionUtils.isNullLiteral(falseArg)) return;
+    private void handleRewrapping(PsiMethodCallExpression call, CallMatcher wrapper) {
       PsiElement parent = PsiUtil.skipParenthesizedExprUp(call.getParent());
       if (!(parent instanceof PsiExpressionList)) return;
       PsiMethodCallExpression parentCall = tryCast(parent.getParent(), PsiMethodCallExpression.class);
-      if (!OPTIONAL_OF_NULLABLE.test(parentCall)) return;
+      if (!wrapper.test(parentCall)) return;
       PsiExpression qualifier = call.getMethodExpression().getQualifierExpression();
       if (qualifier == null ||
           !EquivalenceChecker.getCanonicalPsiEquivalence().typesAreEquivalent(qualifier.getType(), parentCall.getType())) {
         return;
       }
+      if ("get".equals(call.getMethodExpression().getReferenceName()) &&
+          !Boolean.TRUE.equals(CommonDataflow.getExpressionFact(qualifier, DfaFactType.OPTIONAL_PRESENCE))) {
+        return;
+      }
       SimplifyOptionalChainFix fix = new SimplifyOptionalChainFix(qualifier.getText(), "Unwrap", "Unnecessary Optional rewrapping");
       handleSimplification(Objects.requireNonNull(parentCall.getMethodExpression().getReferenceNameElement()), fix);
     }

java/java-tests/testData/inspection/optionalChains/afterRewrapGet.java

@@ -0,0 +1,10 @@
+// "Unwrap" "true"
+import java.util.*;
+
+public class Tests {
+  void test2(Optional<String> optional) {
+    if (optional.isPresent()) {
+      System.out.println(optional);
+    }
+  }
+}

java/java-tests/testData/inspection/optionalChains/afterRewrapGetSecond.java

@@ -0,0 +1,9 @@
+// "Unwrap" "true"
+import java.util.*;
+
+public class Tests {
+  void test3(Optional<String> optional) {
+    System.out.println(Optional.of(optional.get()));
+    System.out.println(optional);
+  }
+}

java/java-tests/testData/inspection/optionalChains/beforeRewrapGet.java

@@ -0,0 +1,10 @@
+// "Unwrap" "true"
+import java.util.*;
+
+public class Tests {
+  void test2(Optional<String> optional) {
+    if (optional.isPresent()) {
+      System.out.println(Optional.o<caret>f(optional.get()));
+    }
+  }
+}

java/java-tests/testData/inspection/optionalChains/beforeRewrapGetFirst.java

@@ -0,0 +1,9 @@
+// "Unwrap" "false"
+import java.util.*;
+
+public class Tests {
+  void test3(Optional<String> optional) {
+    System.out.println(Optional.o<caret>f(optional.get()));
+    System.out.println(Optional.of(optional.get()));
+  }
+}

java/java-tests/testData/inspection/optionalChains/beforeRewrapGetSecond.java

@@ -0,0 +1,9 @@
+// "Unwrap" "true"
+import java.util.*;
+
+public class Tests {
+  void test3(Optional<String> optional) {
+    System.out.println(Optional.of(optional.get()));
+    System.out.println(Optional.o<caret>f(optional.get()));
+  }
+}

java/java-tests/testData/inspection/optionalChains/beforeRewrapGetUnsure.java

@@ -0,0 +1,10 @@
+// "Unwrap" "false"
+import java.util.*;
+
+public class Tests {
+  void test(Optional<String> optional, boolean b) {
+    if (b || optional.isPresent()) {
+      System.out.println(Optional.o<caret>f(optional.get()));
+    }
+  }
+}