Fixing DiktatRuleSet crashes when there are no package and no imports (#464)

Fixing DiktatRuleSet crashes when there are no package and no imports. Issue #267

### What's done:
* Fixed logic in rules
* Added tests
* Added way to disable several rules in smoke test
* Handle cases when package and/or imports are missing in FileStructureRule
* Code style

Author: petertrr

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/files/FileStructureRule.kt

@@ -10,6 +10,11 @@ import com.pinterest.ktlint.core.ast.ElementType.IMPORT_LIST
 import com.pinterest.ktlint.core.ast.ElementType.KDOC
 import com.pinterest.ktlint.core.ast.ElementType.PACKAGE_DIRECTIVE
 import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
+import com.pinterest.ktlint.core.ast.children
+import com.pinterest.ktlint.core.ast.isPartOfComment
+import com.pinterest.ktlint.core.ast.isWhiteSpace
+import com.pinterest.ktlint.core.ast.nextSibling
+import com.pinterest.ktlint.core.ast.prevSibling
 import org.cqfn.diktat.common.config.rules.RuleConfiguration
 import org.cqfn.diktat.common.config.rules.RulesConfig
 import org.cqfn.diktat.common.config.rules.getCommonConfiguration
@@ -21,7 +26,6 @@ import org.cqfn.diktat.ruleset.constants.Warnings.FILE_UNORDERED_IMPORTS
 import org.cqfn.diktat.ruleset.constants.Warnings.FILE_WILDCARD_IMPORTS
 import org.cqfn.diktat.ruleset.rules.PackageNaming.Companion.PACKAGE_SEPARATOR
 import org.cqfn.diktat.ruleset.utils.StandardPlatforms
-import org.cqfn.diktat.ruleset.utils.findChildBefore
 import org.cqfn.diktat.ruleset.utils.getFileName
 import org.cqfn.diktat.ruleset.utils.handleIncorrectOrder
 import org.cqfn.diktat.ruleset.utils.moveChildBefore
@@ -43,7 +47,6 @@ import org.jetbrains.kotlin.psi.KtImportDirective
  * 5. Ensures there are no wildcard imports
  */
 class FileStructureRule(private val configRules: List<RulesConfig>) : Rule("file-structure") {
-
     private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
     private var isFixMode: Boolean = false
     private var fileName: String = ""
@@ -87,45 +90,51 @@ class FileStructureRule(private val configRules: List<RulesConfig>) : Rule("file
     }
 
     private fun checkCodeBlocksOrderAndEmptyLines(node: ASTNode) {
-        // fixme handle other elements that could be present before package (other comments)
-        val copyrightComment = node.findChildBefore(PACKAGE_DIRECTIVE, BLOCK_COMMENT)
-        val headerKdoc = node.findChildBefore(PACKAGE_DIRECTIVE, KDOC)
-        val fileAnnotations = node.findChildByType(FILE_ANNOTATION_LIST)
-        // PACKAGE_DIRECTIVE node is always present in regular kt files and might be absent in kts
-        // kotlin compiler itself enforces it's position in the file if it is present
-        // fixme: handle cases when this node is not present
-        val packageDirectiveNode = (node.psi as KtFile).packageDirective?.node ?: return
-        // fixme: find cases when node.psi.importLists.size > 1, handle cases when it's not present
-        val importsList = (node.psi as KtFile).importList?.node ?: return
+        // PACKAGE_DIRECTIVE node is always present in regular kt files and might be absent in kts.
+        // Kotlin compiler itself enforces it's position in the file if it is present.
+        // If package directive is missing in .kt file (default package), the node is still present in the AST.
+        // fixme: find and handle cases when this node is not present (.kts files?)
+        val packageDirectiveNode = (node.psi as KtFile).packageDirective?.takeUnless { it.isRoot }?.node
+        // fixme: find cases when node.psi.importLists.size > 1, handle cases when it's not present (null)
+        val importsList = (node.psi as KtFile).importList?.takeIf { it.imports.isNotEmpty() }?.node
+
+        // this node will be an anchor with respect to which we will look for all other nodes
+        val firstCodeNode = packageDirectiveNode
+            ?: importsList
+            ?: node.children().firstOrNull {
+                // taking nodes with actual code
+                !it.isWhiteSpace() && !it.isPartOfComment() &&
+                        // but not the ones we are going to move
+                        it.elementType != FILE_ANNOTATION_LIST&& it.elementType != IMPORT_LIST &&
+                        // if we are here, then package is default and we don't need to select the empty PACKAGE_DIRECTIVE node
+                        it.elementType != PACKAGE_DIRECTIVE
+            }
+            ?: return  // at this point it means the file contains only comments
+        // fixme: handle other elements that could be present before package (other comments)
+        var copyrightComment = firstCodeNode.prevSibling { it.elementType == BLOCK_COMMENT }
+        var headerKdoc = firstCodeNode.prevSibling { it.elementType == KDOC }
+        var fileAnnotations = node.findChildByType(FILE_ANNOTATION_LIST)
 
         // checking order
         listOfNotNull(copyrightComment, headerKdoc, fileAnnotations).handleIncorrectOrder({
-            getSiblingBlocks(copyrightComment, headerKdoc, fileAnnotations, packageDirectiveNode)
+            getSiblingBlocks(copyrightComment, headerKdoc, fileAnnotations, firstCodeNode)
         }) { astNode, beforeThisNode ->
-            FILE_INCORRECT_BLOCKS_ORDER.warnAndFix(
-                configRules,
-                emitWarn,
-                isFixMode,
-                astNode.text.lines().first(),
-                astNode.startOffset,
-                astNode
-            ) {
-                node.moveChildBefore(astNode, beforeThisNode, true)
+            FILE_INCORRECT_BLOCKS_ORDER.warnAndFix(configRules, emitWarn, isFixMode, astNode.text.lines().first(), astNode.startOffset, astNode) {
+                val result = node.moveChildBefore(astNode, beforeThisNode, true)
+                result.newNodes.first().run {
+                    // reassign values to the nodes that could have been moved
+                    when (elementType) {
+                        BLOCK_COMMENT -> copyrightComment = this
+                        KDOC -> headerKdoc = this
+                        FILE_ANNOTATION_LIST -> fileAnnotations = this
+                    }
+                }
                 astNode.treeNext?.let { node.replaceChild(it, PsiWhiteSpaceImpl("\n\n")) }
             }
         }
 
         // checking empty lines
-        arrayOf(copyrightComment, headerKdoc, fileAnnotations, packageDirectiveNode, importsList).forEach { astNode ->
-            astNode?.treeNext?.apply {
-                if (elementType == WHITE_SPACE && text.count { it == '\n' } != 2) {
-                    FILE_NO_BLANK_LINE_BETWEEN_BLOCKS.warnAndFix(configRules, emitWarn, isFixMode, astNode.text.lines().first(),
-                        astNode.startOffset, astNode) {
-                        (this as LeafPsiElement).replaceWithText("\n\n${text.replace("\n", "")}")
-                    }
-                }
-            }
-        }
+        insertNewlinesBetweenBlocks(listOf(copyrightComment, headerKdoc, fileAnnotations, packageDirectiveNode, importsList))
     }
 
     @Suppress("UnsafeCallOnNullableType")
@@ -186,6 +195,23 @@ class FileStructureRule(private val configRules: List<RulesConfig>) : Rule("file
             }
     }
 
+    private fun insertNewlinesBetweenBlocks(blocks: List<ASTNode?>) {
+        blocks.forEach { astNode ->
+            // if package directive is missing, node is still present, but it's text is empty, so we need to check treeNext to get meaningful results
+            astNode?.nextSibling { it.text.isNotEmpty() }?.apply {
+                if (elementType == WHITE_SPACE && text.count { it == '\n' } != 2) {
+                    FILE_NO_BLANK_LINE_BETWEEN_BLOCKS.warnAndFix(configRules, emitWarn, isFixMode, astNode.text.lines().first(),
+                        astNode.startOffset, astNode) {
+                        (this as LeafPsiElement).replaceWithText("\n\n${text.replace("\n", "")}")
+                    }
+                }
+            }
+        }
+    }
+
+    /**
+     * @return a pair of nodes between which [this] node should be placed, i.e. after the first and before the second element
+     */
     private fun ASTNode.getSiblingBlocks(
         copyrightComment: ASTNode?,
         headerKdoc: ASTNode?,

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/kdoc/CommentsFormatting.kt

@@ -22,6 +22,7 @@ import com.pinterest.ktlint.core.ast.ElementType.THEN
 import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT_LIST
 import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
 import com.pinterest.ktlint.core.ast.isWhiteSpace
+import com.pinterest.ktlint.core.ast.prevSibling
 import org.cqfn.diktat.common.config.rules.RuleConfiguration
 import org.cqfn.diktat.common.config.rules.RulesConfig
 import org.cqfn.diktat.common.config.rules.getRuleConfig
@@ -38,16 +39,16 @@ import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
 /**
  * This class handles rule 2.6
  * Part 1:
- * there must be 1 space between the comment character and the content of the comment;
- * there must be a newline between a Kdoc and the previous code above and no blank lines after.
- * No need to add a blank line before a first comment in this particular name space (code block), for example between function declaration and first comment in a function body.
+ * * there must be 1 space between the comment character and the content of the comment;
+ * * there must be a newline between a Kdoc and the previous code above and no blank lines after.
+ * * No need to add a blank line before a first comment in this particular name space (code block), for example between function declaration
+ *   and first comment in a function body.
  *
  * Part 2:
- * Leave one single space between the comment on the right side of the code and the code.
- * Comments in if else should be inside code blocks. Exception: General if comment
+ * * Leave one single space between the comment on the right side of the code and the code.
+ * * Comments in if else should be inside code blocks. Exception: General if comment
  */
 class CommentsFormatting(private val configRules: List<RulesConfig>) : Rule("kdoc-comments-codeblocks-formatting") {
-
     companion object {
         private const val MAX_SPACES = 1
         private const val APPROPRIATE_COMMENT_SPACES = 1
@@ -65,22 +66,15 @@ class CommentsFormatting(private val configRules: List<RulesConfig>) : Rule("kdo
         val configuration = CommentsFormattingConfiguration(
                 configRules.getRuleConfig(COMMENT_WHITE_SPACE)?.configuration ?: mapOf())
 
-
         when (node.elementType) {
             CLASS, FUN, PROPERTY -> {
                 checkBlankLineAfterKdoc(node, EOL_COMMENT)
                 checkBlankLineAfterKdoc(node, KDOC)
                 checkBlankLineAfterKdoc(node, BLOCK_COMMENT)
             }
-            IF -> {
-                handleIfElse(node)
-            }
-            EOL_COMMENT, BLOCK_COMMENT -> {
-                handleEolAndBlockComments(node, configuration)
-            }
-            KDOC -> {
-                handleKdocComments(node, configuration)
-            }
+            IF -> handleIfElse(node)
+            EOL_COMMENT, BLOCK_COMMENT -> handleEolAndBlockComments(node, configuration)
+            KDOC -> handleKdocComments(node, configuration)
         }
     }
 
@@ -257,15 +251,14 @@ class CommentsFormatting(private val configRules: List<RulesConfig>) : Rule("kdo
 
     private fun checkClassComment(node: ASTNode) {
         if (isFirstComment(node)) {
-            if (node.isBlockOrClassBody())
+            if (node.isBlockOrClassBody()) {
                 checkFirstCommentSpaces(node)
-            else
+            } else {
                 checkFirstCommentSpaces(node.treeParent)
-
-            return
-        }
-
-        if (node.treeParent.elementType != FILE && !node.treeParent.treePrev.isWhiteSpace()) {
+            }
+        } else if (node.treeParent.elementType != FILE && !node.treeParent.treePrev.isWhiteSpace()) {
+            // fixme: we might face more issues because newline node is inserted in a wrong place which causes consecutive
+            //  white spaces to be split among nodes on different levels. But this requires investigation.
             WRONG_NEWLINES_AROUND_KDOC.warnAndFix(configRules, emitWarn, isFixMode, node.text, node.startOffset, node) {
                 node.treeParent.treeParent.addChild(PsiWhiteSpaceImpl("\n"), node.treeParent)
             }
@@ -289,25 +282,26 @@ class CommentsFormatting(private val configRules: List<RulesConfig>) : Rule("kdo
         }
     }
 
-
     private fun isFirstComment(node: ASTNode): Boolean {
-        // In case when comment is inside of a function or class
-        if (node.isBlockOrClassBody()) {
-            return if (node.treePrev.isWhiteSpace())
+        return if (node.isBlockOrClassBody()) {
+            // In case when comment is inside of a function or class
+            if (node.treePrev.isWhiteSpace()) {
                 node.treePrev.treePrev.elementType == LBRACE
-            else
-                node.treePrev == null || node.treePrev.elementType == LBRACE // null is handled for functional literal
+            } else {
+                node.treePrev == null || node.treePrev.elementType == LBRACE  // null is handled for functional literal
+            }
+        } else if (node.treeParent?.treeParent?.elementType == FILE && node.treeParent.prevSibling { it.text.isNotBlank() } == null) {
+            // `treeParent` is the first not-empty node in a file
+            true
+        } else if (node.treeParent.elementType != FILE && node.treeParent.treePrev != null &&
+                node.treeParent.treePrev.treePrev != null) {
+            // When comment inside of a PROPERTY
+            node.treeParent.treePrev.treePrev.elementType == LBRACE
+        } else {
+            node.treeParent.getAllChildrenWithType(node.elementType).first() == node
         }
-
-        // When comment inside of a PROPERTY
-        if (node.treeParent.elementType != FILE && node.treeParent.treePrev != null &&
-                node.treeParent.treePrev.treePrev != null)
-            return node.treeParent.treePrev.treePrev.elementType == LBRACE
-
-        return node.treeParent.getAllChildrenWithType(node.elementType).first() == node
     }
 
-
     private fun ASTNode.isBlockOrClassBody() : Boolean = treeParent.elementType == BLOCK || treeParent.elementType == CLASS_BODY
 
     class CommentsFormattingConfiguration(config: Map<String, String>) : RuleConfiguration(config) {

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/AstNodeUtils.kt

@@ -29,6 +29,7 @@ import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet
 import org.jetbrains.kotlin.psi.KtAnnotationEntry
 import org.jetbrains.kotlin.psi.KtClass
 import org.jetbrains.kotlin.psi.KtIfExpression
+import org.jetbrains.kotlin.psi.psiUtil.children
 import org.jetbrains.kotlin.psi.psiUtil.parents
 import org.jetbrains.kotlin.psi.psiUtil.siblings
 import org.slf4j.Logger
@@ -392,11 +393,10 @@ fun ASTNode.moveChildBefore(
     beforeThisNode: ASTNode?,
     withNextNode: Boolean = false
 ): ReplacementResult {
-    require(childToMove in getChildren(null)) { "can only move child of this node" }
-    require(beforeThisNode?.let { it in getChildren(null) }
-        ?: true) { "can only place node before another child of this node" }
+    require(childToMove in children()) { "can only move child of this node" }
+    require(beforeThisNode == null || beforeThisNode in children()) { "can only place node before another child of this node" }
     val movedChild = childToMove.clone() as ASTNode
-    val nextMovedChild = childToMove.treeNext?.let { it.clone() as ASTNode }?.takeIf { withNextNode }
+    val nextMovedChild = childToMove.treeNext?.takeIf { withNextNode }?.let { it.clone() as ASTNode }
     val nextOldChild = childToMove.treeNext.takeIf { withNextNode && it != null }
     addChild(movedChild, beforeThisNode)
     if (nextMovedChild != null && nextOldChild != null) {
@@ -450,6 +450,11 @@ fun ASTNode.areChildrenBeforeGroup(children: List<ASTNode>, group: List<ASTNode>
     return children.map { getChildren(null).indexOf(it) }.max()!! < group.map { getChildren(null).indexOf(it) }.min()!!
 }
 
+/**
+ * A function that rearranges nodes in a [this] list.
+ * @param getSiblingBlocks a function which returns nodes that should be before and after the current node
+ * @param incorrectPositionHandler function that moves the current node with respect to node before which in should be placed
+ */
 fun List<ASTNode>.handleIncorrectOrder(
     getSiblingBlocks: ASTNode.() -> Pair<ASTNode?, ASTNode>,
     incorrectPositionHandler: (nodeToMove: ASTNode, beforeThisNode: ASTNode) -> Unit

diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter2/CommentsFormattingFixTest.kt

@@ -29,4 +29,10 @@ class CommentsFormattingFixTest: FixTestBase("test/paragraph2/kdoc/", ::Comments
     fun `test example from code style`() {
         fixAndCompare("KdocCodeBlockFormattingExampleExpected.kt", "KdocCodeBlockFormattingExampleTest.kt")
     }
+
+    @Test
+    @Tag(WRONG_NEWLINES_AROUND_KDOC)
+    fun `regression - should not insert newline before the first comment in a file`() {
+        fixAndCompare("NoPackageNoImportExpected.kt", "NoPackageNoImportTest.kt")
+    }
 }

diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/FileStructureRuleTestFix.kt

@@ -65,4 +65,16 @@ class FileStructureRuleTestFix : FixTestBase("test/paragraph3/file_structure", :
             )
         )
     }
+
+    @Test
+    @Tag(WarningNames.FILE_UNORDERED_IMPORTS)
+    fun `should still work with default package and some imports`() {
+        fixAndCompare("DefaultPackageWithImportsExpected.kt", "DefaultPackageWithImportsTest.kt")
+    }
+
+    @Test
+    @Tag(WarningNames.FILE_UNORDERED_IMPORTS)
+    fun `should still work with default package and no imports`() {
+        fixAndCompare("NoImportNoPackageExpected.kt", "NoImportNoPackageTest.kt")
+    }
 }