Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for FileStructureRule #636

Merged
merged 9 commits into from
Dec 14, 2020
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about EOL comments? Do we need to handle it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We state the following structure (as per our code style): copyright information is placed in a block comment, general file comment is placed in KDoc. All other nodes that can appear before code, including EOL comments, are captured here.

}
.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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to add comments into KDoc?

Copy link
Member Author

@petertrr petertrr Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check that EOL comments are moved correctly here, why would I need KDoc instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would be better to have one KDOC than KDOC and EOL, but I think so well)

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why COMMENT_BLOCK doesn't move exactly like the comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a BLOCK_COMMENT before code, it's considered as containing copyright information. But you are right, I need to check copyright keywords too, like we do in some other rule.

// and some more
/**
* This is an example
*/

package org.cqfn.diktat.example

import org.cqfn.diktat.example.Foo

class Example