Repository: detekt/detekt (kotlin)

Issue:
False negative on `UnnecessaryLet` in nested nullable property
## Expected Behavior
Following TC should pass

```kotlin
@Test
fun `reports nested nullable property`() {
    val findings = subject.compileAndLintWithContext(
        env,
        """
            class Dialog(val window: Int?)
            val dialog: Dialog? = null
            fun test() {
                dialog?.let {
                    it.window?.let {
                        it++
                        println(it)
                    }
                }
            }
    """.trimIndent()
    )
    assertThat(findings).hasSize(1)
}
```

Compliant code should be as below

```kotlin
class Dialog(val window: Int?)
val dialog: Dialog? = null
fun test() {
    dialog?.window?.let {
        it++
        println(it)
    }
}
```

## Observed Behavior
Above TC is failing

## Context
There is existing [issue](https://github.com/detekt/detekt/issues/6222) to create a new rule to detect this but I think this should be handled by the `UnnecessaryLet`

Pull request:
Fixes https://github.com/detekt/detekt/issues/6373

Patch:
diff --git a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt
index 19cc5f438d3e..302febd20c75 100644
--- a/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt
+++ b/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/NullCheckOnMutableProperty.kt
@@ -89,17 +89,16 @@ class NullCheckOnMutableProperty(config: Config) : Rule(
                     nonNullCondition.right as? KtNameReferenceExpression
                 } else {
                     nonNullCondition.left as? KtNameReferenceExpression
-                }?.let { referenceExpression ->
-                    referenceExpression.getResolvedCall(bindingContext)
-                        ?.resultingDescriptor
-                        ?.let {
-                            it.fqNameOrNull()?.takeIf(mutableProperties::contains)
-                        }
-                }?.let { candidateFqName ->
-                    // A candidate mutable property is present, so attach the current
-                    // if-expression to it in the property candidates map.
-                    candidateProperties.getOrPut(candidateFqName) { ArrayDeque() }.apply { add(expression) }
-                }
+                }?.getResolvedCall(bindingContext)
+                    ?.resultingDescriptor
+                    ?.fqNameOrNull()
+                    ?.takeIf(mutableProperties::contains)
+                    ?.let { candidateFqName ->
+                        // A candidate mutable property is present, so attach the current
+                        // if-expression to it in the property candidates map.
+                        candidateProperties.getOrPut(candidateFqName) { ArrayDeque() }
+                            .apply { add(expression) }
+                    }
             }
             // Visit descendant expressions to see whether candidate properties
             // identified in this if-expression are being referenced.
diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt
index 4deb5f850174..6d14998beb82 100644
--- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt
+++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLet.kt
@@ -12,6 +12,7 @@ import io.gitlab.arturbosch.detekt.rules.receiverIsUsed
 import org.jetbrains.kotlin.descriptors.impl.ValueParameterDescriptorImpl.WithDestructuringDeclaration
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.psi.KtCallExpression
+import org.jetbrains.kotlin.psi.KtExpression
 import org.jetbrains.kotlin.psi.KtLambdaExpression
 import org.jetbrains.kotlin.psi.KtNamedDeclaration
 import org.jetbrains.kotlin.psi.KtQualifiedExpression
@@ -47,13 +48,12 @@ class UnnecessaryLet(config: Config) : Rule(
     config,
     "The `let` usage is unnecessary."
 ) {
-
     override fun visitCallExpression(expression: KtCallExpression) {
         super.visitCallExpression(expression)
 
         if (!expression.isCalling(letFqName, bindingContext)) return
-
         val lambdaExpr = expression.lambdaArguments.firstOrNull()?.getLambdaExpression()
+
         val referenceCount = lambdaExpr?.countLambdaParameterReference(bindingContext) ?: 0
         if (referenceCount > 1) return
 
@@ -92,7 +92,7 @@ private fun canBeReplacedWithCall(lambdaExpr: KtLambdaExpression?): Boolean {
     if (lambdaExpr == null) return false
 
     val receiver = when (val statement = lambdaExpr.bodyExpression?.statements?.singleOrNull()) {
-        is KtQualifiedExpression -> statement.receiverExpression
+        is KtQualifiedExpression -> statement.getRootExpression()
         else -> null
     } ?: return false
 
@@ -108,6 +108,15 @@ private fun canBeReplacedWithCall(lambdaExpr: KtLambdaExpression?): Boolean {
     return lambdaParameterNames.any { receiver.textMatches(it) }
 }
 
+private fun KtExpression?.getRootExpression(): KtExpression? {
+    // Look for the expression that was the root of a potential call chain.
+    var receiverExpression = this
+    while (receiverExpression is KtQualifiedExpression) {
+        receiverExpression = receiverExpression.receiverExpression
+    }
+    return receiverExpression
+}
+
 private fun KtLambdaExpression.countLambdaParameterReference(context: BindingContext): Int {
     val bodyExpression = bodyExpression ?: return 0
     val firstParameter = firstParameter(context) ?: return 0
diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt
index 2fcd4e8c5c5e..ee4c85186f79 100644
--- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt
+++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/VarCouldBeVal.kt
@@ -73,7 +73,12 @@ class VarCouldBeVal(config: Config) : Rule(
         file.accept(assignmentVisitor)
 
         assignmentVisitor.getNonReAssignedDeclarations().forEach {
-            report(CodeSmell(Entity.from(it), "Variable '${it.nameAsSafeName.identifier}' could be val."))
+            report(
+                CodeSmell(
+                    Entity.from(it),
+                    "Variable '${it.nameAsSafeName.identifier}' could be val."
+                )
+            )
         }
     }
 
@@ -95,7 +100,8 @@ class VarCouldBeVal(config: Config) : Rule(
             val declarationName = nameAsSafeName.toString()
             val assignments = assignments[declarationName]
             if (assignments.isNullOrEmpty()) return false
-            val declarationDescriptor = bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, this]
+            val declarationDescriptor =
+                bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, this]
             return assignments.any {
                 it.getResolvedCall(bindingContext)?.resultingDescriptor?.original == declarationDescriptor ||
                     // inside an unknown types context? (example: with-statement with unknown type)
@@ -138,9 +144,9 @@ class VarCouldBeVal(config: Config) : Rule(
                 expression.left?.let(::visitAssignment)
 
                 // Check for whether the assignment contains an object literal.
-                val descriptor = (expression.left as? KtNameReferenceExpression)?.let {
-                    it.getResolvedCall(bindingContext)?.resultingDescriptor
-                }
+                val descriptor = (expression.left as? KtNameReferenceExpression)
+                    ?.getResolvedCall(bindingContext)
+                    ?.resultingDescriptor
                 val expressionRight = expression.right
                 if (descriptor != null && expressionRight != null) {
                     evaluateAssignmentExpression(descriptor, expressionRight)


Test patch:
diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt
index ee5c76a2431d..1b4229eec8a6 100644
--- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt
+++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryLetSpec.kt
@@ -244,6 +244,10 @@ class UnnecessaryLetSpec(val env: KotlinCoreEnvironment) {
                         it.plus(1)
                         it.plus(2)
                     }
+                    a?.let {
+                        it.plus(1)
+                        print(2)
+                    }
                     b.let { that ->
                         that.plus(1)
                         that.plus(2)
@@ -265,6 +269,23 @@ class UnnecessaryLetSpec(val env: KotlinCoreEnvironment) {
         assertThat(findings).isEmpty()
     }
 
+    @Test
+    fun `does not report lets with lambda body containing more than one statement with one ref count`() {
+        val findings = subject.compileAndLintWithContext(
+            env,
+            """
+                fun f() {
+                    val a: Int? = if (System.currentTimeMillis() > 0) 1 else null
+                    a?.let {
+                        it.plus(1)
+                        print(2)
+                    }
+                }
+            """.trimIndent()
+        )
+        assertThat(findings).isEmpty()
+    }
+
     @Test
     fun `does not report lets where it is used multiple times`() {
         val findings = subject.compileAndLintWithContext(
@@ -469,6 +490,128 @@ class UnnecessaryLetSpec(val env: KotlinCoreEnvironment) {
         )
         assertThat(findings).hasSize(1)
     }
+
+    @Nested
+    inner class `nested lets` {
+        @Test
+        fun `does not report nested nullable properties used with safe operator - #6373`() {
+            val findings = subject.compileAndLintWithContext(
+                env,
+                """
+                    class Dialog(val window: Int?)
+                    val dialog: Dialog? = null
+                    fun test() {
+                        dialog?.window?.let {
+                            println(it)
+                        }
+                    }
+                """.trimIndent()
+            )
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun `reports nested nullable properties - #6373`() {
+            val findings = subject.compileAndLintWithContext(
+                env,
+                """
+                    class Dialog(val window: Int?)
+                    val dialog: Dialog? = null
+                    fun test() {
+                        dialog?.let {
+                            it.window?.let {
+                                println(it)
+                            }
+                        }
+                    }
+                """.trimIndent()
+            )
+            assertThat(findings).hasSize(1)
+        }
+
+        @Test
+        fun `does not report nested nullable properties when multiple expression are present`() {
+            val findings = subject.compileAndLintWithContext(
+                env,
+                """
+                    class Dialog(val window: Int?)
+                    val dialog: Dialog? = null
+                    fun test() {
+                        dialog?.let {
+                            it.window?.let {
+                                val a = it + 1
+                                println(a)
+                            }
+                            println(it.window)
+                        }
+                    }
+                """.trimIndent()
+            )
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun `reports nested nullable properties when first let add more chain`() {
+            val findings = subject.compileAndLintWithContext(
+                env,
+                """
+                    class Dialog(val window: Int?)
+                    val dialog: Dialog? = null
+                    fun test() {
+                        dialog?.let {
+                            it.window?.inc()?.let {
+                                println(it)
+                            }
+                        }
+                    }
+                """.trimIndent()
+            )
+            assertThat(findings).hasSize(1)
+        }
+
+        @Test
+        fun `does not reports nested nullable properties when first let calls a fun`() {
+            val findings = subject.compileAndLintWithContext(
+                env,
+                """
+                    class Dialog(val window: Int?)
+                    val dialog: Dialog? = null
+                    fun inc(value: Int?) = value?.let { it + 1 }
+                    fun test() {
+                        dialog?.let {
+                            inc(it.window)?.let { window ->
+                                println(window)
+                            }
+                        }
+                        dialog?.window?.inc()?.let { println(it) }
+                    }
+                """.trimIndent()
+            )
+            assertThat(findings).isEmpty()
+        }
+
+        @Test
+        fun `reports double nested nullable properties`() {
+            val findings = subject.compileAndLintWithContext(
+                env,
+                """
+                    class Dialog(val window: Int?)
+                    class ParentDialog(val dialog: Dialog?)
+                    val parentDialog: ParentDialog? = null
+                    fun test() {
+                        parentDialog?.let {
+                            it.dialog?.let { dialog ->
+                                dialog.window?.let {
+                                    println(it)
+                                }
+                            }
+                        }
+                    }
+                """.trimIndent()
+            )
+            assertThat(findings).hasSize(2)
+        }
+    }
 }
 
 private const val MESSAGE_OMIT_LET = "let expression can be omitted"