From 436cb9d440ebe283d8dc383c819d77f9194917d9 Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Thu, 26 May 2022 14:22:35 -0700 Subject: [PATCH 1/2] 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. --- .../rules/style/UnnecessaryInnerClass.kt | 26 ++++++++++++++-- .../rules/style/UnnecessaryInnerClassSpec.kt | 30 +++++++++++++++++++ 2 files changed, 54 insertions(+), 2 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 577b4cce41f..b8a13aadb9a 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,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 = ArrayList().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() ?.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 a20068ddcf4..27c1ed0c030 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 @@ -382,4 +382,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) + } } From 71393c341375c13f57c3bd3d675c0bf14ed9a316 Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Fri, 27 May 2022 23:01:18 -0700 Subject: [PATCH 2/2] UnnecessaryInnerClass: add test for double-nested inner classes using labeled expressions --- .../rules/style/UnnecessaryInnerClassSpec.kt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) 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 27c1ed0c030..de71fb8be23 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 @@ -356,6 +356,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