Implement rule UnnecessaryPartOfBinaryExpression (#5203)

* Implement check for unnecessary part of binary expression

* Update the default config

* Fixed import and code style

* Fixed import

* Fixed ci issues

* Update detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt

Co-authored-by: Nicola Corti <corti.nico@gmail.com>

* Fixed tests

Co-authored-by: v.stelmashchuk <volodymyr.stelmashchuk@pm.bet>
Co-authored-by: Nicola Corti <corti.nico@gmail.com>

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

@@ -391,6 +391,8 @@ performance:
   SpreadOperator:
     active: true
     excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**']
+  UnnecessaryPartOfBinaryExpression:
+    active: false
   UnnecessaryTemporaryInstantiation:
     active: true
 

detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiverSpec.kt

@@ -115,6 +115,8 @@ class SuspendFunWithCoroutineScopeReceiverSpec(val env: KotlinCoreEnvironment) {
         fun `no reports when suspend function has Long as receiver`() {
             val code = """
                 import kotlinx.coroutines.delay
+                import kotlinx.coroutines.launch
+                import kotlinx.coroutines.CoroutineScope
 
                 suspend fun Long.foo() = coroutineScope {
                     launch {

detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt

@@ -20,7 +20,8 @@ class PerformanceProvider : DefaultRuleSetProvider {
             SpreadOperator(config),
             UnnecessaryTemporaryInstantiation(config),
             ArrayPrimitive(config),
-            CouldBeSequence(config)
+            CouldBeSequence(config),
+            UnnecessaryPartOfBinaryExpression(config),
         )
     )
 }

detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt

@@ -0,0 +1,92 @@
+package io.gitlab.arturbosch.detekt.rules.performance
+
+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 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
+
+/**
+ * Unnecessary binary expression add complexity to the code and accomplish nothing. They should be removed.
+ * The rule works with all binary expression included if and when condition. The rule also works with all predicates.
+ * The rule verify binary expression only in case when the expression use only one type of the following
+ * operators || or &&.
+ *
+ * <noncompliant>
+ * val foo = true
+ * val bar = true
+ *
+ * if (foo || bar || foo) {
+ * }
+ * </noncompliant>
+ *
+ * <compliant>
+ * val foo = true
+ * if (foo) {
+ * }
+ * </compliant>
+ *
+ */
+class UnnecessaryPartOfBinaryExpression(config: Config = Config.empty) : Rule(config) {
+
+    override val issue: Issue = Issue(
+        "UnnecessaryPartOfBinaryExpression",
+        Severity.Performance,
+        "Detects duplicate condition into binary expression and recommends to remove unnecessary checks",
+        Debt.FIVE_MINS
+    )
+
+    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"), "") }
+
+            if (allChildren != allChildren.distinct()) {
+                report(CodeSmell(issue, Entity.from(expression), issue.description))
+            }
+        }
+    }
+
+    private fun KtBinaryExpression.getAllVariables(): List<KtElement> {
+        return buildList {
+            addAll(this@getAllVariables.left?.getVariable().orEmpty())
+            addAll(this@getAllVariables.right?.getVariable().orEmpty())
+        }
+    }
+
+    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.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)
+        }
+    }
+}

detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt

@@ -0,0 +1,268 @@
+package io.gitlab.arturbosch.detekt.rules.performance
+
+import io.gitlab.arturbosch.detekt.test.compileAndLint
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+
+class UnnecessaryPartOfBinaryExpressionSpec {
+
+    @Test
+    fun `Verify if condition with several arguments`() {
+        val code = """
+            fun bar() {
+                val foo = true
+                val baz = false
+                if (foo || baz || foo) {
+                    //TODO    
+                }
+            }
+        """
+
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `No report several arguments with object-foo math operator and bool val`() {
+        val code = """
+            class Bar(val bar: Boolean)
+            fun bar() {
+                val foo = true
+                val baz = 10
+                val bar = Bar(true)
+                   
+                if (baz < 10 || foo || bar.bar || baz > 10) {
+                    //TODO    
+                }
+            }
+        """
+
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(0)
+    }
+
+    @Test
+    fun `Report several arguments with object-foo math operator and bool val`() {
+        val code = """
+            class Bar(val bar: Boolean)
+            fun bar() {
+                val foo = true
+                val baz = 10
+                val bar = Bar(true)
+                   
+                if (baz < 10 || foo || bar.bar || baz > 10 || baz < 10) {
+                    //TODO    
+                }
+            }
+        """
+
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `Not report error if condition contains different operators`() {
+        val code = """
+            fun bar() {
+                val foo = true
+                val baz = false
+                if (foo || baz && foo) {
+                    //TODO    
+                }
+            }
+        """
+
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(0)
+    }
+
+    @Test
+    fun `Not report error if condition contains different operators with other binary expression`() {
+        val code = """
+            fun bar() {
+                val foo = 5
+                val baz = false
+                if (foo < 5 || baz && foo > 5) {
+                    //TODO    
+                }
+            }
+        """
+
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(0)
+    }
+
+    @Test
+    fun `verify foo or foo detected`() {
+        val code = """
+            fun bar() {
+                val foo = true
+                if (foo || foo) {
+                    //TODO    
+                }
+            }
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify object-foo or object-foo or object-bar detected`() {
+        val code = """
+            class Bar(val bar: Boolean, val baz: Boolean)
+            fun bar() {
+                val bar = Bar(true, true)
+                
+                if (bar.bar || bar.baz || bar.bar) {
+                    //TODO    
+                }
+            }
+            
+            
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify object-foo or object-foo detected`() {
+        val code = """
+            class Bar(val bar: Boolean)
+            fun bar() {
+                val bar = Bar(true)
+                
+                if (bar.bar || bar.bar) {
+                    //TODO    
+                }
+            }
+            
+            
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify foo more 5 and foo more 5 detected`() {
+        val code = """
+            fun bar() {
+                val foo = 1
+                if (foo > 1 && foo > 1) {
+                    //TODO    
+                }
+            }
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify foo more 5 && foo more 5 detected un trim`() {
+        val code = """
+            fun bar() {
+                val foo = 1
+                if (foo> 1 && foo >1) {
+                    //TODO    
+                }
+            }
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify object-foo && object-foo detected`() {
+        val code = """
+            class Bar(val bar: Boolean)
+            
+            fun bar() {
+                val bar = Bar(true)
+                
+                if (bar.bar && bar.bar) {
+                    //TODO    
+                }
+            }
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify foo does not report`() {
+        val code = """
+            fun bar() {
+                val foo = true
+                if (foo) {
+                    //TODO    
+                }
+            }
+            
+            
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(0)
+    }
+
+    @Test
+    fun `verify more and less if works as expected`() {
+        val code = """
+            fun bar() {
+                val foo = 0
+                val bar = 1
+                if (foo > bar || foo > 1) {
+                    //TODO    
+                }
+            }
+            
+            
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(0)
+    }
+
+    @Test
+    fun `verify into filter function`() {
+        val code = """
+            fun bar() {
+                val list = listOf<Int>()
+
+                list.filter { it > 1 || it > 1 }
+            }
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify into when`() {
+        val code = """
+            fun bar() {
+                val foo = true
+                when {
+                    foo || foo -> {
+                    
+                    }
+                }
+            }
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+
+    @Test
+    fun `verify two or into when`() {
+        val code = """
+            fun bar() {
+                val foo = true
+                val bar = true
+                when {
+                    foo || bar || foo -> {
+                    
+                    }
+                }
+            }
+        """
+        val findings = UnnecessaryPartOfBinaryExpression().compileAndLint(code)
+        assertThat(findings).hasSize(1)
+    }
+}