Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
NestedScopeFunctions - Add rule for nested scope functions (#4788)
- Loading branch information
1 parent
23a03e1
commit 8e27a30
Showing
6 changed files
with
278 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
135 changes: 135 additions & 0 deletions
135
...xity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctions.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| package io.gitlab.arturbosch.detekt.rules.complexity | ||
|
|
||
| import io.github.detekt.tooling.api.FunctionMatcher | ||
| 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 io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution | ||
| import org.jetbrains.kotlin.descriptors.CallableDescriptor | ||
| import org.jetbrains.kotlin.psi.KtCallExpression | ||
| import org.jetbrains.kotlin.psi.KtNamedFunction | ||
| import org.jetbrains.kotlin.resolve.BindingContext | ||
| import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall | ||
|
|
||
| /** | ||
| * 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) | ||
| * | ||
| * <noncompliant> | ||
| * // Try to figure out, what changed, without knowing the details | ||
| * first.apply { | ||
| * second.apply { | ||
| * b = a | ||
| * c = b | ||
| * } | ||
| * } | ||
| * </noncompliant> | ||
| * | ||
| * <compliant> | ||
| * // 'a' is a property of current class | ||
| * // 'b' is a property of class 'first' | ||
| * // 'c' is a property of class 'second' | ||
| * first.b = this.a | ||
| * second.c = first.b | ||
| * </compliant> | ||
| */ | ||
| @RequiresTypeResolution | ||
| 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. " + | ||
| "Function names have to be fully qualified. For example 'kotlin.apply'." | ||
| ) | ||
| private val functions: List<FunctionMatcher> by config(DEFAULT_FUNCTIONS) { | ||
| it.toSet().map(FunctionMatcher::fromFunctionSignature) | ||
| } | ||
|
|
||
| override fun visitNamedFunction(function: KtNamedFunction) { | ||
| if (bindingContext == BindingContext.EMPTY) return | ||
| 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( | ||
| "kotlin.apply", | ||
| "kotlin.run", | ||
| "kotlin.with", | ||
| "kotlin.let", | ||
| "kotlin.also", | ||
| ) | ||
| } | ||
|
|
||
| private inner class FunctionDepthVisitor : DetektVisitor() { | ||
| private var depth = 0 | ||
|
|
||
| override fun visitCallExpression(expression: KtCallExpression) { | ||
| fun callSuper(): Unit = 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 { | ||
| val descriptors = resolveDescriptors() | ||
| return !descriptors.any { it.matchesScopeFunction() } | ||
| } | ||
|
|
||
| private fun KtCallExpression.resolveDescriptors(): List<CallableDescriptor> = | ||
| getResolvedCall(bindingContext)?.resultingDescriptor | ||
| ?.let { listOf(it) + it.overriddenDescriptors } | ||
| .orEmpty() | ||
|
|
||
| private fun CallableDescriptor.matchesScopeFunction(): Boolean = | ||
| !functions.any { it.match(this) } | ||
| } | ||
| } |
131 changes: 131 additions & 0 deletions
131
.../src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctionsSpec.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| package io.gitlab.arturbosch.detekt.rules.complexity | ||
|
|
||
| import io.gitlab.arturbosch.detekt.api.Finding | ||
| import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest | ||
| import io.gitlab.arturbosch.detekt.test.TestConfig | ||
| import io.gitlab.arturbosch.detekt.test.assertThat | ||
| import io.gitlab.arturbosch.detekt.test.compileAndLint | ||
| import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext | ||
| import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment | ||
| import org.junit.jupiter.api.Test | ||
|
|
||
| @KotlinCoreEnvironmentTest | ||
| class NestedScopeFunctionsSpec(private val env: KotlinCoreEnvironment) { | ||
|
|
||
| private val defaultConfig = TestConfig( | ||
| mapOf( | ||
| "threshold" to 1, | ||
| "functions" to listOf("kotlin.run", "kotlin.with") | ||
| ) | ||
| ) | ||
| private val subject = NestedScopeFunctions(defaultConfig) | ||
|
|
||
| private lateinit var givenCode: String | ||
| private lateinit var actual: List<Finding> | ||
|
|
||
| @Test | ||
| fun `should report nesting`() { | ||
| givenCode = """ | ||
| fun f() { | ||
| 1.run { | ||
| 1.run { } | ||
| } | ||
| } | ||
| """ | ||
| whenLintRuns() | ||
| expectSourceLocation(3 to 11) | ||
| expectFunctionInMsg("run") | ||
| } | ||
|
|
||
| @Test | ||
| fun `should report mixed nesting`() { | ||
| givenCode = """ | ||
| fun f() { | ||
| 1.run { | ||
| with(1) { } | ||
| } | ||
| } | ||
| """ | ||
| whenLintRuns() | ||
| expectSourceLocation(3 to 9) | ||
| expectFunctionInMsg("with") | ||
| } | ||
|
|
||
| @Test | ||
| fun `should report when valid scope in between`() { | ||
| givenCode = """ | ||
| fun f() { | ||
| 1.run { | ||
| "valid".apply { | ||
| with(1) { } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
| whenLintRuns() | ||
| expectSourceLocation(4 to 13) | ||
| } | ||
|
|
||
| @Test | ||
| fun `should not report in nested function`() { | ||
| givenCode = """ | ||
| fun f() { | ||
| 1.run { } | ||
| fun f2() { | ||
| with(1) { } | ||
| } | ||
| } | ||
| """ | ||
| whenLintRuns() | ||
| expectNoFindings() | ||
| } | ||
|
|
||
| @Test | ||
| fun `should not report in neighboring scope functions`() { | ||
| givenCode = """ | ||
| fun f() { | ||
| 1.run { } | ||
| 1.run { } | ||
| with(1) {} | ||
| with(1) {} | ||
| } | ||
| """ | ||
| whenLintRuns() | ||
| expectNoFindings() | ||
| } | ||
|
|
||
| @Test | ||
| fun `should not report when binding context is empty`() { | ||
| givenCode = """ | ||
| fun f() { | ||
| 1.run { | ||
| 1.run { } | ||
| } | ||
| } | ||
| """ | ||
| whenLintRunsWithoutContext() | ||
| expectNoFindings() | ||
| } | ||
|
|
||
| private fun whenLintRuns() { | ||
| actual = subject.compileAndLintWithContext(env, givenCode) | ||
| } | ||
|
|
||
| private fun whenLintRunsWithoutContext() { | ||
| actual = subject.compileAndLint(givenCode) | ||
| } | ||
|
|
||
| private fun expectSourceLocation(location: Pair<Int, Int>) { | ||
| 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() | ||
| } | ||
| } |