IDEA-138553 Inspection for usages of Optinoal.ofNullable() for values known to be null or non-null

2026-09-13 21:55:01 +07:00 · 2015-04-14 23:12:38 +03:00
parent 332d570706
commit 1b7a05e26f
19 changed files with 364 additions and 66 deletions
@@ -53,7 +53,6 @@ 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.Nls;
 import org.jetbrains.annotations.NonNls;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -211,7 +210,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
        }
      }

-      ContainerUtil.addIfNotNull(fixes, ReplaceOptionalOfWithOfNullableFix.registerReplaceOptionalOfWithOfNullableFix(qualifier));
+      ContainerUtil.addIfNotNull(fixes, DfaOptionalSupport.registerReplaceOptionalOfWithOfNullableFix(qualifier));
      return fixes.isEmpty() ? null : fixes.toArray(new LocalQuickFix[fixes.size()]);
    }
    catch (IncorrectOperationException e) {
@@ -272,11 +271,29 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
      reportNullableArgumentsPassedToNonAnnotated(visitor, holder, reportedAnchors);
    }

+    reportOptionalOfNullableImprovements(holder, visitor, reportedAnchors);
+
+
    if (REPORT_CONSTANT_REFERENCE_VALUES) {
      reportConstantReferenceValues(holder, visitor, reportedAnchors);
    }
  }

+  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 reportConstantReferenceValues(ProblemsHolder holder, StandardInstructionVisitor visitor, Set<PsiElement> reportedAnchors) {
    for (Pair<PsiReferenceExpression, DfaConstValue> pair : visitor.getConstantReferenceValues()) {
      PsiReferenceExpression ref = pair.first;
@@ -764,67 +781,4 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
      boolean normalOk;
    }
  }
-
-  private static class ReplaceOptionalOfWithOfNullableFix implements LocalQuickFix {
-
-    private static final String GUAVA_OPTIONAL = "com.google.common.base.Optional";
-    private final String myTargetMethodName;
-
-    public ReplaceOptionalOfWithOfNullableFix(final String targetMethodName) {
-      myTargetMethodName = targetMethodName;
-    }
-
-    private static LocalQuickFix registerReplaceOptionalOfWithOfNullableFix(PsiExpression qualifier) {
-      final PsiElement argList = PsiUtil.skipParenthesizedExprUp(qualifier).getParent();
-      if (argList instanceof PsiExpressionList) {
-        final PsiElement parent = argList.getParent();
-        if (parent instanceof PsiMethodCallExpression) {
-          final PsiMethod method = ((PsiMethodCallExpression)parent).resolveMethod();
-          if (method != null) {
-            final PsiClass containingClass = method.getContainingClass();
-            if ("of".equals(method.getName()) && containingClass != null) {
-              final String qualifiedName = containingClass.getQualifiedName();
-              if (CommonClassNames.JAVA_UTIL_OPTIONAL.equals(qualifiedName)) {
-                return new ReplaceOptionalOfWithOfNullableFix("ofNullable");
-              } 
-              else if (GUAVA_OPTIONAL.equals(qualifiedName)) {
-                return new ReplaceOptionalOfWithOfNullableFix("fromNullable");
-              }
-            }
-          }
-        }
-      }
-      return null;
-    }
-    
-    @Nls
-    @NotNull
-    @Override
-    public String getName() {
-      return getFamilyName();
-    }
-
-    @NotNull
-    @Override
-    public String getFamilyName() {
-      return "Replace with '." + myTargetMethodName + "()'";
-    }
-
-    @Override
-    public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
-      final PsiMethodCallExpression
-        methodCallExpression = PsiTreeUtil.getParentOfType(descriptor.getPsiElement(), PsiMethodCallExpression.class);
-      if (methodCallExpression != null) {
-        final PsiElement ofNullableExprName =
-          ((PsiMethodCallExpression)JavaPsiFacade.getElementFactory(project)
-            .createExpressionFromText("Optional.ofNullable(null)", null)).getMethodExpression();
-        final PsiElement referenceNameElement = methodCallExpression.getMethodExpression().getReferenceNameElement();
-        if (referenceNameElement != null) {
-          final PsiElement ofNullableNameElement = ((PsiReferenceExpression)ofNullableExprName).getReferenceNameElement();
-          LOG.assertTrue(ofNullableNameElement != null);
-          referenceNameElement.replace(ofNullableNameElement);
-        }
-      }
-    }
-  }
 }
@@ -0,0 +1,139 @@
+/*
+ * Copyright 2000-2015 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.intellij.codeInspection.dataFlow;
+
+import com.intellij.codeInspection.LocalQuickFix;
+import com.intellij.codeInspection.ProblemDescriptor;
+import com.intellij.openapi.diagnostic.Logger;
+import com.intellij.openapi.project.Project;
+import com.intellij.psi.*;
+import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.psi.util.PsiUtil;
+import org.jetbrains.annotations.Nls;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * @author anet, peter
+ */
+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";
+
+  @Nullable
+  static LocalQuickFix registerReplaceOptionalOfWithOfNullableFix(@NotNull PsiExpression qualifier) {
+    final PsiElement call = findCallExpression(qualifier);
+    final PsiMethod method = call == null ? null : ((PsiMethodCallExpression)call).resolveMethod();
+    final PsiClass containingClass = method == null ? null : method.getContainingClass();
+    if (containingClass != null && "of".equals(method.getName())) {
+      final String qualifiedName = containingClass.getQualifiedName();
+      if (CommonClassNames.JAVA_UTIL_OPTIONAL.equals(qualifiedName)) {
+        return new ReplaceOptionalCallFix("ofNullable", false);
+      }
+      if (GUAVA_OPTIONAL.equals(qualifiedName)) {
+        return new ReplaceOptionalCallFix("fromNullable", false);
+      }
+    }
+    return null;
+  }
+
+  private static PsiMethodCallExpression findCallExpression(@NotNull PsiElement anchor) {
+    final PsiElement argList = PsiUtil.skipParenthesizedExprUp(anchor).getParent();
+    if (argList instanceof PsiExpressionList) {
+      final PsiElement parent = argList.getParent();
+      if (parent instanceof PsiMethodCallExpression) {
+        return (PsiMethodCallExpression)parent;
+      }
+    }
+    return null;
+  }
+  private static boolean isJdkOptional(@NotNull PsiElement anchor) {
+    final PsiElement parent = findCallExpression(anchor);
+    PsiMethod method = parent == null ? null : resolveOfNullable(findCallExpression(anchor));
+    return method != null && "ofNullable".equals(method.getName());
+  }
+
+  @NotNull
+  static LocalQuickFix createReplaceOptionalOfNullableWithEmptyFix(@NotNull PsiElement anchor) {
+    return new ReplaceOptionalCallFix(isJdkOptional(anchor) ? "empty" : "absent", true);
+  }
+
+  @NotNull
+  static LocalQuickFix createReplaceOptionalOfNullableWithOfFix() {
+    return new ReplaceOptionalCallFix("of", false);
+  }
+
+  @Nullable
+  static PsiMethod resolveOfNullable(PsiCallExpression expression) {
+    String name = ((PsiMethodCallExpression)expression).getMethodExpression().getReferenceName();
+    if ("ofNullable".equals(name) || "fromNullable".equals(name)) {
+      PsiMethod method = expression.resolveMethod();
+      PsiClass psiClass = method == null ? null : method.getContainingClass();
+      String qname = psiClass == null ? null : psiClass.getQualifiedName();
+      if (CommonClassNames.JAVA_UTIL_OPTIONAL.equals(qname) || GUAVA_OPTIONAL.equals(qname)) {
+        return method;
+      }
+    }
+    return null;
+  }
+
+  private static class ReplaceOptionalCallFix implements LocalQuickFix {
+    private final String myTargetMethodName;
+    private final boolean myClearArguments;
+
+    public ReplaceOptionalCallFix(final String targetMethodName, boolean clearArguments) {
+      myTargetMethodName = targetMethodName;
+      myClearArguments = clearArguments;
+    }
+
+    @Nls
+    @NotNull
+    @Override
+    public String getName() {
+      return getFamilyName();
+    }
+
+    @NotNull
+    @Override
+    public String getFamilyName() {
+      return "Replace with '." + myTargetMethodName + "()'";
+    }
+
+    @Override
+    public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
+      final PsiMethodCallExpression
+        methodCallExpression = PsiTreeUtil.getParentOfType(descriptor.getPsiElement(), PsiMethodCallExpression.class);
+      if (methodCallExpression != null) {
+        final PsiElement ofNullableExprName =
+          ((PsiMethodCallExpression)JavaPsiFacade.getElementFactory(project)
+            .createExpressionFromText("Optional." + myTargetMethodName + "(null)", null)).getMethodExpression();
+        final PsiElement referenceNameElement = methodCallExpression.getMethodExpression().getReferenceNameElement();
+        if (referenceNameElement != null) {
+          final PsiElement ofNullableNameElement = ((PsiReferenceExpression)ofNullableExprName).getReferenceNameElement();
+          LOG.assertTrue(ofNullableNameElement != null);
+          referenceNameElement.replace(ofNullableNameElement);
+        }
+        if (myClearArguments) {
+          PsiExpressionList argList = methodCallExpression.getArgumentList();
+          PsiExpression[] args = argList.getExpressions();
+          if (args.length > 0) {
+            argList.deleteChildRange(args[0], args[args.length - 1]);
+          }
+        }
+      }
+    }
+  }
+}
@@ -11,4 +11,6 @@ public enum NullabilityProblem {
  nullableReturn,
  passingNullableToNotNullParameter,
  passingNullableArgumentToNonAnnotatedParameter,
+  passingNullToOptional,
+  passingNotNullToOptional
 }
@@ -57,6 +57,15 @@ 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) {
@@ -231,6 +240,10 @@ 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) {
        checkNotNullable(memState, arg, NullabilityProblem.passingNullableArgumentToNonAnnotatedParameter, expr);
      }
@@ -371,8 +384,14 @@ 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) {
+    if (notNullable &&
+        problem != NullabilityProblem.passingNullableArgumentToNonAnnotatedParameter &&
+        problem != NullabilityProblem.passingNullToOptional) {
      DfaValueFactory factory = ((DfaMemoryStateImpl)state).getFactory();
      state.applyCondition(factory.getRelationFactory().createRelation(value, factory.getConstFactory().getNull(), NE, false));
    }
@@ -0,0 +1,6 @@
+// "Replace with '.of()'" "true"
+class A{
+  void test(){
+    com.google.common.base.Optional.of(1<caret>1);
+  }
+}
@@ -0,0 +1,6 @@
+// "Replace with '.absent()'" "true"
+class A{
+  void test(){
+    com.google.common.base.Optional.absent(<caret>);
+  }
+}
@@ -0,0 +1,6 @@
+// "Replace with '.of()'" "true"
+class A{
+  void test(){
+    java.util.Optional.of(1<caret>1);
+  }
+}
@@ -0,0 +1,8 @@
+// "Replace with '.of()'" "true"
+
+class A{
+  void test(String s){
+    assert s != null;
+    java.util.Optional.of(<caret>s);
+  }
+}
@@ -0,0 +1,7 @@
+// "Replace with '.empty()'" "true"
+class A {
+  void test() {
+    String s = null;
+    java.util.Optional.empty(<caret>);
+  }
+}
@@ -0,0 +1,7 @@
+// "Replace with '.empty()'" "true"
+class A {
+  void test() {
+    String s = null;
+    java.util.Optional.empty(<caret>);
+  }
+}
@@ -0,0 +1,6 @@
+// "Replace with '.of()'" "true"
+class A{
+  void test(){
+    com.google.common.base.Optional.fromNullable(1<caret>1);
+  }
+}
@@ -0,0 +1,6 @@
+// "Replace with '.absent()'" "true"
+class A{
+  void test(){
+    com.google.common.base.Optional.fromNullable(n<caret>ull);
+  }
+}
@@ -0,0 +1,6 @@
+// "Replace with '.of()'" "true"
+class A{
+  void test(){
+    java.util.Optional.ofNullable(1<caret>1);
+  }
+}
@@ -0,0 +1,8 @@
+// "Replace with '.of()'" "true"
+
+class A{
+  void test(String s){
+    assert s != null;
+    java.util.Optional.ofNullable(<caret>s);
+  }
+}
@@ -0,0 +1,7 @@
+// "Replace with '.empty()'" "true"
+class A {
+  void test() {
+    String s = null;
+    java.util.Optional.ofNullable(n<caret>ull);
+  }
+}
@@ -0,0 +1,7 @@
+// "Replace with '.empty()'" "true"
+class A {
+  void test() {
+    String s = null;
+    java.util.Optional.ofNullable(<caret>s);
+  }
+}
@@ -0,0 +1,6 @@
+// "Replace with '.of()'" "false"
+class A{
+  void test(String s){
+    java.util.Optional.ofNullable(<caret>s);
+  }
+}
@@ -0,0 +1,86 @@
+/*
+ * Copyright 2000-2015 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/*
+ * User: anna
+ * Date: 21-Mar-2008
+ */
+package com.intellij.codeInsight.daemon.quickFix
+import com.intellij.codeInspection.LocalInspectionTool
+import com.intellij.codeInspection.dataFlow.DataFlowInspection
+import com.intellij.openapi.command.WriteCommandAction
+import com.intellij.openapi.projectRoots.Sdk
+import com.intellij.openapi.vfs.VfsUtil
+import com.intellij.openapi.vfs.VirtualFile
+import com.intellij.psi.JavaPsiFacade
+import com.intellij.psi.search.GlobalSearchScope
+import com.intellij.testFramework.IdeaTestUtil
+import org.jetbrains.annotations.NotNull
+
+public class ReplaceFromOfNullableFixTest extends LightQuickFixParameterizedTestCase {
+  @NotNull
+  @Override
+  protected LocalInspectionTool[] configureLocalInspectionTools() {
+    return [new DataFlowInspection()] as LocalInspectionTool[]
+  }
+
+  public void test() throws Exception {
+     doAllTests();
+   }
+
+  @Override
+  protected String getBasePath() {
+    return "/codeInsight/daemonCodeAnalyzer/quickFix/replaceFromOfNullable";
+  }
+
+  static void addGuavaOptional() {
+    if (JavaPsiFacade.getInstance(project).findClass("com.google.common.base.Optional", GlobalSearchScope.allScope(project))) {
+      return
+    }
+
+    WriteCommandAction.runWriteCommandAction(project) {
+      VirtualFile optional = getSourceRoot()
+        .createChildDirectory(this, "com")
+        .createChildDirectory(this, "google")
+        .createChildDirectory(this, "common")
+        .createChildDirectory(this, "base")
+        .createChildData(this, "Optional.java");
+      VfsUtil.saveText(optional, """
+package com.google.common.base;
+public abstract class Optional<T> {
+  public static <T> Optional<T> absent() { }
+
+  public static <T> Optional<T> of(T reference) { }
+
+  public static <T> Optional<T> fromNullable(@Nullable T nullableReference) { }
+}
+""")
+    }
+  }
+
+  @Override
+  protected void beforeActionStarted(String testName, String contents) {
+    if (testName.contains("Guava")) {
+      addGuavaOptional()
+    }
+    super.beforeActionStarted(testName, contents)
+  }
+
+  @Override
+  protected Sdk getProjectJDK() {
+    return IdeaTestUtil.getMockJdk18();
+  }
+}
@@ -17,6 +17,9 @@ package com.intellij.codeInspection;

 import com.intellij.codeInsight.completion.NormalCompletionDfaTest;
 import com.intellij.codeInsight.completion.SmartTypeCompletionDfaTest;
+import com.intellij.codeInsight.daemon.quickFix.AddAssertStatementFixTest;
+import com.intellij.codeInsight.daemon.quickFix.ReplaceFromOfNullableFixTest;
+import com.intellij.codeInsight.daemon.quickFix.ReplaceWithOfNullableFixTest;
 import com.intellij.slicer.SliceBackwardTest;
 import com.intellij.slicer.SliceTreeTest;
 import junit.framework.Test;
@@ -25,20 +28,29 @@ import junit.framework.TestSuite;
 public class DataFlowInspectionTestSuite {
  public static Test suite() {
    TestSuite suite = new TestSuite();
+
    suite.addTestSuite(DataFlowInspectionTest.class);
    suite.addTestSuite(DataFlowInspection8Test.class);
    suite.addTestSuite(DataFlowInspectionAncientTest.class);
    suite.addTestSuite(ContractCheckTest.class);
+
    suite.addTestSuite(ContractInferenceFromSourceTest.class);
    suite.addTestSuite(NullityInferenceFromSourceTestCase.DfaInferenceTest.class);
    suite.addTestSuite(NullityInferenceFromSourceTestCase.LightInferenceTest.class);
    suite.addTestSuite(PurityInferenceFromSourceTest.class);
+
    suite.addTestSuite(SliceTreeTest.class);
    suite.addTestSuite(SliceBackwardTest.class);
+
    suite.addTestSuite(SmartTypeCompletionDfaTest.class);
    suite.addTestSuite(NormalCompletionDfaTest.class);
+
    suite.addTestSuite(NullableStuffInspectionTest.class);
    suite.addTestSuite(NullableStuffInspection14Test.class);
+
+    suite.addTestSuite(AddAssertStatementFixTest.class);
+    suite.addTestSuite(ReplaceWithOfNullableFixTest.class);
+    suite.addTestSuite(ReplaceFromOfNullableFixTest.class);
    return suite;
  }
 }