[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-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt

@@ -0,0 +1,136 @@
+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.isNonNullCheck
+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.psiUtil.containingClassOrObject
+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>
+ *
+ * <compliant>
+ * class A(private var a: Int?) {
+ *   fun foo() {
+ *     val a = a
+ *     if (a != null) {
+ *       println(2 + a)
+ *     }
+ *   }
+ * }
+ * </compliant>
+ */
+
+@RequiresTypeResolution
+class NullCheckOnMutableProperty(config: Config) : Rule(config) {
+    private val mutableProperties = mutableMapOf<String?, MutableSet<String>>()
+    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)
+        mutableProperties.clear()
+    }
+
+    override fun visitPrimaryConstructor(constructor: KtPrimaryConstructor) {
+        val containerName = constructor.getContainingClassOrObject().name
+        constructor.valueParameters.asSequence()
+            .filter { it.isMutable }
+            .mapNotNull { it.name }
+            .forEach { mutableProperties.getOrPut(containerName) { mutableSetOf() }.add(it) }
+        super.visitPrimaryConstructor(constructor)
+    }
+
+    override fun visitProperty(property: KtProperty) {
+        if (property.isVar) {
+            property.name?.let { propName ->
+                val containerName = property.containingClassOrObject?.name.orEmpty() // A root-level property if empty.
+                mutableProperties.getOrPut(
+                    containerName
+                ) { mutableSetOf() }.add(propName)
+            }
+        }
+        super.visitProperty(property)
+    }
+
+    override fun visitIfExpression(expression: KtIfExpression) {
+        val condition = expression.condition
+        if (condition is KtBinaryExpression && condition.isNonNullCheck()) {
+            // Determine which of the two sides of the condition is the null constant and use the other.
+            if (condition.left is KtConstantExpression) {
+                condition.right
+            } else {
+                condition.left
+            }?.let {
+                // Only proceed with evaluating if the checked expression is for a variable.
+                it as? KtNameReferenceExpression
+            }?.let {
+                evaluateNameReferenceExpression(expression, it)
+            }
+        }
+        super.visitIfExpression(expression)
+    }
+
+    private fun evaluateNameReferenceExpression(
+        expression: KtIfExpression,
+        referenceExpression: KtNameReferenceExpression
+    ) {
+        val resolvedCall = referenceExpression.getResolvedCall(bindingContext) ?: return
+        val containingDeclaration = resolvedCall.resultingDescriptor.containingDeclaration
+        if (
+            mutableProperties[containingDeclaration.fqNameOrNull()?.asString()]
+                ?.contains(referenceExpression.text) == true
+        ) {
+            report(
+                CodeSmell(
+                    issue,
+                    Entity.from(expression),
+                    "Null-check is being called on a mutable property."
+                )
+            )
+        }
+    }
+}

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),

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

@@ -0,0 +1,173 @@
+package io.gitlab.arturbosch.detekt.rules.bugs
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.rules.setupKotlinEnvironment
+import io.gitlab.arturbosch.detekt.test.compileAndLint
+import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
+import org.assertj.core.api.Assertions
+import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
+import org.spekframework.spek2.Spek
+import org.spekframework.spek2.style.specification.describe
+
+class NullCheckOnMutablePropertySpec : Spek({
+    setupKotlinEnvironment()
+
+    val env: KotlinCoreEnvironment by memoized()
+    val subject by memoized { NullCheckOnMutableProperty(Config.empty) }
+
+    describe("NullCheckOnMutableProperty Rule") {
+        it("should report a null-check on a mutable constructor property") {
+            val code = """
+                class A(private var a: Int?) {
+                    fun foo() {
+                        if (a != null) {
+                            println(2 + a!!)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+        }
+
+        it("should not report a null-check on a shadowed property") {
+            val code = """
+                class A(private var a: Int?) {
+                    fun foo() {
+                        val a = a
+                        if (a != null) {
+                            println(2 + a)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+        }
+
+        it("should not report a null-check on a non-mutable constructor property") {
+            val code = """
+                class A(private val a: Int?) {
+                    fun foo() {
+                        if (a != null) {
+                            println(2 + a)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+        }
+
+        it("should report a null-check on a mutable class property") {
+            val code = """
+                class A {
+                    private var a: Int? = 5
+                    fun foo() {
+                        if (a != null) {
+                            println(2 + a!!)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+        }
+
+        it("should not report a null-check on a non-mutable class property") {
+            val code = """
+                class A {
+                    private val a: Int? = 5
+                    fun foo() {
+                        if (a != null) {
+                            println(2 + a)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+        }
+
+        it("should report a null-check on a mutable file property") {
+            val code = """
+                private var a: Int? = 5
+
+                class A {
+                    fun foo() {
+                        if (a != null) {
+                            println(2 + a!!)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+        }
+
+        it("should not report a null-check on a non-mutable file property") {
+            val code = """
+                private val a: Int? = 5
+
+                class A {
+                    fun foo() {
+                        if (a != null) {
+                            println(2 + a)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+        }
+
+        it("should not report a null-check when there is no binding context") {
+            val code = """
+                class A(private var a: Int?) {
+                    fun foo() {
+                        if (a != null) {
+                            println(2 + a!!)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLint(code)).isEmpty()
+        }
+
+        it("should report a null-check when null is the first element in the if-statement") {
+            val code = """
+                class A(private var a: Int?) {
+                    fun foo() {
+                        if (null != a) {
+                            println(2 + a!!)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+        }
+
+        it("should not report when the if-expression has no explicit null value") {
+            val code = """
+                class A(private var a: Int?) {
+                    fun foo() {
+                        val otherA = null
+                        if (a != otherA) {
+                            println(2 + a!!)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+        }
+
+        it("should not report a null-check on a function") {
+            val code = """
+                class A {
+                    private fun otherFoo(): Int? {
+                        return null
+                    }
+                    fun foo() {
+                        if (otherFoo() != null) {
+                            println(2 + otherFoo()!!)
+                        } 
+                    }
+                }
+                """
+            Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+        }
+    }
+})