NestedScopeFunctions - Add rule for nested scope functions (#4788)

.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`.

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:

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)

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),

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)
+ *
+ * <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) }
+    }
+}

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<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()
+    }
+}