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.

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

Fix false negative MultilineLambdaItParameter on complex multiline single statement #5397

Merged
merged 1 commit into from Oct 9, 2022

Conversation

t-kameyama
Copy link
Contributor

Fixes #5393

@schalkms schalkms added this to the 1.22.0 milestone Oct 9, 2022
@schalkms schalkms merged commit 965e681 into detekt:main Oct 9, 2022
21 checks passed
@t-kameyama t-kameyama deleted the issue_5393 branch October 9, 2022 22:16
@@ -74,8 +76,8 @@ class MultilineLambdaItParameter(val config: Config) : Rule(config) {

override fun visitLambdaExpression(lambdaExpression: KtLambdaExpression) {
super.visitLambdaExpression(lambdaExpression)
val size = lambdaExpression.bodyExpression?.statements?.size
if (size == null || size <= 1) return
val size = lambdaExpression.collectDescendantsOfType<KtBlockExpression>().flatMap { it.statements }.size
Copy link
Member

Choose a reason for hiding this comment

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

@t-kameyama thanks for the quick fix!

Small note: unnecessary creation of intermediate list...

val size = lambdaExpression.collectDescendantsOfType<KtBlockExpression>().sumOf { it.statements.size }

looks like we could have a UseSumInsteadOfSize similar to UseAnyOrNoneInsteadOfFind, I'll open an issue.

Copy link
Member

Choose a reason for hiding this comment

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

@eygraber
Copy link
Contributor

eygraber commented Nov 3, 2022

Would this change have caused the following to trigger MultilineLambdaItParameter:

?.map { it.seekTieFighters { ForegroundColorSpan(colorLink) } }

If yes, I'd argue that I want to keep that as it

edit

Another example:

textView.isVisible = text?.also {
    textView.text = phrase(it, false, false) { null }
} != null

@TWiStErRob
Copy link
Member

Example 1 is covered in #5504


Example 2: I agree that this is simple enough (more on this later), but I guess maybe the rule needs to be smarter, and either consider this style as a "single line", or detect that the receiver is a simple name. Because it might as well be foo().bar.baz().z { fux(it) }?.lam?.da?.also { ..., in which case naming the it is paramount. Again, a separate issue will help to discuss this. Did you open #5503 as a generalization of this example? Here's a repro:

    @Test
    fun `example2`() {
        val code = """
            interface TextView {
                var isVisible: Boolean
                var text: String
            }
            fun phrase(text: String, b1: Boolean, b2: Boolean, block: () -> String?): String = TODO()
            fun f(textView: TextView, text: String?) {
                textView.isVisible = text?.also {
                    textView.text = phrase(it, false, false) { null }
                } != null
            }
        """.trimIndent()
        val findings = subject.compileAndLintWithContext(env, code)
        assertThat(findings).isEmpty()
    }

Now, as for "simple enough". I strongly believe that when a Detekt (or any inspection / static analysis tool) flags a line of code, it is for a reason. But we should never take the suggestions literally as the rules are rules, but the solution might be something totally different. For example in your second example, it took me about a 30 seconds to decypher the data flow and nullabilities, which is unnecessarily long. The code could be way simpler:

val somekindOfPhrase = phrase(text, false, false) { null } // name based on those (false, false, null) behavior flags
textView.isVisible = somekindOfPhrase != null
textView.text = somekindOfPhrase

ok, this is not a single statement, but we don't need to take "in Kotlin, everything is an expression" literally, right? 🤓

Note that the two TextView interactions lend themselves really nicely for an extension var TextView.textOrGone: CharSequence?, which is common enough in my experience to warrant a util. At which point we're down to:

textView.textOrGone = phrase(text, false, false) { null } 

@eygraber
Copy link
Contributor

eygraber commented Nov 4, 2022

I strongly believe that when a Detekt (or any inspection / static analysis tool) flags a line of code, it is for a reason

I strongly agree! In fact, I hate this snippet of code, and we did eventually make something like textOrGone but only after noticing this pattern happening a lot.

#5503 is a generalization of this (and I like my example there better), but this one was failing due to this PR so I included it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False negative MultilineLambdaItParameter on complex multiline single statement
4 participants