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

MultilineLambdaItParameter: fix false positive for one-line statements with a lambda argument #5505

Merged
merged 2 commits into from Nov 5, 2022
Merged

MultilineLambdaItParameter: fix false positive for one-line statements with a lambda argument #5505

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4dac979
MultilineLambdaItParameter: fix false positive for one-line statement…
t-kameyama Nov 4, 2022
993cb9c
Clarify tests
t-kameyama Nov 4, 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
15 changes: 12 additions & 3 deletions ...yle/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameter.kt
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -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
Comment on lines +83 to +84
Copy link
Member

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:

Suggested change
val single = statements.singleOrNull()
if (single != null && (single.hasNoLineBreak() || single.hasNoStatements())) return
val statement = statements.singleOrNull() ?: return
if (statement.isSingleLine() || statement.hasNoSubStatements()) return

Copy link
Contributor Author

@t-kameyama t-kameyama Nov 4, 2022 •
edited

Choose a reason for hiding this comment

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

val statement = statements.singleOrNull() ?: return

We can't return here because we need to flag the case of multiple statements.

Copy link
Member

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.


val parameterNames = lambdaExpression.valueParameters.map { it.name }
// Explicit `it`
Expand Down Expand Up @@ -106,4 +110,9 @@ class MultilineLambdaItParameter(val config: Config) : Rule(config) {
}
}
}

private fun KtExpression.hasNoLineBreak() = !textContains('\n')
Copy link
Member

Choose a reason for hiding this comment

The 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 \r only.
Is it possible to get the beginning/ending line number of the PsiElement and calculate how many lines it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line separators are converted to \n.

detekt/detekt-parser/src/main/kotlin/io/github/detekt/parser/KtCompiler.kt

Line 33 in ec6cd40

StringUtilRt.convertLineSeparators(content)

Copy link
Member

@TWiStErRob TWiStErRob Nov 5, 2022 •
edited

Choose a reason for hiding this comment

The 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() }
}
31 changes: 31 additions & 0 deletions ...src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameterSpec.kt
View file
Open in desktop
Expand Up @@ -129,6 +129,24 @@ class MultilineLambdaItParameterSpec(val env: KotlinCoreEnvironment) {
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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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 it isn't the first line.

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 it represents here. Although I could think of examples where it is clear.

Copy link
Member

Choose a reason for hiding this comment

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

 val digits = 1234.let { it ->
      println(it)
      listOf(it)
 }

Let's keep the behavior to be consistent with the spec name MultilineLambdaItParameter

y = 2
)
}
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}
}

@Nested
Expand Down Expand Up @@ -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(it) }
cortinico marked this conversation as resolved.
Show resolved Hide resolved
}
}
""".trimIndent()
val findings = subject.compileAndLintWithContext(env, code)
Expand Down