Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NestedScopeFunctions - Add rule for nested scope functions #4788

Merged
merged 7 commits into from Jun 2, 2022
Merged

NestedScopeFunctions - Add rule for nested scope functions #4788

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f53a497
Fix CONTRIBUTING.md link
WildOrangutan Apr 27, 2022
995a48d
Add NestedScopeFunctions Rule
WildOrangutan Apr 28, 2022
83f2fa5
Fix NestedScopeFunctions CI errors
WildOrangutan Apr 30, 2022
67143a1
Change NestedScopeFunctions Rule to fully-qualified names
WildOrangutan Apr 30, 2022
bac48f7
Add NestedScopeFunctions 'let' and 'also' to default
WildOrangutan Apr 30, 2022
d5368cb
Change NestedScopeFunctions to early return on empty context
WildOrangutan May 1, 2022
1acbf18
Fix NestedScopeFunctions detekt errors
WildOrangutan May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions detekt-core/src/main/resources/default-detekt-config.yml
View file
Open in desktop
Expand Up @@ -137,9 +137,11 @@ complexity:
active: false
threshold: 1
functions:
- 'apply'
- 'run'
- 'with'
- 'kotlin.apply'
- 'kotlin.run'
- 'kotlin.with'
- 'kotlin.let'
- 'kotlin.also'
ReplaceSafeCallChainWithRun:
active: false
StringLiteralDuplication:
Expand Down
1 change: 1 addition & 0 deletions detekt-rules-complexity/build.gradle.kts
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ plugins {
dependencies {
compileOnly(projects.detektApi)
compileOnly(projects.detektMetrics)
compileOnly(projects.detektTooling)
testImplementation(projects.detektMetrics)
testImplementation(projects.detektTest)
testImplementation(libs.assertj)
Expand Down
42 changes: 32 additions & 10 deletions ...xity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctions.kt
View file
Open in desktop
@@ -1,5 +1,6 @@
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
Expand All @@ -11,9 +12,11 @@ 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.psi.psiUtil.getCallNameExpression
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
Expand All @@ -40,6 +43,7 @@ import org.jetbrains.kotlin.psi.psiUtil.getCallNameExpression
* second.c = first.b
* </compliant>
*/
@RequiresTypeResolution
class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not a super big fan of this rule explicitly mentioning ScopeFunctions while in reality you can customize the functions. So theoretically you could use this rule to check the nesting of your own non-scope functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, dunno. Judging by this, scope is general term and not explicitly tied to "scope functions" like apply.

I'm not a Kotlin expert tough, so let me know, if you have better naming in mind. Not sure how else to name it tough. One alternative I see is NestedFunctions, but that can be confusing (function definition in a function).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to hear what @BraisGabin and others are thinking about this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a better name in mind right now. I'm fine with the current one.


override val issue = Issue(
Expand All @@ -52,8 +56,13 @@ class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) {
@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<String> by config(DEFAULT_FUNCTIONS) { it.toSet() }
@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) {
function.accept(FunctionDepthVisitor())
WildOrangutan marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -72,17 +81,19 @@ class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) {

private companion object {
val DEFAULT_FUNCTIONS = listOf(
"apply",
"run",
"with",
"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() = super.visitCallExpression(expression)
fun callSuper(): Unit = super.visitCallExpression(expression)

if (expression.isScopeFunction()) {
doWithIncrementedDepth {
Expand All @@ -106,8 +117,19 @@ class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) {
}
}

private fun KtCallExpression.isScopeFunction(): Boolean =
getCallNameExpression()?.text?.let { functions.contains(it) }
?: false
private fun KtCallExpression.isScopeFunction(): Boolean {
val descriptors = resolveDescriptors()
val descriptor = descriptors.find { it.matchesScopeFunction() }
return descriptor != null
}

private fun KtCallExpression.resolveDescriptors(): List<CallableDescriptor> =
getResolvedCall(bindingContext)?.resultingDescriptor
?.let { listOf(it) + it.overriddenDescriptors }
?: emptyList()

private fun CallableDescriptor.matchesScopeFunction(): Boolean {
return functions.find { it.match(this) } != null
}
}
}
20 changes: 14 additions & 6 deletions .../src/test/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/NestedScopeFunctionsSpec.kt
View file
Open in desktop
@@ -1,14 +1,22 @@
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

class NestedScopeFunctionsSpec {
@KotlinCoreEnvironmentTest
class NestedScopeFunctionsSpec(private val env: KotlinCoreEnvironment) {

private var defaultConfig = TestConfig(mapOf("threshold" to 1, "functions" to listOf("run", "with")))
private var defaultConfig = TestConfig(
mapOf(
"threshold" to 1,
"functions" to listOf("kotlin.run", "kotlin.with")
)
)
private val subject = NestedScopeFunctions(defaultConfig)

private lateinit var givenCode: String
Expand Down Expand Up @@ -47,7 +55,7 @@ class NestedScopeFunctionsSpec {
givenCode = """
fun f() {
1.run {
1.valid {
"valid".apply {
with(1) { }
}
}
Expand All @@ -63,7 +71,7 @@ class NestedScopeFunctionsSpec {
fun f() {
1.run { }
fun f2() {
1.with { }
with(1) { }
}
}
"""
Expand All @@ -86,7 +94,7 @@ class NestedScopeFunctionsSpec {
}

private fun whenLintRuns() {
actual = subject.compileAndLint(givenCode)
actual = subject.compileAndLintWithContext(env, givenCode)
}

private fun expectSourceLocation(location: Pair<Int, Int>) {
Expand Down