diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index f45a48bac..f394727ff 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -231,6 +231,8 @@ style: active: true UnnecessaryLet: active: true + UnnecessaryInnerClass: + active: true UntilInsteadOfRangeTo: active: true UnusedImports: diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index a2ddf6bed..ffce83d11 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -752,6 +752,8 @@ style: active: false UnnecessaryInheritance: active: true + UnnecessaryInnerClass: + active: false UnnecessaryLet: active: false UnnecessaryParentheses: diff --git a/detekt-metrics/src/main/kotlin/io/github/detekt/metrics/CognitiveComplexity.kt b/detekt-metrics/src/main/kotlin/io/github/detekt/metrics/CognitiveComplexity.kt index 32f7442ef..969bc2472 100644 --- a/detekt-metrics/src/main/kotlin/io/github/detekt/metrics/CognitiveComplexity.kt +++ b/detekt-metrics/src/main/kotlin/io/github/detekt/metrics/CognitiveComplexity.kt @@ -46,9 +46,7 @@ class CognitiveComplexity private constructor() : DetektVisitor() { data class BinExprHolder(val expr: KtBinaryExpression, val op: IElementType, val isEnclosed: Boolean) @Suppress("detekt.TooManyFunctions") // visitor pattern - inner class FunctionComplexity( - private val givenFunction: KtNamedFunction - ) : DetektVisitor() { + class FunctionComplexity(private val givenFunction: KtNamedFunction) : DetektVisitor() { internal var complexity: Int = 0 private var nesting: Int = 0 diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt index 804231269..83c102869 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt @@ -46,6 +46,7 @@ class StyleGuideProvider : DefaultRuleSetProvider { UnnecessaryAnnotationUseSiteTarget(config), UnnecessaryParentheses(config), UnnecessaryInheritance(config), + UnnecessaryInnerClass(config), UtilityClassWithPublicConstructor(config), ObjectLiteralToLambda(config), OptionalAbstractKeyword(config), 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 new file mode 100644 index 000000000..5358d6692 --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClass.kt @@ -0,0 +1,171 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution +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.callUtil.getResolvedCall +import org.jetbrains.kotlin.resolve.descriptorUtil.classId + +/** + * This rule reports unnecessary inner classes. Nested classes that do not access members from the outer class do + * not require the `inner` qualifier. + * + * + * class A { + * val foo = "BAR" + * + * inner class B { + * val fizz = "BUZZ" + * + * fun printFizz() { + * println(fizz) + * } + * } + * } + * + */ +@Suppress("TooManyFunctions") +@RequiresTypeResolution +class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) { + + private val candidateClasses = mutableMapOf>() + private val classChain = ArrayDeque() + + override val issue: Issue = Issue( + javaClass.simpleName, + Severity.Style, + "The 'inner' qualifier is unnecessary.", + Debt.FIVE_MINS + ) + + override fun visit(root: KtFile) { + if (bindingContext == BindingContext.EMPTY) return + super.visit(root) + } + + override fun visitClass(klass: KtClass) { + classChain.add(klass) + if (klass.isInner()) { + candidateClasses[klass] = buildParentClassChain(klass) + } + + // Visit the class to determine whether it contains any references + // to outer class members. + super.visitClass(klass) + + if (candidateClasses.contains(klass)) { + report( + CodeSmell( + issue, + Entity.Companion.from(klass), + "Class '${klass.name}' does not require `inner` keyword." + ) + ) + candidateClasses.remove(klass) + } + classChain.pop() + } + + override fun visitProperty(property: KtProperty) { + super.visitProperty(property) + checkForOuterUsage { parentClasses -> + property.initializer.belongsToParentClass(parentClasses) + } + } + + override fun visitNamedFunction(function: KtNamedFunction) { + super.visitNamedFunction(function) + checkForOuterUsage { parentClasses -> + function.initializer.belongsToParentClass(parentClasses) + } + } + + override fun visitCallExpression(expression: KtCallExpression) { + super.visitCallExpression(expression) + checkForOuterUsage { parentClasses -> + expression.belongsToParentClass(parentClasses) || + expression.collectDescendantsOfType { + it.belongsToParentClass(parentClasses) + }.isNotEmpty() + } + } + + override fun visitParameter(parameter: KtParameter) { + super.visitParameter(parameter) + checkForOuterUsage { parentClasses -> + parameter.defaultValue.belongsToParentClass(parentClasses) + } + } + + override fun visitBinaryExpression(expression: KtBinaryExpression) { + super.visitBinaryExpression(expression) + checkForOuterUsage { parentClasses -> + expression.left.belongsToParentClass(parentClasses) || + expression.right.belongsToParentClass(parentClasses) + } + } + + override fun visitIfExpression(expression: KtIfExpression) { + super.visitIfExpression(expression) + checkForOuterUsage { parentClasses -> + val condition = expression.condition + condition is KtReferenceExpression && condition.belongsToParentClass(parentClasses) + } + } + + override fun visitDotQualifiedExpression(expression: KtDotQualifiedExpression) { + super.visitDotQualifiedExpression(expression) + checkForOuterUsage { parentClasses -> + expression.receiverExpression.belongsToParentClass(parentClasses) + } + } + + // Replace this "constructor().apply{}" pattern with buildSet() when the Kotlin + // API version is upgraded to 1.6 + private fun buildParentClassChain(klass: KtClass) = HashSet().apply { + var containingClass = klass.containingClass() + while (containingClass != null) { + containingClass.getClassId()?.let { add(it) } + 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 KtElement?.belongsToParentClass(parentClasses: Set): Boolean { + return this?.getResolvedCall(bindingContext) + ?.resultingDescriptor + ?.containingDeclaration + ?.let { (it as? ClassifierDescriptor)?.classId } + ?.let(parentClasses::contains) == true + } +} 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 new file mode 100644 index 000000000..90f3f08ef --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryInnerClassSpec.kt @@ -0,0 +1,329 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.rules.setupKotlinEnvironment +import io.gitlab.arturbosch.detekt.test.assertThat +import io.gitlab.arturbosch.detekt.test.lintWithContext +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.spekframework.spek2.Spek +import org.spekframework.spek2.style.specification.describe + +class UnnecessaryInnerClassSpec : Spek({ + setupKotlinEnvironment() + + val env: KotlinCoreEnvironment by memoized() + val subject by memoized { UnnecessaryInnerClass(Config.empty) } + + describe("UnnecessaryInnerClass Rule") { + it("reports when an inner class does not access members of its outer class") { + val code = """ + val fileFoo = "FILE_FOO" + + class A { + val foo = "BAR" + + fun printFoo() { + println(foo) + } + + inner class B { + val fizz = "BUZZ" + + fun printFizz() { + println(fileFoo) + println(fizz) + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).hasSize(1) + } + + context("does not report an inner class accessing outer-class members") { + it("as a default argument for a constructor") { + val code = """ + class A { + val foo = "BAR" + + inner class B(val fizz: String = foo) + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("as a property initializer") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + val fizz = foo + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + context("in a variable assignment") { + it("where the outer-class variable is on the left") { + val code = """ + class A { + var foo = "BAR" + + inner class B { + val fizz = "BUZZ" + init { + foo = fizz + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("where the outer-class variable is on the right") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + val fizz: String + init { + fizz = foo + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("where the outer-class variable is in a compound statement") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + val fizz: String + init { + fizz = "FOO" + foo + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + } + + context("in an if-statement") { + it("where the outer-class variable is the only expression") { + val code = """ + class A(val foo: Boolean) { + + inner class B { + fun printFoo() { + if (foo) { + println("FOO") + } + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("where the outer-class variable is on the left") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + fun printFoo() { + if (foo == "BAR") { + println("FOO") + } + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("where the outer-class variable is on the right") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + fun printFoo() { + if ("BAR" == foo) { + println("FOO") + } + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("where the outer-class variable is in a compound statement") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + val fizz = "BUZZ" + fun printFoo() { + if (fizz == "BUZZ" && foo) { + println("FOO") + } + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + } + + it("as a function initializer") { + val code = """ + class A { + fun printFoo() { + println("FOO") + } + + inner class B { + fun printFizz() = printFoo() + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("as a function call") { + val code = """ + class A { + fun printFoo() { + println("FOO") + } + + inner class B { + fun printFizz() { + printFoo() + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("as a function argument") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + fun printFizz() { + println(foo) + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("as a default value in a function signature") { + val code = """ + class A { + val foo = "BAR" + + inner class B { + fun printFizz(fizzVal: String = foo) { + println(fizzVal) + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("to call a function of the member") { + val code = """ + class FooClass { + fun printFoo() { + println("FOO") + } + } + + class A { + val foo = FooClass() + + inner class B { + fun printFizz() { + foo.printFoo() + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + } + + 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) + } + } + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + + it("does not report anonymous inner classes") { + val code = """ + interface FooInterface { + fun doFoo() + } + + class Foo { + fun runFoo(fi: FooInterface) { + fi.doFoo() + } + + fun run() { + runFoo(object : FooInterface { + override fun doFoo() { + println("FOO") + } + }) + } + } + """.trimIndent() + + assertThat(subject.lintWithContext(env, code)).isEmpty() + } + } +})