Potential new rule: DoubleNegativeLambda

detekt-core/src/main/resources/default-detekt-config.yml

@@ -524,6 +524,14 @@ style:
   DestructuringDeclarationWithTooManyEntries:
     active: true
     maxDestructuringEntries: 3
+  DoubleNegativeLambda:
+    active: false
+    negativeFunctions:
+      - reason: 'takeIf'
+        value: 'takeUnless'
+    negativeFunctionNameParts:
+      - 'not'
+      - 'non'
   EqualsNullCall:
     active: true
   EqualsOnSignatureLine:

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambda.kt

@@ -0,0 +1,130 @@
+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.config
+import io.gitlab.arturbosch.detekt.api.internal.Configuration
+import io.gitlab.arturbosch.detekt.api.valuesWithReason
+import org.jetbrains.kotlin.lexer.KtTokens
+import org.jetbrains.kotlin.psi.KtCallExpression
+import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.KtOperationReferenceExpression
+import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType
+
+/**
+ * Detects negation in lambda blocks where the function name is also in the negative (like `takeUnless`).
+ * A double negative is harder to read than a positive. In particular, if there are multiple conditions with `&&` etc. inside
+ * the lambda, then the reader may need to unpack these using DeMorgan's laws. Consider rewriting the lambda to use a positive version
+ * of the function (like `takeIf`).
+ *
+ * <noncompliant>
+ * fun Int.evenOrNull() = takeUnless { it % 2 != 0 }
+ * </noncompliant>
+ * <compliant>
+ * fun Int.evenOrNull() = takeIf { it % 2 == 0 }
+ * </compliant>
+ */
+class DoubleNegativeLambda(config: Config = Config.empty) : Rule(config) {
+
+    override val issue = Issue(
+        "DoubleNegativeLambda",
+        Severity.Style,
+        "Double negative from a function name expressed in the negative (like `takeUnless`) with a lambda block " +
+            "that also contains negation. This is more readable when rewritten using a positive form of the function " +
+            "(like `takeIf`).",
+        Debt.FIVE_MINS,
+    )
+
+    private val splitCamelCaseRegex = "(?<=[a-z])(?=[A-Z])".toRegex()
+
+    private val negationTokens = listOf(
+        KtTokens.EXCL,
+        KtTokens.EXCLEQ,
+        KtTokens.EXCLEQEQEQ,
+        KtTokens.NOT_IN,
+        KtTokens.NOT_IS,
+    )
+
+    @Configuration(
+        "Function names expressed in the negative that can form double negatives with their lambda blocks. " +
+            "These are grouped together with the positive counterpart of the function, or `null` if this is unknown."
+    )
+    private val negativeFunctions: List<NegativeFunction> by config(
+        valuesWithReason(
+            "takeUnless" to "takeIf",
+        )
+    ) { list ->
+        list.map { NegativeFunction(simpleName = it.value, positiveCounterpart = it.reason) }
+    }
+
+    @Configuration(
+        "Function name parts to look for in the lambda block when deciding " +
+            "if the lambda contains a negative."
+    )
+    private val negativeFunctionNameParts: Set<String> by config(listOf("not", "non")) { it.toSet() }
+
+    override fun visitCallExpression(expression: KtCallExpression) {
+        super.visitCallExpression(expression)
+        val calleeExpression = expression.calleeExpression?.text ?: return
+        val negativeFunction = negativeFunctions.firstOrNull { it.simpleName == calleeExpression } ?: return
+        val lambdaArgument = expression.lambdaArguments.firstOrNull() ?: return
+
+        val forbiddenChildren = lambdaArgument.collectDescendantsOfType<KtExpression> {
+            it.isForbiddenNegation()
+        }
+
+        if (forbiddenChildren.isNotEmpty()) {
+            report(
+                CodeSmell(
+                    issue,
+                    Entity.from(expression),
+                    formatMessage(forbiddenChildren, negativeFunction)
+                )
+            )
+        }
+    }
+
+    private fun KtExpression.isForbiddenNegation(): Boolean {
+        return when (this) {
+            is KtOperationReferenceExpression -> operationSignTokenType in negationTokens
+            is KtCallExpression -> text == "not()" || text.split(splitCamelCaseRegex).map { it.lowercase() }
+                .any { it in negativeFunctionNameParts }
+
+            else -> false
+        }
+    }
+
+    private fun formatMessage(
+        forbiddenChildren: List<KtExpression>,
+        negativeFunction: NegativeFunction,
+    ) = buildString {
+        append("Double negative through using ${forbiddenChildren.joinInBackTicks()} inside a ")
+        append("`${negativeFunction.simpleName}` lambda. ")
+        append("Rewrite in the positive")
+        if (negativeFunction.positiveCounterpart != null) {
+            append(" with `${negativeFunction.positiveCounterpart}`.")
+        } else {
+            append(".")
+        }
+    }
+
+    private fun List<KtExpression>.joinInBackTicks() = joinToString { "`${it.text}`" }
+
+    /**
+     * A function that can form a double negative with its lambda.
+     */
+    private data class NegativeFunction(
+        val simpleName: String,
+        val positiveCounterpart: String?,
+    )
+
+    companion object {
+        const val NEGATIVE_FUNCTIONS = "negativeFunctions"
+        const val NEGATIVE_FUNCTION_NAME_PARTS = "negativeFunctionNameParts"
+    }
+}

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt

@@ -106,6 +106,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
             MaxChainedCallsOnSameLine(config),
             AlsoCouldBeApply(config),
             UseSumOfInsteadOfFlatMapSize(config),
+            DoubleNegativeLambda(config),
         )
     )
 }

detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambdaSpec.kt

@@ -0,0 +1,238 @@
+package io.gitlab.arturbosch.detekt.rules.style
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.ValueWithReason
+import io.gitlab.arturbosch.detekt.test.TestConfig
+import io.gitlab.arturbosch.detekt.test.assertThat
+import io.gitlab.arturbosch.detekt.test.compileAndLint
+import io.gitlab.arturbosch.detekt.test.toConfig
+import org.junit.jupiter.api.Test
+
+class DoubleNegativeLambdaSpec {
+
+    private val subject = DoubleNegativeLambda(Config.empty)
+
+    @Test
+    fun `reports simple logical not`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isEven() = this % 2 == 0
+            val rand = Random.Default.nextInt().takeUnless { !it.isEven() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports logical not in binary expression`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isEven() = this % 2 == 0
+            val rand = Random.Default.nextInt().takeUnless { it > 0 && !it.isEven() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports logical not prefixing brackets`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isEven() = this % 2 == 0
+            val rand = Random.Default.nextInt().takeUnless { !(it == 0 || it.isEven()) }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports negation inside nested lambda`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isEven() = this % 2 == 0
+            val rand = Random.Default.nextInt().takeUnless { it.isEven().takeIf { i -> !i } ?: false }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports double nested negation`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isEven() = this % 2 == 0
+            val rand = Random.Default.nextInt().takeUnless { it.isEven().takeUnless { i -> !i } ?: false }
+        """.trimIndent()
+
+        val findings = subject.compileAndLint(code)
+        assertThat(findings).hasSize(2)
+        assertThat(findings[0]).hasSourceLocation(3, 62) // second takeUnless
+        assertThat(findings[1]).hasSourceLocation(3, 37) // first takeUnless
+    }
+
+    @Test
+    fun `reports function with 'not' in the name`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isNotZero() = this != 0
+            val rand = Random.Default.nextInt().takeUnless { it.isNotZero() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports zero-param function with 'non' in the name`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isNonNegative() = 0 < this
+            val rand = Random.Default.nextInt().takeUnless { it.isNonNegative() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports single-param function with 'non' in the name`() {
+        val code = """
+            import kotlin.random.Random            
+            fun Int.isNotGreaterThan(other: Int) = other < this
+            val rand = Random.Default.nextInt().takeUnless { it.isNotGreaterThan(0) }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports not equal`() {
+        val code = """
+            import kotlin.random.Random
+            val rand = Random.Default.nextInt().takeUnless { it != 0 }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports not equal by reference`() {
+        val code = """
+            import kotlin.random.Random
+            val rand = Random.Default.nextInt().takeUnless { it !== 0 }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports !in`() {
+        val code = """
+            import kotlin.random.Random
+            val rand = Random.Default.nextInt().takeUnless { it !in 1..3 }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports !is`() {
+        val code = """
+            val list = listOf(3, "a", true)
+            val maybeBoolean = list.firstOrNull().takeUnless { it !is Boolean }
+        """.trimIndent()
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports use of operator fun not`() {
+        val code = """
+            import kotlin.random.Random
+            val rand = Random.Default.nextInt().takeUnless { (it > 0).not() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `does not report function with 'not' in part of the name`() {
+        val code = """
+            fun String.hasAnnotations() = this.contains("annotations")
+            val nonAnnotated = "".takeUnless { it.hasAnnotations() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).isEmpty()
+    }
+
+    @Test
+    fun `does not report non-null assert in takeUnless`() {
+        val code = """
+            val x = "".takeUnless { it!!.isEmpty() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).isEmpty()
+    }
+
+    @Test
+    fun `reports negative function name from config`() {
+        val config =
+            TestConfig(
+                DoubleNegativeLambda.NEGATIVE_FUNCTIONS to listOf(
+                    ValueWithReason(value = "none", reason = "any").toConfig(),
+                    ValueWithReason(value = "filterNot", reason = "filter").toConfig(),
+                )
+            )
+        val code = """
+            fun Int.isEven() = this % 2 == 0
+            val isValid = listOf(1, 2, 3).filterNot { !it.isEven() }.none { it != 0 }
+        """.trimIndent()
+
+        assertThat(DoubleNegativeLambda(config).compileAndLint(code)).hasSize(2)
+    }
+
+    @Test
+    fun `reports negative function name parts from config`() {
+        val config = TestConfig(DoubleNegativeLambda.NEGATIVE_FUNCTION_NAME_PARTS to listOf("isnt"))
+        val code = """
+            import kotlin.random.Random
+            fun Int.isntOdd() = this % 2 == 0
+            val rand = Random.Default.nextInt().takeUnless { it.isntOdd() }
+        """.trimIndent()
+
+        assertThat(DoubleNegativeLambda(config).compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports multiple negations in message`() {
+        val code = """
+            import kotlin.random.Random
+            val list: List<Int> = listOf(1, 2, 3)
+            val rand = Random.Default.nextInt().takeUnless { it !in list && it != 0 }
+        """.trimIndent()
+
+        val findings = subject.compileAndLint(code)
+        assertThat(findings).hasSize(1)
+        assertThat(findings).hasStartSourceLocation(3, 37)
+        assertThat(findings).hasEndSourceLocation(3, 74)
+        assertThat(findings[0]).hasMessage(
+            "Double negative through using `!in`, `!=` inside a `takeUnless` lambda. Rewrite in the positive with `takeIf`."
+        )
+    }
+
+    @Test
+    fun `report for negative function with no positive counterpart`() {
+        val config =
+            TestConfig(
+                DoubleNegativeLambda.NEGATIVE_FUNCTIONS to listOf(
+                    ValueWithReason(value = "none", reason = null).toConfig(),
+                )
+            )
+        val code = """
+            val list = listOf(1, 2, 3)
+            val result = list.none { it != 0 }
+        """.trimIndent()
+
+        val findings = DoubleNegativeLambda(config).compileAndLint(code)
+        assertThat(findings[0]).hasMessage(
+            "Double negative through using `!=` inside a `none` lambda. Rewrite in the positive."
+        )
+    }
+}