Update the implementation of ClassOrdering to handle false negatives

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrdering.kt

@@ -61,59 +61,65 @@ class ClassOrdering(config: Config = Config.empty) : Rule(config) {
         Debt.FIVE_MINS
     )
 
-    private val comparator: Comparator<KtDeclaration> = Comparator { dec1: KtDeclaration, dec2: KtDeclaration ->
-        if (dec1.priority == null || dec2.priority == null) return@Comparator 0
-        compareValues(dec1.priority, dec2.priority)
-    }
-
     override fun visitClassBody(classBody: KtClassBody) {
         super.visitClassBody(classBody)
 
-        val misorders = comparator.findOutOfOrder(classBody.declarations)
-        if (misorders.isNotEmpty()) {
-            report(
-                misorders.map {
-                    CodeSmell(
-                        issue = issue,
-                        entity = Entity.from(it.first),
-                        message = "${it.first.description} should not come before ${it.second.description}",
-                        references = listOf(Entity.from(classBody))
+        var currentSection = Section(0)
+        for (ktDeclaration in classBody.declarations) {
+            val section = ktDeclaration.toSection() ?: continue
+            when {
+                section < currentSection -> {
+                    val message =
+                        "${ktDeclaration.toDescription()} should be declared before ${currentSection.toDescription()}."
+                    report(
+                        CodeSmell(
+                            issue = issue,
+                            entity = Entity.from(ktDeclaration),
+                            message = message,
+                            references = listOf(Entity.from(classBody))
+                        )
                     )
                 }
-            )
+                section > currentSection -> currentSection = section
+            }
         }
     }
 }
 
-private fun Comparator<KtDeclaration>.findOutOfOrder(
-    declarations: List<KtDeclaration>
-): List<Pair<KtDeclaration, KtDeclaration>> =
-    declarations
-        .zipWithNext { a, b -> if (compare(a, b) > 0) Pair(a, b) else null }
-        .filterNotNull()
+private fun KtDeclaration.toDescription(): String = when {
+    this is KtProperty -> "property `$name`"
+    this is KtClassInitializer -> "initializer blocks"
+    this is KtSecondaryConstructor -> "secondary constructor"
+    this is KtNamedFunction -> "method `$name()`"
+    this is KtObjectDeclaration && isCompanion() -> "companion object"
+    else -> ""
+}
+
+@Suppress("MagicNumber")
+private fun KtDeclaration.toSection(): Section? = when {
+    this is KtProperty -> Section(0)
+    this is KtClassInitializer -> Section(0)
+    this is KtSecondaryConstructor -> Section(1)
+    this is KtNamedFunction -> Section(2)
+    this is KtObjectDeclaration && isCompanion() -> Section(3)
+    else -> null // For declarations not relevant for ordering, such as nested classes.
+}
 
-private val KtDeclaration.description: String
-    get() = when (this) {
-        is KtClassInitializer -> "class initializer"
-        is KtObjectDeclaration -> if (isCompanion()) "Companion object" else ""
-        else -> "$name ($printableDeclaration)"
+@JvmInline
+@Suppress("MagicNumber")
+private value class Section(val priority: Int) : Comparable<Section> {
+
+    init {
+        require(priority in 0..3)
     }
 
-private val KtDeclaration.printableDeclaration: String
-    get() = when (this) {
-        is KtProperty -> "property"
-        is KtSecondaryConstructor -> "secondary constructor"
-        is KtNamedFunction -> "function"
+    fun toDescription(): String = when (priority) {
+        0 -> "property declarations and initializer blocks"
+        1 -> "secondary constructors"
+        2 -> "method declarations"
+        3 -> "companion object"
         else -> ""
     }
 
-@Suppress("MagicNumber")
-private val KtDeclaration.priority: Int?
-    get() = when (this) {
-        is KtProperty -> 0
-        is KtClassInitializer -> 0
-        is KtSecondaryConstructor -> 1
-        is KtNamedFunction -> 2
-        is KtObjectDeclaration -> if (isCompanion()) 3 else null
-        else -> null
-    }
+    override fun compareTo(other: Section): Int = priority.compareTo(other.priority)
+}

detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ClassOrderingSpec.kt

@@ -77,8 +77,7 @@ class ClassOrderingSpec : Spek({
             val findings = subject.compileAndLint(code)
             assertThat(findings).hasSize(1)
             assertThat(findings[0].message).isEqualTo(
-                "OutOfOrder (secondary constructor) " +
-                    "should not come before class initializer"
+                "initializer blocks should be declared before secondary constructors."
             )
         }
 
@@ -102,10 +101,12 @@ class ClassOrderingSpec : Spek({
             """
 
             val findings = subject.compileAndLint(code)
-            assertThat(findings).hasSize(1)
+            assertThat(findings).hasSize(2)
             assertThat(findings[0].message).isEqualTo(
-                "OutOfOrder (secondary constructor) " +
-                    "should not come before y (property)"
+                "property `y` should be declared before secondary constructors."
+            )
+            assertThat(findings[1].message).isEqualTo(
+                "initializer blocks should be declared before secondary constructors."
             )
         }
 
@@ -129,8 +130,13 @@ class ClassOrderingSpec : Spek({
             """
 
             val findings = subject.compileAndLint(code)
-            assertThat(findings).hasSize(1)
-            assertThat(findings[0].message).isEqualTo("returnX (function) should not come before y (property)")
+            assertThat(findings).hasSize(3)
+            assertThat(findings[0].message)
+                .isEqualTo("property `y` should be declared before method declarations.")
+            assertThat(findings[1].message)
+                .isEqualTo("initializer blocks should be declared before method declarations.")
+            assertThat(findings[2].message)
+                .isEqualTo("secondary constructor should be declared before method declarations.")
         }
 
         it("reports when companion object is out of order") {
@@ -154,7 +160,7 @@ class ClassOrderingSpec : Spek({
 
             val findings = subject.compileAndLint(code)
             assertThat(findings).hasSize(1)
-            assertThat(findings[0].message).isEqualTo("Companion object should not come before returnX (function)")
+            assertThat(findings[0].message).isEqualTo("method `returnX()` should be declared before companion object.")
         }
 
         it("does not report nested class order") {
@@ -201,6 +207,37 @@ class ClassOrderingSpec : Spek({
             assertThat(subject.compileAndLint(code)).hasSize(0)
         }
 
+        it("report all issues with interleaving nested class") {
+            val code = """
+                class MultipleMisorders(private val x: String) {
+                    companion object {
+                        const val IMPORTANT_VALUE = 3
+                    }
+
+                    class Nested { }
+
+                    fun returnX() = x
+
+                    class Nested2 { }
+
+                    constructor(z: Int): this(z.toString())
+
+                    class Nested3 { }
+
+                    val y = x
+                }
+            """
+
+            val findings = subject.compileAndLint(code)
+            assertThat(findings).hasSize(3)
+            assertThat(findings[0].message)
+                .isEqualTo("method `returnX()` should be declared before companion object.")
+            assertThat(findings[1].message)
+                .isEqualTo("secondary constructor should be declared before companion object.")
+            assertThat(findings[2].message)
+                .isEqualTo("property `y` should be declared before companion object.")
+        }
+
         it("does report all issues in a class with multiple misorderings") {
             val code = """
                 class MultipleMisorders(private val x: String) {
@@ -219,11 +256,11 @@ class ClassOrderingSpec : Spek({
             val findings = subject.compileAndLint(code)
             assertThat(findings).hasSize(3)
             assertThat(findings[0].message)
-                .isEqualTo("Companion object should not come before returnX (function)")
+                .isEqualTo("method `returnX()` should be declared before companion object.")
             assertThat(findings[1].message)
-                .isEqualTo("returnX (function) should not come before MultipleMisorders (secondary constructor)")
+                .isEqualTo("secondary constructor should be declared before companion object.")
             assertThat(findings[2].message)
-                .isEqualTo("MultipleMisorders (secondary constructor) should not come before y (property)")
+                .isEqualTo("property `y` should be declared before companion object.")
         }
     }
 })