Skip to content

Commit

Permalink
Refactor AnnotationRule to improve maintainability (#1574)
Browse files Browse the repository at this point in the history 
* Refactor AnnotationRule to improve maintainability

Replace (biggest part of) processing of MODIFIER_LIST and FILE_ANNOTATION_LIST with processing of
individual ANNOTATION_ENTRY. This allows ANNOTATION_ENTRY to be followed by EOL_COMMENT plus
another ANNOTATION_ENTRY.

Reword the violations to be more clear.

Annotation with parameter(s) on same line as annotated construct is rewritten to
annotation on a separate line directly as otherwise a lint violation would remain
after the first format.

Closes #1539
  • Loading branch information
@paul-dingemans
paul-dingemans authored Aug 13, 2022
1 parent 6fdb80e commit fb7f5b4
Show file tree
Hide file tree
Showing 3 changed files with 298 additions and 121 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,14 @@ Although, Ktlint 0.47.0 falls back on property `disabled_rules` whenever `ktlint
* Fix multiline if-statements `multiline-if-else` ([#828](https://github.com/pinterest/ktlint/issues/828))
* Prevent class cast exception on ".editorconfig" property `ktlint_code_style` ([#1559](https://github.com/pinterest/ktlint/issues/1559))
* Handle trailing comma in enums `trailing-comma` ([#1542](https://github.com/pinterest/ktlint/pull/1542))
* Allow EOL comment after annotation ([#1539](https://github.com/pinterest/ktlint/issues/1539))
* Split rule `trailing-comma` into `trailing-comma-on-call-site` and `trailing-comma-on-declaration-site` ([#1555](https://github.com/pinterest/ktlint/pull/1555))

### Changed

* Print an error message and return with non-zero exit code when no files are found that match with the globs ([#629](https://github.com/pinterest/ktlint/issue/629)).
* Invoke callback on `format` function for all errors including errors that are autocorrected ([#1491](https://github.com/pinterest/ktlint/issues/1491))
* Improve rule `annotation` ([#1574](https://github.com/pinterest/ktlint/pull/1574))
* Rename `.editorconfig` property `disabled_rules` to `ktlint_disabled_rules` ([#701](https://github.com/pinterest/ktlint/issues/701))

### Removed
Expand Down
296 changes: 209 additions & 87 deletions ...-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/AnnotationRule.kt
View file
Original file line number Diff line number Diff line change
@@ -1,30 +1,38 @@
package com.pinterest.ktlint.ruleset.standard

import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType.ANNOTATION_ENTRY
import com.pinterest.ktlint.core.ast.ElementType.FILE_ANNOTATION_LIST
import com.pinterest.ktlint.core.ast.ElementType.MODIFIER_LIST
import com.pinterest.ktlint.core.ast.ElementType.TYPE_ARGUMENT_LIST
import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT
import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT_LIST
import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER
import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
import com.pinterest.ktlint.core.ast.children
import com.pinterest.ktlint.core.ast.firstChildLeafOrSelf
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.lastChildLeafOrSelf
import com.pinterest.ktlint.core.ast.lineNumber
import com.pinterest.ktlint.core.ast.nextCodeLeaf
import com.pinterest.ktlint.core.ast.nextLeaf
import com.pinterest.ktlint.core.ast.nextSibling
import com.pinterest.ktlint.core.ast.prevSibling
import com.pinterest.ktlint.core.ast.upsertWhitespaceBeforeMe
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.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl
import org.jetbrains.kotlin.descriptors.annotations.AnnotationUseSiteTarget
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.KtScript
import org.jetbrains.kotlin.psi.psiUtil.children
import org.jetbrains.kotlin.psi.psiUtil.endOffset
import org.jetbrains.kotlin.psi.psiUtil.getNextSiblingIgnoringWhitespaceAndComments
import org.jetbrains.kotlin.psi.psiUtil.nextLeaf
import org.jetbrains.kotlin.psi.psiUtil.leaves
import org.jetbrains.kotlin.psi.psiUtil.siblings
import org.jetbrains.kotlin.utils.addToStdlib.safeAs

/**
* Ensures multiple annotations are not on the same line as the annotated declaration. Also ensures that annotations
Expand All @@ -34,117 +42,236 @@ import org.jetbrains.kotlin.psi.psiUtil.nextLeaf
*
* @see [AnnotationSpacingRule] for white space rules. Moved since
*/
class AnnotationRule : Rule("annotation") {

companion object {
const val multipleAnnotationsOnSameLineAsAnnotatedConstructErrorMessage =
"Multiple annotations should not be placed on the same line as the annotated construct"
const val annotationsWithParametersAreNotOnSeparateLinesErrorMessage =
"Annotations with parameters should all be placed on separate lines prior to the annotated construct"
const val fileAnnotationsShouldBeSeparated =
"File annotations should be separated from file contents with a blank line"
}

public class AnnotationRule : Rule("annotation") {
override fun beforeVisitChildNodes(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {
if (node.elementType != MODIFIER_LIST && node.elementType != FILE_ANNOTATION_LIST) {
return
when (node.elementType) {
FILE_ANNOTATION_LIST -> {
visitFileAnnotationList(node, emit, autoCorrect)
}
ANNOTATION_ENTRY ->
visitAnnotationEntry(node, emit, autoCorrect)
}
}

private fun visitAnnotationEntry(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean
) {
require(node.elementType == ANNOTATION_ENTRY)

val annotations =
node.children()
.mapNotNull { it.psi as? KtAnnotationEntry }
.toList()
if (annotations.isEmpty()) {
return
if (node.isAnnotationEntryWithValueArgumentList() &&
node.treeParent.treeParent.elementType != VALUE_PARAMETER && // fun fn(@Ann("blah") a: String)
node.treeParent.treeParent.elementType != VALUE_ARGUMENT && // fn(@Ann("blah") "42")
!node.isPartOf(TYPE_ARGUMENT_LIST) && // val property: Map<@Ann("blah") String, Int>
node.isNotReceiverTargetAnnotation()
) {
checkForAnnotationWithParameterToBePlacedOnSeparateLine(node, emit, autoCorrect)
}

// Join the nodes that immediately follow the annotations (whitespace), then add the final whitespace
// if it's not a child of root. This happens when a new line separates the annotations from the annotated
// construct. In the following example, there are no whitespace children of root, but root's next sibling is the
// new line whitespace.
//
// @JvmField
// val s: Any
//
val whiteSpaces = (annotations.asSequence().map { it.nextSibling } + node.treeNext)
.filterIsInstance<PsiWhiteSpace>()
.take(annotations.size)
.toList()

val noWhiteSpaceAfterAnnotation = node.elementType != FILE_ANNOTATION_LIST &&
(whiteSpaces.isEmpty() || whiteSpaces.last().nextSibling is KtAnnotationEntry)
if (noWhiteSpaceAfterAnnotation) {
if ((node.isFollowedByOtherAnnotationEntry() && node.isOnSameLineAsNextAnnotationEntry()) ||
(node.isPrecededByOtherAnnotationEntry() && node.isOnSameLineAsAnnotatedConstruct())
) {
checkForAnnotationToBePlacedOnSeparateLine(node, emit, autoCorrect)
}

if (node.isPrecededByOtherAnnotationEntry() && node.isOnSameLineAsAnnotatedConstruct()) {
checkForMultipleAnnotationsOnSameLineAsAnnotatedConstruct(node, emit, autoCorrect)
}
}

private fun checkForAnnotationWithParameterToBePlacedOnSeparateLine(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean
) {
if (node.isPrecededByOtherAnnotationEntry() && node.isOnSameLineAsPreviousAnnotationEntry()) {
emit(
annotations.last().endOffset - 1,
"Missing spacing after ${annotations.last().text}",
node.startOffset,
"Annotation with parameter(s) should be placed on a separate line prior to the annotated construct",
true
)
if (autoCorrect) {
(annotations.last().nextLeaf() as? LeafPsiElement)?.upsertWhitespaceBeforeMe(" ")
node
.firstChildLeafOrSelf()
.safeAs<LeafPsiElement>()
?.upsertWhitespaceBeforeMe(" ")
}
}

val multipleAnnotationsOnSameLineAsAnnotatedConstruct =
annotations.size > 1 && !whiteSpaces.last().textContains('\n') && doesNotEndWithAComment(whiteSpaces)
if (multipleAnnotationsOnSameLineAsAnnotatedConstruct) {
if (node.isOnSameLineAsNextAnnotationEntryOrAnnotatedConstruct()) {
emit(
annotations.first().node.startOffset,
multipleAnnotationsOnSameLineAsAnnotatedConstructErrorMessage,
node.startOffset,
"Annotation with parameter(s) should be placed on a separate line prior to the annotated construct",
true
)
if (autoCorrect) {
(whiteSpaces.last() as LeafPsiElement).rawReplaceWithText(getNewlineWithIndent(node))
node
.lastChildLeafOrSelf()
.nextLeaf()
.safeAs<LeafPsiElement>()
?.let {
if (it.elementType == WHITE_SPACE) {
it.replaceWithText(getNewlineWithIndent(node.treeParent))
} else {
it.rawInsertBeforeMe(
PsiWhiteSpaceImpl(getNewlineWithIndent(node.treeParent))
)
}
}
}
}
}

private fun checkForMultipleAnnotationsOnSameLineAsAnnotatedConstruct(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean
) {
if (node.isLastAnnotationEntry()) {
val noAnnotationWithParameters =
node
.siblings(forward = false)
.none { it.isAnnotationEntryWithValueArgumentList() }
if (noAnnotationWithParameters) {
emit(
node.treeParent.startOffset,
"Multiple annotations should not be placed on the same line as the annotated construct",
true
)
if (autoCorrect) {
node
.lastChildLeafOrSelf()
.nextCodeLeaf()
.safeAs<LeafPsiElement>()
?.upsertWhitespaceBeforeMe(getNewlineWithIndent(node.treeParent))
}
}
}
}

val annotationsWithParametersAreNotOnSeparateLines =
annotations.any { it.valueArgumentList != null } &&
!whiteSpaces.all { it.textContains('\n') } &&
doesNotEndWithAComment(whiteSpaces) &&
node.treeParent.elementType != VALUE_PARAMETER && // fun fn(@Ann("blah") a: String)
node.treeParent.elementType != VALUE_ARGUMENT && // fn(@Ann("blah") "42")
!node.isPartOf(TYPE_ARGUMENT_LIST) && // val property: Map<@Ann("blah") String, Int>
annotations.none { it.useSiteTarget?.getAnnotationUseSiteTarget() == AnnotationUseSiteTarget.RECEIVER }
if (annotationsWithParametersAreNotOnSeparateLines) {
private fun checkForAnnotationToBePlacedOnSeparateLine(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean
) {
val isFollowedWithAnnotationHavingValueArgumentList =
node
.siblings(forward = true)
.any { it.isAnnotationEntryWithValueArgumentList() }
val isPrecededWithAnnotationOnOtherLine =
node
.siblings(forward = false)
.any { it.isWhiteSpaceWithNewline() }
if (isFollowedWithAnnotationHavingValueArgumentList || isPrecededWithAnnotationOnOtherLine) {
emit(
annotations.first().node.startOffset,
annotationsWithParametersAreNotOnSeparateLinesErrorMessage,
node.startOffset,
"Annotation must be placed on separate line",
true
)
if (autoCorrect) {
whiteSpaces.forEach {
(it as LeafPsiElement).rawReplaceWithText(getNewlineWithIndent(node))
}
node
.lastChildLeafOrSelf()
.nextLeaf()
.safeAs<LeafPsiElement>()
?.let {
if (it.elementType == WHITE_SPACE) {
it.replaceWithText(getNewlineWithIndent(node.treeParent))
} else {
it.upsertWhitespaceBeforeMe(
getNewlineWithIndent(node.treeParent)
)
}
}
}
}
}

if (node.elementType == FILE_ANNOTATION_LIST) {
val lineNumber = node.lineNumber()
val next = node.nextSibling { it.textLength > 0 }?.let { next ->
val psi = next.psi
((psi as? KtScript)?.blockExpression?.firstChildNode ?: next).nextSibling {
!it.isWhiteSpace() && !(it.isPartOfComment() && it.lineNumber() == lineNumber)
}
private fun ASTNode.isNotReceiverTargetAnnotation() =
getAnnotationUseSiteTarget() != AnnotationUseSiteTarget.RECEIVER

private fun ASTNode.getAnnotationUseSiteTarget() =
psi
.safeAs<KtAnnotationEntry>()
?.useSiteTarget
?.getAnnotationUseSiteTarget()

private fun ASTNode.isAnnotationEntryWithValueArgumentList() =
getAnnotationEntryValueArgumentList() != null

private fun ASTNode.getAnnotationEntryValueArgumentList() =
takeIf { it.elementType == ANNOTATION_ENTRY }
?.findChildByType(VALUE_ARGUMENT_LIST)

private fun ASTNode.isLastAnnotationEntry() =
treeParent
.children()
.lastOrNull { it.elementType == ANNOTATION_ENTRY }
.let { it == this }

private fun ASTNode.isPrecededByOtherAnnotationEntry() =
siblings(forward = false).any { it.elementType == ANNOTATION_ENTRY }

private fun ASTNode.isOnSameLineAsPreviousAnnotationEntry() =
siblings(forward = false)
.takeWhile { it.elementType != ANNOTATION_ENTRY }
.none { it.isWhiteSpaceWithNewline() }

private fun ASTNode.isFollowedByOtherAnnotationEntry() =
siblings(forward = true).any { it.elementType == ANNOTATION_ENTRY }

private fun ASTNode.isOnSameLineAsNextAnnotationEntry() =
siblings(forward = true)
.takeWhile { it.elementType != ANNOTATION_ENTRY }
.none { it.isWhiteSpaceWithNewline() }

private fun ASTNode.isOnSameLineAsAnnotatedConstruct() =
lastChildLeafOrSelf()
.leaves(forward = true)
.takeWhile { it.isWhiteSpace() || it.isPartOfComment() }
.none { it.isWhiteSpaceWithNewline() }

private fun ASTNode.isOnSameLineAsNextAnnotationEntryOrAnnotatedConstruct() =
if (isFollowedByOtherAnnotationEntry()) {
isOnSameLineAsNextAnnotationEntry()
} else {
isOnSameLineAsAnnotatedConstruct()
}

private fun visitFileAnnotationList(
node: ASTNode,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
autoCorrect: Boolean
) {
val lineNumber = node.lineNumber()
val next = node.nextSibling { it.textLength > 0 }?.let { next ->
val psi = next.psi
((psi as? KtScript)?.blockExpression?.firstChildNode ?: next).nextSibling {
!it.isWhiteSpace() && !(it.isPartOfComment() && it.lineNumber() == lineNumber)
}
val nextLineNumber = next?.lineNumber()
if (lineNumber != null && nextLineNumber != null) {
val diff = nextLineNumber - lineNumber
if (diff < 2) {
val psi = node.psi
emit(psi.endOffset - 1, fileAnnotationsShouldBeSeparated, true)
if (autoCorrect) {
if (diff == 0) {
psi.getNextSiblingIgnoringWhitespaceAndComments(withItself = false)?.node
?.prevSibling { it.isWhiteSpace() }
?.let { (it as? LeafPsiElement)?.delete() }
next.treeParent.addChild(PsiWhiteSpaceImpl("\n"), next)
}
}
val nextLineNumber = next?.lineNumber()
if (lineNumber != null && nextLineNumber != null) {
val diff = nextLineNumber - lineNumber
if (diff < 2) {
val psi = node.psi
emit(
psi.endOffset - 1,
"File annotations should be separated from file contents with a blank line",
true
)
if (autoCorrect) {
if (diff == 0) {
psi.getNextSiblingIgnoringWhitespaceAndComments(withItself = false)?.node
?.prevSibling { it.isWhiteSpace() }
?.let { (it as? LeafPsiElement)?.delete() }
next.treeParent.addChild(PsiWhiteSpaceImpl("\n"), next)
}
next.treeParent.addChild(PsiWhiteSpaceImpl("\n"), next)
}
}
}
Expand All @@ -162,9 +289,4 @@ class AnnotationRule : Rule("annotation") {
newLineWithIndent
}
}

private fun doesNotEndWithAComment(whiteSpaces: List<PsiWhiteSpace>): Boolean {
val lastNode = whiteSpaces.lastOrNull()?.nextLeaf()
return lastNode !is PsiComment || lastNode.nextLeaf()?.textContains('\n') == false
}
}
Loading

0 comments on commit fb7f5b4

Please sign in to comment.