ConditionalCanBeOptionalInspection

Fixes IDEA-179273 Nullability check can be simplified when return type is Optional

java/java-impl/src/META-INF/JavaPlugin.xml

@@ -516,6 +516,11 @@
                      groupKey="group.names.code.style.issues" enabledByDefault="true" level="WARNING"
                      implementationClass="com.intellij.codeInspection.OptionalAssignedToNullInspection"
                      displayName="Null value for Optional type"/>
+    <localInspection groupPath="Java" language="JAVA" shortName="ConditionalCanBeOptional"
+                     groupBundle="messages.InspectionsBundle"
+                     groupKey="group.names.code.style.issues" enabledByDefault="true" level="INFORMATION"
+                     implementationClass="com.intellij.codeInspection.ConditionalCanBeOptionalInspection"
+                     displayName="Conditional can be replaced with Optional"/>
     <localInspection groupPath="Java,Java language level migration aids" language="JAVA" shortName="ReplaceNullCheck"
                      groupBundle="messages.InspectionsBundle"
                      groupKey="group.names.language.level.specific.issues.and.migration.aids9" enabledByDefault="true" level="WARNING"

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

@@ -0,0 +1,162 @@
+// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
+package com.intellij.codeInspection;
+
+import com.intellij.codeInspection.dataFlow.Nullness;
+import com.intellij.codeInspection.dataFlow.NullnessUtil;
+import com.intellij.codeInspection.util.LambdaGenerationUtil;
+import com.intellij.codeInspection.util.OptionalUtil;
+import com.intellij.openapi.project.Project;
+import com.intellij.profile.codeInspection.InspectionProjectProfileManager;
+import com.intellij.psi.*;
+import com.intellij.psi.codeStyle.JavaCodeStyleManager;
+import com.intellij.psi.search.LocalSearchScope;
+import com.intellij.psi.search.searches.ReferencesSearch;
+import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.psi.util.PsiUtil;
+import com.siyeh.ig.psiutils.CommentTracker;
+import com.siyeh.ig.psiutils.ExpressionUtils;
+import com.siyeh.ig.psiutils.TypeUtils;
+import com.siyeh.ig.psiutils.VariableAccessUtils;
+import org.jetbrains.annotations.Contract;
+import org.jetbrains.annotations.Nls;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+public class ConditionalCanBeOptionalInspection extends AbstractBaseJavaLocalInspectionTool {
+  @NotNull
+  @Override
+  public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+    if (!PsiUtil.isLanguageLevel8OrHigher(holder.getFile())) {
+      return PsiElementVisitor.EMPTY_VISITOR;
+    }
+    return new JavaElementVisitor() {
+      @Override
+      public void visitConditionalExpression(PsiConditionalExpression ternary) {
+        TernaryNullCheck ternaryNullCheck = TernaryNullCheck.from(ternary);
+        if (ternaryNullCheck == null) return;
+        PsiVariable variable = ternaryNullCheck.myVariable;
+        PsiExpression nullBranch = ternaryNullCheck.myNullBranch;
+        PsiExpression notNullBranch = ternaryNullCheck.myNotNullBranch;
+        if (!ExpressionUtils.isSimpleExpression(nullBranch) && !LambdaGenerationUtil.canBeUncheckedLambda(nullBranch, variable::equals)) {
+          return;
+        }
+        if (!VariableAccessUtils.variableIsUsed(variable, notNullBranch) ||
+            !LambdaGenerationUtil.canBeUncheckedLambda(notNullBranch, variable::equals)) {
+          return;
+        }
+        if (!areTypesCompatible(nullBranch, notNullBranch)) return;
+        boolean mayChangeSemantics =
+          !ExpressionUtils.isNullLiteral(nullBranch) && NullnessUtil.getExpressionNullness(notNullBranch, true) != Nullness.NOT_NULL;
+        if (!isOnTheFly && mayChangeSemantics) return;
+        boolean informationLevel = mayChangeSemantics || InspectionProjectProfileManager.isInformationLevel(getShortName(), ternary);
+        holder.registerProblem(informationLevel ? ternary : ternary.getCondition(),
+                               "Can be replaced with Optional.ofNullable()",
+                               mayChangeSemantics ? ProblemHighlightType.INFORMATION : ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
+                               new ReplaceConditionWithOptionalFix(mayChangeSemantics));
+      }
+
+      private boolean areTypesCompatible(PsiExpression nullBranch, PsiExpression notNullBranch) {
+        PsiType notNullType = ((PsiExpression)notNullBranch.copy()).getType();
+        PsiType nullType = ((PsiExpression)nullBranch.copy()).getType();
+        if (nullType == null || notNullType == null) return false;
+        if (nullType.isAssignableFrom(notNullType)) return true;
+        if (nullType.equals(PsiType.NULL)) return true;
+        if (OptionalUtil.isOptionalEmptyCall(nullBranch) && TypeUtils.isOptional(notNullType)) return true;
+        return false;
+      }
+    };
+  }
+
+  private static class ReplaceConditionWithOptionalFix implements LocalQuickFix {
+    private boolean myChangesSemantics;
+
+    public ReplaceConditionWithOptionalFix(boolean changesSemantics) {
+      myChangesSemantics = changesSemantics;
+    }
+
+    @Nls
+    @NotNull
+    @Override
+    public String getName() {
+      return getFamilyName() + (myChangesSemantics ? " (may change semantics)" : "");
+    }
+
+    @Nls
+    @NotNull
+    @Override
+    public String getFamilyName() {
+      return "Replace with Optional.ofNullable() chain";
+    }
+
+    @Override
+    public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
+      PsiConditionalExpression ternary = PsiTreeUtil.getNonStrictParentOfType(descriptor.getStartElement(), PsiConditionalExpression.class);
+      TernaryNullCheck ternaryNullCheck = TernaryNullCheck.from(ternary);
+      if (ternaryNullCheck == null) return;
+      PsiVariable variable = ternaryNullCheck.myVariable;
+      String name = variable.getName();
+      if (name == null) return;
+      String inLambdaName = JavaCodeStyleManager.getInstance(project).suggestUniqueVariableName(name, ternary, true);
+      PsiExpression nullBranch = ternaryNullCheck.myNullBranch;
+      PsiExpression notNullBranch = ternaryNullCheck.myNotNullBranch;
+      PsiElementFactory factory = JavaPsiFacade.getElementFactory(project);
+      for (PsiReference reference : ReferencesSearch.search(variable, new LocalSearchScope(nullBranch)).findAll()) {
+        if (reference instanceof PsiReferenceExpression) {
+          PsiElement result = ((PsiReferenceExpression)reference).replace(factory.createExpressionFromText("null", ternary));
+          if (nullBranch == reference) {
+            nullBranch = (PsiExpression)result;
+          }
+        }
+      }
+      for (PsiReference reference : ReferencesSearch.search(variable, new LocalSearchScope(notNullBranch)).findAll()) {
+        if (reference instanceof PsiReferenceExpression) {
+          ExpressionUtils.bindReferenceTo((PsiReferenceExpression)reference, inLambdaName);
+        }
+      }
+      CommentTracker ct = new CommentTracker();
+      PsiLambdaExpression trueLambda =
+        (PsiLambdaExpression)factory.createExpressionFromText("(" + variable.getType().getCanonicalText() + " " +
+                                                              inLambdaName + ")->" + ct.text(notNullBranch), ternary);
+      PsiParameter lambdaParameter = trueLambda.getParameterList().getParameters()[0];
+      PsiExpression trueBody = (PsiExpression)trueLambda.getBody();
+      String replacement = OptionalUtil.generateOptionalUnwrap(CommonClassNames.JAVA_UTIL_OPTIONAL + ".ofNullable(" + name + ")",
+                                                               lambdaParameter, trueBody, ct.markUnchanged(nullBranch), ternary.getType(),
+                                                               !ExpressionUtils.isSimpleExpression(nullBranch));
+      PsiElement result = ct.replaceAndRestoreComments(ternary, replacement);
+      JavaCodeStyleManager.getInstance(project).shortenClassReferences(result);
+      LambdaCanBeMethodReferenceInspection.replaceAllLambdasWithMethodReferences(result);
+    }
+  }
+
+  private static class TernaryNullCheck {
+    final PsiVariable myVariable;
+    final PsiExpression myNullBranch;
+    final PsiExpression myNotNullBranch;
+
+    public TernaryNullCheck(PsiVariable variable, PsiExpression nullBranch, PsiExpression notNullBranch) {
+      myVariable = variable;
+      myNullBranch = nullBranch;
+      myNotNullBranch = notNullBranch;
+    }
+
+    @Contract("null -> null")
+    @Nullable
+    public static TernaryNullCheck from(@Nullable PsiConditionalExpression ternary) {
+      if (ternary == null) return null;
+      PsiExpression condition = ternary.getCondition();
+      boolean isNull = true;
+      PsiVariable variable = ExpressionUtils.getVariableFromNullComparison(condition, true);
+      if (variable == null) {
+        isNull = false;
+        variable = ExpressionUtils.getVariableFromNullComparison(condition, false);
+      }
+      if (variable == null || variable instanceof PsiField) return null;
+      PsiExpression nullBranch = isNull ? ternary.getThenExpression() : ternary.getElseExpression();
+      PsiExpression notNullBranch = isNull ? ternary.getElseExpression() : ternary.getThenExpression();
+      if (nullBranch == null || notNullBranch == null) {
+        return null;
+      }
+      return new TernaryNullCheck(variable, nullBranch, notNullBranch);
+    }
+  }
+}

java/java-impl/src/com/intellij/codeInspection/util/OptionalUtil.java

@@ -237,7 +237,7 @@ public class OptionalUtil {
         TypeConversionUtil.isAssignable(type, exprType)) {
       if (falseExpression == null) return "";
       PsiType falseType = falseExpression.getType();
-      if (falseType != null && falseType.isAssignableFrom(exprType)) return "";
+      if (falseType != null && (falseType.isAssignableFrom(exprType) || falseType.equals(PsiType.NULL))) return "";
     }
     return "<" + type.getCanonicalText() + ">";
   }

java/java-tests/testData/inspection/conditionalCanBeOptional/afterMapping.java

@@ -0,0 +1,9 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+import java.util.Optional;
+
+class Test {
+  String select(String foo) {
+    return Optional.ofNullable(foo).map(String::trim).orElse("");
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullable.java

@@ -0,0 +1,13 @@
+// "Replace with Optional.ofNullable() chain (may change semantics)" "INFORMATION"
+
+import java.util.Optional;
+
+class Test {
+  String trim(String s) {
+    return s.isEmpty() ? null : s.trim();
+  }
+
+  String select(String foo) {
+    return Optional.ofNullable(foo).map(this::trim).orElse("");
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/afterMappingNullableOk.java

@@ -0,0 +1,13 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+import java.util.Optional;
+
+class Test {
+  String trim(String s) {
+    return s.isEmpty() ? null : s.trim();
+  }
+
+  String select(String foo) {
+    return Optional.ofNullable(foo).map(this::trim).orElse(null);
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/afterOptionalReturn.java

@@ -0,0 +1,16 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+import java.util.Optional;
+
+class Test {
+  interface V {}
+
+  interface Type {
+    V getValue();
+  }
+
+  // IDEA-179273
+  public Optional<V> foo(Type arg) {
+    return Optional.ofNullable(arg).map(Type::getValue);
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/afterOrElseGet.java

@@ -0,0 +1,13 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+import java.util.Optional;
+
+class Test {
+  String getDefault() {
+    return "";
+  }
+
+  String select(String foo) {
+    return Optional.ofNullable(foo).map(String::trim).orElseGet(this::getDefault);
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/afterSimple.java

@@ -0,0 +1,9 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+import java.util.Optional;
+
+class Test {
+  String select(String foo, String bar) {
+    return Optional.ofNullable(foo).orElse(bar);
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMapping.java

@@ -0,0 +1,7 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+class Test {
+  String select(String foo) {
+    return foo !<caret>= null ? foo.trim() : "";
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullable.java

@@ -0,0 +1,11 @@
+// "Replace with Optional.ofNullable() chain (may change semantics)" "INFORMATION"
+
+class Test {
+  String trim(String s) {
+    return s.isEmpty() ? null : s.trim();
+  }
+
+  String select(String foo) {
+    return foo !<caret>= null ? trim(foo) : "";
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/beforeMappingNullableOk.java

@@ -0,0 +1,11 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+class Test {
+  String trim(String s) {
+    return s.isEmpty() ? null : s.trim();
+  }
+
+  String select(String foo) {
+    return foo !<caret>= null ? trim(foo) : null;
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOptionalReturn.java

@@ -0,0 +1,16 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+import java.util.Optional;
+
+class Test {
+  interface V {}
+
+  interface Type {
+    V getValue();
+  }
+
+  // IDEA-179273
+  public Optional<V> foo(Type arg) {
+    return arg =<caret>= null ? Optional.empty() : Optional.of(arg.getValue());
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/beforeOrElseGet.java

@@ -0,0 +1,11 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+class Test {
+  String getDefault() {
+    return "";
+  }
+
+  String select(String foo) {
+    return foo =<caret>= null ? getDefault() : foo.trim();
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/beforeSimple.java

@@ -0,0 +1,7 @@
+// "Replace with Optional.ofNullable() chain" "GENERIC_ERROR_OR_WARNING"
+
+class Test {
+  String select(String foo, String bar) {
+    return foo =<caret>= null ? bar : foo;
+  }
+}

java/java-tests/testData/inspection/conditionalCanBeOptional/beforeUnused.java

@@ -0,0 +1,7 @@
+// "Replace with Optional.ofNullable() chain" "false"
+
+class Test {
+  String select(String foo, String bar) {
+    return foo =<caret>= null ? bar : bar.trim();
+  }
+}

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

@@ -0,0 +1,27 @@
+// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
+package com.intellij.java.codeInspection;
+
+import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase;
+import com.intellij.codeInspection.ConditionalCanBeOptionalInspection;
+import com.intellij.codeInspection.LocalInspectionTool;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * @author Tagir Valeev
+ */
+public class ConditionalCanBeOptionalInspectionTest extends LightQuickFixParameterizedTestCase {
+  @NotNull
+  @Override
+  protected LocalInspectionTool[] configureLocalInspectionTools() {
+    return new LocalInspectionTool[]{new ConditionalCanBeOptionalInspection()};
+  }
+
+  public void test() {
+    doAllTests();
+  }
+
+  @Override
+  protected String getBasePath() {
+    return "/inspection/conditionalCanBeOptional/";
+  }
+}

resources-en/src/inspectionDescriptions/ConditionalCanBeOptional.html

@@ -0,0 +1,16 @@
+<html>
+<body>
+Suggests to replace a null-check condition with an <b>Optional</b> chain. E.g.
+<pre>return str == null ? "" : str.trim();</pre>
+Could be rewritten as
+<pre>return Optional.ofNullable(str).map(String::trim).orElse("");</pre>
+<p>While the replacement is not always shorter, this could be a helpful step for further refactoring
+  (e.g. changing the method return value to an Optional).</p>
+<p>Note that when not-null branch of the condition returns null, the corresponding mapping step will produce an empty Optional
+possibly changing the semantics. If it cannot be statically proven that semantics will be preserved, quick-fix action name
+will contain "(may change semantics)" notice and inspection highlighting will be turned off.</p>
+<!-- tooltip end -->
+<p>This inspection only reports if the project or module is configured to use a language level of 8 or higher.</p>
+<p><small>New in 2018.1</small></p>
+</body>
+</html>