Skip to content

Commit

Permalink
Fix false positive of UnncessaryInnerClass
Browse files Browse the repository at this point in the history
  • Loading branch information
@chao2zhang
chao2zhang committed Jan 23, 2022
1 parent 8a69676 commit b720787
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 56 deletions.
78 changes: 35 additions & 43 deletions ...es-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClass.kt
View file
Expand Up @@ -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
Expand All @@ -51,7 +52,7 @@ import org.jetbrains.kotlin.resolve.descriptorUtil.classId
@RequiresTypeResolution
class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) {

private val candidateClasses = mutableMapOf<KtClass, Set<ClassId>>()
private val candidateClassToParentClasses = mutableMapOf<KtClass, List<KtClass>>()
private val classChain = ArrayDeque<KtClass>()

override val issue: Issue = Issue(
Expand All @@ -69,103 +70,94 @@ 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,
Entity.Companion.from(klass),
"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<KtReferenceExpression> {
it.belongsToParentClass(parentClasses)
}.isNotEmpty()
}
checkForOuterUsage(expression.collectDescendantsOfType<KtReferenceExpression>() + 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<ClassId>().apply {
var containingClass = klass.containingClass()
private fun findParentClasses(ktClass: KtClass): List<KtClass> = ArrayList<KtClass>().apply {
var containingClass = ktClass.containingClass()
while (containingClass != null) {
containingClass.getClassId()?.let { add(it) }
add(containingClass)
containingClass = containingClass.containingClass()
}
}

private fun checkForOuterUsage(checkBlock: (Set<ClassId>) -> Boolean) {
val containingClass = classChain.peek() ?: return
val parentClasses = candidateClasses[containingClass] ?: return
if (checkBlock.invoke(parentClasses)) {
candidateClasses.remove(containingClass)
private fun checkForOuterUsage(expressionsToResolve: List<KtElement>) {
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<ClassId>): 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<ClassifierDescriptor>()
?.classId
}
}
47 changes: 34 additions & 13 deletions ...tyle/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClassSpec.kt
View file
Expand Up @@ -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(foo)
}
}
}
}
}
""".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") {
Expand Down

0 comments on commit b720787

Please sign in to comment.