UnnecessaryInnerClass: fix false positives labeled expression to oute…

…r class (#4865)

* UnnecessaryInnerClass: fix false positives labeled expression to outer class

Fix an issue where inner classes which reference their outer class via a labeled
expression would still be reported as unnecessary. This seems to be due to a
missing visitExpressionWithLabel() check which resolves the label name: if the
label is referencing a parent class, then the nested class must be `inner`.

I wasn't able to find a way to resolve the labeled expression as a ClassId to
match the existing class check from KtReferenceExpression, so I settled for
using the class name instead, which is easy to obtain. This means there's a fair
bit of duplication in overriden checkForOuterUsage() methods to check parent
classes which could probably be removed, but I didn't see a particularly clean
way to do so.

* UnnecessaryInnerClass: add test for double-nested inner classes using labeled expressions

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

@@ -13,6 +13,7 @@ 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.psiUtil.containingClass
@@ -85,6 +86,11 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) {
         checkForOuterUsage(expression)
     }
 
+    override fun visitExpressionWithLabel(expression: KtExpressionWithLabel) {
+        super.visitExpressionWithLabel(expression)
+        checkForOuterUsage(expression)
+    }
+
     // Replace this "constructor().apply{}" pattern with buildList() when the Kotlin
     // API version is upgraded to 1.6
     private fun findParentClasses(ktClass: KtClass): List<KtClass> = ArrayList<KtClass>().apply {
@@ -111,8 +117,24 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) {
         }
     }
 
-    private fun findResolvedContainingClassId(reference: KtReferenceExpression): ClassId? {
-        return bindingContext[BindingContext.REFERENCE_TARGET, reference]
+    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 }
+        if (index >= 0) {
+            candidateClassToParentClasses.remove(currentClass)
+            parentClasses.subList(0, index).forEach { candidateClassToParentClasses.remove(it) }
+        }
+    }
+
+    private fun findResolvedContainingClassId(expression: KtReferenceExpression): ClassId? {
+        return bindingContext[BindingContext.REFERENCE_TARGET, expression]
             ?.containingDeclaration
             ?.safeAs<ClassifierDescriptor>()
             ?.classId

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

@@ -373,6 +373,23 @@ class UnnecessaryInnerClassSpec(val env: KotlinCoreEnvironment) {
 
             assertThat(subject.lintWithContext(env, code)).isEmpty()
         }
+
+        @Test
+        fun `when the innermost class refers the outermost class via a labeled expression`() {
+            val code = """
+                class A {
+                    inner class B {
+                        inner class C {
+                            fun outer(): A {
+                                return this@A
+                            }
+                        }
+                    }
+                }
+            """.trimIndent()
+
+            assertThat(subject.lintWithContext(env, code)).isEmpty()
+        }
     }
 
     @Test
@@ -399,4 +416,34 @@ class UnnecessaryInnerClassSpec(val env: KotlinCoreEnvironment) {
 
         assertThat(subject.lintWithContext(env, code)).isEmpty()
     }
+
+    @Test
+    fun `does not report labeled expressions to outer class`() {
+        val code = """
+            class A {
+                inner class B {
+                    fun outer(): A {
+                        return this@A
+                    }
+                }
+            }
+        """.trimIndent()
+
+        assertThat(subject.lintWithContext(env, code)).isEmpty()
+    }
+
+    @Test
+    fun `reports irrelevant labeled expressions`() {
+        val code = """
+            class A {
+                inner class B {
+                    fun inner() {
+                        return Unit.apply { this@inner }
+                    }
+                }
+            }
+        """.trimIndent()
+
+        assertThat(subject.lintWithContext(env, code)).hasSize(1)
+    }
 }