IDEA-165108 Objects.requireNonNull with argument of primitive type should be reported as warning

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

@@ -0,0 +1,134 @@
+/*
+ * Copyright 2000-2017 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;
+
+import com.intellij.codeInspection.dataFlow.ControlFlowAnalyzer;
+import com.intellij.codeInspection.dataFlow.MethodContract;
+import com.intellij.codeInspection.dataFlow.StandardMethodContract;
+import com.intellij.openapi.project.Project;
+import com.intellij.psi.*;
+import com.intellij.psi.util.PsiTreeUtil;
+import com.intellij.psi.util.PsiUtil;
+import com.intellij.util.ObjectUtils;
+import com.siyeh.ig.callMatcher.CallMatcher;
+import com.siyeh.ig.psiutils.BlockUtils;
+import com.siyeh.ig.psiutils.CommentTracker;
+import com.siyeh.ig.psiutils.ExpressionUtils;
+import com.siyeh.ig.psiutils.SideEffectChecker;
+import one.util.streamex.StreamEx;
+import org.jetbrains.annotations.Nls;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+import java.util.List;
+
+/**
+ * @author Tagir Valeev
+ */
+public class ObviousNullCheckInspection extends BaseJavaBatchLocalInspectionTool {
+  // Methods which are known to return the null-checked argument,
+  // so calling them is useless even if return value is used
+  private static final CallMatcher REQUIRE_NON_NULL_METHOD = CallMatcher.anyOf(
+    CallMatcher.staticCall("java.util.Objects", "requireNonNull"),
+    CallMatcher.staticCall("com.google.common.base.Preconditions", "checkNotNull"));
+
+  @NotNull
+  @Override
+  public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
+    return new JavaElementVisitor() {
+      @Override
+      public void visitMethodCallExpression(PsiMethodCallExpression call) {
+        Integer nullIndex = getNullParameterIndex(call);
+        if (nullIndex == null) return;
+        if (!(call.getParent() instanceof PsiExpressionStatement) && !REQUIRE_NON_NULL_METHOD.test(call)) return;
+        PsiExpression[] args = call.getArgumentList().getExpressions();
+        if (args.length <= nullIndex) return;
+        PsiExpression nullArg = PsiUtil.skipParenthesizedExprDown(args[nullIndex]);
+        String explanation = getObviouslyNonNullExplanation(nullArg);
+        if (explanation == null) return;
+        holder.registerProblem(nullArg, InspectionsBundle.message("inspection.useless.null.check.message", explanation),
+                               new RemoveNullCheckFix());
+      }
+    };
+  }
+
+  @Nullable
+  private static String getObviouslyNonNullExplanation(PsiExpression arg) {
+    if (arg == null) return null;
+    if (arg instanceof PsiNewExpression) return "newly created object";
+    if (arg instanceof PsiLiteralExpression && !ExpressionUtils.isNullLiteral(arg)) return "literal";
+    if (arg.getType() instanceof PsiPrimitiveType) return "a value of primitive type";
+    if (arg instanceof PsiPolyadicExpression && ((PsiPolyadicExpression)arg).getOperationTokenType() == JavaTokenType.PLUS) {
+      return "concatenation";
+    }
+    return null;
+  }
+
+  @Nullable
+  private static Integer getNullParameterIndex(PsiMethodCallExpression call) {
+    PsiMethod method = call.resolveMethod();
+    if (method == null) return null;
+    if (!ControlFlowAnalyzer.isPure(method)) return null;
+    List<? extends MethodContract> contracts = ControlFlowAnalyzer.getMethodCallContracts(method, call);
+    if (contracts.size() != 1) return null;
+    StandardMethodContract contract = ObjectUtils.tryCast(contracts.get(0), StandardMethodContract.class);
+    if (contract == null || contract.getReturnValue() != MethodContract.ValueConstraint.THROW_EXCEPTION) return null;
+    MethodContract.ValueConstraint[] arguments = contract.arguments;
+    Integer nullIndex = null;
+    for (int i = 0; i < arguments.length; i++) {
+      MethodContract.ValueConstraint argument = arguments[i];
+      if (argument == MethodContract.ValueConstraint.NULL_VALUE) {
+        if (nullIndex != null) return null;
+        nullIndex = i;
+      }
+      else if (argument != MethodContract.ValueConstraint.ANY_VALUE) {
+        return null;
+      }
+    }
+    return nullIndex;
+  }
+
+  public static class RemoveNullCheckFix implements LocalQuickFix {
+    @Nls
+    @NotNull
+    @Override
+    public String getFamilyName() {
+      return InspectionsBundle.message("inspection.useless.null.check.fix.family.name");
+    }
+
+    @Override
+    public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
+      PsiElement startElement = descriptor.getStartElement();
+      PsiMethodCallExpression call = PsiTreeUtil.getParentOfType(startElement, PsiMethodCallExpression.class);
+      if (call == null) return;
+      PsiElement parent = call.getParent();
+      CommentTracker ct = new CommentTracker();
+      if (parent instanceof PsiExpressionStatement) {
+        PsiElementFactory factory = JavaPsiFacade.getElementFactory(project);
+        PsiStatement[] sideEffectStatements = StreamEx.of(call.getArgumentList().getExpressions())
+          .flatCollection(SideEffectChecker::extractSideEffectExpressions)
+          .map(expr -> factory.createStatementFromText(ct.text(expr) + ";", call))
+          .toArray(PsiStatement[]::new);
+        if(sideEffectStatements.length > 0) {
+          BlockUtils.addBefore((PsiStatement)parent, sideEffectStatements);
+        }
+        ct.deleteAndRestoreComments(parent);
+      } else {
+        ct.replaceAndRestoreComments(call, ct.markUnchanged(startElement));
+      }
+    }
+  }
+}

java/java-tests/testData/inspection/obviousNotNull/ObviousNullCheck.java

@@ -0,0 +1,29 @@
+import org.jetbrains.annotations.*;
+import java.util.*;
+
+abstract class ObviousNullCheck {
+  abstract String getFoo();
+
+  abstract String getBar();
+
+  void test() {
+    assertNotNull(<warning descr="Useless null-check: a value of primitive type is never null">5 + 6</warning>);
+
+    Objects.requireNonNull(<warning descr="Useless null-check: literal is never null">"xyz"</warning>, "xyz");
+    Objects.requireNonNull((<warning descr="Useless null-check: concatenation is never null">getFoo() + getBar()</warning>));
+    Objects.requireNonNull(<warning descr="Useless null-check: newly created object is never null">new ArrayList()</warning>, () -> "new returned null");
+
+    String s = Objects.requireNonNull(<warning descr="Useless null-check: literal is never null">" x "</warning>);
+    String s1 = trim(" x ");
+  }
+
+  @Contract(value="null -> fail", pure=true)
+  String trim(String s) {
+    return s.trim();
+  }
+
+  @Contract(value="null -> fail", pure=true)
+  static void assertNotNull(Object obj) {
+    if(obj == null) throw new NullPointerException();
+  }
+}

java/java-tests/testData/inspection/obviousNotNull/afterNonNull.java

@@ -0,0 +1,7 @@
+// "Remove useless null-check" "true"
+import java.util.*;
+
+public class Test {
+  public void test() {
+  }
+}

java/java-tests/testData/inspection/obviousNotNull/afterNonNullAssign.java

@@ -0,0 +1,10 @@
+// "Remove useless null-check" "true"
+import java.util.*;
+
+public class Test {
+  Test(int x) {}
+
+  public void test() {
+    String s = new Test(1)+":"+new Test(2)+":"+new Test(3+new Test(4).hashCode());
+  }
+}

java/java-tests/testData/inspection/obviousNotNull/afterNonNullSideEffect.java

@@ -0,0 +1,12 @@
+// "Remove useless null-check" "true"
+import java.util.*;
+
+public class Test {
+  Test(int x) {}
+
+  public void test() {
+      new Test(1);
+      new Test(2);
+      new Test(3 + new Test(4).hashCode());
+  }
+}

java/java-tests/testData/inspection/obviousNotNull/beforeNonNull.java

@@ -0,0 +1,8 @@
+// "Remove useless null-check" "true"
+import java.util.*;
+
+public class Test {
+  public void test() {
+    Objects.requireNonNull(5<caret>5);
+  }
+}

java/java-tests/testData/inspection/obviousNotNull/beforeNonNullAssign.java

@@ -0,0 +1,10 @@
+// "Remove useless null-check" "true"
+import java.util.*;
+
+public class Test {
+  Test(int x) {}
+
+  public void test() {
+    String s = Objects.requireNonNull(new <caret>Test(1)+":"+new Test(2)+":"+new Test(3+new Test(4).hashCode()));
+  }
+}

java/java-tests/testData/inspection/obviousNotNull/beforeNonNullSideEffect.java

@@ -0,0 +1,10 @@
+// "Remove useless null-check" "true"
+import java.util.*;
+
+public class Test {
+  Test(int x) {}
+
+  public void test() {
+    Objects.requireNonNull(new <caret>Test(1)+":"+new Test(2)+":"+new Test(3+new Test(4).hashCode()));
+  }
+}

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

@@ -0,0 +1,77 @@
+/*
+ * Copyright 2000-2017 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;
+
+import com.intellij.JavaTestUtil;
+import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase;
+import com.intellij.openapi.projectRoots.Sdk;
+import com.intellij.testFramework.IdeaTestUtil;
+import com.intellij.testFramework.LightProjectDescriptor;
+import com.intellij.testFramework.PsiTestUtil;
+import com.intellij.testFramework.fixtures.DefaultLightProjectDescriptor;
+import com.siyeh.ig.LightInspectionTestCase;
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * @author Tagir Valeev
+ */
+public class ObviousNullCheckInspectionTest extends LightInspectionTestCase {
+  public static final String TEST_DATA_DIR = "/inspection/obviousNotNull/";
+
+  private static final LightProjectDescriptor JAVA_8_WITH_ANNOTATIONS = new DefaultLightProjectDescriptor() {
+    @Override
+    public Sdk getSdk() {
+      return PsiTestUtil.addJdkAnnotations(IdeaTestUtil.getMockJdk18());
+    }
+  };
+
+  public void testObviousNullCheck() {
+    doTest();
+  }
+
+  @Override
+  protected InspectionProfileEntry getInspection() {
+    return new ObviousNullCheckInspection();
+  }
+
+  @Override
+  protected String getBasePath() {
+    return JavaTestUtil.getRelativeJavaTestDataPath() + TEST_DATA_DIR;
+  }
+
+  @NotNull
+  @Override
+  protected LightProjectDescriptor getProjectDescriptor() {
+    return JAVA_8_WITH_ANNOTATIONS;
+  }
+
+  public static class ObviousNullCheckInspectionFixTest extends LightQuickFixParameterizedTestCase {
+    @NotNull
+    @Override
+    protected LocalInspectionTool[] configureLocalInspectionTools() {
+      return new LocalInspectionTool[]{new ObviousNullCheckInspection()};
+    }
+
+    public void test() throws Exception {
+      doAllTests();
+    }
+
+    @Override
+    protected String getBasePath() {
+      return TEST_DATA_DIR;
+    }
+  }
+}

java/jdkAnnotations/java/util/annotations.xml

@@ -1099,12 +1099,20 @@
   </item>
   <item name='java.util.Objects T requireNonNull(T)'>
     <annotation name='org.jetbrains.annotations.Contract'>
-      <val val="&quot;null-&gt;fail&quot;"/>
+      <val name="value" val="&quot;null-&gt;fail&quot;"/>
+      <val name="pure" val="true"/>
     </annotation>
   </item>
   <item name='java.util.Objects T requireNonNull(T, java.lang.String)'>
     <annotation name='org.jetbrains.annotations.Contract'>
-      <val val="&quot;null,_-&gt;fail&quot;"/>
+      <val name="value" val="&quot;null,_-&gt;fail&quot;"/>
+      <val name="pure" val="true"/>
+    </annotation>
+  </item>
+  <item name='java.util.Objects T requireNonNull(T, java.util.function.Supplier&lt;java.lang.String&gt;)'>
+    <annotation name='org.jetbrains.annotations.Contract'>
+      <val name="value" val="&quot;null, _ -&gt; fail&quot;"/>
+      <val name="pure" val="true"/>
     </annotation>
   </item>
   <item name='java.util.Optional T get()'>

platform/platform-resources-en/src/messages/InspectionsBundle.properties

@@ -866,3 +866,6 @@ inspection.reflection.member.access.check.exists.exclude.chooser=Class to exclud
 
 inspection.replace.with.trivial.lambda.fix.family.name=Replace with trivial lambda
 inspection.replace.with.trivial.lambda.fix.name=Replace with lambda returning ''{0}''
+
+inspection.useless.null.check.message=Useless null-check: {0} is never null
+inspection.useless.null.check.fix.family.name=Remove useless null-check

resources-en/src/inspectionDescriptions/ObviousNullCheck.html

@@ -0,0 +1,8 @@
+<html>
+<body>
+<p>This inspection reports if null-checking method (like <code>Objects.requireNonNull</code> or <code>Assert.assertNotNull</code>) is
+called on the value which is obviously non-null. Such check is redundant and may indicate a programming error.</p>
+<!-- tooltip end -->
+<p>New in 2017.2</p>
+</body>
+</html>

resources/src/META-INF/IdeaPlugin.xml

@@ -889,6 +889,11 @@
                      groupKey="group.names.declaration.redundancy" enabledByDefault="true" level="WARNING"
                      implementationClass="com.intellij.codeInspection.RedundantStreamOptionalCallInspection"
                      displayName="Redundant step in Stream or Optional call chain"/>
+    <localInspection groupPath="Java" language="JAVA" shortName="ObviousNullCheck"
+                     groupBundle="messages.InspectionsBundle"
+                     groupKey="group.names.declaration.redundancy" enabledByDefault="true" level="WARNING"
+                     implementationClass="com.intellij.codeInspection.ObviousNullCheckInspection"
+                     displayName="Null-check method is called with obviously non-null argument"/>
     <localInspection groupPath="Java" language="JAVA" shortName="SimplifyStreamApiCallChains"
                      groupBundle="messages.InspectionsBundle"
                      groupKey="group.names.declaration.redundancy" enabledByDefault="true" level="WARNING"