Extend CanBeNonNullable rule to check function params

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

@@ -9,20 +9,36 @@ 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 io.gitlab.arturbosch.detekt.rules.isOperator
+import org.jetbrains.kotlin.descriptors.CallableDescriptor
+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.KtIsExpression
+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.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.KtTypeReference
+import org.jetbrains.kotlin.psi.KtWhenConditionIsPattern
+import org.jetbrains.kotlin.psi.KtWhenConditionWithExpression
+import org.jetbrains.kotlin.psi.KtWhenExpression
 import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
 import org.jetbrains.kotlin.psi.psiUtil.isPrivate
 import org.jetbrains.kotlin.resolve.BindingContext
@@ -49,6 +65,10 @@ import org.jetbrains.kotlin.types.isNullable
  *     val a: Int?
  *         get() = 5
  * }
+ *
+ * fun foo(a: Int?) {
+ *     val b = a!! + 2
+ * }
  * </noncompliant>
  *
  * <compliant>
@@ -64,6 +84,10 @@ import org.jetbrains.kotlin.types.isNullable
  *     val a: Int
  *         get() = 5
  * }
+ *
+ * fun foo(a: Int) {
+ *     val b = a + 2
+ * }
  * </compliant>
  */
 @RequiresTypeResolution
@@ -76,12 +100,179 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
     )
 
     override fun visitKtFile(file: KtFile) {
-        if (bindingContext == BindingContext.EMPTY) return
-        NonNullableCheckVisitor().visitKtFile(file)
         super.visitKtFile(file)
+        PropertyCheckVisitor().visitKtFile(file)
+        ParameterCheckVisitor().visitKtFile(file)
+    }
+
+    @Suppress("TooManyFunctions")
+    private inner class ParameterCheckVisitor : DetektVisitor() {
+        private val candidateParams = mutableMapOf<DeclarationDescriptor, KtParameter>()
+
+        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 { (_, param) ->
+                report(
+                    CodeSmell(
+                        issue,
+                        Entity.from(param),
+                        "The nullable parameter '${param.name}' can be made non-nullable."
+                    )
+                )
+            }
+        }
+
+        override fun visitNamedFunction(function: KtNamedFunction) {
+            // Operator functions - like setValue() - may require a nullable param to
+            // correspond with function type parameters, so those functions would not
+            // have the choice to mark the param as non-nullable.
+            if (!function.isOperator()) {
+                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 visitWhenExpression(expression: KtWhenExpression) {
+            val subjectDescriptor = expression.subjectExpression
+                ?.let { it as? KtNameReferenceExpression }
+                ?.getResolvedCall(bindingContext)
+                ?.resultingDescriptor
+            val whenConditions = expression.entries.flatMap { it.conditions.asList() }
+            if (subjectDescriptor != null) {
+                val isNullCheck = whenConditions.any { whenCondition ->
+                    when (whenCondition) {
+                        is KtWhenConditionWithExpression -> whenCondition.expression?.text == "null"
+                        is KtWhenConditionIsPattern -> {
+                            (whenCondition.typeReference.isNullable(bindingContext) || whenCondition.isNegated)
+                        }
+                        else -> false
+                    }
+                }
+                if (isNullCheck || expression.elseExpression.isValidElseExpression()) {
+                    candidateParams.remove(subjectDescriptor)
+                }
+            } else {
+                whenConditions.forEach { whenCondition ->
+                    if (whenCondition is KtWhenConditionWithExpression) {
+                        whenCondition.expression.evaluateCheckStatement(expression.elseExpression)
+                    }
+                }
+            }
+            super.visitWhenExpression(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.evaluateCheckStatement(expression.`else`)
+            super.visitIfExpression(expression)
+        }
+
+        private fun KtExpression?.getNonNullChecks(): List<CallableDescriptor>? {
+            return when (this) {
+                is KtBinaryExpression -> evaluateBinaryExpression()
+                is KtIsExpression -> evaluateIsExpression()
+                else -> null
+            }
+        }
+
+        private fun KtExpression?.evaluateCheckStatement(elseExpression: KtExpression?) {
+            this.getNonNullChecks()?.let { nonNullChecks ->
+                if (elseExpression.isValidElseExpression()) {
+                    nonNullChecks.forEach(candidateParams::remove)
+                }
+            }
+        }
+
+        private fun KtExpression?.isValidElseExpression(): Boolean {
+            return this != null && this !is KtIfExpression && this !is KtWhenExpression
+        }
+
+        private fun KtBinaryExpression.evaluateBinaryExpression(): List<CallableDescriptor> {
+            val leftExpression = left
+            val rightExpression = right
+            val nonNullChecks = mutableListOf<CallableDescriptor>()
+
+            fun getDescriptor(leftExpression: KtExpression?, rightExpression: KtExpression?): CallableDescriptor? {
+                return when {
+                    leftExpression is KtNameReferenceExpression -> leftExpression
+                    rightExpression is KtNameReferenceExpression -> rightExpression
+                    else -> null
+                }?.getResolvedCall(bindingContext)
+                    ?.resultingDescriptor
+            }
+
+            if (isNullCheck()) {
+                getDescriptor(leftExpression, rightExpression)?.let(candidateParams::remove)
+            } else if (isNonNullCheck()) {
+                getDescriptor(leftExpression, rightExpression)?.let(nonNullChecks::add)
+            }
+
+            // Recursively iterate into the if-check if possible
+            leftExpression.getNonNullChecks()?.let(nonNullChecks::addAll)
+            rightExpression.getNonNullChecks()?.let(nonNullChecks::addAll)
+            return nonNullChecks
+        }
+
+        private fun KtIsExpression.evaluateIsExpression(): List<CallableDescriptor> {
+            val descriptor = this.leftHandSide.getResolvedCall(bindingContext)?.resultingDescriptor
+                ?: return emptyList()
+            return if (typeReference.isNullable(bindingContext) || isNegated) {
+                candidateParams.remove(descriptor)
+                emptyList()
+            } else {
+                listOf(descriptor)
+            }
+        }
+
+        private fun KtTypeReference?.isNullable(bindingContext: BindingContext): Boolean {
+            return this?.let { bindingContext[BindingContext.TYPE, it] }?.isMarkedNullable == true
+        }
     }
 
-    private inner class NonNullableCheckVisitor : DetektVisitor() {
+    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 +303,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) {