Extend CanBeNonNullable rule to check function params

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

@@ -9,20 +9,30 @@ 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.isNonNullCheck
+import io.gitlab.arturbosch.detekt.rules.isNullCheck
 import io.gitlab.arturbosch.detekt.rules.isOpen
+import org.jetbrains.kotlin.descriptors.DeclarationDescriptor
 import org.jetbrains.kotlin.descriptors.PropertyDescriptor
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.psi.KtBinaryExpression
 import org.jetbrains.kotlin.psi.KtClass
 import org.jetbrains.kotlin.psi.KtConstantExpression
+import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
 import org.jetbrains.kotlin.psi.KtExpression
 import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.kotlin.psi.KtIfExpression
+import org.jetbrains.kotlin.psi.KtNameReferenceExpression
+import org.jetbrains.kotlin.psi.KtNamedFunction
+import org.jetbrains.kotlin.psi.KtNullableType
+import org.jetbrains.kotlin.psi.KtParameter
+import org.jetbrains.kotlin.psi.KtPostfixExpression
 import org.jetbrains.kotlin.psi.KtProperty
 import org.jetbrains.kotlin.psi.KtPropertyAccessor
 import org.jetbrains.kotlin.psi.KtPropertyDelegate
 import org.jetbrains.kotlin.psi.KtReturnExpression
+import org.jetbrains.kotlin.psi.KtSafeQualifiedExpression
 import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
 import org.jetbrains.kotlin.psi.psiUtil.isPrivate
 import org.jetbrains.kotlin.resolve.BindingContext
@@ -49,6 +59,10 @@ import org.jetbrains.kotlin.types.isNullable
  *     val a: Int?
  *         get() = 5
  * }
+ *
+ * fun foo(a: Int?) {
+ *     val b = a!! + 2
+ * }
  * </noncompliant>
  *
  * <compliant>
@@ -64,6 +78,14 @@ import org.jetbrains.kotlin.types.isNullable
  *     val a: Int
  *         get() = 5
  * }
+ *
+ * fun foo(a: Int?) {
+ *     val b = (a ?: 0) + 2
+ * }
+ *
+ * fun foo(a: Int) {
+ *     val b = a + 2
+ * }
  * </compliant>
  */
 @RequiresTypeResolution
@@ -77,11 +99,115 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
 
     override fun visitKtFile(file: KtFile) {
         if (bindingContext == BindingContext.EMPTY) return
-        NonNullableCheckVisitor().visitKtFile(file)
+        PropertyCheckVisitor().visitKtFile(file)
+        ParameterCheckVisitor().visitKtFile(file)
         super.visitKtFile(file)
     }
 
-    private inner class NonNullableCheckVisitor : DetektVisitor() {
+    private inner class ParameterCheckVisitor : DetektVisitor() {
+        private val candidateParams = mutableMapOf<DeclarationDescriptor, KtParameter>()
+        private val referencedParams = mutableSetOf<DeclarationDescriptor>()
+
+        override fun visitKtFile(file: KtFile) {
+            super.visitKtFile(file)
+            // Any candidate params that were not removed during the inspection
+            // of the Kotlin file and were not added to referenceParams were never
+            // treated as nullable params in the code, thus they can be converted
+            // to non-nullable.
+            candidateParams.forEach { (descriptor, param) ->
+                if (referencedParams.contains(descriptor)) {
+                    report(
+                        CodeSmell(
+                            issue,
+                            Entity.from(param),
+                            "The nullable parameter '${param.name}' can be made non-nullable."
+                        )
+                    )
+                }
+            }
+        }
+
+        override fun visitNamedFunction(function: KtNamedFunction) {
+            function.valueParameters.asSequence()
+                .filter {
+                    it.typeReference?.typeElement is KtNullableType
+                }.mapNotNull { parameter ->
+                    bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, parameter]?.let {
+                        it to parameter
+                    }
+                }.forEach { (k, v) -> candidateParams[k] = v }
+            super.visitNamedFunction(function)
+        }
+
+        override fun visitPostfixExpression(expression: KtPostfixExpression) {
+            val descriptor = expression.baseExpression
+                ?.getResolvedCall(bindingContext)
+                ?.resultingDescriptor
+            if (descriptor != null && expression.operationToken == KtTokens.EXCLEXCL) {
+                referencedParams.add(descriptor)
+            }
+            super.visitPostfixExpression(expression)
+        }
+
+        override fun visitSafeQualifiedExpression(expression: KtSafeQualifiedExpression) {
+            expression.receiverExpression
+                .getResolvedCall(bindingContext)
+                ?.resultingDescriptor
+                ?.let(candidateParams::remove)
+            super.visitSafeQualifiedExpression(expression)
+        }
+
+        override fun visitDotQualifiedExpression(expression: KtDotQualifiedExpression) {
+            val isExtensionForNullable = expression.getResolvedCall(bindingContext)
+                ?.resultingDescriptor
+                ?.extensionReceiverParameter
+                ?.type
+                ?.isMarkedNullable
+            if (isExtensionForNullable == true) {
+                expression.receiverExpression
+                    .getResolvedCall(bindingContext)
+                    ?.resultingDescriptor
+                    ?.let(candidateParams::remove)
+            }
+            super.visitDotQualifiedExpression(expression)
+        }
+
+        override fun visitBinaryExpression(expression: KtBinaryExpression) {
+            if (expression.operationToken == KtTokens.ELVIS) {
+                expression.left
+                    ?.getResolvedCall(bindingContext)
+                    ?.resultingDescriptor
+                    ?.let(candidateParams::remove)
+            }
+            super.visitBinaryExpression(expression)
+        }
+
+        override fun visitIfExpression(expression: KtIfExpression) {
+            expression.condition?.let { it as? KtBinaryExpression }?.evaluateIfExpression()
+            super.visitIfExpression(expression)
+        }
+
+        private fun KtBinaryExpression.evaluateIfExpression() {
+            val leftExpression = left
+            val rightExpression = right
+            if (isNullCheck() || isNonNullCheck()) {
+                // Determine whether either side of the expression is a variable
+                // that could be removed from candidateParams
+                when {
+                    leftExpression is KtNameReferenceExpression -> leftExpression
+                    rightExpression is KtNameReferenceExpression -> rightExpression
+                    else -> null
+                }?.getResolvedCall(bindingContext)
+                    ?.resultingDescriptor
+                    ?.let(candidateParams::remove)
+            }
+            // Recursively iterate into the if-check if possible
+            (leftExpression as? KtBinaryExpression)?.evaluateIfExpression()
+            (rightExpression as? KtBinaryExpression)?.evaluateIfExpression()
+        }
+    }
+
+    private inner class PropertyCheckVisitor : DetektVisitor() {
         // A list of properties that are marked as nullable during their
         // declaration but do not explicitly receive a nullable value in
         // the declaration, so they could potentially be marked as non-nullable
@@ -112,11 +238,13 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
         }
 
         override fun visitProperty(property: KtProperty) {
-            if (property.getKotlinTypeForComparison(bindingContext)?.isNullable() != true) return
-            val fqName = property.fqName ?: return
-            if (property.isCandidate()) {
-                candidateProps[fqName] = property
+            if (property.getKotlinTypeForComparison(bindingContext)?.isNullable() == true) {
+                val fqName = property.fqName
+                if (property.isCandidate() && fqName != null) {
+                    candidateProps[fqName] = property
+                }
             }
+            super.visitProperty(property)
         }
 
         override fun visitBinaryExpression(expression: KtBinaryExpression) {

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

@@ -383,5 +383,107 @@ class CanBeNonNullableSpec : Spek({
                 """
             Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
         }
+
+        context("evaluating nullable function parameters") {
+            context("reports when they are never treated as nullable") {
+                it("and use a double-bang de-nullifier") {
+                    val code = """
+                        fun foo(a: Int?) {
+                            val b = a!! + 2
+                        }
+                    """.trimIndent()
+                    Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+                }
+
+                it("and use a double-bang qualified expression") {
+                    val code = """
+                        fun foo(a: Int?) {
+                            val b = a!!.plus(2)
+                        }
+                    """.trimIndent()
+                    Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+                }
+            }
+
+            context("does not report when they are checked with an if-statement") {
+                it("on nullity") {
+                    val code = """
+                        fun foo(a: Int?) {
+                            if (a == null) {
+                                println("'a' is null")
+                            }
+                            val b = a!! + 2
+                        }
+                    """.trimIndent()
+                    Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+                }
+
+                it("on non-nullity") {
+                    val code = """
+                        fun foo(a: Int?) {
+                            if (a != null) {
+                                println(a + 5)
+                            }
+                            val b = a!! + 2
+                        }
+                    """.trimIndent()
+                    Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+                }
+
+                it("in a reversed manner") {
+                    val code = """
+                        fun foo(a: Int?) {
+                            if (null != a) {
+                                println(a + 5)
+                            }
+                            val b = a!! + 2
+                        }
+                    """.trimIndent()
+                    Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+                }
+
+                it("with multiple clauses") {
+                    val code = """
+                        fun foo(a: Int?, other: Int) {
+                            if (a != null && other % 2 == 0) {
+                                println(a + 5)
+                            }
+                            val b = a!! + 2
+                        }
+                    """.trimIndent()
+                    Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+                }
+            }
+
+            it("does not report when they are used in an assignment with an Elvis operator") {
+                val code = """
+                    fun foo(a: Int?) {
+                        val b = (a ?: 0) + 2
+                        val c = a!! + 2
+                    }
+                """.trimIndent()
+                Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+            }
+
+            it("does not report when they are accessed using a null-safe call") {
+                val code = """
+                    fun foo(a: Int?) {
+                        val b = a?.plus(2)
+                        val c = a!! + 2
+                    }
+                """.trimIndent()
+                Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+            }
+
+            it("does not report when they are accessed using a null-safe extension function") {
+                val code = """
+                    fun foo(a: List<String>?) {
+                        val b = a.orEmpty()
+                        val c = a!! + "TEST"
+                    }
+                """.trimIndent()
+                Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+            }
+        }
     }
 })