Annotation spacing fix (#1087)

* Fixing bug with experiment:annotation-spacing-rule with comments * updating changelog * deleting unused function * fix imports * fix build
pinterest · Feb 23, 2021 · be9626d · be9626d
1 parent 17a6928
commit be9626d
Show file tree

Hide file tree

Showing 3 changed files with 125 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 - Fix formatting with comments (`colon-spacing`) ([#1057](https://github.com/pinterest/ktlint/issues/1057))
 - Fix IndexOutOfBoundsException in `argument-list-wrapping-rule` formatting file with many corrections ([#1081](https://github.com/pinterest/ktlint/issues/1081))
 - Fix formatting in arguments (`multiline-if-else`) ([#1079](https://github.com/pinterest/ktlint/issues/1079))
+- Fix experimental:annotation-spacing-rule autocorrection with comments
 
 ### Changed
 - Update Gradle shadow plugin to `6.1.0` version

diff --git a/...mental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/AnnotationSpacingRule.kt b/...mental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/AnnotationSpacingRule.kt
@@ -6,6 +6,7 @@ import com.pinterest.ktlint.core.ast.isPartOf
 import com.pinterest.ktlint.core.ast.isPartOfComment
 import com.pinterest.ktlint.core.ast.isWhiteSpace
 import com.pinterest.ktlint.core.ast.isWhiteSpaceWithNewline
+import com.pinterest.ktlint.core.ast.nextCodeSibling
 import com.pinterest.ktlint.core.ast.nextLeaf
 import com.pinterest.ktlint.core.ast.nextSibling
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
@@ -17,15 +18,14 @@ import org.jetbrains.kotlin.psi.psiUtil.children
 import org.jetbrains.kotlin.psi.psiUtil.endOffset
 
 /**
- * Ensures there are not multiple line breaks between annotations and declarations.
+ * Ensures annotations occur immediately prior to the annotated construct
  *
  * https://kotlinlang.org/docs/reference/coding-conventions.html#annotation-formatting
  */
 class AnnotationSpacingRule : Rule("annotation-spacing") {
 
     companion object {
-        const val fileAnnotationsLineBreaks =
-            "There should not be empty lines between an annotation and the object that it's annotating"
+        const val ERROR_MESSAGE = "Annotations should occur immediately before the annotated construct"
     }
 
     override fun visit(
@@ -62,29 +62,44 @@ class AnnotationSpacingRule : Rule("annotation-spacing") {
             {
                 !it.isWhiteSpace() &&
                     it.textLength > 0 &&
-                    !(it.isPartOfComment() /* && it.lineNumber() == lineNumber*/) &&
                     !it.isPartOf(ElementType.FILE_ANNOTATION_LIST)
             },
             {
-                val s = it.text
-                // Ensure at least one occurrence of two line breaks
-                s.indexOf("\n") != s.lastIndexOf("\n")
+                // Disallow multiple white spaces as well as comments
+                if (it.psi is PsiWhiteSpace) {
+                    val s = it.text
+                    // Ensure at least one occurrence of two line breaks
+                    s.indexOf("\n") != s.lastIndexOf("\n")
+                } else it.isPartOfComment()
             }
         )
         if (next != null) {
             if (node.elementType != ElementType.FILE_ANNOTATION_LIST) {
                 val psi = node.psi
-                emit(psi.endOffset - 1, fileAnnotationsLineBreaks, true)
+                emit(psi.endOffset - 1, ERROR_MESSAGE, true)
                 if (autoCorrect) {
-                    removeExtraLineBreaks(node)
+                    // Special-case autocorrection when the annotation is separated from the annotated construct
+                    // by a comment: we need to swap the order of the comment and the annotation
+                    if (next.isPartOfComment()) {
+                        // Remove the annotation and the following whitespace
+                        val nextSibling = node.nextSibling { it.isWhiteSpace() }
+                        node.treeParent.removeChild(node)
+                        nextSibling?.treeParent?.removeChild(nextSibling)
+                        // Insert the annotation prior to the annotated construct
+                        val space = PsiWhiteSpaceImpl("\n")
+                        next.treeParent.addChild(space, next.nextCodeSibling())
+                        next.treeParent.addChild(node, space)
+                    } else {
+                        removeExtraLineBreaks(node)
+                    }
                 }
             }
         }
         if (whiteSpaces.isNotEmpty() && annotations.size > 1 && node.elementType != ElementType.FILE_ANNOTATION_LIST) {
             // Check to make sure there are multi breaks between annotations
             if (whiteSpaces.any { psi -> psi.textToCharArray().filter { it == '\n' }.count() > 1 }) {
                 val psi = node.psi
-                emit(psi.endOffset - 1, fileAnnotationsLineBreaks, true)
+                emit(psi.endOffset - 1, ERROR_MESSAGE, true)
                 if (autoCorrect) {
                     removeIntraLineBreaks(node, annotations.last())
                 }

diff --git a/...al/src/test/kotlin/com/pinterest/ktlint/ruleset/experimental/AnnotationSpacingRuleTest.kt b/...al/src/test/kotlin/com/pinterest/ktlint/ruleset/experimental/AnnotationSpacingRuleTest.kt
@@ -2,7 +2,6 @@ package com.pinterest.ktlint.ruleset.experimental
 
 import com.pinterest.ktlint.core.KtLint
 import com.pinterest.ktlint.core.LintError
-import com.pinterest.ktlint.ruleset.experimental.AnnotationSpacingRule.Companion.fileAnnotationsLineBreaks
 import com.pinterest.ktlint.test.format
 import com.pinterest.ktlint.test.lint
 import java.util.ArrayList
@@ -37,7 +36,7 @@ class AnnotationSpacingRuleTest {
             )
         ).isEqualTo(
             listOf(
-                LintError(1, 9, "annotation-spacing", fileAnnotationsLineBreaks)
+                LintError(1, 9, "annotation-spacing", AnnotationSpacingRule.ERROR_MESSAGE)
             )
         )
     }
@@ -328,4 +327,102 @@ class AnnotationSpacingRuleTest {
             it.ruleId == "experimental:argument-list-wrapping"
         }
     }
+
+    @Test
+    fun `annotations should not be separated by comments from the annotated construct`() {
+        val code =
+            """
+                @Suppress("DEPRECATION") @Hello
+                /**
+                 * block comment
+                 */
+                class Foo {
+                }
+            """.trimIndent()
+        assertThat(
+            AnnotationSpacingRule().lint(code)
+        ).isEqualTo(
+            listOf(
+                LintError(1, 31, "annotation-spacing", AnnotationSpacingRule.ERROR_MESSAGE)
+            )
+        )
+    }
+
+    @Test
+    fun `annotations should be moved after comments`() {
+        val code =
+            """
+                @Suppress("DEPRECATION") @Hello
+                /**
+                 * block comment
+                 */
+                class Foo {
+                }
+            """.trimIndent()
+        assertThat(
+            AnnotationSpacingRule().format(code)
+        ).isEqualTo(
+            """
+                /**
+                 * block comment
+                 */
+                @Suppress("DEPRECATION") @Hello
+                class Foo {
+                }
+            """.trimIndent()
+        )
+
+        val codeEOL =
+            """
+                @Suppress("DEPRECATION") @Hello
+                // hello
+                class Foo {
+                }
+            """.trimIndent()
+        assertThat(
+            AnnotationSpacingRule().format(codeEOL)
+        ).isEqualTo(
+            """
+                // hello
+                @Suppress("DEPRECATION") @Hello
+                class Foo {
+                }
+            """.trimIndent()
+        )
+    }
+
+    @Test
+    fun `preceding whitespaces are preserved`() {
+        val code =
+            """
+                package a.b.c
+
+                val hello = 5
+
+
+                @Suppress("DEPRECATION") @Hello
+                /**
+                 * block comment
+                 */
+                class Foo {
+                }
+            """.trimIndent()
+        assertThat(
+            AnnotationSpacingRule().format(code)
+        ).isEqualTo(
+            """
+                package a.b.c
+
+                val hello = 5
+
+
+                /**
+                 * block comment
+                 */
+                @Suppress("DEPRECATION") @Hello
+                class Foo {
+                }
+            """.trimIndent()
+        )
+    }
 }