UnnecessaryPartOfBinaryExpression: fix false positive with pair creat…

…ion (#5516)
detekt · Nov 9, 2022 · d4d6cbb · d4d6cbb
1 parent be825ff
commit d4d6cbb
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 36 deletions.
diff --git a/...kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt b/...kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt
@@ -10,8 +10,8 @@ import io.gitlab.arturbosch.detekt.api.Severity
 import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.KtBinaryExpression
-import org.jetbrains.kotlin.psi.KtElement
 import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.utils.addIfNotNull
 
 /**
  * Unnecessary binary expression add complexity to the code and accomplish nothing. They should be removed.
@@ -45,48 +45,34 @@ class UnnecessaryPartOfBinaryExpression(config: Config = Config.empty) : Rule(co
 
     override fun visitBinaryExpression(expression: KtBinaryExpression) {
         super.visitBinaryExpression(expression)
-        if (expression.parent is KtBinaryExpression) {
-            return
-        }
-
-        val isOrOr = expression.getAllOperations().all { it != KtTokens.OROR }
-        val isAndAnd = expression.getAllOperations().all { it != KtTokens.ANDAND }
 
-        if (isOrOr || isAndAnd) {
-            val allChildren = expression.getAllVariables().map { it.text.replace(Regex("\\s"), "") }
+        val operator = expression.operationToken
+        if (operator != KtTokens.OROR && operator != KtTokens.ANDAND) return
 
-            if (allChildren != allChildren.distinct()) {
-                report(CodeSmell(issue, Entity.from(expression), issue.description))
-            }
-        }
-    }
+        val parent = expression.parent
+        if (parent is KtBinaryExpression && parent.operationToken == operator) return
 
-    private fun KtBinaryExpression.getAllVariables(): List<KtElement> {
-        return buildList {
-            addAll(this@getAllVariables.left?.getVariable().orEmpty())
-            addAll(this@getAllVariables.right?.getVariable().orEmpty())
+        val expressions = expression.expressions(operator).map { it.text.replace(whiteSpace, "") }
+        if (expressions.size != expressions.distinct().size) {
+            report(CodeSmell(issue, Entity.from(expression), issue.description))
         }
     }
 
-    private fun KtExpression.getVariable(): List<KtElement> {
-        return if (this is KtBinaryExpression &&
-            (this.operationToken == KtTokens.OROR || this.operationToken == KtTokens.ANDAND)
-        ) {
-            this.getAllVariables()
-        } else {
-            listOf(this)
+    private fun KtBinaryExpression.expressions(operator: IElementType): List<KtExpression> {
+        val expressions = mutableListOf<KtExpression>()
+        fun collect(expression: KtExpression?) {
+            if (expression is KtBinaryExpression && expression.operationToken == operator) {
+                collect(expression.left)
+                collect(expression.right)
+            } else {
+                expressions.addIfNotNull(expression)
+            }
         }
+        collect(this)
+        return expressions
     }
 
-    private fun KtBinaryExpression.getAllOperations(): List<IElementType> {
-        return buildList {
-            (this@getAllOperations.left as? KtBinaryExpression)?.let {
-                addAll(it.getAllOperations())
-            }
-            (this@getAllOperations.right as? KtBinaryExpression)?.let {
-                addAll(it.getAllOperations())
-            }
-            add(this@getAllOperations.operationToken)
-        }
+    companion object {
+        private val whiteSpace = "\\s".toRegex()
     }
 }
diff --git a/...in/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt b/...in/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt
@@ -1,7 +1,7 @@
 package io.gitlab.arturbosch.detekt.rules.performance
 
+import io.gitlab.arturbosch.detekt.test.assertThat
 import io.gitlab.arturbosch.detekt.test.compileAndLint
-import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
 
 class UnnecessaryPartOfBinaryExpressionSpec {
@@ -92,6 +92,22 @@ class UnnecessaryPartOfBinaryExpressionSpec {
         assertThat(findings).hasSize(0)
     }
 
+    @Test
+    fun `Report error if condition contains different operators`() {
+        val code = """
+            fun bar() {
+                val foo = true
+                val baz = false
+                if (foo || baz && baz) {
+                    //TODO    
+                }
+            }
+        """.trimIndent()
+
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1).hasTextLocations("baz && baz")
+    }
+
     @Test
     fun `verify foo or foo detected`() {
         val code = """
@@ -268,4 +284,16 @@ class UnnecessaryPartOfBinaryExpressionSpec {
         val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
         assertThat(findings).hasSize(1)
     }
+
+    @Test
+    fun `Don't raise issues with pair creation`() {
+        val code = """
+            fun foo() {
+                1 to 1
+            }
+        """.trimIndent()
+
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(0)
+    }
 }