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.
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
NestedScopeFunctions - Add rule for nested scope functions #4788
Changes from 2 commits
f53a497
995a48d
83f2fa5
67143a1
bac48f7
d5368cb
1acbf18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
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) | ||
WildOrangutan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* <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> | ||
*/ | ||
class NestedScopeFunctions(config: Config = Config.empty) : Rule(config) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to hear what @BraisGabin and others are thinking about this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
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<String> by config(DEFAULT_FUNCTIONS) { it.toSet() } | ||
|
||
override fun visitNamedFunction(function: KtNamedFunction) { | ||
function.accept(FunctionDepthVisitor()) | ||
WildOrangutan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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("run", "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 { | ||
1.valid { | ||
with(1) { } | ||
} | ||
} | ||
} | ||
""" | ||
whenLintRuns() | ||
expectSourceLocation(4 to 13) | ||
} | ||
|
||
@Test | ||
fun `should not report in nested function`() { | ||
givenCode = """ | ||
fun f() { | ||
1.run { } | ||
fun f2() { | ||
1.with { } | ||
} | ||
} | ||
""" | ||
whenLintRuns() | ||
expectNoFindings() | ||
} | ||
|
||
@Test | ||
fun `should not report in neighboring scope functions`() { | ||
givenCode = """ | ||
fun f() { | ||
1.run { } | ||
1.run { } | ||
with(1) {} | ||
with(1) {} | ||
} | ||
""" | ||
whenLintRuns() | ||
expectNoFindings() | ||
} | ||
|
||
private fun whenLintRuns() { | ||
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() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add
also
andlet
too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not sure. I think they are less problematic, since you can rename
it
. I think they maybe better fit toNestedBlockDepth
orCognitiveComplexity
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure this is debatible. The good part is that this is configuratble so anyone could decide if they want to add or to remove some functions.
My 2 cents: If we add
let
andalso
and someone doesn't agree it's easy to remove. If we don't add them and someone want those too added is less likely that he/she add them. For that reason I'm thinking about adding them. But I'm ok about keeping them out. If someone else could share his/her opinion I think it would be great.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe smth in the middle - report nesting, when multiple
it
(or same name) are used, since that's problematic from my pov. Maybe in a next MR, so we don't drag this one too much. I can add them for time being.