Skip to content

Commit

Permalink
Fix false-negative for CanBeNonNullable (#4993)
Browse files Browse the repository at this point in the history 
* Report null-check returning 'Unit'

The rule will report code smell when null-check expression returns 'Unit'

* Not report guard statement with side effect ahead
  • Loading branch information
@VitalyVPinchuk
VitalyVPinchuk committed Jun 24, 2022
1 parent 8dc783a commit 4742842
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 10 deletions.
43 changes: 33 additions & 10 deletions ...t-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt
View file
Expand Up @@ -20,6 +20,7 @@ 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.KtBlockExpression
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtConstantExpression
Expand All @@ -46,6 +47,7 @@ import org.jetbrains.kotlin.psi.KtWhenConditionWithExpression
import org.jetbrains.kotlin.psi.KtWhenExpression
import org.jetbrains.kotlin.psi.psiUtil.allChildren
import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
import org.jetbrains.kotlin.psi.psiUtil.isFirstStatement
import org.jetbrains.kotlin.psi.psiUtil.isPrivate
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.calls.smartcasts.getKotlinTypeForComparison
Expand Down Expand Up @@ -83,6 +85,11 @@ import org.jetbrains.kotlin.types.isNullable
* println(a)
* }
* }
*
* fun foo(a: Int?) {
* if (a == null) return
* println(a)
* }
* </noncompliant>
*
* <compliant>
Expand Down Expand Up @@ -175,7 +182,7 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
// the param, either via an if/when check or with a safe-qualified expression.
.filter {
val onlyNonNullCheck = validSingleChildExpression && it.isNonNullChecked && !it.isNullChecked
it.isNonNullForced || onlyNonNullCheck
it.isNonNullForced || it.isNullCheckReturnsUnit || onlyNonNullCheck
}
.forEach { nullableParam ->
report(
Expand Down Expand Up @@ -235,9 +242,24 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {

override fun visitIfExpression(expression: KtIfExpression) {
expression.condition.evaluateCheckStatement(expression.`else`)
if (expression.isFirstStatement()) {
evaluateNullCheckReturnsUnit(expression.condition, expression.then)
}
super.visitIfExpression(expression)
}

private fun evaluateNullCheckReturnsUnit(condition: KtExpression?, then: KtExpression?) {
val thenExpression = if (then is KtBlockExpression) then.firstStatement else then
if (thenExpression !is KtReturnExpression) return
if (thenExpression.returnedExpression != null) return

if (condition is KtBinaryExpression && condition.isNullCheck()) {
getDescriptor(condition.left, condition.right)
?.let { nullableParams[it] }
?.let { it.isNullCheckReturnsUnit = true }
}
}

override fun visitSafeQualifiedExpression(expression: KtSafeQualifiedExpression) {
updateNullableParam(expression.receiverExpression) { it.isNonNullChecked = true }
super.visitSafeQualifiedExpression(expression)
Expand Down Expand Up @@ -323,15 +345,6 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
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 { nullableParams[it] }
Expand All @@ -346,6 +359,15 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
return nonNullChecks
}

private fun getDescriptor(leftExpression: KtExpression?, rightExpression: KtExpression?): CallableDescriptor? {
return when {
leftExpression is KtNameReferenceExpression -> leftExpression
rightExpression is KtNameReferenceExpression -> rightExpression
else -> null
}?.getResolvedCall(bindingContext)
?.resultingDescriptor
}

private fun KtIsExpression.evaluateIsExpression(): List<CallableDescriptor> {
val descriptor = this.leftHandSide.getResolvedCall(bindingContext)?.resultingDescriptor
?: return emptyList()
Expand Down Expand Up @@ -417,6 +439,7 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {
var isNullChecked = false
var isNonNullChecked = false
var isNonNullForced = false
var isNullCheckReturnsUnit = false
}

private inner class PropertyCheckVisitor : DetektVisitor() {
Expand Down
46 changes: 46 additions & 0 deletions ...les-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt
View file
Expand Up @@ -897,6 +897,52 @@ class CanBeNonNullableSpec(val env: KotlinCoreEnvironment) {
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `does report null-check returning unit type`() {
val code = """
fun foo(a: Int?) {
if (a == null) return
println(a)
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `does report null-check returning unit type in block`() {
val code = """
fun foo(a: Int?) {
if (a == null) { return }
println(a)
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
}

@Test
fun `does not report guard statement with side effect ahead`() {
val code = """
fun foo(a: Int?) {
println("side effect")
if (a == null) return
println(a)
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(0)
}

@Test
fun `does not report null-check returning non-unit type`() {
val code = """
fun foo(a: Int?): Int {
if (a == null) return 0
println(a)
return a
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(0)
}

@Test
fun `does not report when the parameter is checked on non-nullity with an else statement`() {
val code = """
Expand Down

0 comments on commit 4742842

Please sign in to comment.