UnnecessaryLet: fix false positive when let is used for destructuring (…

…#2968)

* UnnecessaryLet: fix false positive when let is used for destructuring

* Remove redundant assingemnt

* Simplify code

* Fix test names

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt

@@ -60,7 +60,7 @@ class UnnecessaryLet(config: Config) : Rule(config) {
                 report(CodeSmell(issue, Entity.from(expression), "let expression can be omitted"))
             }
         } else {
-            val lambdaReferenceCount = lambdaExpr.countReferences() ?: 0
+            val lambdaReferenceCount = lambdaExpr.countReferences()
             if (lambdaReferenceCount == 0 && !expression.receiverIsUsed(bindingContext) && isNullSafeOperator) {
                 report(
                     CodeSmell(
@@ -89,13 +89,14 @@ private fun canBeReplacedWithCall(lambdaExpr: KtLambdaExpression?): Boolean {
         else -> null
     }
 
-    return if (exprReceiver != null) {
-        val isLetWithImplicitParam = lambdaParameter == null && exprReceiver.textMatches(IT_LITERAL)
-        val isLetWithExplicitParam = lambdaParameter != null && exprReceiver.textMatches(lambdaParameter)
-
-        isLetWithExplicitParam || isLetWithImplicitParam
-    } else {
-        false
+    return when {
+        exprReceiver == null -> false
+        lambdaParameter == null -> exprReceiver.textMatches(IT_LITERAL)
+        else -> {
+            val destructuringDeclaration = lambdaParameter.destructuringDeclaration
+            destructuringDeclaration?.entries?.any { exprReceiver.textMatches(it.nameAsSafeName.asString()) }
+                ?: exprReceiver.textMatches(lambdaParameter.nameAsSafeName.asString())
+        }
     }
 }
 
@@ -110,6 +111,13 @@ private fun KtBlockExpression.hasOnlyOneStatement() = this.children.size == 1
 private fun PsiElement.countVarRefs(varName: String): Int =
     children.sumBy { it.countVarRefs(varName) + if (it.textMatches(varName) && it !is ValueArgument) 1 else 0 }
 
-private fun KtLambdaExpression.countReferences(): Int? {
-    return bodyExpression?.countVarRefs(firstParameter?.text ?: IT_LITERAL)
+private fun KtLambdaExpression.countReferences(): Int {
+    val bodyExpression = bodyExpression ?: return 0
+    val destructuringDeclaration = firstParameter?.destructuringDeclaration
+    return if (destructuringDeclaration != null) {
+        destructuringDeclaration.entries.sumBy { bodyExpression.countVarRefs(it.nameAsSafeName.asString()) }
+    } else {
+        val parameterName = firstParameter?.nameAsSafeName?.asString() ?: IT_LITERAL
+        bodyExpression.countVarRefs(parameterName)
+    }
 }

detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt

@@ -191,6 +191,96 @@ class UnnecessaryLetSpec : Spek({
                 }""")
             assertThat(findings).isEmpty()
         }
+
+        context("destructuring declarations") {
+            it("does not report `let` when parameters are used more than once") {
+                val content = """
+                data class Foo(val a: Int, val b: Int)
+                
+                fun test(foo: Foo) {
+                    foo.let { (a, b) -> a + b }
+                }
+            """
+                val findings = subject.compileAndLint(content)
+                assertThat(findings).isEmpty()
+            }
+
+            it("does not report `let` with a safe call when a parameter is used more than once") {
+                val content = """
+                data class Foo(val a: Int, val b: Int)
+                
+                fun test(foo: Foo) {
+                    foo.let { (a, _) -> a + a }
+                }
+            """
+                val findings = subject.compileAndLint(content)
+                assertThat(findings).isEmpty()
+            }
+
+            it("does not report `let` when parameters with types are used more than once") {
+                val content = """
+                data class Foo(val a: Int, val b: Int)
+                
+                fun test(foo: Foo) {
+                    foo.let { (a: Int, b: Int) -> a + b }
+                }
+            """
+                val findings = subject.compileAndLint(content)
+                assertThat(findings).isEmpty()
+            }
+
+            it("reports `let` when parameters are used only once") {
+                val content = """
+                data class Foo(val a: Int, val b: Int)
+                
+                fun test(foo: Foo) {
+                    foo.let { (a, _) -> a }
+                }
+            """
+                val findings = subject.compileAndLint(content)
+                assertThat(findings).hasSize(1)
+                assertThat(findings).allMatch { it.message == MESSAGE_OMIT_LET }
+            }
+
+            it("reports `let` with a safe call when parameters are used only once") {
+                val content = """
+                data class Foo(val a: Int, val b: Int)
+                
+                fun test(foo: Foo?) {
+                    foo?.let { (_, b) -> b.plus(1) }
+                }
+            """
+                val findings = subject.compileAndLint(content)
+                assertThat(findings).hasSize(1)
+                assertThat(findings).allMatch { it.message == MESSAGE_OMIT_LET }
+            }
+
+            it("reports `let` when parameters are not used") {
+                val content = """
+                data class Foo(val a: Int, val b: Int)
+                
+                fun test(foo: Foo) {
+                    foo.let { (_, _) -> 0 }
+                }
+            """
+                val findings = subject.compileAndLint(content)
+                assertThat(findings).hasSize(1)
+                assertThat(findings).allMatch { it.message == MESSAGE_OMIT_LET }
+            }
+
+            it("reports `let` with a safe call when parameters are not used") {
+                val content = """
+                data class Foo(val a: Int, val b: Int)
+                
+                fun test(foo: Foo?) {
+                    foo?.let { (_, _) -> 0 }
+                }
+            """
+                val findings = subject.compileAndLint(content)
+                assertThat(findings).hasSize(1)
+                assertThat(findings).allMatch { it.message == MESSAGE_USE_IF }
+            }
+        }
     }
 })