From f6440e3358d7748f30670869032529db6af6971c Mon Sep 17 00:00:00 2001 From: severn-everett Date: Fri, 31 Dec 2021 15:17:57 +0100 Subject: [PATCH] [NullCheckOnMutableProperty] Created rule for when a null-check is performed on a mutable property (#4353) * Created rule for when a null-check is performed on a mutable property * Fixed Detekt issue * Added tests to improve code coverage * Fixed Detekt issues * Rule will now only fire when the candidate variable is actually referenced within the if-expression block; Moved all relevant inspection code to within a private class to ensure resource cleanup after visiting a Kotlin file * Consolidated null-checking on candidateFqName * More consolidation within visitIfExpression; Fixed Detekt issue in NullCheckOnMutablePropertySpec * Switch to interface type specification in declaration of candidateProperties * Added checks for multi-clause if-expressions * Fixed Detekt issue * Added check for val properties that have getters that return potentially-nullable values * Address Detekt issue * Fix failures in test compilation * Simplified check on getter - will now only report if a val property possesses a getter * Addressing PR comments * Fixed typo --- .../main/resources/default-detekt-config.yml | 2 + .../detekt/rules/KtBinaryExpression.kt | 6 +- .../rules/bugs/NullCheckOnMutableProperty.kt | 159 ++++++++++ .../detekt/rules/bugs/PotentialBugProvider.kt | 1 + .../bugs/NullCheckOnMutablePropertySpec.kt | 298 ++++++++++++++++++ 5 files changed, 464 insertions(+), 2 deletions(-) create mode 100644 detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt create mode 100644 detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutablePropertySpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index b2c589ed08d..afb0108513a 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/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: diff --git a/detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/KtBinaryExpression.kt b/detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/KtBinaryExpression.kt index 70c2ce8c845..d7e2803ee41 100644 --- a/detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/KtBinaryExpression.kt +++ b/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" diff --git a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt new file mode 100644 index 00000000000..cf8df370259 --- /dev/null +++ b/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. + * + * + * class A(private var a: Int?) { + * fun foo() { + * if (a != null) { + * println(2 + a!!) + * } + * } + * } + * + * + * + * class A(private val a: Int?) { + * fun foo() { + * if (a != null) { + * println(2 + a) + * } + * } + * } + * + */ +@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() + private val candidateProperties = mutableMapOf>() + + 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 { + return if (isNonNullCheck()) { + listOf(this) + } else { + val nonNullChecks = mutableListOf() + (left as? KtBinaryExpression)?.let { nonNullChecks.addAll(it.collectNonNullChecks()) } + (right as? KtBinaryExpression)?.let { nonNullChecks.addAll(it.collectNonNullChecks()) } + nonNullChecks + } + } + } +} diff --git a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/PotentialBugProvider.kt b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/PotentialBugProvider.kt index 8e0f255dc58..c8c31f688ca 100644 --- a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/PotentialBugProvider.kt +++ b/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), diff --git a/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutablePropertySpec.kt b/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutablePropertySpec.kt new file mode 100644 index 00000000000..f1ff4cbca52 --- /dev/null +++ b/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutablePropertySpec.kt @@ -0,0 +1,298 @@ +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 report a null-check on a mutable property in non-initial clauses in an if-statement") { + val code = """ + class A(private var a: Int?, private val b: Int) { + fun foo() { + if (b == 5 && a != null) { + println(2 + a!!) + } + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + it("should report a null-check on a mutable property used in the same if-statement") { + val code = """ + class A(private var a: Int?) { + fun foo() { + if (a != null && a == 2) { + println("a is 2") + } + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + it("should report on a mutable property that is not subject to a double-bang") { + val code = """ + class A(private var a: Int?) { + fun foo() { + if (a != null) { + println(a) + } + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + it("should report on a mutable property even if it is checked multiple times") { + val code = """ + class A(private var a: Int?) { + fun foo() { + if (a != null) { + if (a == null) { + return + } + println(2 + a!!) + } + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + it("should not report when the checked property is not used afterwards") { + val code = """ + class A(private var a: Int?) { + fun foo() { + if (a != null) { + println("'a' is not null") + } + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + 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 val 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 val property with a getter") { + val code = """ + import kotlin.random.Random + + class A { + private val a: Int? + get() = genA() + fun foo() { + if (a != null) { + println(2 + a!!) + } + } + private fun genA(): Int? { + val randInt = Random.nextInt() + return if (randInt % 2 == 0) randInt else null + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + it("should report a null-check conducted within an inner class") { + val code = """ + class A(private var a: Int?) { + inner class B { + fun foo() { + if (a != null) { + println(2 + a!!) + } + } + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + it("should report an inner-class mutable property") { + val code = """ + class A(private val a: Int?) { + inner class B(private var a: Int) { + fun foo() { + if (a != null) { + println(2 + a!!) + } + } + } + + fun foo() { + if (a != null) { + println(2 + a) + } + } + } + """ + Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + 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() + } + } +})