From 045a847d3dc0e262fef4395196710f7a9fe3a64f Mon Sep 17 00:00:00 2001 From: Toshiaki Kameyama Date: Tue, 8 Nov 2022 08:37:05 +0900 Subject: [PATCH] UnnecessaryPartOfBinaryExpression: fix false positive with pair creation --- .../UnnecessaryPartOfBinaryExpression.kt | 56 +++++++------------ .../UnnecessaryPartOfBinaryExpressionSpec.kt | 30 +++++++++- 2 files changed, 50 insertions(+), 36 deletions(-) diff --git a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt index fd764c619da..11d4c3dfecf 100644 --- a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt +++ b/detekt-rules-performance/src/main/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 { - 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 { - 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 { + val expressions = mutableListOf() + 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 { - 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/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt b/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt index b5f4afd41b0..a9e83432de6 100644 --- a/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt +++ b/detekt-rules-performance/src/test/kotlin/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) + } }