Added rule UnnecessaryInnerClass (#4394)

* Added rule UnnecessaryInnerClass

* Addressed Detekt issues; Added test case

* Removed unnecessary code

* Addressing PR comment

* Enabled rule for self-tests on the Detekt library

* Fixing inner class issue in CognitiveComplexity

* Updated code comment

config/detekt/detekt.yml

@@ -231,6 +231,8 @@ style:
     active: true
   UnnecessaryLet:
     active: true
+  UnnecessaryInnerClass:
+    active: true
   UntilInsteadOfRangeTo:
     active: true
   UnusedImports:

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:

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

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),

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.
+ *
+ * <noncompliant>
+ * class A {
+ *     val foo = "BAR"
+ *
+ *     inner class B {
+ *         val fizz = "BUZZ"
+ *
+ *         fun printFizz() {
+ *             println(fizz)
+ *         }
+ *     }
+ * }
+ * </noncompliant>
+ */
+@Suppress("TooManyFunctions")
+@RequiresTypeResolution
+class UnnecessaryInnerClass(config: Config = Config.empty) : Rule(config) {
+
+    private val candidateClasses = mutableMapOf<KtClass, Set<ClassId>>()
+    private val classChain = ArrayDeque<KtClass>()
+
+    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<KtReferenceExpression> {
+                    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<ClassId>().apply {
+        var containingClass = klass.containingClass()
+        while (containingClass != null) {
+            containingClass.getClassId()?.let { add(it) }
+            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 KtElement?.belongsToParentClass(parentClasses: Set<ClassId>): Boolean {
+        return this?.getResolvedCall(bindingContext)
+            ?.resultingDescriptor
+            ?.containingDeclaration
+            ?.let { (it as? ClassifierDescriptor)?.classId }
+            ?.let(parentClasses::contains) == true
+    }
+}

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()
+        }
+    }
+})