Repository: detekt/detekt (kotlin)

Issue:
False positive on `UnnecessaryInnerClass` calls to `this`
## Expected Behavior
If an `inner class` has a plan reference to `this`, `UnnecessaryInnerClass` should still raise an issue.

## Observed Behavior
`UnnecessaryInnerClass` does not raise an issue, even though the class could be made non-`inner`.

## Steps to Reproduce
```
@Test
fun `reports even with calls to this`() {
    val code = """
        class A {
            inner class B {
                fun foo() {
                    this
                }
            }
        }
    """.trimIndent()

    assertThat(subject.lintWithContext(env, code)).hasSize(1)
}
```

## Context
I came across this issue while working on labeled expressions in #4865 but was unable to find a resolution. It seems that `visitReferenceExpression()` will be invoked for any plain call to `this` and resolve it in the example above as class `A` when it should be `B`. Even though it's a reference to `this`, the expression is not a `KtThisExpression` but a plain `KtNameReferenceExpression` with a referenced name of `this`; aside from checking against that referenced name I couldn't find a way to distinguish it from other expressions.

I don't have a particular use case, but it does seem to interfere with labeled expressions in some cases, e.g.
```
@Test
fun `reports irrelevant labeled expressions`() {
    val code = """
        class A {
            inner class B {
                fun inner() {
                    return Unit.apply { this@B }
                }
            }
        }
    """.trimIndent()

    assertThat(subject.lintWithContext(env, code)).hasSize(1)
}
```

this test will fail, not because of labeled expression as per #4865 but because of the use of `this@B`, even though that doesn't require `B` to be `inner`.

## Your Environment
* current main branch (`fcd9c48159685fc049da6e6d2b3a77db3aea00bd`)

Pull request:
Fixes #4866

Patch:
diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClass.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClass.kt
index b8a13aadb9ab..3b4ea4c6f0a3 100644
--- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClass.kt
+++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClass.kt
@@ -13,11 +13,12 @@ import org.jetbrains.kotlin.backend.common.pop
 import org.jetbrains.kotlin.descriptors.ClassifierDescriptor
 import org.jetbrains.kotlin.name.ClassId
 import org.jetbrains.kotlin.psi.KtClass
-import org.jetbrains.kotlin.psi.KtExpressionWithLabel
 import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.kotlin.psi.KtReferenceExpression
+import org.jetbrains.kotlin.psi.KtThisExpression
 import org.jetbrains.kotlin.psi.psiUtil.containingClass
 import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
 import org.jetbrains.kotlin.resolve.descriptorUtil.classId
 import org.jetbrains.kotlin.utils.addToStdlib.safeAs
 
@@ -83,12 +84,11 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) {
 
     override fun visitReferenceExpression(expression: KtReferenceExpression) {
         super.visitReferenceExpression(expression)
-        checkForOuterUsage(expression)
+        checkForOuterUsage { findResolvedContainingClassId(expression) }
     }
 
-    override fun visitExpressionWithLabel(expression: KtExpressionWithLabel) {
-        super.visitExpressionWithLabel(expression)
-        checkForOuterUsage(expression)
+    override fun visitThisExpression(expression: KtThisExpression) {
+        checkForOuterUsage { expression.referenceClassId() }
     }
 
     // Replace this "constructor().apply{}" pattern with buildList() when the Kotlin
@@ -101,32 +101,16 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) {
         }
     }
 
-    private fun checkForOuterUsage(expressionToResolve: KtReferenceExpression) {
+    private fun checkForOuterUsage(getTargetClassId: () -> ClassId?) {
         val currentClass = classChain.peek() ?: return
         val parentClasses = candidateClassToParentClasses[currentClass] ?: return
 
-        val resolvedContainingClassId = findResolvedContainingClassId(expressionToResolve)
+        val targetClassId = getTargetClassId() ?: return
         /*
          * If class A -> inner class B -> inner class C, and class C has outer usage of A,
          * then both B and C should stay as inner classes.
          */
-        val index = parentClasses.indexOfFirst { it.getClassId() == resolvedContainingClassId }
-        if (index >= 0) {
-            candidateClassToParentClasses.remove(currentClass)
-            parentClasses.subList(0, index).forEach { candidateClassToParentClasses.remove(it) }
-        }
-    }
-
-    private fun checkForOuterUsage(expressionToResolve: KtExpressionWithLabel) {
-        val currentClass = classChain.peek() ?: return
-        val parentClasses = candidateClassToParentClasses[currentClass] ?: return
-
-        val resolvedContainingClassName = expressionToResolve.getLabelName()
-        /*
-         * If class A -> inner class B -> inner class C, and class C has outer usage of A,
-         * then both B and C should stay as inner classes.
-         */
-        val index = parentClasses.indexOfFirst { it.name == resolvedContainingClassName }
+        val index = parentClasses.indexOfFirst { it.getClassId() == targetClassId }
         if (index >= 0) {
             candidateClassToParentClasses.remove(currentClass)
             parentClasses.subList(0, index).forEach { candidateClassToParentClasses.remove(it) }
@@ -139,4 +123,13 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) {
             ?.safeAs<ClassifierDescriptor>()
             ?.classId
     }
+
+    private fun KtThisExpression.referenceClassId(): ClassId? {
+        return getResolvedCall(bindingContext)
+            ?.resultingDescriptor
+            ?.returnType
+            ?.constructor
+            ?.declarationDescriptor
+            ?.classId
+    }
 }


Test patch:
diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClassSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClassSpec.kt
index e5fa7a5ae1ce..2413d01883ca 100644
--- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClassSpec.kt
+++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClassSpec.kt
@@ -446,4 +446,19 @@ class UnnecessaryInnerClassSpec(val env: KotlinCoreEnvironment) {
 
         assertThat(subject.lintWithContext(env, code)).hasSize(1)
     }
+
+    @Test
+    fun `reports when an inner class has this references`() {
+        val code = """
+            class A {
+                inner class B {
+                    fun foo() {
+                        this
+                    }
+                }
+            }
+        """.trimIndent()
+
+        assertThat(subject.lintWithContext(env, code)).hasSize(1)
+    }
 }