Fix UnnecessaryLet false negatives

[BraisGabin]

Closes #2826

There were some false negatives in this rule:

• a.let { 1.plus(1) } // 1.plus(1)
• a?.let { 1.plus(1) } // if (a != null) 1.plus(1) This one is to avoid overuse of let when you get nothing from it.
• a.let { println(it) } // println(it)
• a.let { println(it); println("hola") }
• etc.

I know, the tests are a real mess. I'll try to refactor them.

I know that we have a script/project to run detekt over different projects to ensure that the new rule works... Where is it? I'll like to prove this on real projects.

[codecov]

Codecov Report

Merging #2828 into master will increase coverage by 0.02%.
The diff coverage is 76.31%.

@@             Coverage Diff              @@
##             master    #2828      +/-   ##
============================================
+ Coverage     80.62%   80.65%   +0.02%     
+ Complexity     2350     2346       -4     
============================================
  Files           388      388              
  Lines          7025     7039      +14     
  Branches       1282     1288       +6     
============================================
+ Hits           5664     5677      +13     
  Misses          717      717              
- Partials        644      645       +1     

Impacted Files	Coverage Δ	Complexity Δ
...n/kotlin/io/gitlab/arturbosch/detekt/rules/Junk.kt	70.83% <60.00%> (-2.86%)	0.00 <0.00> (ø)
...ab/arturbosch/detekt/rules/style/UnnecessaryLet.kt	79.48% <78.12%> (+8.65%)	13.00 <1.00> (-4.00)	⬆️
.../arturbosch/detekt/rules/style/UnnecessaryApply.kt	88.00% <100.00%> (+4.66%)	7.00 <0.00> (ø)
...ules/style/optional/MandatoryBracesIfStatements.kt	61.53% <0.00%> (-2.75%)	14.00% <0.00%> (ø%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e3d83a...a3d55a7. Read the comment docs.

[BraisGabin]

😭 I never understand codecov. I did improved the test coverage here. Is it complainly for moving a function from one file to another?

[schalkms]

👏 good job

[arturbosch]

Great job!

I know that we have a script/project to run detekt over different projects to ensure that the new rule works... Where is it? I'll like to prove this on real projects.

The script is located at /scripts/get_analysis_projects.groovy.

In case you are not familiar with Groovy:
get it via sdkman, run sdk i groovy and then groovy scripts/get_analysis_projects.groovy ~/git/<test_repos>

[BraisGabin]

All this examples are flagged by the new rule. I need your opinion if they are valid, false positives or they are edge cases where a @Suppress will have more sense:

1. ktlint/ktlint-test/src/main/kotlin/com/pinterest/ktlint/test/DumpAST.kt:40:22

lineNumberColumnLength =
    (node.lastChildLeafOrSelf().lineNumber() ?: 1)
        .let { var v = it; var c = 0; while (v > 0) { c++; v /= 10 }; c }

2. ktlint/ktlint-ruleset-experimental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/MultiLineIfElseRule.kt:46:14

val rightBraceIndent = node.treeParent
    .prevLeaf { it is PsiWhiteSpace && it.textContains('\n') }?.text.orEmpty()
    .let { "\n${it.substringAfterLast("\n")}" }

3. kotlinx/ui/kotlinx-coroutines-android/animation-app/app/src/main/java/org/jetbrains/kotlinx/animation/Animation.kt:109:23

while (true) {
    val dt = time.let { old -> awaitFrame().also { time = it } - old }
    if (dt > 0.5e9) continue // don't animate through over a half second lapses

4. javalin/javalin/src/test/java/io/javalin/TestRequest.kt:257:84

Unirest.get("${http.origin}/html.html").basicAuth("u", "p").asString().let { assertThat(it.body).contains("HTML works") }

5. detekt/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/internal/Suppressions.kt:46:16

ruleSetId?.let { acceptedSuppressionIds.addAll(listOf(ruleSetId, "$ruleSetId.$id", "$ruleSetId:$id")) }

6. kotlintest/kotest-assertions/kotest-assertions-core/src/jvmMain/kotlin/io/kotest/matchers/reflection/classMatchers.kt:52:23

fun KClass<*>.shouldHaveFunction(name: String, block: (KFunction<*>) -> Unit) {
  this should haveFunction(name)
  findFunction(name)?.let(block)
}

I think that all of them can be ommited and will improve the code. Some of them just removing the let and others moving code to a function. But in any case I think that let is a good fit there.

The only case where I'm not that sure is the 6th. Should let be used in this way? Should a scope function used like this? I think that you should use it with a "real lambda" and no with a "parameter".

[schalkms]

Thanks for testing this new rule thoroughly.
I agree with 1-5. What should be the preferred way of coding for 6?

fun KClass<*>.shouldHaveFunction(name: String, block: (KFunction<*>) -> Unit) {
  this should haveFunction(name)
  findFunction(name)?.let(block)
}

[BraisGabin]

What should be the preferred way of coding for 6?

fun KClass<*>.shouldHaveFunction(name: String, block: (KFunction<*>) -> Unit) {
  this should haveFunction(name)
  findFunction(name)?.let(block)
}

That code is the same as this one:

fun KClass<*>.shouldHaveFunction(name: String, block: (KFunction<*>) -> Unit) {
  this should haveFunction(name)
  findFunction(name)?.let { block(it) }
}

And this is legit. So I imagine that we should allow every use of let using the operator ?. if it provides the lambda as parameter. But flag the ones that call let just with ..

I'll add test for this two cases and fix them 👍.

[BraisGabin]

Ok, false positive fixed. Let me know what do you think now :). It's ready to merge for me.

[schalkms]

Can we rename this to lambdaReferenceCount to make it more readable?

[BraisGabin]

done