[NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property

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

@@ -575,6 +575,8 @@ potential-bugs:
   MissingWhenCase:
     active: true
     allowElseExpression: true
+  NullCheckOnMutableProperty:
+    active: false
   NullableToStringCall:
     active: false
   RedundantElseInWhen:

detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/KtBinaryExpression.kt

@@ -4,9 +4,11 @@ import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.KtBinaryExpression
 
 fun KtBinaryExpression.isNonNullCheck(): Boolean {
-    return operationToken == KtTokens.EXCLEQ && (left?.text == "null" || right?.text == "null")
+    return operationToken == KtTokens.EXCLEQ && (left?.text == NULL_TEXT || right?.text == NULL_TEXT)
 }
 
 fun KtBinaryExpression.isNullCheck(): Boolean {
-    return operationToken == KtTokens.EQEQ && (left?.text == "null" || right?.text == "null")
+    return operationToken == KtTokens.EQEQ && (left?.text == NULL_TEXT || right?.text == NULL_TEXT)
 }
+
+private const val NULL_TEXT = "null"

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

@@ -0,0 +1,159 @@
+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.DetektVisitor
+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.isNonNullCheck
+import io.gitlab.arturbosch.detekt.rules.isNullCheck
+import org.jetbrains.kotlin.backend.common.peek
+import org.jetbrains.kotlin.backend.common.pop
+import org.jetbrains.kotlin.name.FqName
+import org.jetbrains.kotlin.psi.KtBinaryExpression
+import org.jetbrains.kotlin.psi.KtConstantExpression
+import org.jetbrains.kotlin.psi.KtFile
+import org.jetbrains.kotlin.psi.KtIfExpression
+import org.jetbrains.kotlin.psi.KtNameReferenceExpression
+import org.jetbrains.kotlin.psi.KtPrimaryConstructor
+import org.jetbrains.kotlin.psi.KtProperty
+import org.jetbrains.kotlin.psi.KtReferenceExpression
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
+import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull
+
+/**
+ * Reports null-checks on mutable properties, as these properties' value can be
+ * changed - and thus make the null-check invalid - after the execution of the
+ * if-statement.
+ *
+ * <noncompliant>
+ * class A(private var a: Int?) {
+ *   fun foo() {
+ *     if (a != null) {
+ *       println(2 + a!!)
+ *     }
+ *   }
+ * }
+ * </noncompliant>
+ *
+ * <compliant>
+ * class A(private val a: Int?) {
+ *   fun foo() {
+ *     if (a != null) {
+ *       println(2 + a)
+ *     }
+ *   }
+ * }
+ * </compliant>
+ */
+@RequiresTypeResolution
+class NullCheckOnMutableProperty(config: Config) : Rule(config) {
+    override val issue = Issue(
+        javaClass.simpleName,
+        Severity.Defect,
+        "Checking nullability on a mutable property is not useful because the " +
+            "property may be set to null afterwards.",
+        Debt.TEN_MINS
+    )
+
+    override fun visitKtFile(file: KtFile) {
+        if (bindingContext == BindingContext.EMPTY) return
+        super.visitKtFile(file)
+        NullCheckVisitor().visitKtFile(file)
+    }
+
+    private inner class NullCheckVisitor : DetektVisitor() {
+        private val mutableProperties = mutableSetOf<FqName>()
+        private val candidateProperties = mutableMapOf<FqName, MutableList<KtIfExpression>>()
+
+        override fun visitPrimaryConstructor(constructor: KtPrimaryConstructor) {
+            super.visitPrimaryConstructor(constructor)
+            constructor.valueParameters.asSequence()
+                .filter { it.isMutable }
+                .mapNotNull { it.fqName }
+                .forEach(mutableProperties::add)
+        }
+
+        override fun visitProperty(property: KtProperty) {
+            super.visitProperty(property)
+            val fqName = property.fqName
+            if (fqName != null && (property.isVar || property.getter != null)) {
+                mutableProperties.add(fqName)
+            }
+        }
+
+        override fun visitIfExpression(expression: KtIfExpression) {
+            // Extract all possible null-checks within the if-expression.
+            val nonNullChecks = (expression.condition as? KtBinaryExpression)
+                ?.collectNonNullChecks()
+                .orEmpty()
+
+            val modifiedCandidateQueues = nonNullChecks.mapNotNull { nonNullCondition ->
+                if (nonNullCondition.left is KtConstantExpression) {
+                    nonNullCondition.right as? KtNameReferenceExpression
+                } else {
+                    nonNullCondition.left as? KtNameReferenceExpression
+                }?.let { referenceExpression ->
+                    referenceExpression.getResolvedCall(bindingContext)
+                        ?.resultingDescriptor
+                        ?.let {
+                            it.fqNameOrNull()?.takeIf(mutableProperties::contains)
+                        }
+                }?.let { candidateFqName ->
+                    // A candidate mutable property is present, so attach the current
+                    // if-expression to it in the property candidates map.
+                    candidateProperties.getOrPut(candidateFqName) { ArrayDeque() }.apply { add(expression) }
+                }
+            }
+            // Visit descendant expressions to see whether candidate properties
+            // identified in this if-expression are being referenced.
+            super.visitIfExpression(expression)
+            // Remove the if-expression after having iterated out of its code block.
+            modifiedCandidateQueues.forEach { it.pop() }
+        }
+
+        override fun visitReferenceExpression(expression: KtReferenceExpression) {
+            super.visitReferenceExpression(expression)
+            expression.getResolvedCall(bindingContext)
+                ?.resultingDescriptor
+                ?.fqNameOrNull()
+                ?.let { fqName ->
+                    val expressionParent = expression.parent
+                    // Don't check the reference expression if it's being invoked in the if-expression
+                    // where it's being null-checked.
+                    if (
+                        expressionParent !is KtBinaryExpression ||
+                        (!expressionParent.isNonNullCheck() && !expressionParent.isNullCheck())
+                    ) {
+                        // If there's an if-expression attached to the candidate property, a null-checked
+                        // mutable property is being referenced.
+                        candidateProperties[fqName]?.peek()?.let { ifExpression ->
+                            report(
+                                CodeSmell(
+                                    issue,
+                                    Entity.from(ifExpression),
+                                    "Null-check is being called on mutable property '$fqName'."
+                                )
+                            )
+                        }
+                    }
+                }
+        }
+
+        private fun KtBinaryExpression.collectNonNullChecks(): List<KtBinaryExpression> {
+            return if (isNonNullCheck()) {
+                listOf(this)
+            } else {
+                val nonNullChecks = mutableListOf<KtBinaryExpression>()
+                (left as? KtBinaryExpression)?.let { nonNullChecks.addAll(it.collectNonNullChecks()) }
+                (right as? KtBinaryExpression)?.let { nonNullChecks.addAll(it.collectNonNullChecks()) }
+                nonNullChecks
+            }
+        }
+    }
+}

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

@@ -34,6 +34,7 @@ class PotentialBugProvider : DefaultRuleSetProvider {
             MapGetWithNotNullAssertionOperator(config),
             MissingPackageDeclaration(config),
             MissingWhenCase(config),
+            NullCheckOnMutableProperty(config),
             RedundantElseInWhen(config),
             UnconditionalJumpStatementInLoop(config),
             UnnecessaryNotNullOperator(config),