From c1178f28cb4a9e5b0e766bfad2a03be5a91bc80f Mon Sep 17 00:00:00 2001 From: Volodymyr Stelmashchuk Date: Wed, 7 Sep 2022 11:38:53 +0300 Subject: [PATCH] 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 * Fixed tests Co-authored-by: v.stelmashchuk Co-authored-by: Nicola Corti --- .../main/resources/default-detekt-config.yml | 2 + ...uspendFunWithCoroutineScopeReceiverSpec.kt | 2 + .../rules/performance/PerformanceProvider.kt | 3 +- .../UnnecessaryPartOfBinaryExpression.kt | 92 ++++++ .../UnnecessaryPartOfBinaryExpressionSpec.kt | 268 ++++++++++++++++++ 5 files changed, 366 insertions(+), 1 deletion(-) create mode 100644 detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpression.kt create mode 100644 detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryPartOfBinaryExpressionSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 219fcc7ad02..69ddbbbab94 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/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 diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiverSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiverSpec.kt index 517985d5694..f8e1d1529b3 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunWithCoroutineScopeReceiverSpec.kt +++ b/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 { diff --git a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt index 869647d0e7e..b453c23d1d5 100644 --- a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/PerformanceProvider.kt +++ b/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), ) ) } 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 new file mode 100644 index 00000000000..fd764c619da --- /dev/null +++ b/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 &&. + * + * + * val foo = true + * val bar = true + * + * if (foo || bar || foo) { + * } + * + * + * + * val foo = true + * if (foo) { + * } + * + * + */ +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 { + return buildList { + addAll(this@getAllVariables.left?.getVariable().orEmpty()) + addAll(this@getAllVariables.right?.getVariable().orEmpty()) + } + } + + 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.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) + } + } +} 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 new file mode 100644 index 00000000000..d296920b857 --- /dev/null +++ b/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() + + 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) + } +}