diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 67fcbc2c6c9..04d72e456a8 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -470,6 +470,8 @@ potential-bugs: active: false UnconditionalJumpStatementInLoop: active: false + UnnecessaryNotNullCheck: + active: false UnnecessaryNotNullOperator: active: true UnnecessarySafeCall: 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 693d0bc3f99..91c18d83ecf 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 @@ -39,6 +39,7 @@ class PotentialBugProvider : DefaultRuleSetProvider { @Suppress("DEPRECATION") RedundantElseInWhen(config), UnconditionalJumpStatementInLoop(config), UnnecessaryNotNullOperator(config), + UnnecessaryNotNullCheck(config), UnnecessarySafeCall(config), UnreachableCode(config), UnsafeCallOnNullableType(config), diff --git a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheck.kt b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheck.kt new file mode 100644 index 00000000000..1c7b7dafa6e --- /dev/null +++ b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheck.kt @@ -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. + * + * + * var string = "foo" + * println(requireNotNull(string)) + * + * + * + * var string : String? = "foo" + * println(requireNotNull(string)) + * + */ +@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") + } +} diff --git a/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheckSpec.kt b/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheckSpec.kt new file mode 100644 index 00000000000..a6b2b351964 --- /dev/null +++ b/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnecessaryNotNullCheckSpec.kt @@ -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 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` { + + @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 foo(x: T): T { + return x + } + fun bar() { + requireNotNull(foo(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() + } + } +}