From a949686e71eb2a8bd83e08d45ab782ab97b6b648 Mon Sep 17 00:00:00 2001
From: Marie Katrine Ekeberg <5732795+themkat@users.noreply.github.com>
Date: Wed, 1 Jun 2022 10:59:46 +0200
Subject: [PATCH] Add new rule CouldBeSequence (#4855)
* Add new rule CouldBeSequence
* Add test case for CouldBeSequence
* Fix inconsistency in compliant example for CoudlBeSequence
---
.../main/resources/default-detekt-config.yml | 3 +
.../rules/performance/CouldBeSequence.kt | 102 ++++++++++++++++++
.../rules/performance/PerformanceProvider.kt | 3 +-
.../rules/performance/CouldBeSequenceSpec.kt | 69 ++++++++++++
4 files changed, 176 insertions(+), 1 deletion(-)
create mode 100644 detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequence.kt
create mode 100644 detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequenceSpec.kt
diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml
index d1f23451b23..17c114392c4 100644
--- a/detekt-core/src/main/resources/default-detekt-config.yml
+++ b/detekt-core/src/main/resources/default-detekt-config.yml
@@ -367,6 +367,9 @@ performance:
active: true
ArrayPrimitive:
active: true
+ CouldBeSequence:
+ active: false
+ threshold: 3
ForEachOnRange:
active: true
excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**']
diff --git a/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequence.kt b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequence.kt
new file mode 100644
index 00000000000..178cded16eb
--- /dev/null
+++ b/detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequence.kt
@@ -0,0 +1,102 @@
+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 io.gitlab.arturbosch.detekt.api.config
+import io.gitlab.arturbosch.detekt.api.internal.Configuration
+import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
+import org.jetbrains.kotlin.name.FqName
+import org.jetbrains.kotlin.psi.KtCallExpression
+import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForReceiver
+import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForSelectorOrThis
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
+import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameOrNull
+
+/**
+ * Long chains of collection operations will have a performance penalty due to a new list being created for each call. Consider using sequences instead. Read more about this in the [documentation](https://kotlinlang.org/docs/sequences.html)
+ *
+ *
+ * listOf(1, 2, 3, 4).map { it*2 }.filter { it < 4 }.map { it*it }
+ *
+ *
+ *
+ * listOf(1, 2, 3, 4).asSequence().map { it*2 }.filter { it < 4 }.map { it*it }.toList()
+ *
+ * listOf(1, 2, 3, 4).map { it*2 }
+ *
+ */
+@RequiresTypeResolution
+class CouldBeSequence(config: Config = Config.empty) : Rule(config) {
+
+ override val issue: Issue = Issue(
+ "CouldBeSequence",
+ Severity.Performance,
+ "Several chained collection operations that should be a sequence.",
+ Debt.FIVE_MINS
+ )
+
+ @Configuration("the number of chained collection operations required to trigger rule")
+ private val threshold: Int by config(defaultValue = 3)
+
+ private var visitedCallExpressions = mutableListOf()
+
+ @Suppress("ReturnCount")
+ override fun visitCallExpression(expression: KtCallExpression) {
+ super.visitCallExpression(expression)
+
+ if (bindingContext == BindingContext.EMPTY) return
+
+ if (visitedCallExpressions.contains(expression)) return
+
+ if (!expression.isCalling(operationsFqNames)) return
+
+ var counter = 1
+ var nextCall = expression.nextChainedCall()
+ while (counter < threshold && nextCall != null) {
+ visitedCallExpressions += nextCall
+ if (!nextCall.isCalling(operationsFqNames)) {
+ break
+ }
+
+ counter++
+ nextCall = nextCall.nextChainedCall()
+ }
+
+ if (counter >= threshold) {
+ val message = "${expression.text} could be .asSequence().${expression.text}"
+ report(CodeSmell(issue, Entity.from(expression), message))
+ }
+ }
+
+ private fun KtExpression.nextChainedCall(): KtExpression? {
+ val expression = this.getQualifiedExpressionForSelectorOrThis()
+ return expression.getQualifiedExpressionForReceiver()?.selectorExpression
+ }
+
+ private fun KtExpression.isCalling(fqNames: List): Boolean {
+ val calleeText = (this as? KtCallExpression)?.calleeExpression?.text ?: this.text
+ val targetFqNames = fqNames.filter { it.shortName().asString() == calleeText }
+ if (targetFqNames.isEmpty()) return false
+ return getResolvedCall(bindingContext)?.resultingDescriptor?.fqNameOrNull() in targetFqNames
+ }
+
+ companion object {
+ private val operationsFqNames = listOf(
+ FqName("kotlin.collections.filter"),
+ FqName("kotlin.collections.filterIndexed"),
+ FqName("kotlin.collections.map"),
+ FqName("kotlin.collections.mapIndexed"),
+ FqName("kotlin.collections.flatMap"),
+ FqName("kotlin.collections.flatMapIndexed"),
+ FqName("kotlin.collections.reduce"),
+ FqName("kotlin.collections.zip")
+ )
+ }
+}
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 c7ad20c54c8..869647d0e7e 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
@@ -19,7 +19,8 @@ class PerformanceProvider : DefaultRuleSetProvider {
ForEachOnRange(config),
SpreadOperator(config),
UnnecessaryTemporaryInstantiation(config),
- ArrayPrimitive(config)
+ ArrayPrimitive(config),
+ CouldBeSequence(config)
)
)
}
diff --git a/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequenceSpec.kt b/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequenceSpec.kt
new file mode 100644
index 00000000000..1deab2c4678
--- /dev/null
+++ b/detekt-rules-performance/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/performance/CouldBeSequenceSpec.kt
@@ -0,0 +1,69 @@
+package io.gitlab.arturbosch.detekt.rules.performance
+
+import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
+import io.gitlab.arturbosch.detekt.test.TestConfig
+import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
+import org.assertj.core.api.Assertions.assertThat
+import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
+import org.junit.jupiter.api.Test
+
+@KotlinCoreEnvironmentTest
+class CouldBeSequenceSpec(val env: KotlinCoreEnvironment) {
+ val subject = CouldBeSequence(TestConfig(mapOf("threshold" to 3)))
+
+ @Test
+ fun `long collection chain should be sequence`() {
+ val code = """
+ val myCollection = listOf(1, 2, 3, 4, 5, 6, 7, 8)
+ val processed = myCollection.filter {
+ it % 2 == 0
+ }.map {
+ it*2
+ }.filter {
+ it > 5
+ }
+ """
+ assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
+ }
+
+ @Test
+ fun `one collection operation should not trigger rule`() {
+ val code = """
+ val myCollection = listOf(1, 2, 3, 4, 5, 6, 7, 8)
+ val processed = myCollection.filter {
+ it % 2 == 0
+ }
+ """
+ assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+ }
+
+ @Test
+ fun `sequence operations should not trigger rule`() {
+ val code = """
+ val myCollection = listOf(1, 2, 3, 4, 5, 6, 7, 8)
+ val processed = myCollection.asSequence().filter {
+ it % 2 == 0
+ }.map {
+ it*2
+ }.filter {
+ it > 5
+ }
+ """
+ assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+ }
+
+ @Test
+ fun `sequence should not trigger rule`() {
+ val code = """
+ val mySequence = sequenceOf(1,10,4,6,8,39)
+ val processed = mySequence.filter {
+ it % 2 == 0
+ }.map {
+ it*2
+ }.filter {
+ it > 5
+ }.toList()
+ """
+ assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
+ }
+}