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.

Sign up for GitHub

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

Fix ktlint-disable directive on package statement #1038

Closed
wants to merge 7 commits into from
Closed

Fix ktlint-disable directive on package statement #1038

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f76c486
Fix ktlint-disable directive on package statement
Dec 27, 2020
b736e49
Remove unused import
Dec 27, 2020
df34451
Move rule initialization out of normal visit of node
Jan 17, 2021
54fc997
Simplify and improve check on suppressed regions
Jan 17, 2021
5e29d52
Remove unused parameter isRoot
Jan 17, 2021
3cb7649
Remove ktlint-disable directive on package statement
Jan 17, 2021
dd31987
Clarify documentation regarding disabling of rules
Jan 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 51 additions & 26 deletions ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package com.pinterest.ktlint.core.internal

import com.pinterest.ktlint.core.ast.prevLeaf
import com.pinterest.ktlint.core.ast.ElementType
import com.pinterest.ktlint.core.ast.visit
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiComment
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace
import org.jetbrains.kotlin.psi.KtAnnotated
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.psiUtil.endOffset
import org.jetbrains.kotlin.psi.psiUtil.prevLeaf
import org.jetbrains.kotlin.psi.psiUtil.prevLeafs
import org.jetbrains.kotlin.psi.psiUtil.startOffset

/**
Expand Down Expand Up @@ -73,32 +76,19 @@ private fun collect(
val text = node.getText()
if (text.startsWith("//")) {
val commentText = text.removePrefix("//").trim()
parseHintArgs(commentText, "ktlint-disable")?.let { args ->
val lineStart = (
node.prevLeaf { it is PsiWhiteSpace && it.textContains('\n') } as
PsiWhiteSpace?
)?.let { it.node.startOffset + it.text.lastIndexOf('\n') + 1 } ?: 0
result.add(SuppressionHint(IntRange(lineStart, node.startOffset), HashSet(args)))
if (node.isDisabledOnPackageStatement()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

KtLint readme on this case is not very clear.

import package.* // ktlint-disable

Should disable any rule check on this particular line.

It doesn't make sense to disable all the rules for the whole file - better to either not pass it to KtLint or exclude it via GLOB pattern. To disable all the rules for the region, better to use:

/* ktlint-disable */
<some code>
/* ktlint-enable */

So, for the case <some code> // ktlint-disable please use createLineDisableSupressionHint and pass emptySet() as disabledRules.

Copy link
Collaborator Author

@paul-dingemans paul-dingemans Jan 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I really like puzzles. But this review remark really confused me at first. I have the feeling that we were discussing different, albeit related, topics. Let's try to unravel this.

KtLint readme on this case is not very clear.

import package.* // ktlint-disable

Should disable any rule check on this particular line.

This code example is confusing for different reasons:

  • There is no apparent reason why this statement should be excluded by ktlint as there is no rule violated.
  • The word package after import took precedence for me. So I read this statement as being the package statement:
package com.example.mypackage // ktlint-disable

I have modified this.

It doesn't make sense to disable all the rules for the whole file - better to either not pass it to KtLint or exclude it via GLOB pattern.
It is only possible to follow this approach when you know which files need to be excluded from processing by ktlint. In bigger code bases with which you are not familiar you may just not know which files would need to be excluded. When you are able to specify it explicitly in the file itself, you can also document the reasoning for this. This approach allows to lint/format the entire code base without having to know the files which need to be excluded.

So, for the case <some code> // ktlint-disable please use createLineDisableSupressionHint and pass emptySet() as disabledRules.

I have removed the code for the ktlint-disable directive on the package statement as I no longer needed it after fixing some other bugs.

I found out that certain bugs are caused by the fact that some rules perform a kind of initialization as soon as the rootNode is visited. If for some reasons, for example disabling a rule, the rootNode is not visited then the rule would not have been initialized correctly. I have split initialization and visiting.

Next problem was that the IndentationRule modifies the ASTNode hierarchy by inserting additional node. Disabling the indentation rule was not always recognized by the SupressedRegionLocator. I have fixed and simplified this locator.

The changes can best be reviewed per commit.

node.createBlockDisableSuppressionHint(commentText)
?.let { suppressionHint -> open.add(suppressionHint) }
} else {
node.createLineDisableSupressionHint(commentText)
?.let { suppressionHint -> result.add(suppressionHint) }
}
} else {
val commentText = text.removePrefix("/*").removeSuffix("*/").trim()
parseHintArgs(commentText, "ktlint-disable")?.apply {
open.add(SuppressionHint(IntRange(node.startOffset, node.startOffset), HashSet(this)))
}
?: parseHintArgs(commentText, "ktlint-enable")?.apply {
// match open hint
val disabledRules = HashSet(this)
val openHintIndex = open.indexOfLast { it.disabledRules == disabledRules }
if (openHintIndex != -1) {
val openingHint = open.removeAt(openHintIndex)
result.add(
SuppressionHint(
IntRange(openingHint.range.first, node.startOffset),
disabledRules
)
)
}
}
node.createBlockDisableSuppressionHint(commentText)
?.let { suppressionHint -> open.add(suppressionHint) }
node.createBlockEnableSuppressionHint(commentText, open)
?.let { suppressionHint -> result.add(suppressionHint) }
}
}
// Extract all Suppress annotations and create SuppressionHints
Expand All @@ -118,15 +108,50 @@ private fun collect(
return result
}

private fun PsiElement.isDisabledOnPackageStatement(): Boolean =
prevLeafs.any { it.node.elementType == ElementType.PACKAGE_KEYWORD }

private fun PsiElement.createLineDisableSupressionHint(commentText: String): SuppressionHint? {
return parseHintArgs(commentText, "ktlint-disable")
?.let { hints ->
val rangeStartOffset = (
prevLeaf { it is PsiWhiteSpace && it.textContains('\n') } as
PsiWhiteSpace?
)?.let { it.node.startOffset + it.text.lastIndexOf('\n') + 1 } ?: 0
SuppressionHint(IntRange(rangeStartOffset, startOffset), hints)
}
}

private fun PsiElement.createBlockDisableSuppressionHint(commentText: String): SuppressionHint? {
return parseHintArgs(commentText, "ktlint-disable")
?.let { hints -> SuppressionHint(IntRange(node.startOffset, node.startOffset), hints) }
}

private fun PsiElement.createBlockEnableSuppressionHint(commentText: String, open: java.util.ArrayList<SuppressionHint>): SuppressionHint? {
return parseHintArgs(commentText, "ktlint-enable")?.let { hints ->
// match open hint
val openHintIndex = open.indexOfLast { it.disabledRules == hints }
if (openHintIndex != -1) {
val openingHint = open.removeAt(openHintIndex)
SuppressionHint(
IntRange(openingHint.range.first, node.startOffset),
hints
)
} else {
null
}
}
}

private fun parseHintArgs(
commentText: String,
key: String
): List<String>? {
): HashSet<String>? {
if (commentText.startsWith(key)) {
val parsedComment = splitCommentBySpace(commentText)
// assert exact match
if (parsedComment[0] == key) {
return parsedComment.tail()
return HashSet(parsedComment.tail())
}
}
return null
Expand Down
94 changes: 74 additions & 20 deletions ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,29 +10,71 @@ import org.junit.Test

class ErrorSuppressionTest {

@Test
fun testErrorSuppression() {
class NoWildcardImportsRule : Rule("no-wildcard-imports") {
override fun visit(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, corrected: Boolean) -> Unit
) {
if (node is LeafPsiElement && node.textMatches("*") && node.isPartOf(ElementType.IMPORT_DIRECTIVE)) {
emit(node.startOffset, "Wildcard import", false)
}
class NoWildcardImportsRule : Rule("no-wildcard-imports") {
paul-dingemans marked this conversation as resolved.
Show resolved Hide resolved
override fun visit(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, corrected: Boolean) -> Unit
) {
if (node is LeafPsiElement && node.textMatches("*") && node.isPartOf(ElementType.IMPORT_DIRECTIVE)) {
emit(node.startOffset, "Wildcard import", false)
}
}
fun lint(text: String) =
ArrayList<LintError>().apply {
KtLint.lint(
KtLint.Params(
text = text,
ruleSets = listOf(RuleSet("standard", NoWildcardImportsRule())),
cb = { e, _ -> add(e) }
)
}

fun lint(text: String) =
ArrayList<LintError>().apply {
KtLint.lint(
KtLint.Params(
text = text,
ruleSets = listOf(RuleSet("standard", NoWildcardImportsRule())),
cb = { e, _ -> add(e) }
)
}
)
}

@Test
fun testErrorSuppressionDisableAllOnPackage() {
assertThat(
lint(
"""
package com.pinterest.ktlint // ktlint-disable

import a.* // Should not trigger an error due to ktlin-disable directive on package

/* ktlint-enable */
import b.* // will trigger an error
""".trimIndent()
)
).isEqualTo(
listOf(
LintError(6, 10, "no-wildcard-imports", "Wildcard import")
)
)
}

@Test
fun testErrorSuppressionDisableRuleOnPackage() {
assertThat(
lint(
"""
package com.pinterest.ktlint // ktlint-disable no-wildcard-imports

import a.* // Should not trigger an error due to ktlin-disable directive on package

/* ktlint-enable no-wildcard-imports */
import b.* // will trigger an error
""".trimIndent()
)
).isEqualTo(
listOf(
LintError(6, 10, "no-wildcard-imports", "Wildcard import")
)
)
}

@Test
fun testErrorSuppressionDisableAllOnImport() {
assertThat(
lint(
"""
Expand All @@ -45,6 +87,10 @@ class ErrorSuppressionTest {
LintError(2, 10, "no-wildcard-imports", "Wildcard import")
)
)
}

@Test
fun testErrorSuppressionDisableRuleOnImport() {
assertThat(
lint(
"""
Expand All @@ -57,6 +103,10 @@ class ErrorSuppressionTest {
LintError(2, 10, "no-wildcard-imports", "Wildcard import")
)
)
}

@Test
fun testErrorSuppressionDisableAllInBlock() {
assertThat(
lint(
"""
Expand All @@ -72,6 +122,10 @@ class ErrorSuppressionTest {
LintError(5, 10, "no-wildcard-imports", "Wildcard import")
)
)
}

@Test
fun testErrorSuppressionDisableRuleInBlock() {
assertThat(
lint(
"""
Expand Down