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.

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

UnnecessaryInnerClass: fix false positives labeled expression to outer class #4865

Merged
merged 2 commits into from May 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Expand Up @@ -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
Expand Down Expand Up @@ -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<KtClass> = ArrayList<KtClass>().apply {
Expand All @@ -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 }
dzirbel marked this conversation as resolved.
Show resolved Hide resolved
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<ClassifierDescriptor>()
?.classId
Expand Down
Expand Up @@ -356,6 +356,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
Expand All @@ -382,4 +399,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)
}
}