From 8e27a307ee284bdd4f190bf34108e97ad48161f1 Mon Sep 17 00:00:00 2001 From: Peter Date: Thu, 2 Jun 2022 14:18:16 +0200 Subject: [PATCH] NestedScopeFunctions - Add rule for nested scope functions (#4788) --- .github/CONTRIBUTING.md | 2 +- .../main/resources/default-detekt-config.yml | 9 ++ detekt-rules-complexity/build.gradle.kts | 1 + .../rules/complexity/ComplexityProvider.kt | 1 + .../rules/complexity/NestedScopeFunctions.kt | 135 ++++++++++++++++++ .../complexity/NestedScopeFunctionsSpec.kt | 131 +++++++++++++++++ 6 files changed, 278 insertions(+), 1 deletion(-) 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/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 05e319e085d..e9269568bbd 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -10,7 +10,7 @@ - We use [JUnit 5](https://junit.org/junit5/docs/current/user-guide/) for testing. Please use the `Spec.kt` suffix on new test classes. If your new rule requires type resolution (i.e. it utilises `BindingContext`) then annotate your test class with `@KotlinCoreEnvironmentTest` and have the test class accept `KotlinCoreEnvironment` as a parameter. - See "Testing a rule that uses type resolution" section of the [Using Type Resolution](../docs/pages/gettingstarted/type-resolution.md) + See "Testing a rule that uses type resolution" section of the [Using Type Resolution](../website/docs/gettingstarted/type-resolution.md) guide for details. - The code in `detekt-api` and any rule in `detekt-rules` must be documented. We generate documentation for our website based on these modules. - If some Kotlin code in `resources` folder (like `detekt-formatting`) shows a compilation error, right click on it and use `Mark as plain text`. diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 73c008d754f..8d2177cc0d5 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -136,6 +136,15 @@ complexity: NestedBlockDepth: active: true threshold: 4 + NestedScopeFunctions: + active: false + threshold: 1 + functions: + - 'kotlin.apply' + - 'kotlin.run' + - 'kotlin.with' + - 'kotlin.let' + - 'kotlin.also' ReplaceSafeCallChainWithRun: active: false StringLiteralDuplication: diff --git a/detekt-rules-complexity/build.gradle.kts b/detekt-rules-complexity/build.gradle.kts index 2db788f8ab8..60141b714c9 100644 --- a/detekt-rules-complexity/build.gradle.kts +++ b/detekt-rules-complexity/build.gradle.kts @@ -5,6 +5,7 @@ plugins { dependencies { compileOnly(projects.detektApi) compileOnly(projects.detektMetrics) + compileOnly(projects.detektTooling) testImplementation(projects.detektMetrics) testImplementation(projects.detektTest) testImplementation(libs.assertj) 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 745e206e0c7..ff00e730793 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 00000000000..81a0dc39d7f --- /dev/null +++ b/detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctions.kt @@ -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) + * + * + * // Try to figure out, what changed, without knowing the details + * first.apply { + * second.apply { + * b = a + * c = b + * } + * } + * + * + * + * // '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 + * + */ +@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 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 = + getResolvedCall(bindingContext)?.resultingDescriptor + ?.let { listOf(it) + it.overriddenDescriptors } + .orEmpty() + + private fun CallableDescriptor.matchesScopeFunction(): Boolean = + !functions.any { it.match(this) } + } +} 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 00000000000..df1817d52b5 --- /dev/null +++ b/detekt-rules-complexity/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctionsSpec.kt @@ -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 + + @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) { + 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() + } +}