UselessCallOnNotNull: fix false positive for unresolved types (#4880)

Fix an issue where UselessCallOnNotNull would report issues when some of the argument types resolved as ErrorType. While it correctly handles cases where the BindingContext cannot resolve a type (the first test case), in cases where a call is made on an unknown type (the second test case, e.g. via takeIf{}) the BindingContext will report it as an ErrorType. Since ErrorType.isNullable() apparently returns false we need to make an exception to ensure the rule behaves as expected.

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UselessCallOnNotNull.kt

@@ -18,6 +18,7 @@ import org.jetbrains.kotlin.resolve.BindingContext
 import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
 import org.jetbrains.kotlin.resolve.calls.util.getType
 import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull
+import org.jetbrains.kotlin.types.ErrorType
 import org.jetbrains.kotlin.types.isNullable
 
 /*
@@ -92,22 +93,29 @@ class UselessCallOnNotNull(config: Config = Config.empty) : Rule(config) {
         val fqName = resolvedCall.resultingDescriptor.fqNameOrNull()
         if (fqName == listOfNotNull) {
             val varargs = resolvedCall.valueArguments.entries.single().value.arguments
-            if (varargs.none { it.isNullable() }) {
+            if (varargs.all { it.isNullable() == false }) {
                 report(CodeSmell(issue, Entity.from(expression), "Replace listOfNotNull with listOf"))
             }
         }
     }
 
-    private fun ValueArgument.isNullable(): Boolean {
-        val wrapperType = getArgumentExpression()?.getType(bindingContext) ?: return false
+    /**
+     * Determines whether this [ValueArgument] is nullable, returning null if its type cannot be
+     * determined.
+     */
+    private fun ValueArgument.isNullable(): Boolean? {
+        val wrapperType = getArgumentExpression()?.getType(bindingContext) ?: return null
         val type = if (getSpreadElement() != null) {
             // in case of a spread operator (`*list`),
             // we actually want to get the generic parameter from the collection
             wrapperType.arguments.first().type
         } else {
             wrapperType
         }
-        return type.isNullable()
+
+        return type
+            .takeUnless { it is ErrorType }
+            ?.isNullable()
     }
 
     private data class Conversion(val replacementName: String? = null)

detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UselessCallOnNotNullSpec.kt

@@ -2,6 +2,7 @@ package io.gitlab.arturbosch.detekt.rules.style
 
 import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
 import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
+import io.gitlab.arturbosch.detekt.test.lintWithContext
 import org.assertj.core.api.Assertions.assertThat
 import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
 import org.junit.jupiter.api.Test
@@ -233,6 +234,32 @@ class UselessCallOnNotNullSpec(val env: KotlinCoreEnvironment) {
         assertThat(findings).isEmpty()
     }
 
+    @Test
+    fun `does not report when calling listOfNotNull with values whose type is unknown`() {
+        val code = """
+            fun test() {
+                listOfNotNull(unknown)
+            }
+        """.trimIndent()
+
+        val findings = subject.lintWithContext(env, code)
+        assertThat(findings).isEmpty()
+    }
+
+    @Test
+    fun `does not report when calling listOfNotNull with values whose type is derived and unknown`() {
+        val code = """
+            import kotlin.random.Random
+
+            fun test() {
+                listOfNotNull(unknown.takeIf { Random.nextBoolean() })
+            }
+        """.trimIndent()
+
+        val findings = subject.lintWithContext(env, code)
+        assertThat(findings).isEmpty()
+    }
+
     @Test
     fun `reports when calling isNullOrEmpty on a list`() {
         val code = """