From 2f5b8f593361615dda1f0acb460afdd014ab460d Mon Sep 17 00:00:00 2001 From: Dominic Zirbel Date: Sun, 29 May 2022 02:52:17 -0700 Subject: [PATCH] UnnecessaryInnerClass: fix false positives labeled expression to outer 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 --- .../rules/style/UnnecessaryInnerClass.kt | 26 +++++++++- .../rules/style/UnnecessaryInnerClassSpec.kt | 47 +++++++++++++++++++ 2 files changed, 71 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 e3f6d02cc6a..e5fa7a5ae1c 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 @@ -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) + } }