SpacingBetweenDeclarationsWithCommentsRule: fix false positives/negat…

…ives - Fix false positives when a comment is not between declarations (pinterest#865) - Fix false negatives when declarations is in class
t-kameyama · Oct 19, 2020 · 3a28022 · 3a28022
1 parent 69cc0f7
commit 3a28022
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 5 deletions.
diff --git a/...n/com/pinterest/ktlint/ruleset/experimental/SpacingBetweenDeclarationsWithCommentsRule.kt b/...n/com/pinterest/ktlint/ruleset/experimental/SpacingBetweenDeclarationsWithCommentsRule.kt
@@ -3,11 +3,14 @@ package com.pinterest.ktlint.ruleset.experimental
 import com.pinterest.ktlint.core.Rule
 import com.pinterest.ktlint.core.ast.ElementType.FILE
 import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
+import com.pinterest.ktlint.core.ast.prevLeaf
 import com.pinterest.ktlint.core.ast.prevSibling
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
 import org.jetbrains.kotlin.com.intellij.psi.PsiComment
 import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace
 import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement
+import org.jetbrains.kotlin.psi.KtDeclaration
+import org.jetbrains.kotlin.psi.psiUtil.getPrevSiblingIgnoringWhitespaceAndComments
 
 /**
  * @see https://youtrack.jetbrains.com/issue/KT-35088
@@ -20,19 +23,23 @@ class SpacingBetweenDeclarationsWithCommentsRule : Rule("spacing-between-declara
         emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
     ) {
         if (node is PsiComment) {
-            val prevSibling = node.parent.node.prevSibling { it.elementType != WHITE_SPACE }
+            val declaration = node.parent as? KtDeclaration ?: return
+            if (declaration.getPrevSiblingIgnoringWhitespaceAndComments() !is KtDeclaration) return
+
+            val prevSibling = declaration.node.prevSibling { it.elementType != WHITE_SPACE }
             if (prevSibling != null &&
                 prevSibling.elementType != FILE &&
                 prevSibling !is PsiComment
             ) {
-                if (node.parent.prevSibling is PsiWhiteSpace && node.parent.prevSibling.text == "\n") {
+                if (declaration.prevSibling is PsiWhiteSpace && declaration.prevSibling.text.count { it == '\n' } < 2) {
                     emit(
                         node.startOffset,
                         "Declarations and declarations with comments should have an empty space between.",
                         true
                     )
                     if (autoCorrect) {
-                        (node.parent.prevSibling.node as LeafPsiElement).rawReplaceWithText("\n\n")
+                        val indent = node.prevLeaf()?.text?.trim('\n') ?: ""
+                        (declaration.prevSibling.node as LeafPsiElement).rawReplaceWithText("\n\n$indent")
                     }
                 }
             }

diff --git a/...m/pinterest/ktlint/ruleset/experimental/SpacingBetweenDeclarationsWithCommentsRuleTest.kt b/...m/pinterest/ktlint/ruleset/experimental/SpacingBetweenDeclarationsWithCommentsRuleTest.kt
@@ -119,13 +119,13 @@ class SpacingBetweenDeclarationsWithCommentsRuleTest {
              * Doc 1
              */
             fun one() = 1
-            
+
             /**
              * Doc 2
              */
             fun two() = 2
             fun three() = 42
-            
+
             // comment
             fun four() = 44
             """.trimIndent(),
@@ -146,4 +146,79 @@ class SpacingBetweenDeclarationsWithCommentsRuleTest {
             )
         )
     }
+
+    @Test
+    fun `not a declaration comment`() {
+        assertThat(
+            SpacingBetweenDeclarationsWithCommentsRule().lint(
+                """
+                fun function() {
+                    with(binding) {
+                        loginButton.setOnClickListener {
+                //            comment
+                            showScreen()
+                        }
+                    }
+                }
+                """.trimIndent()
+            )
+        ).isEmpty()
+    }
+
+    @Test
+    fun `not a comment between declarations`() {
+        assertThat(
+            SpacingBetweenDeclarationsWithCommentsRule().lint(
+                """
+                class C {
+                    fun test() {
+                        println()
+                        // comment1
+                        fun one() = 1
+                    }
+                }
+                """.trimIndent()
+            )
+        ).isEmpty()
+    }
+
+    @Test
+    fun `declarations in class`() {
+        val actual = SpacingBetweenDeclarationsWithCommentsRule().format(
+            """
+            class C {
+                /**
+                 * Doc 1
+                 */
+                fun one() = 1
+                /**
+                 * Doc 2
+                 */
+                fun two() = 2
+                fun three() = 42
+                // comment
+                fun four() = 44
+            }
+            """.trimIndent()
+        )
+        assertThat(actual).isEqualTo(
+            """
+            class C {
+                /**
+                 * Doc 1
+                 */
+                fun one() = 1
+
+                /**
+                 * Doc 2
+                 */
+                fun two() = 2
+                fun three() = 42
+
+                // comment
+                fun four() = 44
+            }
+            """.trimIndent()
+        )
+    }
 }