From 3be97d1bc4aebfd734fae03b25a053bf8de0c8b2 Mon Sep 17 00:00:00 2001 From: Peter Mandeljc Date: Thu, 28 Apr 2022 17:11:43 +0200 Subject: [PATCH] Add NestedScopeFunctions Rule --- .../main/resources/default-detekt-config.yml | 7 ++ .../rules/complexity/ComplexityProvider.kt | 1 + .../rules/complexity/NestedScopeFunctions.kt | 95 ++++++++++++++++ .../complexity/NestedScopeFunctionsSpec.kt | 105 ++++++++++++++++++ 4 files changed, 208 insertions(+) create mode 100644 detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctions.kt create mode 100644 detekt-rules-complexity/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctionsSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index f67bed5e645d..3fe241bbc746 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -133,6 +133,13 @@ complexity: NestedBlockDepth: active: true threshold: 4 + NestedScopeFunctions: + active: false + threshold: 1 + functions: + - 'apply' + - 'run' + - 'with' ReplaceSafeCallChainWithRun: active: false StringLiteralDuplication: diff --git a/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/ComplexityProvider.kt b/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/ComplexityProvider.kt index 745e206e0c7c..ff00e730793c 100644 --- a/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/ComplexityProvider.kt +++ b/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/ComplexityProvider.kt @@ -24,6 +24,7 @@ class ComplexityProvider : DefaultRuleSetProvider { StringLiteralDuplication(config), MethodOverloading(config), NestedBlockDepth(config), + NestedScopeFunctions(config), TooManyFunctions(config), ComplexCondition(config), LabeledExpression(config), diff --git a/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctions.kt b/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctions.kt new file mode 100644 index 000000000000..c8be0b7c54f8 --- /dev/null +++ b/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctions.kt @@ -0,0 +1,95 @@ +package io.gitlab.arturbosch.detekt.rules.complexity + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.DetektVisitor +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Metric +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import io.gitlab.arturbosch.detekt.api.ThresholdedCodeSmell +import io.gitlab.arturbosch.detekt.api.config +import io.gitlab.arturbosch.detekt.api.internal.Configuration +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.psiUtil.getCallNameExpression + +/** + * Although the scope functions are a way of making the code more concise, avoid overusing them: it can decrease + * your code readability and lead to errors. Avoid nesting scope functions and be careful when chaining them: + * it's easy to get confused about the current context object and the value of this or it. + * + * [Reference](https://kotlinlang.org/docs/scope-functions.html) + */ +class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) { + + override val issue = Issue( + javaClass.simpleName, + Severity.Maintainability, + "Over-using scope functions makes code confusing, hard to read and bug prone.", + Debt.FIVE_MINS + ) + + @Configuration("Number of nested scope functions allowed.") + private val threshold: Int by config(defaultValue = 1) + + @Configuration("Set of scope function names which add complexity.") + private val functions: Set by config(DEFAULT_FUNCTIONS) { it.toSet() } + + override fun visitNamedFunction(function: KtNamedFunction) { + function.accept(FunctionDepthVisitor()) + } + + private fun report(element: KtCallExpression, depth: Int) { + val finding = ThresholdedCodeSmell( + issue, + Entity.from(element), + Metric("SIZE", depth, threshold), + "The scope function '${element.calleeExpression?.text}' is nested too deeply ('$depth'). " + + "Complexity threshold is set to '$threshold'." + ) + report(finding) + } + + private companion object { + val DEFAULT_FUNCTIONS = listOf( + "apply", + "run", + "with", + ) + } + + private inner class FunctionDepthVisitor : DetektVisitor() { + private var depth = 0 + + override fun visitCallExpression(expression: KtCallExpression) { + fun callSuper() = super.visitCallExpression(expression) + + if (expression.isScopeFunction()) { + doWithIncrementedDepth { + reportIfOverThreshold(expression) + callSuper() + } + } else { + callSuper() + } + } + + private fun doWithIncrementedDepth(block: () -> Unit) { + depth++ + block() + depth-- + } + + private fun reportIfOverThreshold(expression: KtCallExpression) { + if (depth > threshold) { + report(expression, depth) + } + } + + private fun KtCallExpression.isScopeFunction(): Boolean = + getCallNameExpression()?.text?.let { functions.contains(it) } + ?: false + } +} diff --git a/detekt-rules-complexity/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctionsSpec.kt b/detekt-rules-complexity/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctionsSpec.kt new file mode 100644 index 000000000000..6f5d653069a8 --- /dev/null +++ b/detekt-rules-complexity/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctionsSpec.kt @@ -0,0 +1,105 @@ +package io.gitlab.arturbosch.detekt.rules.complexity + +import io.gitlab.arturbosch.detekt.api.Finding +import io.gitlab.arturbosch.detekt.test.TestConfig +import io.gitlab.arturbosch.detekt.test.assertThat +import io.gitlab.arturbosch.detekt.test.compileAndLint +import org.junit.jupiter.api.Test + +class NestedScopeFunctionsSpec { + + private var defaultConfig = TestConfig(mapOf("threshold" to 1, "functions" to listOf("one", "two"))) + private val subject = NestedScopeFunctions(defaultConfig) + + private lateinit var givenCode: String + private lateinit var actual: List + + @Test + fun `should report nesting`() { + givenCode = """ + fun f() { + 1.one { + 1.one { } + } + } + """ + whenLintRuns() + expectSourceLocation(3 to 11) + expectFunctionInMsg("one") + } + + @Test + fun `should report mixed nesting`() { + givenCode = """ + fun f() { + 1.one { + two(1) { } + } + } + """ + whenLintRuns() + expectSourceLocation(3 to 9) + expectFunctionInMsg("two") + } + + @Test + fun `should report when valid scope in between`() { + givenCode = """ + fun f() { + 1.one { + 1.valid { + two(1) { } + } + } + } + """ + whenLintRuns() + expectSourceLocation(4 to 13) + } + + @Test + fun `should not report in nested function`() { + givenCode = """ + fun f() { + 1.one { } + fun f2() { + 1.two { } + } + } + """ + whenLintRuns() + expectNoFindings() + } + + @Test + fun `should not report in neighboring scope functions`() { + givenCode = """ + fun f() { + 1.one { } + 1.one { } + two(1) {} + two(1) {} + } + """ + whenLintRuns() + expectNoFindings() + } + + private fun whenLintRuns() { + actual = subject.compileAndLint(givenCode) + } + + private fun expectSourceLocation(location: Pair) { + assertThat(actual).hasSourceLocation(location.first, location.second) + } + + private fun expectFunctionInMsg(scopeFunction: String) { + val expected = "The scope function '$scopeFunction' is nested too deeply ('2'). " + + "Complexity threshold is set to '1'." + assertThat(actual[0]).hasMessage(expected) + } + + private fun expectNoFindings() { + assertThat(actual).describedAs("findings size").isEmpty() + } +}