From 9f4b0ca76240ecaf9301010fc1ba071436aeaa9a Mon Sep 17 00:00:00 2001 From: Toshiaki Kameyama Date: Sat, 15 Oct 2022 04:37:30 +0900 Subject: [PATCH] Add UseSumOfInsteadOfFlatMapSize rule (#5405) * Add UseSumOfInsteadOfFlatMapSize rule * Add tests --- .../main/resources/default-detekt-config.yml | 2 + .../rules/style/MultilineLambdaItParameter.kt | 2 +- .../detekt/rules/style/StyleGuideProvider.kt | 1 + .../style/UseSumOfInsteadOfFlatMapSize.kt | 87 ++++++++++ .../style/UseSumOfInsteadOfFlatMapSizeSpec.kt | 161 ++++++++++++++++++ 5 files changed, 252 insertions(+), 1 deletion(-) create mode 100644 detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSize.kt create mode 100644 detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSizeSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 04d72e456a8..5be6cb2c5fd 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -717,6 +717,8 @@ style: active: true UseRequireNotNull: active: true + UseSumOfInsteadOfFlatMapSize: + active: false UselessCallOnNotNull: active: true UtilityClassWithPublicConstructor: diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameter.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameter.kt index a493f2786e8..5e352f64459 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameter.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameter.kt @@ -76,7 +76,7 @@ class MultilineLambdaItParameter(val config: Config) : Rule(config) { override fun visitLambdaExpression(lambdaExpression: KtLambdaExpression) { super.visitLambdaExpression(lambdaExpression) - val size = lambdaExpression.collectDescendantsOfType().flatMap { it.statements }.size + val size = lambdaExpression.collectDescendantsOfType().sumOf { it.statements.size } if (size <= 1) return val parameterNames = lambdaExpression.valueParameters.map { it.name } diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt index cdab5191a92..3843aaa9e1b 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt @@ -105,6 +105,7 @@ class StyleGuideProvider : DefaultRuleSetProvider { UnnecessaryBackticks(config), MaxChainedCallsOnSameLine(config), AlsoCouldBeApply(config), + UseSumOfInsteadOfFlatMapSize(config), ) ) } diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSize.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSize.kt new file mode 100644 index 00000000000..670de062005 --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSize.kt @@ -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. + * + * + * class Foo(val foo: List) + * 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 + * + * + * + * 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 } + * + */ +@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()?.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()?.text == "size") { + val receiverType = receiver.getType(bindingContext) ?: return false + val descriptor = receiverType.constructor.declarationDescriptor?.safeAs() ?: return false + return descriptor.isSubclassOf(DefaultBuiltIns.Instance.list) + } + return safeAs()?.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") + } +} diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSizeSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSizeSpec.kt new file mode 100644 index 00000000000..937939e5337 --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseSumOfInsteadOfFlatMapSizeSpec.kt @@ -0,0 +1,161 @@ +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() { + val code = """ + fun test(list: List) { + list.flatMap { it.foo }.size + } + class Foo(val foo: List) + """.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) { + list.flatMap { it.foo }.count() + } + class Foo(val foo: List) + """.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) { + list.flatMap { it.foo }.count { it > 2 } + } + class Foo(val foo: List) + """.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.flatten().size + } + class Foo(val foo: List) + """.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?) { + list?.flatMap { it.foo }?.size + } + class Foo(val foo: List) + """.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.test() { + flatMap { it.foo }.size + sumOf { it.foo.size } + } + class Foo(val foo: List) + """.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) { + set.flatMap { it.bar }.count() + } + class Bar(val bar: Set) + """.trimIndent() + val actual = subject.compileAndLintWithContext(env, code) + assertThat(actual).hasSize(1) + } + + @Test + @DisplayName("Does not report flatMap") + fun noReportFlatMap() { + val code = """ + fun test(list: List) { + list.flatMap { it.foo } + } + class Foo(val foo: List) + """.trimIndent() + val actual = subject.compileAndLintWithContext(env, code) + assertThat(actual).isEmpty() + } + + @Test + @DisplayName("Does not report flatMap and first") + fun noReportFlatMapAndFirst() { + val code = """ + fun test(list: List) { + list.flatMap { it.foo }.first() + } + class Foo(val foo: List) + """.trimIndent() + val actual = subject.compileAndLintWithContext(env, code) + assertThat(actual).isEmpty() + } + + @Test + @DisplayName("Does not report flatten") + fun noReportFlatten() { + val code = """ + fun test(list: List>) { + list.flatten() + } + class Foo(val foo: List) + """.trimIndent() + val actual = subject.compileAndLintWithContext(env, code) + assertThat(actual).isEmpty() + } + + @Test + @DisplayName("Does not report flatten and last") + fun noReportFlattenAndLast() { + val code = """ + fun test(list: List>) { + list.flatten().last() + } + class Foo(val foo: List) + """.trimIndent() + val actual = subject.compileAndLintWithContext(env, code) + assertThat(actual).isEmpty() + } +}