From 0695d7e5e0eaf22ecb626fec312b46bef56c2363 Mon Sep 17 00:00:00 2001 From: Toshiaki Kameyama Date: Wed, 20 Apr 2022 15:54:54 +0900 Subject: [PATCH] UnnecessaryInnerClass: fix false positive with references to function type variables (#4738) --- .../rules/style/UnnecessaryInnerClass.kt | 73 ++++--------------- .../rules/style/UnnecessaryInnerClassSpec.kt | 17 +++++ 2 files changed, 32 insertions(+), 58 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 068ec85b9..577b4cce4 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 @@ -12,21 +12,11 @@ import org.jetbrains.kotlin.backend.common.peek import org.jetbrains.kotlin.backend.common.pop import org.jetbrains.kotlin.descriptors.ClassifierDescriptor import org.jetbrains.kotlin.name.ClassId -import org.jetbrains.kotlin.psi.KtBinaryExpression -import org.jetbrains.kotlin.psi.KtCallExpression import org.jetbrains.kotlin.psi.KtClass -import org.jetbrains.kotlin.psi.KtDotQualifiedExpression -import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtFile -import org.jetbrains.kotlin.psi.KtIfExpression -import org.jetbrains.kotlin.psi.KtNamedFunction -import org.jetbrains.kotlin.psi.KtParameter -import org.jetbrains.kotlin.psi.KtProperty import org.jetbrains.kotlin.psi.KtReferenceExpression -import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType 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 @@ -90,39 +80,9 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) { classChain.pop() } - override fun visitProperty(property: KtProperty) { - super.visitProperty(property) - checkForOuterUsage(listOfNotNull(property.initializer)) - } - - override fun visitNamedFunction(function: KtNamedFunction) { - super.visitNamedFunction(function) - checkForOuterUsage(listOfNotNull(function.initializer)) - } - - override fun visitCallExpression(expression: KtCallExpression) { - super.visitCallExpression(expression) - checkForOuterUsage(expression.collectDescendantsOfType() + expression) - } - - override fun visitParameter(parameter: KtParameter) { - super.visitParameter(parameter) - checkForOuterUsage(listOfNotNull(parameter.defaultValue)) - } - - override fun visitBinaryExpression(expression: KtBinaryExpression) { - super.visitBinaryExpression(expression) - checkForOuterUsage(listOfNotNull(expression.left, expression.right)) - } - - override fun visitIfExpression(expression: KtIfExpression) { - super.visitIfExpression(expression) - checkForOuterUsage(listOfNotNull(expression.condition as? KtReferenceExpression)) - } - - override fun visitDotQualifiedExpression(expression: KtDotQualifiedExpression) { - super.visitDotQualifiedExpression(expression) - checkForOuterUsage(listOf(expression.receiverExpression)) + override fun visitReferenceExpression(expression: KtReferenceExpression) { + super.visitReferenceExpression(expression) + checkForOuterUsage(expression) } // Replace this "constructor().apply{}" pattern with buildList() when the Kotlin @@ -135,27 +95,24 @@ class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) { } } - private fun checkForOuterUsage(expressionsToResolve: List) { + private fun checkForOuterUsage(expressionToResolve: KtReferenceExpression) { 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) } - } + val resolvedContainingClassId = findResolvedContainingClassId(expressionToResolve) + /* + * 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 findResolvedContainingClassId(ktElement: KtElement): ClassId? { - return ktElement.getResolvedCall(bindingContext) - ?.resultingDescriptor + private fun findResolvedContainingClassId(reference: KtReferenceExpression): ClassId? { + return bindingContext[BindingContext.REFERENCE_TARGET, reference] ?.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 a3623a4c7..3c341f655 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 @@ -298,6 +298,23 @@ class UnnecessaryInnerClassSpec(val env: KotlinCoreEnvironment) { assertThat(subject.lintWithContext(env, code)).isEmpty() } + + @Test + fun `to call a function type variable of the member`() { + val code = """ + class A { + val foo: () -> Unit = {} + + inner class B { + fun bar() { + foo() + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } } @Nested