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
MultilineLambdaItParameter: fix false positive for one-line statements with a lambda argument #5505
Changes from all commits
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 | ||
---|---|---|---|---|
|
@@ -12,8 +12,10 @@ import io.gitlab.arturbosch.detekt.rules.IT_LITERAL | |||
import io.gitlab.arturbosch.detekt.rules.hasImplicitParameterReference | ||||
import io.gitlab.arturbosch.detekt.rules.implicitParameter | ||||
import org.jetbrains.kotlin.psi.KtBlockExpression | ||||
import org.jetbrains.kotlin.psi.KtExpression | ||||
import org.jetbrains.kotlin.psi.KtLambdaExpression | ||||
import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType | ||||
import org.jetbrains.kotlin.psi.psiUtil.anyDescendantOfType | ||||
import org.jetbrains.kotlin.utils.ifEmpty | ||||
|
||||
/** | ||||
* Lambda expressions are very useful in a lot of cases, and they often include very small chunks of | ||||
|
@@ -76,8 +78,10 @@ class MultilineLambdaItParameter(val config: Config) : Rule(config) { | |||
|
||||
override fun visitLambdaExpression(lambdaExpression: KtLambdaExpression) { | ||||
super.visitLambdaExpression(lambdaExpression) | ||||
val size = lambdaExpression.collectDescendantsOfType<KtBlockExpression>().sumOf { it.statements.size } | ||||
if (size <= 1) return | ||||
|
||||
val statements = lambdaExpression.bodyExpression?.statements.orEmpty().ifEmpty { return } | ||||
val single = statements.singleOrNull() | ||||
if (single != null && (single.hasNoLineBreak() || single.hasNoStatements())) return | ||||
|
||||
val parameterNames = lambdaExpression.valueParameters.map { it.name } | ||||
// Explicit `it` | ||||
|
@@ -106,4 +110,9 @@ class MultilineLambdaItParameter(val config: Config) : Rule(config) { | |||
} | ||||
} | ||||
} | ||||
|
||||
private fun KtExpression.hasNoLineBreak() = !textContains('\n') | ||||
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. You're assuming file separators, which will work for most, but some might use 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. Line separators are converted to
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. Hmm, a related thought: you're assuming here that Detekt compiles the PSI, but what if it's run in a different environment? #5492 at that point all assumptions about line breaks will fail in Detekt rules. cc @3flex I think checking the line numbers to detect single line is still the most versatile solution that can be also adopted by others when they find this code and copy it. |
||||
|
||||
private fun KtExpression.hasNoStatements() = | ||||
!anyDescendantOfType<KtBlockExpression> { it.statements.isNotEmpty() } | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,14 +121,32 @@ class MultilineLambdaItParameterSpec(val env: KotlinCoreEnvironment) { | |
fun f() { | ||
val digits = 1234.let { | ||
check(it > 0) { | ||
println(it) | ||
println("error") | ||
} | ||
} | ||
} | ||
""".trimIndent() | ||
val findings = subject.compileAndLintWithContext(env, code) | ||
assertThat(findings).hasSize(1) | ||
} | ||
|
||
@Test | ||
fun `does not report when statement has no nested statements`() { | ||
cortinico marked this conversation as resolved.
Show resolved
Hide resolved
|
||
val code = """ | ||
data class Foo(val x: Int, val y: Int) | ||
|
||
fun f(i: Int?) { | ||
val foo = i?.let { | ||
Foo( | ||
x = it, | ||
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'm a little less sure about this case, since it's multiline and the Not sure if the example is too generic, but I'm trying to look at this test through the lens of a reader not familiar with the code, and it's not immediately clear to me what the 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. Agree with @eygraber . The following block is non-compliant according to the kDoc.
Let's keep the behavior to be consistent with the spec name |
||
y = 2 | ||
) | ||
} | ||
} | ||
""".trimIndent() | ||
val findings = subject.compileAndLintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
} | ||
|
||
@Nested | ||
|
@@ -171,6 +189,19 @@ class MultilineLambdaItParameterSpec(val env: KotlinCoreEnvironment) { | |
val code = """ | ||
fun f() { | ||
val digits = 1234.let { param -> listOf(param) } | ||
} | ||
""".trimIndent() | ||
val findings = subject.compileAndLintWithContext(env, code) | ||
assertThat(findings).isEmpty() | ||
} | ||
|
||
@Test | ||
fun `does not report when statement has one-line lambda argument`() { | ||
val code = """ | ||
fun f() { | ||
val digits = 1234.let { | ||
check(it > 0) { println("error") } | ||
} | ||
} | ||
""".trimIndent() | ||
val findings = subject.compileAndLintWithContext(env, code) | ||
|
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.
It feels like semantically this might be cleaner (return early + renames), but up to you:
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.
We can't return here because we need to flag the case of multiple statements.
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.
Oh, have to remember to expand the code below the changed lines. Agreed. Please consider renames anyway.