Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new rule CouldBeSequence #4855

Merged
merged 3 commits into from Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Expand Up @@ -364,6 +364,9 @@ performance:
active: true
ArrayPrimitive:
active: true
CouldBeSequence:
active: false
threshold: 3
ForEachOnRange:
active: true
excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**']
Expand Down
@@ -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)
*
* <noncompliant>
* listOf(1, 2, 3, 4).map { it*2 }.filter { it < 4 }.map { it*it }
* </noncompliant>
*
* <compliant>
* listOf(1, 2, 3, 4).asSequence().map { it*2 }.filter { it < 4 }.map { it*it }
themkat marked this conversation as resolved.
Show resolved Hide resolved
*
* listOf(1, 2, 3, 4).map { it*2 }
* </compliant>
*/
@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<KtExpression>()

@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<FqName>): 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")
)
}
}
Expand Up @@ -19,7 +19,8 @@ class PerformanceProvider : DefaultRuleSetProvider {
ForEachOnRange(config),
SpreadOperator(config),
UnnecessaryTemporaryInstantiation(config),
ArrayPrimitive(config)
ArrayPrimitive(config),
CouldBeSequence(config)
)
)
}
@@ -0,0 +1,54 @@
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)
themkat marked this conversation as resolved.
Show resolved Hide resolved
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()
}
}