Add NullableBooleanCheck rule (#4872)

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`.

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:

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).
+ *
+ * <noncompliant>
+ * value ?: true
+ * value ?: false
+ * </noncompliant>
+ *
+ * <compliant>
+ * value != false
+ * value == true
+ * </compliant>
+ */
+@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
+}

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),

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()
+    }
+}