From ca9a3fa0bbe33ef0d3af231672f88ede3ab84f2b Mon Sep 17 00:00:00 2001 From: Chao Zhang Date: Sun, 23 Jan 2022 10:17:44 -0800 Subject: [PATCH] Fix false positive of UnnecessaryInnerClass (#4509) * Fix false positive of UnncessaryInnerClass * Exclude Nested annotation because junit test requires @Nested test class to be inner * Fix test code snippet --- config/detekt/detekt.yml | 1 + .../rules/style/UnnecessaryInnerClass.kt | 78 +++++++++---------- .../rules/style/UnnecessaryInnerClassSpec.kt | 47 +++++++---- 3 files changed, 70 insertions(+), 56 deletions(-) diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 73c28d19f..c7ff829f3 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -233,6 +233,7 @@ style: active: true UnnecessaryInnerClass: active: true + ignoreAnnotated: ['Nested'] UntilInsteadOfRangeTo: active: true UnusedImports: 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 5358d6692..deed8802b 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 @@ -28,6 +28,7 @@ import org.jetbrains.kotlin.psi.psiUtil.containingClass import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall import org.jetbrains.kotlin.resolve.descriptorUtil.classId +import org.jetbrains.kotlin.utils.addToStdlib.safeAs /** * This rule reports unnecessary inner classes. Nested classes that do not access members from the outer class do @@ -51,7 +52,7 @@ import org.jetbrains.kotlin.resolve.descriptorUtil.classId @RequiresTypeResolution class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) { - private val candidateClasses = mutableMapOf>() + private val candidateClassToParentClasses = mutableMapOf>() private val classChain = ArrayDeque() override val issue: Issue = Issue( @@ -69,14 +70,14 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) { override fun visitClass(klass: KtClass) { classChain.add(klass) if (klass.isInner()) { - candidateClasses[klass] = buildParentClassChain(klass) + candidateClassToParentClasses[klass] = findParentClasses(klass) } // Visit the class to determine whether it contains any references // to outer class members. super.visitClass(klass) - if (candidateClasses.contains(klass)) { + if (klass.isInner() && candidateClassToParentClasses.contains(klass)) { report( CodeSmell( issue, @@ -84,88 +85,79 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) { "Class '${klass.name}' does not require `inner` keyword." ) ) - candidateClasses.remove(klass) + candidateClassToParentClasses.remove(klass) } classChain.pop() } override fun visitProperty(property: KtProperty) { super.visitProperty(property) - checkForOuterUsage { parentClasses -> - property.initializer.belongsToParentClass(parentClasses) - } + checkForOuterUsage(listOfNotNull(property.initializer)) } override fun visitNamedFunction(function: KtNamedFunction) { super.visitNamedFunction(function) - checkForOuterUsage { parentClasses -> - function.initializer.belongsToParentClass(parentClasses) - } + checkForOuterUsage(listOfNotNull(function.initializer)) } override fun visitCallExpression(expression: KtCallExpression) { super.visitCallExpression(expression) - checkForOuterUsage { parentClasses -> - expression.belongsToParentClass(parentClasses) || - expression.collectDescendantsOfType { - it.belongsToParentClass(parentClasses) - }.isNotEmpty() - } + checkForOuterUsage(expression.collectDescendantsOfType() + expression) } override fun visitParameter(parameter: KtParameter) { super.visitParameter(parameter) - checkForOuterUsage { parentClasses -> - parameter.defaultValue.belongsToParentClass(parentClasses) - } + checkForOuterUsage(listOfNotNull(parameter.defaultValue)) } override fun visitBinaryExpression(expression: KtBinaryExpression) { super.visitBinaryExpression(expression) - checkForOuterUsage { parentClasses -> - expression.left.belongsToParentClass(parentClasses) || - expression.right.belongsToParentClass(parentClasses) - } + checkForOuterUsage(listOfNotNull(expression.left, expression.right)) } override fun visitIfExpression(expression: KtIfExpression) { super.visitIfExpression(expression) - checkForOuterUsage { parentClasses -> - val condition = expression.condition - condition is KtReferenceExpression && condition.belongsToParentClass(parentClasses) - } + checkForOuterUsage(listOfNotNull(expression.condition as? KtReferenceExpression)) } override fun visitDotQualifiedExpression(expression: KtDotQualifiedExpression) { super.visitDotQualifiedExpression(expression) - checkForOuterUsage { parentClasses -> - expression.receiverExpression.belongsToParentClass(parentClasses) - } + checkForOuterUsage(listOf(expression.receiverExpression)) } - // Replace this "constructor().apply{}" pattern with buildSet() when the Kotlin + // Replace this "constructor().apply{}" pattern with buildList() when the Kotlin // API version is upgraded to 1.6 - private fun buildParentClassChain(klass: KtClass) = HashSet().apply { - var containingClass = klass.containingClass() + private fun findParentClasses(ktClass: KtClass): List = ArrayList().apply { + var containingClass = ktClass.containingClass() while (containingClass != null) { - containingClass.getClassId()?.let { add(it) } + add(containingClass) containingClass = containingClass.containingClass() } } - private fun checkForOuterUsage(checkBlock: (Set) -> Boolean) { - val containingClass = classChain.peek() ?: return - val parentClasses = candidateClasses[containingClass] ?: return - if (checkBlock.invoke(parentClasses)) { - candidateClasses.remove(containingClass) + private fun checkForOuterUsage(expressionsToResolve: List) { + val currentClass = classChain.peek() ?: return + val parentClasses = candidateClassToParentClasses[currentClass] ?: return + + expressionsToResolve.forEach { ktElement -> + val resolvedContainingClassId = findResolvedContainingClassId(ktElement) + /* + * 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 KtElement?.belongsToParentClass(parentClasses: Set): Boolean { - return this?.getResolvedCall(bindingContext) + private fun findResolvedContainingClassId(ktElement: KtElement): ClassId? { + return ktElement.getResolvedCall(bindingContext) ?.resultingDescriptor ?.containingDeclaration - ?.let { (it as? ClassifierDescriptor)?.classId } - ?.let(parentClasses::contains) == true + ?.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 90f3f08ef..0a7a70c58 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 @@ -283,23 +283,44 @@ class UnnecessaryInnerClassSpec : Spek({ } } - it("does not report a double-nested inner class accessing from an outer-class member") { - val code = """ - class A { - val foo = "BAR" - - inner class B { - val fizz = foo - inner class C { - fun printFoo() { - println(foo) + context("does not report a double-nested inner class accessing from an outer-class member") { + + it("when the innermost class refers a inner class and the inner class refers the outermost class") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + val fizz = foo + inner class C { + fun printFoo() { + println(fizz) + } } } } - } - """.trimIndent() + """.trimIndent() - assertThat(subject.lintWithContext(env, code)).isEmpty() + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("when the innermost class refers the outermost class") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + inner class C { + fun printFoo() { + println(foo) + } + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } } it("does not report anonymous inner classes") {