Add UnnecessaryNotNullCheck rule (#5218)

* add `UnnecessaryNotNullCheck` rule

* change issue name

* add tests

* change config order

* remove unexpected blank lines

* add backticks for expression text

* use fully qualified name

* update description

* change message

* add tests for generics

* fix detekt

* remove debug printing

* use value instead of `null` to be more clear about types

detekt-core/src/main/resources/default-detekt-config.yml

@@ -470,6 +470,8 @@ potential-bugs:
     active: false
   UnconditionalJumpStatementInLoop:
     active: false
+  UnnecessaryNotNullCheck:
+    active: false
   UnnecessaryNotNullOperator:
     active: true
   UnnecessarySafeCall:

detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/PotentialBugProvider.kt

@@ -39,6 +39,7 @@ class PotentialBugProvider : DefaultRuleSetProvider {
             @Suppress("DEPRECATION") RedundantElseInWhen(config),
             UnconditionalJumpStatementInLoop(config),
             UnnecessaryNotNullOperator(config),
+            UnnecessaryNotNullCheck(config),
             UnnecessarySafeCall(config),
             UnreachableCode(config),
             UnsafeCallOnNullableType(config),

detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheck.kt

@@ -0,0 +1,70 @@
+package io.gitlab.arturbosch.detekt.rules.bugs
+
+import io.gitlab.arturbosch.detekt.api.CodeSmell
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.Debt
+import io.gitlab.arturbosch.detekt.api.Entity
+import io.gitlab.arturbosch.detekt.api.Issue
+import io.gitlab.arturbosch.detekt.api.Rule
+import io.gitlab.arturbosch.detekt.api.Severity
+import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
+import io.gitlab.arturbosch.detekt.rules.isCalling
+import org.jetbrains.kotlin.name.FqName
+import org.jetbrains.kotlin.psi.KtCallExpression
+import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.psiUtil.getCallNameExpression
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.calls.util.getType
+import org.jetbrains.kotlin.types.typeUtil.TypeNullability
+import org.jetbrains.kotlin.types.typeUtil.nullability
+
+/**
+ * Reports unnecessary not-null checks with `requireNotNull` or `checkNotNull` that can be removed by the user.
+ *
+ * <noncompliant>
+ * var string = "foo"
+ * println(requireNotNull(string))
+ * </noncompliant>
+ *
+ * <compliant>
+ * var string : String? = "foo"
+ * println(requireNotNull(string))
+ * </compliant>
+ */
+@RequiresTypeResolution
+class UnnecessaryNotNullCheck(config: Config = Config.empty) : Rule(config) {
+
+    override val issue = Issue(
+        "UnnecessaryNotNullCheck",
+        Severity.Defect,
+        "Remove unnecessary not-null checks on non-null types.",
+        Debt.FIVE_MINS,
+    )
+
+    override fun visitCallExpression(expression: KtCallExpression) {
+        super.visitCallExpression(expression)
+
+        if (bindingContext == BindingContext.EMPTY) return
+
+        if (expression.isCalling(requireNotNullFunctionFqName, bindingContext) ||
+            expression.isCalling(checkNotNullFunctionFqName, bindingContext)
+        ) {
+            val argument = expression.valueArguments[0].lastChild as KtExpression
+            if (argument.getType(bindingContext)?.nullability() == TypeNullability.NOT_NULL) {
+                val callName = expression.getCallNameExpression()?.text
+                report(
+                    CodeSmell(
+                        issue = issue,
+                        entity = Entity.from(expression),
+                        message = "Using `$callName` on non-null `${argument.text}` is unnecessary",
+                    )
+                )
+            }
+        }
+    }
+
+    companion object {
+        private val requireNotNullFunctionFqName = FqName("kotlin.requireNotNull")
+        private val checkNotNullFunctionFqName = FqName("kotlin.checkNotNull")
+    }
+}

detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheckSpec.kt

@@ -0,0 +1,158 @@
+package io.gitlab.arturbosch.detekt.rules.bugs
+
+import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
+import io.gitlab.arturbosch.detekt.test.assertThat
+import io.gitlab.arturbosch.detekt.test.lintWithContext
+import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
+import org.junit.jupiter.api.Nested
+import org.junit.jupiter.api.Test
+
+@KotlinCoreEnvironmentTest
+class UnnecessaryNotNullCheckSpec(private val env: KotlinCoreEnvironment) {
+    private val subject = UnnecessaryNotNullCheck()
+
+    @Nested
+    inner class `check unnecessary not null checks` {
+
+        @Test
+        fun shouldDetectNotNullCallOnVariable() {
+            val code = """
+                val x = 5
+                val y = requireNotNull(x)
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings).hasTextLocations(18 to 35)
+        }
+
+        @Test
+        fun shouldDetectNotNullCallOnVariableUsingCheckNotNull() {
+            val code = """
+                val x = 5
+                val y = checkNotNull(x)
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings).hasTextLocations(18 to 33)
+        }
+
+        @Test
+        fun shouldDetectNotNullCallOnFunctionReturn() {
+            val code = """
+                fun foo(): Int {
+                    return 5
+                }
+                fun bar() {
+                    requireNotNull(foo())
+                }
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings).hasTextLocations(48 to 69)
+        }
+
+        @Test
+        fun shouldDetectWhenCallingDefinitelyNonNullableGenericFunction() {
+            val code = """
+                fun <T> foo(x: T & Any): T & Any {
+                    return x
+                }
+                fun bar() {
+                    requireNotNull(foo(5))
+                }
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings).hasTextLocations(66 to 88)
+        }
+
+        @Test
+        fun shouldDetectWhenCallingPrimitiveJavaMethod() {
+            val code = """
+                fun foo() {
+                    requireNotNull(System.currentTimeMillis())
+                }
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).hasSize(1)
+            assertThat(findings).hasTextLocations(16 to 58)
+        }
+    }
+
+    @Nested
+    inner class `check valid not null check usage` {
+
+        @Test
+        fun shouldIgnoreNotNullCallOnNullableVariableWithValue() {
+            val code = """
+                val x: Int? = 5
+                val y = requireNotNull(x)
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun shouldIgnoreNotNullCallOnNullableVariableWithNull() {
+            val code = """
+                val x: Int? = null
+                val y = requireNotNull(x)
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun shouldIgnoreNotNullCallOnNullableFunctionReturnWithValue() {
+            val code = """
+                fun foo(): Int? {
+                    return 5
+                }
+                fun bar() {
+                    requireNotNull(foo())
+                }
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun shouldIgnoreNotNullCallOnNullableFunctionReturnWithNull() {
+            val code = """
+                fun foo(): Int? {
+                    return null
+                }
+                fun bar() {
+                    requireNotNull(foo())
+                }
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun shouldIgnoreWhenCallingNullableGenericFunction() {
+            val code = """
+                fun <T> foo(x: T): T {
+                    return x
+                }
+                fun bar() {
+                    requireNotNull(foo<Int?>(5))
+                }
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun shouldIgnoreWhenCallingObjectJavaMethod() {
+            val code = """
+                fun foo() {
+                    requireNotNull(System.getLogger())
+                }
+            """.trimIndent()
+            val findings = subject.lintWithContext(env, code)
+            assertThat(findings).isEmpty()
+        }
+    }
+}