Skip to content

Commit

Permalink
Fixes for FileStructureRule (#636)
Browse files Browse the repository at this point in the history
### What's done:
* Fixed logic
* Added tests
  • Loading branch information
petertrr authored Dec 14, 2020
1 parent 6ea31f7 commit d690c0f
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import org.cqfn.diktat.ruleset.constants.Warnings.HEADER_MISSING_OR_WRONG_COPYRI
import org.cqfn.diktat.ruleset.constants.Warnings.HEADER_NOT_BEFORE_PACKAGE
import org.cqfn.diktat.ruleset.constants.Warnings.HEADER_WRONG_FORMAT
import org.cqfn.diktat.ruleset.constants.Warnings.WRONG_COPYRIGHT_YEAR
import org.cqfn.diktat.ruleset.utils.copyrightWords
import org.cqfn.diktat.ruleset.utils.findChildAfter
import org.cqfn.diktat.ruleset.utils.findChildBefore
import org.cqfn.diktat.ruleset.utils.getAllChildrenWithType
Expand Down Expand Up @@ -40,7 +41,6 @@ import java.time.LocalDate
*/
@Suppress("ForbiddenComment")
class HeaderCommentRule(private val configRules: List<RulesConfig>) : Rule("header-comment") {
private val copyrightWords = setOf("copyright", "版权")
private var isFixMode: Boolean = false
private lateinit var emitWarn: EmitType

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ 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.copyrightWords
import org.cqfn.diktat.ruleset.utils.handleIncorrectOrder
import org.cqfn.diktat.ruleset.utils.moveChildBefore

Expand All @@ -37,6 +38,7 @@ import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet
import org.jetbrains.kotlin.name.Name
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.KtImportDirective
import org.jetbrains.kotlin.psi.psiUtil.siblings

/**
* Visitor for checking internal file structure.
Expand Down Expand Up @@ -100,17 +102,17 @@ class FileStructureRule(private val configRules: List<RulesConfig>) : Rule("file
return hasCode
}

@Suppress("TOO_LONG_FUNCTION")
@Suppress("ComplexMethod", "TOO_LONG_FUNCTION")
private fun checkCodeBlocksOrderAndEmptyLines(node: ASTNode) {
// PACKAGE_DIRECTIVE node is always present in regular kt files and might be absent in kts.
// From KtFile.kt: 'scripts have no package directive, all other files must have package directives'.
// 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)
// There is a private property node.psi.importLists, but it's size can't be > 1 in valid kotlin code. It exists to help in situations
// when, e.g. merge conflict marker breaks the imports list. We shouldn't handle this situation here.
val importsList = (node.psi as KtFile)
.importList
?.takeIf { it.imports.isNotEmpty() }
Expand All @@ -123,19 +125,34 @@ class FileStructureRule(private val configRules: List<RulesConfig>) : Rule("file
// 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 != FILE_ANNOTATION_LIST &&
// if we are here, then IMPORT_LIST either is not present in the AST, or is empty. Either way, we don't need to select it.
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)
// We consider the first block comment of the file to be the one that possibly contains copyright information.
var copyrightComment = firstCodeNode.prevSibling { it.elementType == BLOCK_COMMENT }
?.takeIf { blockCommentNode ->
copyrightWords.any { blockCommentNode.text.contains(it, ignoreCase = true) }
}
var headerKdoc = firstCodeNode.prevSibling { it.elementType == KDOC }
// Annotations with target`file` can only be placed before `package` directive.
var fileAnnotations = node.findChildByType(FILE_ANNOTATION_LIST)
// We also collect all other elements that are placed on top of the file.
// These may be other comments, so we just place them before the code starts.
val otherNodesBeforeCode = firstCodeNode.siblings(forward = false)
.filterNot {
it.isWhiteSpace() ||
it == copyrightComment || it == headerKdoc || it == fileAnnotations
}
.toList()
.reversed()

// checking order
listOfNotNull(copyrightComment, headerKdoc, fileAnnotations).handleIncorrectOrder({
getSiblingBlocks(copyrightComment, headerKdoc, fileAnnotations, firstCodeNode)
listOfNotNull(copyrightComment, headerKdoc, fileAnnotations, *otherNodesBeforeCode.toTypedArray()).handleIncorrectOrder({
getSiblingBlocks(copyrightComment, headerKdoc, fileAnnotations, firstCodeNode, otherNodesBeforeCode)
}) { astNode, beforeThisNode ->
FILE_INCORRECT_BLOCKS_ORDER.warnAndFix(configRules, emitWarn, isFixMode, astNode.text.lines().first(), astNode.startOffset, astNode) {
val result = node.moveChildBefore(astNode, beforeThisNode, true)
Expand Down Expand Up @@ -237,12 +254,13 @@ class FileStructureRule(private val configRules: List<RulesConfig>) : Rule("file
copyrightComment: ASTNode?,
headerKdoc: ASTNode?,
fileAnnotations: ASTNode?,
packageDirectiveNode: ASTNode
): Pair<ASTNode?, ASTNode> = when (elementType) {
BLOCK_COMMENT -> null to listOfNotNull(headerKdoc, fileAnnotations, packageDirectiveNode).first()
KDOC -> copyrightComment to (fileAnnotations ?: packageDirectiveNode)
FILE_ANNOTATION_LIST -> (headerKdoc ?: copyrightComment) to packageDirectiveNode
else -> error("Only BLOCK_COMMENT, KDOC and FILE_ANNOTATION_LIST are valid inputs.")
firstCodeNode: ASTNode,
otherNodesBeforeFirst: List<ASTNode>
): Pair<ASTNode?, ASTNode> = when (this) {
copyrightComment -> null to listOfNotNull(headerKdoc, fileAnnotations, otherNodesBeforeFirst.firstOrNull(), firstCodeNode).first()
headerKdoc -> copyrightComment to (fileAnnotations ?: otherNodesBeforeFirst.firstOrNull() ?: firstCodeNode)
fileAnnotations -> (headerKdoc ?: copyrightComment) to (otherNodesBeforeFirst.firstOrNull() ?: firstCodeNode)
else -> (headerKdoc ?: copyrightComment) to firstCodeNode
}

@Suppress("TYPE_ALIAS")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ internal const val SET_PREFIX = "set"
val emptyBlockList = listOf(LBRACE, WHITE_SPACE, SEMICOLON, RBRACE)

val commentType = listOf(BLOCK_COMMENT, EOL_COMMENT, KDOC)
val copyrightWords = setOf("copyright", "版权")

internal const val EMPTY_BLOCK_TEXT = "{}"

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* Various utility methods to work with kotlin AST
* FixMe: fix suppressed inspections on KDocs
*/

// todo fix inspections on KDocs
@file:Suppress("FILE_NAME_MATCH_CLASS", "KDOC_WITHOUT_RETURN_TAG", "KDOC_WITHOUT_PARAM_TAG")

package org.cqfn.diktat.ruleset.utils
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,10 @@ class FileStructureRuleFixTest : FixTestBase("test/paragraph3/file_structure", :
fun `should still work with default package and no imports`() {
fixAndCompare("NoImportNoPackageExpected.kt", "NoImportNoPackageTest.kt")
}

@Test
@Tag(WarningNames.FILE_UNORDERED_IMPORTS)
fun `should move other comments before package node`() {
fixAndCompare("OtherCommentsExpected.kt", "OtherCommentsTest.kt")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,84 @@ class FileStructureRuleTest : LintTestBase(::FileStructureRule) {
""".trimMargin(), rulesConfigList = rulesConfigListWildCardImports
)
}

@Test
@Tag(WarningNames.FILE_INCORRECT_BLOCKS_ORDER)
fun `should warn if there are other misplaced comments before package - positive example`() {
lintMethod(
"""
|/**
| * This is an example
| */
|
|// some notes on this file
|package org.cqfn.diktat.example
|
|import org.cqfn.diktat.example.Foo
|
|class Example
""".trimMargin()
)
}

@Test
@Tag(WarningNames.FILE_INCORRECT_BLOCKS_ORDER)
fun `should warn if there are other misplaced comments before package`() {
lintMethod(
"""
|// some notes on this file
|/**
| * This is an example
| */
|
|package org.cqfn.diktat.example
|
|import org.cqfn.diktat.example.Foo
|
|class Example
""".trimMargin(),
LintError(1, 1, ruleId, "${FILE_INCORRECT_BLOCKS_ORDER.warnText()} // some notes on this file", true),
LintError(2, 1, ruleId, "${FILE_INCORRECT_BLOCKS_ORDER.warnText()} /**", true),
)
}

@Test
@Tag(WarningNames.FILE_INCORRECT_BLOCKS_ORDER)
fun `block comment should be detected as copyright - positive example`() {
lintMethod(
"""
|/*
| * Copyright Example Inc. (c)
| */
|
|@file:Annotation
|
|package org.cqfn.diktat.example
|
|import org.cqfn.diktat.example.Foo
|
|class Example
""".trimMargin()
)
}

@Test
@Tag(WarningNames.FILE_INCORRECT_BLOCKS_ORDER)
fun `block comment shouldn't be detected as copyright without keywords`() {
lintMethod(
"""
|/*
| * Just a regular block comment
| */
|@file:Annotation
|
|package org.cqfn.diktat.example
|
|import org.cqfn.diktat.example.Foo
|
|class Example
""".trimMargin(),
LintError(4, 1, ruleId, "${FILE_INCORRECT_BLOCKS_ORDER.warnText()} @file:Annotation", true)
)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* This is an example
*/

// some notes on this file
// and some more
package org.cqfn.diktat.example

import org.cqfn.diktat.example.Foo

class Example
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// some notes on this file
// and some more
/**
* This is an example
*/

package org.cqfn.diktat.example

import org.cqfn.diktat.example.Foo

class Example

0 comments on commit d690c0f

Please sign in to comment.