Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix false-negative for CanBeNonNullable #4993

Merged
merged 3 commits into from Jun 24, 2022
Merged

Fix false-negative for CanBeNonNullable #4993

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f1e1a7
Report null-check returning 'Unit'
VitalyVPinchuk Jun 23, 2022
eca25ec
Merge remote-tracking branch 'origin/main' into null-check-returns-unit
VitalyVPinchuk Jun 23, 2022
ef091d1
Not report guard statement with side effect ahead
VitalyVPinchuk Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 30 additions & 10 deletions ...t-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullable.kt
View file
Open in desktop
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 Down Expand Up @@ -83,6 +84,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 +181,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 +241,22 @@ class CanBeNonNullable(config: Config = Config.empty) : Rule(config) {

override fun visitIfExpression(expression: KtIfExpression) {
expression.condition.evaluateCheckStatement(expression.`else`)
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 +342,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 +356,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 +436,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
34 changes: 34 additions & 0 deletions ...les-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/CanBeNonNullableSpec.kt
View file
Open in desktop
Expand Up @@ -897,6 +897,40 @@ 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 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