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 UseSumOfInsteadOfFlatMapSize rule #5405

Merged
merged 2 commits into from Oct 14, 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
2 changes: 2 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Expand Up @@ -717,6 +717,8 @@ style:
active: true
UseRequireNotNull:
active: true
UseSumOfInsteadOfFlatMapSize:
active: false
UselessCallOnNotNull:
active: true
UtilityClassWithPublicConstructor:
Expand Down
Expand Up @@ -76,7 +76,7 @@ class MultilineLambdaItParameter(val config: Config) : Rule(config) {

override fun visitLambdaExpression(lambdaExpression: KtLambdaExpression) {
super.visitLambdaExpression(lambdaExpression)
val size = lambdaExpression.collectDescendantsOfType<KtBlockExpression>().flatMap { it.statements }.size
val size = lambdaExpression.collectDescendantsOfType<KtBlockExpression>().sumOf { it.statements.size }
if (size <= 1) return

val parameterNames = lambdaExpression.valueParameters.map { it.name }
Expand Down
Expand Up @@ -105,6 +105,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
UnnecessaryBackticks(config),
MaxChainedCallsOnSameLine(config),
AlsoCouldBeApply(config),
UseSumOfInsteadOfFlatMapSize(config),
)
)
}
@@ -0,0 +1,87 @@
package io.gitlab.arturbosch.detekt.rules.style

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.internal.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.rules.isCalling
import io.gitlab.arturbosch.detekt.rules.safeAs
import org.jetbrains.kotlin.builtins.DefaultBuiltIns
import org.jetbrains.kotlin.descriptors.ClassDescriptor
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.psi.KtCallExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtNameReferenceExpression
import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForReceiver
import org.jetbrains.kotlin.psi.psiUtil.getQualifiedExpressionForSelectorOrThis
import org.jetbrains.kotlin.resolve.calls.util.getType
import org.jetbrains.kotlin.resolve.descriptorUtil.isSubclassOf

/**
* Turn on this rule to flag `flatMap` and `size/count` calls that can be replaced with a `sumOf` call.
*
* <noncompliant>
* class Foo(val foo: List<Int>)
* list.flatMap { it.foo }.size
* list.flatMap { it.foo }.count()
* list.flatMap { it.foo }.count { it > 2 }
* listOf(listOf(1), listOf(2, 3)).flatten().size
* </noncompliant>
*
* <compliant>
* list.sumOf { it.foo.size }
* list.sumOf { it.foo.count() }
* list.sumOf { it.foo.count { foo -> foo > 2 } }
* listOf(listOf(1), listOf(2, 3)).sumOf { it.size }
* </compliant>
*/
@RequiresTypeResolution
class UseSumOfInsteadOfFlatMapSize(config: Config = Config.empty) : Rule(config) {
override val issue: Issue = Issue(
javaClass.simpleName,
Severity.Style,
"Use `sumOf` instead of `flatMap` and `size/count` calls",
Debt.FIVE_MINS
)

@Suppress("ReturnCount")
override fun visitCallExpression(expression: KtCallExpression) {
super.visitCallExpression(expression)

val calleeText = expression.calleeExpression?.text ?: return
if (!expression.isFlatMapOrFlatten()) return

val receiver = expression.getQualifiedExpressionForSelectorOrThis()
val selector = receiver.getQualifiedExpressionForReceiver()?.selectorExpression ?: return
if (!selector.isSizeOrCount(receiver)) return

val selectorText = selector.safeAs<KtCallExpression>()?.calleeExpression?.text ?: selector.text
val message = "Use 'sumOf' instead of '$calleeText' and '$selectorText'"
report(CodeSmell(issue, Entity.from(expression), message))
}

private fun KtCallExpression.isFlatMapOrFlatten(): Boolean = isCalling(flatMapAndFlattenFqName, bindingContext)

@Suppress("ReturnCount")
private fun KtExpression.isSizeOrCount(receiver: KtExpression): Boolean {
if (safeAs<KtNameReferenceExpression>()?.text == "size") {
val receiverType = receiver.getType(bindingContext) ?: return false
val descriptor = receiverType.constructor.declarationDescriptor?.safeAs<ClassDescriptor>() ?: return false
return descriptor.isSubclassOf(DefaultBuiltIns.Instance.list)
}
return safeAs<KtCallExpression>()?.isCalling(countFqName, bindingContext) == true
}

companion object {
private val flatMapAndFlattenFqName = listOf(
FqName("kotlin.collections.flatMap"),
FqName("kotlin.collections.flatten"),
)

private val countFqName = FqName("kotlin.collections.count")
}
}
@@ -0,0 +1,109 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest
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.DisplayName
import org.junit.jupiter.api.Test

@KotlinCoreEnvironmentTest
class UseSumOfInsteadOfFlatMapSizeSpec(val env: KotlinCoreEnvironment) {
val subject = UseSumOfInsteadOfFlatMapSize()

@Test
@DisplayName("Reports flatMap and size")
fun reportFlatMapAndSize() {
Comment on lines +15 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason why this is not:

fun `reports flatMap and size`() {

It seems like this goes a bit against the rest of the codebase convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val code = """
fun test(list: List<Foo>) {
list.flatMap { it.foo }.size
}
class Foo(val foo: List<Int>)
""".trimIndent()
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
assertThat(actual[0].message).isEqualTo("Use 'sumOf' instead of 'flatMap' and 'size'")
}

@Test
@DisplayName("Reports flatMap and count")
fun reportFlatMapAndCount() {
val code = """
fun test(list: List<Foo>) {
list.flatMap { it.foo }.count()
}
class Foo(val foo: List<Int>)
""".trimIndent()
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
assertThat(actual[0].message).isEqualTo("Use 'sumOf' instead of 'flatMap' and 'count'")
}

@Test
@DisplayName("Reports flatMap and count with a argument")
fun reportFlatMapAndCountWithArgument() {
val code = """
fun test(list: List<Foo>) {
list.flatMap { it.foo }.count { it > 2 }
}
class Foo(val foo: List<Int>)
""".trimIndent()
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
assertThat(actual[0].message).isEqualTo("Use 'sumOf' instead of 'flatMap' and 'count'")
}

@Test
@DisplayName("Reports flatten and size")
fun reportFlattenAndSize() {
val code = """
fun test(list: List<List<Int>>) {
list.flatten().size
}
class Foo(val foo: List<Int>)
""".trimIndent()
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
assertThat(actual[0].message).isEqualTo("Use 'sumOf' instead of 'flatten' and 'size'")
}

@Test
@DisplayName("Reports flatMap and size on nullable list")
fun reportFlatMapAndSizeOnNullableList() {
val code = """
fun test(list: List<Foo>?) {
list?.flatMap { it.foo }?.size
}
class Foo(val foo: List<Int>)
""".trimIndent()
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
}

@Test
@DisplayName("Reports flatMap and size on implicit list receiver")
fun reportFlatMapAndSizeOnImplicitListReceiver() {
val code = """
fun List<Foo>.test() {
flatMap { it.foo }.size
sumOf { it.foo.size }
}
class Foo(val foo: List<Int>)
""".trimIndent()
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
}

@Test
@DisplayName("Reports flatMap and count on Set")
fun reportFlatMapAndCountOnSet() {
val code = """
fun test(set: Set<Bar>) {
set.flatMap { it.bar }.count()
}
class Bar(val bar: Set<Int>)
""".trimIndent()
val actual = subject.compileAndLintWithContext(env, code)
assertThat(actual).hasSize(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some tests to ensure that this rule doesn't flag all the flatMap calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}