Potential new rule: DoubleNegativeLambda

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

@@ -524,6 +524,10 @@ style:
   DestructuringDeclarationWithTooManyEntries:
     active: true
     maxDestructuringEntries: 3
+  DoubleNegativeLambda:
+    active: false
+    negativeFunctions:
+      - 'takeUnless'
   EqualsNullCall:
     active: true
   EqualsOnSignatureLine:

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

@@ -0,0 +1,95 @@
+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 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>
+ * Random.Default.nextInt().takeUnless { !it.isEven() }
+ * </noncompliant>
+ * <compliant>
+ * Random.Default.nextInt().takeIf { it.isOdd() }
+ * </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,
+    )
+
+    private val negatingFunctionNameParts = listOf("not", "non")
+
+    @Configuration("Function names expressed in the negative that can form double negatives with their lambda blocks.")
+    private val negativeFunctions: Set<String> by config(listOf("takeUnless")) { it.toSet() }
+
+    override fun visitCallExpression(expression: KtCallExpression) {
+        super.visitCallExpression(expression)
+        val calleeExpression = expression.calleeExpression?.text ?: return
+
+        if (calleeExpression in negativeFunctions) {
+            val lambdaExpression = expression.lambdaArguments.firstOrNull() ?: return
+            val forbiddenChildren = lambdaExpression.collectDescendantsOfType<KtExpression> {
+                it.isForbiddenNegation()
+            }
+
+            if (forbiddenChildren.isNotEmpty()) {
+                report(
+                    CodeSmell(
+                        issue,
+                        Entity.from(expression),
+                        "Double negative through using ${forbiddenChildren.joinInBackTicks()} inside a " +
+                            "`$calleeExpression` lambda. Rewrite in the positive."
+                    )
+                )
+            }
+        }
+    }
+
+    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 negatingFunctionNameParts }
+
+            else -> false
+        }
+    }
+
+    private fun List<KtExpression>.joinInBackTicks() = joinToString { "`${it.text}`" }
+
+    companion object {
+        const val NEGATIVE_FUNCTIONS = "negativeFunctions"
+    }
+}

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,163 @@
+package io.gitlab.arturbosch.detekt.rules.style
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.test.TestConfig
+import io.gitlab.arturbosch.detekt.test.assertThat
+import io.gitlab.arturbosch.detekt.test.compileAndLint
+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 = kotlin.random.Random.Default.nextInt().takeUnless { it > 0 && !it.isEven() }
+        """.trimIndent()
+
+        assertThat(subject.compileAndLint(code)).hasSize(1)
+    }
+
+    @Test
+    fun `reports function with 'not' in the name`() {
+        val code = """
+            import kotlin.random.Random
+            fun Int.isNotZero() = this != 0
+            val rand = kotlin.random.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 function name from config`() {
+        val config = TestConfig(DoubleNegativeLambda.NEGATIVE_FUNCTIONS to listOf("none", "filterNot"))
+        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 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."
+        )
+    }
+}