Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix false positive of UnnecessaryInnerClass #4509

Merged
merged 3 commits into from Jan 23, 2022
Merged

Fix false positive of UnnecessaryInnerClass #4509

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b720787
Fix false positive of UnncessaryInnerClass
chao2zhang Jan 23, 2022
b39bc51
Exclude Nested annotation because junit test requires @Nested test cl…
chao2zhang Jan 23, 2022
2e9ee29
Fix test code snippet
chao2zhang Jan 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 35 additions & 43 deletions ...es-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClass.kt
View file
Open in desktop
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
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to write println(fizz) here? At least that is what the test description suggests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great catch.

}
}
}
}
}
""".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