From ce0d308edf2e405743eaae19eb9f087541c13104 Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Sat, 28 May 2022 13:47:08 -0700 Subject: [PATCH] Add NullableBooleanCheck rule Add a new rule which enforces the recommendation of the Kotlin coding conventions to prefer `==` over `?:` when converting a `Boolean?` into a `Boolean`: https://kotlinlang.org/docs/coding-conventions.html#nullable-boolean-values-in-conditions. This is simple to implement and enforces consistency for the two essentially equivalent methods of coalescing nullable boolean values. The coding conventions only specify this preference for usages "in a conditional statement" but the same rationale applies to any statement which converts a Boolean? to a Boolean, so I have implemented it for any situation. An alternative is to add a configuration setting to toggle application only in if statements. Unfortunately, this rule requires type resolution since there are technically valid usages of e.g. `value ?: true` for cases where `value` is not a `Boolean?` but an `Any?`. Running without type resolution will not be able to distinguish between these valid usages and ones which could be converted to `== false`. --- .../main/resources/default-detekt-config.yml | 2 + .../rules/style/NullableBooleanCheck.kt | 72 +++++++++ .../detekt/rules/style/StyleGuideProvider.kt | 1 + .../rules/style/NullableBooleanCheckSpec.kt | 140 ++++++++++++++++++ 4 files changed, 215 insertions(+) create mode 100644 detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheck.kt create mode 100644 detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheckSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 673c6c951f8..c7d8ccb55a1 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -581,6 +581,8 @@ style: active: true NoTabs: active: false + NullableBooleanCheck: + active: false ObjectLiteralToLambda: active: false OptionalAbstractKeyword: diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheck.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheck.kt new file mode 100644 index 00000000000..889644bad63 --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheck.kt @@ -0,0 +1,72 @@ +package io.gitlab.arturbosch.detekt.rules.style + +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 org.jetbrains.kotlin.KtNodeTypes +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.resolve.calls.util.getType +import org.jetbrains.kotlin.types.typeUtil.isBooleanOrNullableBoolean + +/** + * Detects nullable boolean checks which use an elvis expression `?:` rather than equals `==`. + * + * Per the [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html#nullable-boolean-values-in-conditions) + * converting a nullable boolean property to non-null should be done via `!= false` or `== true` + * rather than `?: true` or `?: false` (respectively). + * + * + * value ?: true + * value ?: false + * + * + * + * value != false + * value == true + * + */ +@RequiresTypeResolution +class NullableBooleanCheck(config: Config = Config.empty) : Rule(config) { + override val issue = Issue( + javaClass.simpleName, + Severity.Style, + "Nullable boolean check should use `==` rather than `?:`", + Debt.FIVE_MINS, + ) + + override fun visitBinaryExpression(expression: KtBinaryExpression) { + if (expression.operationToken == KtTokens.ELVIS && + expression.right?.isBooleanConstant() == true && + expression.left?.getType(bindingContext)?.isBooleanOrNullableBoolean() == true + ) { + if (expression.right?.text == "true") { + report( + CodeSmell( + issue, + Entity.from(expression), + "The nullable boolean check `${expression.text}` should use `!= false` rather than `?: true`", + ) + ) + } else { + report( + CodeSmell( + issue, + Entity.from(expression), + "The nullable boolean check `${expression.text}` should use `== true` rather than `?: false`", + ) + ) + } + } + + super.visitBinaryExpression(expression) + } + + private fun PsiElement.isBooleanConstant() = node.elementType == KtNodeTypes.BOOLEAN_CONSTANT +} diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt index bd496f55cfe..70473385904 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt @@ -73,6 +73,7 @@ class StyleGuideProvider : DefaultRuleSetProvider { PreferToOverPairSyntax(config), MandatoryBracesIfStatements(config), MandatoryBracesLoops(config), + NullableBooleanCheck(config), VarCouldBeVal(config), ForbiddenVoid(config), ExplicitItLambdaParameter(config), diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheckSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheckSpec.kt new file mode 100644 index 00000000000..e7c9799731a --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheckSpec.kt @@ -0,0 +1,140 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest +import io.gitlab.arturbosch.detekt.test.compileAndLint +import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext +import org.assertj.core.api.Assertions.assertThat +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.junit.jupiter.api.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource + +@KotlinCoreEnvironmentTest +class NullableBooleanCheckSpec(val env: KotlinCoreEnvironment) { + val subject = NullableBooleanCheck(Config.empty) + + /** + * The recommended replacement string for `?: [fallback]`. + */ + private fun replacementForElvis(fallback: Boolean): String { + return if (fallback) "!= false" else "== true" + } + + @ParameterizedTest + @ValueSource(booleans = [true, false]) + fun `does not report when there is no context`(bool: Boolean) { + val code = """ + import kotlin.random.Random + + fun nullableBoolean(): Boolean? = true.takeIf { Random.nextBoolean() } + + fun foo(): Boolean { + return nullableBoolean() ?: $bool + } + """ + + assertThat(subject.compileAndLint(code)).isEmpty() + } + + @ParameterizedTest + @ValueSource(booleans = [true, false]) + fun `reports elvis in statement`(bool: Boolean) { + val code = """ + import kotlin.random.Random + + fun nullableBoolean(): Boolean? = true.takeIf { Random.nextBoolean() } + + fun foo(): Boolean { + return nullableBoolean() ?: $bool + } + """ + + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + assertThat(findings).first().extracting { it.message }.isEqualTo( + "The nullable boolean check `nullableBoolean() ?: $bool` should use " + + "`${replacementForElvis(bool)}` rather than `?: $bool`" + ) + } + + @ParameterizedTest + @ValueSource(booleans = [true, false]) + fun `reports elvis in if condition`(bool: Boolean) { + val code = """ + import kotlin.random.Random + + fun nullableBoolean(): Boolean? = true.takeIf { Random.nextBoolean() } + + fun foo() { + if (nullableBoolean() ?: $bool) println("foo") + } + """ + + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + assertThat(findings).first().extracting { it.message }.isEqualTo( + "The nullable boolean check `nullableBoolean() ?: $bool` should use " + + "`${replacementForElvis(bool)}` rather than `?: $bool`" + ) + } + + @Test + fun `does not report for non-constant fallback`() { + val code = """ + import kotlin.random.Random + + fun nullableBoolean(): Boolean? = true.takeIf { Random.nextBoolean() } + + fun foo(): Boolean { + return nullableBoolean() ?: Random.nextBoolean() + } + """ + + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @ParameterizedTest + @ValueSource(booleans = [true, false]) + fun `does not report elvis for non-boolean statement with boolean default`(bool: Boolean) { + val code = """ + import kotlin.random.Random + + fun nullableAny(): Any? = Unit.takeIf { Random.nextBoolean() } + + fun foo(): Any { + return nullableAny() ?: $bool + } + """ + + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `does not report non-boolean elvis`() { + val code = """ + import kotlin.random.Random + + fun nullableInt(): Int? = 42.takeIf { Random.nextBoolean() } + + fun foo(): Int { + return nullableInt() ?: 0 + } + """ + + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `does not report non-elvis binary expression`() { + val code = """ + import kotlin.random.Random + + fun foo(): Boolean { + return Random.nextBoolean() || false + } + """ + + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } +}