From 05cd79612f00a52ec14f012c08d9531042ae1e95 Mon Sep 17 00:00:00 2001
From: Toshiaki Kameyama <kmymtsak@gmail.com>
Date: Thu, 2 Jun 2022 01:55:51 +0900
Subject: [PATCH] UnnecessaryInnerClass: fix false negative with `this`
 references (#4884)

---
 .../rules/style/UnnecessaryInnerClass.kt      | 41 ++++++++-----------
 .../rules/style/UnnecessaryInnerClassSpec.kt  | 15 +++++++
 2 files changed, 32 insertions(+), 24 deletions(-)

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 b8a13aadb9a..3b4ea4c6f0a 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
+    }
 }
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 e5fa7a5ae1c..2413d01883c 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)
+    }
 }