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

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

Merged
merged 2 commits into from Nov 5, 2022

Conversation

t-kameyama
Copy link
Contributor

Fixes #5504

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

StringUtilRt.convertLineSeparators(content)

Copy link
Member

@TWiStErRob TWiStErRob Nov 5, 2022

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.

Comment on lines +83 to +84
val single = statements.singleOrNull()
if (single != null && (single.hasNoLineBreak() || single.hasNoStatements())) return
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

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.

@TWiStErRob
Copy link
Member

Amazingly fast response @t-kameyama 👏🏻!

@TWiStErRob
Copy link
Member

cc @eygraber

Copy link
Contributor

@eygraber eygraber left a comment

Choose a reason for hiding this comment

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

Thanks for the quick response!

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

@chao2zhang chao2zhang added this to the 1.22.0 milestone Nov 5, 2022
@cortinico cortinico merged commit 9306a27 into detekt:main Nov 5, 2022
@t-kameyama t-kameyama deleted the issue_5504 branch November 5, 2022 21:25
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.

MultilineLambdaItParameter: false positive for one-line statements with a lambda argument
6 participants