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.
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
Add UnnecessaryNotNullCheck
rule
#5218
Changes from all commits
be11c97
6485665
19a3173
174c564
26156a0
5411caf
20a70f0
d1f0d6a
48cf4a8
d241d2e
7fc0d6a
0177805
d647c39
eb2aa3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
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.isCalling | ||
import org.jetbrains.kotlin.name.FqName | ||
import org.jetbrains.kotlin.psi.KtCallExpression | ||
import org.jetbrains.kotlin.psi.KtExpression | ||
import org.jetbrains.kotlin.psi.psiUtil.getCallNameExpression | ||
import org.jetbrains.kotlin.resolve.BindingContext | ||
import org.jetbrains.kotlin.resolve.calls.util.getType | ||
import org.jetbrains.kotlin.types.typeUtil.TypeNullability | ||
import org.jetbrains.kotlin.types.typeUtil.nullability | ||
|
||
/** | ||
* Reports unnecessary not-null checks with `requireNotNull` or `checkNotNull` that can be removed by the user. | ||
* | ||
* <noncompliant> | ||
* var string = "foo" | ||
* println(requireNotNull(string)) | ||
* </noncompliant> | ||
* | ||
* <compliant> | ||
* var string : String? = "foo" | ||
* println(requireNotNull(string)) | ||
* </compliant> | ||
*/ | ||
@RequiresTypeResolution | ||
class UnnecessaryNotNullCheck(config: Config = Config.empty) : Rule(config) { | ||
|
||
override val issue = Issue( | ||
"UnnecessaryNotNullCheck", | ||
Severity.Defect, | ||
"Remove unnecessary not-null checks on non-null types.", | ||
Debt.FIVE_MINS, | ||
) | ||
|
||
override fun visitCallExpression(expression: KtCallExpression) { | ||
super.visitCallExpression(expression) | ||
|
||
if (bindingContext == BindingContext.EMPTY) return | ||
|
||
if (expression.isCalling(requireNotNullFunctionFqName, bindingContext) || | ||
expression.isCalling(checkNotNullFunctionFqName, bindingContext) | ||
) { | ||
val argument = expression.valueArguments[0].lastChild as KtExpression | ||
if (argument.getType(bindingContext)?.nullability() == TypeNullability.NOT_NULL) { | ||
val callName = expression.getCallNameExpression()?.text | ||
report( | ||
CodeSmell( | ||
issue = issue, | ||
entity = Entity.from(expression), | ||
message = "Using `$callName` on non-null `${argument.text}` is unnecessary", | ||
) | ||
) | ||
} | ||
} | ||
} | ||
|
||
companion object { | ||
private val requireNotNullFunctionFqName = FqName("kotlin.requireNotNull") | ||
private val checkNotNullFunctionFqName = FqName("kotlin.checkNotNull") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,158 @@ | ||
package io.gitlab.arturbosch.detekt.rules.bugs | ||
|
||
import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest | ||
import io.gitlab.arturbosch.detekt.test.assertThat | ||
import io.gitlab.arturbosch.detekt.test.lintWithContext | ||
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment | ||
import org.junit.jupiter.api.Nested | ||
import org.junit.jupiter.api.Test | ||
|
||
@KotlinCoreEnvironmentTest | ||
class UnnecessaryNotNullCheckSpec(private val env: KotlinCoreEnvironment) { | ||
private val subject = UnnecessaryNotNullCheck() | ||
|
||
@Nested | ||
inner class `check unnecessary not null checks` { | ||
|
||
@Test | ||
fun shouldDetectNotNullCallOnVariable() { | ||
val code = """ | ||
val x = 5 | ||
val y = requireNotNull(x) | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).hasSize(1) | ||
assertThat(findings).hasTextLocations(18 to 35) | ||
} | ||
|
||
@Test | ||
fun shouldDetectNotNullCallOnVariableUsingCheckNotNull() { | ||
val code = """ | ||
val x = 5 | ||
val y = checkNotNull(x) | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).hasSize(1) | ||
assertThat(findings).hasTextLocations(18 to 33) | ||
} | ||
|
||
@Test | ||
fun shouldDetectNotNullCallOnFunctionReturn() { | ||
val code = """ | ||
fun foo(): Int { | ||
return 5 | ||
} | ||
fun bar() { | ||
requireNotNull(foo()) | ||
} | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).hasSize(1) | ||
assertThat(findings).hasTextLocations(48 to 69) | ||
} | ||
|
||
@Test | ||
fun shouldDetectWhenCallingDefinitelyNonNullableGenericFunction() { | ||
val code = """ | ||
fun <T> foo(x: T & Any): T & Any { | ||
return x | ||
} | ||
fun bar() { | ||
requireNotNull(foo(5)) | ||
} | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).hasSize(1) | ||
assertThat(findings).hasTextLocations(66 to 88) | ||
} | ||
|
||
@Test | ||
fun shouldDetectWhenCallingPrimitiveJavaMethod() { | ||
val code = """ | ||
fun foo() { | ||
requireNotNull(System.currentTimeMillis()) | ||
} | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).hasSize(1) | ||
assertThat(findings).hasTextLocations(16 to 58) | ||
} | ||
} | ||
|
||
@Nested | ||
inner class `check valid not null check usage` { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it's worth adding one test case in this PR?
If this sounds more expensive, we could definitely follow up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I already have the first one with |
||
|
||
@Test | ||
fun shouldIgnoreNotNullCallOnNullableVariableWithValue() { | ||
val code = """ | ||
val x: Int? = 5 | ||
val y = requireNotNull(x) | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
|
||
@Test | ||
fun shouldIgnoreNotNullCallOnNullableVariableWithNull() { | ||
val code = """ | ||
val x: Int? = null | ||
val y = requireNotNull(x) | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
|
||
@Test | ||
fun shouldIgnoreNotNullCallOnNullableFunctionReturnWithValue() { | ||
val code = """ | ||
fun foo(): Int? { | ||
return 5 | ||
} | ||
fun bar() { | ||
requireNotNull(foo()) | ||
} | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
|
||
@Test | ||
fun shouldIgnoreNotNullCallOnNullableFunctionReturnWithNull() { | ||
val code = """ | ||
fun foo(): Int? { | ||
return null | ||
} | ||
fun bar() { | ||
requireNotNull(foo()) | ||
} | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
|
||
@Test | ||
fun shouldIgnoreWhenCallingNullableGenericFunction() { | ||
val code = """ | ||
fun <T> foo(x: T): T { | ||
return x | ||
} | ||
fun bar() { | ||
requireNotNull(foo<Int?>(5)) | ||
} | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
|
||
@Test | ||
fun shouldIgnoreWhenCallingObjectJavaMethod() { | ||
val code = """ | ||
fun foo() { | ||
requireNotNull(System.getLogger()) | ||
} | ||
""".trimIndent() | ||
val findings = subject.lintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think using
UnnecessaryNotNullCheck
would be catching error-prone bugs. What do you think aboutstyle
rules instead ofpotential-bugs
section (Define the rule in:detekt-rules-style
instead of `:detekt-rules-errorprone)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like similar rules like
UnnecessaryNotNullOperator
andUnnecessarySafeCall
are inpotential-bugs
as well. I think the reason for that is probably that treating a non-null value as nullable shows a potential misuse of code and while not terribly likely to cause bugs, it still could. For example, consider the following:This use of
requireNotNull
is probably assuming thatgetSomeValue
returnsnull
when it can't find the value, but we can see that isn't the case; it could be returning-1
or even throw an exception. The former might and the latter definitely would cause a bug as the exception wouldn't be handled.