From f76c486acef5ba6af168c6a37df02f53dcbbef5a Mon Sep 17 00:00:00 2001 From: Paul Dingemans Date: Sun, 27 Dec 2020 19:42:42 +0100 Subject: [PATCH 1/7] Fix ktlint-disable directive on package statement * Split unit tests for error suppression into multiple tests * Extract the creation of the SuppressionHints to extension methods for readability and reuseability --- .../core/internal/SupressedRegionLocator.kt | 78 ++++++++++----- .../ktlint/core/ErrorSuppressionTest.kt | 94 +++++++++++++++---- 2 files changed, 126 insertions(+), 46 deletions(-) diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt index f5e464c430..7aea0a8419 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt @@ -1,13 +1,17 @@ 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.logStructure 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 /** @@ -73,32 +77,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()) { + 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 @@ -118,15 +109,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? { + 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? { +): HashSet? { if (commentText.startsWith(key)) { val parsedComment = splitCommentBySpace(commentText) // assert exact match if (parsedComment[0] == key) { - return parsedComment.tail() + return HashSet(parsedComment.tail()) } } return null diff --git a/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt b/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt index dfbda068b7..d0d010bf3f 100644 --- a/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt +++ b/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt @@ -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") { + 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().apply { - KtLint.lint( - KtLint.Params( - text = text, - ruleSets = listOf(RuleSet("standard", NoWildcardImportsRule())), - cb = { e, _ -> add(e) } - ) + } + + fun lint(text: String) = + ArrayList().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( """ @@ -45,6 +87,10 @@ class ErrorSuppressionTest { LintError(2, 10, "no-wildcard-imports", "Wildcard import") ) ) + } + + @Test + fun testErrorSuppressionDisableRuleOnImport() { assertThat( lint( """ @@ -57,6 +103,10 @@ class ErrorSuppressionTest { LintError(2, 10, "no-wildcard-imports", "Wildcard import") ) ) + } + + @Test + fun testErrorSuppressionDisableAllInBlock() { assertThat( lint( """ @@ -72,6 +122,10 @@ class ErrorSuppressionTest { LintError(5, 10, "no-wildcard-imports", "Wildcard import") ) ) + } + + @Test + fun testErrorSuppressionDisableRuleInBlock() { assertThat( lint( """ From b736e499369f6075302c4e341782d909fe217511 Mon Sep 17 00:00:00 2001 From: Paul Dingemans Date: Sun, 27 Dec 2020 20:11:54 +0100 Subject: [PATCH 2/7] Remove unused import --- .../com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt index 7aea0a8419..8be3881255 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt @@ -1,7 +1,6 @@ package com.pinterest.ktlint.core.internal import com.pinterest.ktlint.core.ast.ElementType -import com.pinterest.ktlint.core.ast.logStructure import com.pinterest.ktlint.core.ast.visit import org.jetbrains.kotlin.com.intellij.lang.ASTNode import org.jetbrains.kotlin.com.intellij.psi.PsiComment From df34451c8bafb71df505f93c168c0f62d25437bd Mon Sep 17 00:00:00 2001 From: Paul Dingemans Date: Sun, 17 Jan 2021 09:31:48 +0100 Subject: [PATCH 3/7] Move rule initialization out of normal visit of node Make a clearer distinction between the initialization of a rule versus processing (e.g. visiting) the rule with a node. A rule that implements the Rule.Modifier.InitializeRoot interface will be called with the rootNode to initialize the rule. After that the rule will be visited for each node (including the root node) unless the node is suppressed by a ktlint-disable directive. --- .../com/pinterest/ktlint/core/KtLint.kt | 19 ++++ .../kotlin/com/pinterest/ktlint/core/Rule.kt | 8 ++ .../experimental/ArgumentListWrappingRule.kt | 13 ++- .../ruleset/standard/ImportOrderingRule.kt | 13 +-- .../ruleset/standard/MaxLineLengthRule.kt | 87 ++++++++++--------- .../ruleset/standard/NoUnusedImportsRule.kt | 17 ++-- .../standard/ParameterListWrappingRule.kt | 13 ++- .../com/pinterest/ruleset/test/DumpASTRule.kt | 17 ++-- 8 files changed, 121 insertions(+), 66 deletions(-) diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt index ea1781d409..893189b654 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt @@ -2,6 +2,7 @@ package com.pinterest.ktlint.core import com.pinterest.ktlint.core.api.EditorConfigProperties import com.pinterest.ktlint.core.api.FeatureInAlphaState +import com.pinterest.ktlint.core.ast.isRoot import com.pinterest.ktlint.core.ast.visit import com.pinterest.ktlint.core.internal.EditorConfigGenerator import com.pinterest.ktlint.core.internal.EditorConfigLoader @@ -88,6 +89,12 @@ public object KtLint { val errors = mutableListOf() visitor(preparedCode.rootNode, params.ruleSets).invoke { node, rule, fqRuleId -> + if (node.isRoot() && rule is Rule.Modifier.InitializeRoot) { + // In case the rule has a root initializer then it should always be called regardless whether offset 0 + // of the file is suppressed by a ktlint-disable directive. + rule.initializeRoot(node) + } + // fixme: enforcing suppression based on node.startOffset is wrong // (not just because not all nodes are leaves but because rules are free to emit (and fix!) errors at any position) if ( @@ -297,6 +304,12 @@ public object KtLint { var mutated = false visitor(preparedCode.rootNode, params.ruleSets, concurrent = false) .invoke { node, rule, fqRuleId -> + if (node.isRoot() && rule is Rule.Modifier.InitializeRoot) { + // In case the rule has a root initializer then it should always be called regardless whether offset + // 0 of the file is suppressed by a ktlint-disable directive. + rule.initializeRoot(node) + } + // fixme: enforcing suppression based on node.startOffset is wrong // (not just because not all nodes are leaves but because rules are free to emit (and fix!) errors at any position) if ( @@ -324,6 +337,12 @@ public object KtLint { if (tripped) { val errors = mutableListOf>() visitor(preparedCode.rootNode, params.ruleSets).invoke { node, rule, fqRuleId -> + if (node.isRoot() && rule is Rule.Modifier.InitializeRoot) { + // In case the rule has a root initializer then it should always be called regardless whether offset 0 + // of the file is suppressed by a ktlint-disable directive. + rule.initializeRoot(node) + } + // fixme: enforcing suppression based on node.startOffset is wrong // (not just because not all nodes are leaves but because rules are free to emit (and fix!) errors at any position) if ( diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/Rule.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/Rule.kt index c32dfad372..886df4587b 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/Rule.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/Rule.kt @@ -47,5 +47,13 @@ abstract class Rule(val id: String) { */ interface RestrictToRootLast : RestrictToRoot interface Last + /** + * Any rule implementing this interface will be given the root ([FileASTNode]) node so that the rule is + * guaranteed to be initialized even in case the first line of the file contains a ktlint-disable directive + * for the rule. + */ + interface InitializeRoot { + fun initializeRoot(node: ASTNode) + } } } diff --git a/ktlint-ruleset-experimental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/ArgumentListWrappingRule.kt b/ktlint-ruleset-experimental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/ArgumentListWrappingRule.kt index aa29ff9fb5..878ccf78ad 100644 --- a/ktlint-ruleset-experimental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/ArgumentListWrappingRule.kt +++ b/ktlint-ruleset-experimental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/ArgumentListWrappingRule.kt @@ -37,20 +37,25 @@ import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType * - maxLineLength exceeded (and separating arguments with \n would actually help) * in addition, "(" and ")" must be on separates line if any of the arguments are (otherwise on the same) */ -class ArgumentListWrappingRule : Rule("argument-list-wrapping") { +class ArgumentListWrappingRule : + Rule("argument-list-wrapping"), + Rule.Modifier.InitializeRoot { private var indentSize = -1 private var maxLineLength = -1 + override fun initializeRoot(node: ASTNode) { + val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_USER_DATA_KEY)!! + indentSize = editorConfig.indentSize + maxLineLength = editorConfig.maxLineLength + } + override fun visit( node: ASTNode, autoCorrect: Boolean, emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit ) { if (node.isRoot()) { - val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_USER_DATA_KEY)!! - indentSize = editorConfig.indentSize - maxLineLength = editorConfig.maxLineLength return } if (indentSize <= 0) { diff --git a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ImportOrderingRule.kt b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ImportOrderingRule.kt index 2cf507968a..75bbac4735 100644 --- a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ImportOrderingRule.kt +++ b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ImportOrderingRule.kt @@ -38,6 +38,7 @@ import org.jetbrains.kotlin.psi.KtImportDirective @OptIn(FeatureInAlphaState::class) public class ImportOrderingRule : Rule("import-ordering"), + Rule.Modifier.InitializeRoot, UsesEditorConfigProperties { private lateinit var importsLayout: List @@ -149,19 +150,21 @@ public class ImportOrderingRule : ideaImportsLayoutProperty, ) + override fun initializeRoot(node: ASTNode) { + val android = node.getUserData(KtLint.ANDROID_USER_DATA_KEY) ?: false + val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_PROPERTIES_USER_DATA_KEY)!! + importsLayout = editorConfig.resolveImportsLayout(android) + importSorter = ImportSorter(importsLayout) + } + override fun visit( node: ASTNode, autoCorrect: Boolean, emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit ) { if (node.isRoot()) { - val android = node.getUserData(KtLint.ANDROID_USER_DATA_KEY) ?: false - val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_PROPERTIES_USER_DATA_KEY)!! - importsLayout = editorConfig.resolveImportsLayout(android) - importSorter = ImportSorter(importsLayout) return } - if (node.elementType == ElementType.IMPORT_LIST) { val children = node.getChildren(null) if (children.isNotEmpty()) { diff --git a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/MaxLineLengthRule.kt b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/MaxLineLengthRule.kt index dd6a8d85df..73e0b15d4c 100644 --- a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/MaxLineLengthRule.kt +++ b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/MaxLineLengthRule.kt @@ -14,32 +14,44 @@ import org.jetbrains.kotlin.kdoc.psi.api.KDoc import org.jetbrains.kotlin.psi.KtImportDirective import org.jetbrains.kotlin.psi.KtPackageDirective -class MaxLineLengthRule : Rule("max-line-length"), Rule.Modifier.Last { +class MaxLineLengthRule : + Rule("max-line-length"), + Rule.Modifier.InitializeRoot, + Rule.Modifier.Last { private var maxLineLength: Int = -1 private var rangeTree = RangeTree() - override fun visit( - node: ASTNode, - autoCorrect: Boolean, - emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit - ) { - if (node.isRoot()) { - val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_USER_DATA_KEY)!! - maxLineLength = editorConfig.maxLineLength - if (maxLineLength <= 0) { - return - } - val errorOffset = arrayListOf() - val text = node.text - val lines = text.split("\n") - var offset = 0 - for (line in lines) { - if (line.length > maxLineLength) { - val el = node.psi.findElementAt(offset + line.length - 1)!!.node - if (!el.isPartOf(KDoc::class) && !el.isPartOfRawMultiLineString()) { - if (!el.isPartOf(PsiComment::class)) { - if (!el.isPartOf(KtPackageDirective::class) && !el.isPartOf(KtImportDirective::class)) { + override fun initializeRoot(node: ASTNode) { + val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_USER_DATA_KEY)!! + maxLineLength = editorConfig.maxLineLength + + if (maxLineLength <= 0) { + return + } + val errorOffset = arrayListOf() + val text = node.text + val lines = text.split("\n") + var offset = 0 + for (line in lines) { + if (line.length > maxLineLength) { + val el = node.psi.findElementAt(offset + line.length - 1)!!.node + if (!el.isPartOf(KDoc::class) && !el.isPartOfRawMultiLineString()) { + if (!el.isPartOf(PsiComment::class)) { + if (!el.isPartOf(KtPackageDirective::class) && !el.isPartOf(KtImportDirective::class)) { + // fixme: + // normally we would emit here but due to API limitations we need to hold off until + // node spanning the same offset is 'visit'ed + // (for ktlint-disable directive to have effect (when applied)) + // this will be rectified in the upcoming release(s) + errorOffset.add(offset) + } + } else { + // Allow ktlint-disable comments to exceed max line length + if (!el.text.startsWith("// ktlint-disable")) { + // if comment is the only thing on the line - fine, otherwise emit an error + val prevLeaf = el.prevCodeSibling() + if (prevLeaf != null && prevLeaf.startOffset >= offset) { // fixme: // normally we would emit here but due to API limitations we need to hold off until // node spanning the same offset is 'visit'ed @@ -47,27 +59,24 @@ class MaxLineLengthRule : Rule("max-line-length"), Rule.Modifier.Last { // this will be rectified in the upcoming release(s) errorOffset.add(offset) } - } else { - // Allow ktlint-disable comments to exceed max line length - if (!el.text.startsWith("// ktlint-disable")) { - // if comment is the only thing on the line - fine, otherwise emit an error - val prevLeaf = el.prevCodeSibling() - if (prevLeaf != null && prevLeaf.startOffset >= offset) { - // fixme: - // normally we would emit here but due to API limitations we need to hold off until - // node spanning the same offset is 'visit'ed - // (for ktlint-disable directive to have effect (when applied)) - // this will be rectified in the upcoming release(s) - errorOffset.add(offset) - } - } } } } - offset += line.length + 1 } - rangeTree = RangeTree(errorOffset) - } else if (!rangeTree.isEmpty() && node.psi is LeafPsiElement) { + offset += line.length + 1 + } + rangeTree = RangeTree(errorOffset) + } + + override fun visit( + node: ASTNode, + autoCorrect: Boolean, + emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit + ) { + if (node.isRoot()) { + return + } + if (!rangeTree.isEmpty() && node.psi is LeafPsiElement) { rangeTree .query(node.startOffset, node.startOffset + node.textLength) .forEach { offset -> diff --git a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/NoUnusedImportsRule.kt b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/NoUnusedImportsRule.kt index 0f8d6b5a28..6cae169450 100644 --- a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/NoUnusedImportsRule.kt +++ b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/NoUnusedImportsRule.kt @@ -20,7 +20,9 @@ import org.jetbrains.kotlin.psi.KtDotQualifiedExpression import org.jetbrains.kotlin.psi.KtImportDirective import org.jetbrains.kotlin.psi.KtPackageDirective -class NoUnusedImportsRule : Rule("no-unused-imports") { +class NoUnusedImportsRule : + Rule("no-unused-imports"), + Rule.Modifier.InitializeRoot { private val componentNRegex = Regex("^component\\d+$") @@ -73,17 +75,20 @@ class NoUnusedImportsRule : Rule("no-unused-imports") { private var packageName = "" private var rootNode: ASTNode? = null + override fun initializeRoot(node: ASTNode) { + rootNode = node + ref.clear() // rule can potentially be executed more than once (when formatting) + ref.add(Reference("*", false)) + parentExpressions.clear() + imports.clear() + } + override fun visit( node: ASTNode, autoCorrect: Boolean, emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit ) { if (node.isRoot()) { - rootNode = node - ref.clear() // rule can potentially be executed more than once (when formatting) - ref.add(Reference("*", false)) - parentExpressions.clear() - imports.clear() node.visit { vnode -> val psi = vnode.psi val type = vnode.elementType diff --git a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ParameterListWrappingRule.kt b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ParameterListWrappingRule.kt index 738da77313..4ed828c0ff 100644 --- a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ParameterListWrappingRule.kt +++ b/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/ParameterListWrappingRule.kt @@ -27,20 +27,25 @@ import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl import org.jetbrains.kotlin.psi.KtTypeArgumentList import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType -class ParameterListWrappingRule : Rule("parameter-list-wrapping") { +class ParameterListWrappingRule : + Rule("parameter-list-wrapping"), + Rule.Modifier.InitializeRoot { private var indentSize = -1 private var maxLineLength = -1 + override fun initializeRoot(node: ASTNode) { + val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_USER_DATA_KEY)!! + indentSize = editorConfig.indentSize + maxLineLength = editorConfig.maxLineLength + } + override fun visit( node: ASTNode, autoCorrect: Boolean, emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit ) { if (node.isRoot()) { - val editorConfig = node.getUserData(KtLint.EDITOR_CONFIG_USER_DATA_KEY)!! - indentSize = editorConfig.indentSize - maxLineLength = editorConfig.maxLineLength return } if (indentSize <= 0) { diff --git a/ktlint-ruleset-test/src/main/kotlin/com/pinterest/ruleset/test/DumpASTRule.kt b/ktlint-ruleset-test/src/main/kotlin/com/pinterest/ruleset/test/DumpASTRule.kt index 4c3f14baec..5bfe5958f7 100644 --- a/ktlint-ruleset-test/src/main/kotlin/com/pinterest/ruleset/test/DumpASTRule.kt +++ b/ktlint-ruleset-test/src/main/kotlin/com/pinterest/ruleset/test/DumpASTRule.kt @@ -2,7 +2,6 @@ package com.pinterest.ruleset.test import com.pinterest.ktlint.core.Rule import com.pinterest.ktlint.core.ast.ElementType -import com.pinterest.ktlint.core.ast.isRoot import com.pinterest.ktlint.core.ast.lastChildLeafOrSelf import com.pinterest.ktlint.core.ast.lineNumber import com.pinterest.ruleset.test.internal.Color @@ -15,7 +14,8 @@ import org.jetbrains.kotlin.lexer.KtTokens public class DumpASTRule @JvmOverloads constructor( private val out: PrintStream = System.err, private val color: Boolean = false -) : Rule("dump") { +) : Rule("dump"), + Rule.Modifier.InitializeRoot { private companion object { val elementTypeSet = ElementType::class.members.map { it.name }.toSet() @@ -24,17 +24,18 @@ public class DumpASTRule @JvmOverloads constructor( private var lineNumberColumnLength: Int = 0 private var lastNode: ASTNode? = null + override fun initializeRoot(node: ASTNode) { + lineNumberColumnLength = + (node.lastChildLeafOrSelf().lineNumber() ?: 1) + .let { var v = it; var c = 0; while (v > 0) { c++; v /= 10 }; c } + lastNode = node.lastChildLeafOrSelf() + } + override fun visit( node: ASTNode, autoCorrect: Boolean, emit: (offset: Int, errorMessage: String, corrected: Boolean) -> Unit ) { - if (node.isRoot()) { - lineNumberColumnLength = - (node.lastChildLeafOrSelf().lineNumber() ?: 1) - .let { var v = it; var c = 0; while (v > 0) { c++; v /= 10 }; c } - lastNode = node.lastChildLeafOrSelf() - } var level = -1 var parent: ASTNode? = node do { From 54fc99794e1177fd99c4aa88702d3fc61aac0c89 Mon Sep 17 00:00:00 2001 From: Paul Dingemans Date: Sun, 17 Jan 2021 12:42:08 +0100 Subject: [PATCH 4/7] Simplify and improve check on suppressed regions When starting a new block in which a rule is disabled, then define the range of this block from the startOffset of that block until eternity (Int.MAX_VALUE). When the block is closed explicitly by a ktlint-enable directive, or when closing all open blocks at the end of the collect phase, the end position of the block is overwritten with the offset of the end of the file. This fixes a very specific bug in the IndentationRule which on occasion modifies the structure of the ASTNodes by inserting new nodes. As of such, the rootNode can contain offsets which are not equal to zero but which still need to be suppressed. This bug can not be tested in the unit test of the SupressedRegionLocator but is verified in the unit test IndentationRuleTest. --- .../core/internal/SupressedRegionLocator.kt | 25 +++++++----------- .../ruleset/standard/IndentationRuleTest.kt | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt index 8be3881255..23a0bf745e 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt @@ -39,20 +39,12 @@ internal fun buildSuppressedRegionsLocator( return if (hintsList.isEmpty()) { noSuppression } else { offset, ruleId, isRoot -> - if (isRoot) { - val h = hintsList[0] - h.range.last == 0 && ( - h.disabledRules.isEmpty() || - h.disabledRules.contains(ruleId) - ) - } else { - hintsList.any { hint -> - ( - hint.disabledRules.isEmpty() || - hint.disabledRules.contains(ruleId) - ) && - hint.range.contains(offset) - } + hintsList.any { hint -> + ( + hint.disabledRules.isEmpty() || + hint.disabledRules.contains(ruleId) + ) && + hint.range.contains(offset) } } } @@ -77,7 +69,8 @@ private fun collect( if (text.startsWith("//")) { val commentText = text.removePrefix("//").trim() if (node.isDisabledOnPackageStatement()) { - node.createBlockDisableSuppressionHint(commentText) + parseHintArgs(commentText, "ktlint-disable") + ?.let { hints -> SuppressionHint(IntRange(0, Int.MAX_VALUE), hints) } ?.let { suppressionHint -> open.add(suppressionHint) } } else { node.createLineDisableSupressionHint(commentText) @@ -124,7 +117,7 @@ private fun PsiElement.createLineDisableSupressionHint(commentText: String): Sup private fun PsiElement.createBlockDisableSuppressionHint(commentText: String): SuppressionHint? { return parseHintArgs(commentText, "ktlint-disable") - ?.let { hints -> SuppressionHint(IntRange(node.startOffset, node.startOffset), hints) } + ?.let { hints -> SuppressionHint(IntRange(node.startOffset, Int.MAX_VALUE), hints) } } private fun PsiElement.createBlockEnableSuppressionHint(commentText: String, open: java.util.ArrayList): SuppressionHint? { diff --git a/ktlint-ruleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt b/ktlint-ruleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt index 70932a0746..57f1ad88fc 100644 --- a/ktlint-ruleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt +++ b/ktlint-ruleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt @@ -849,4 +849,30 @@ internal class IndentationRuleTest { ) ).isEmpty() } + + // Explicitly test disabling this rule as the code hierarchy is changed when new elements need to be inserted for + // certain indentation fixes. + @Test + fun `lint can be suppressed on incorrectly indented lines`() { + val code = + """ + class Foo { + // Next lines should not fail due to EOL comment containing suppression + val bar1 = "bar" // ktlint-disable + val bar2 = "bar" // ktlint-disable indent + + /* ktlint-disable */ + val bar3 = "bar" // Should not fail due to block comment above + + /* ktlint-enable */ + val bar5 = "bar" // Should fail + } + """.trimIndent() + + assertThat(IndentationRule().lint(code)).isEqualTo( + listOf( + LintError(line = 10, col = 1, ruleId = "indent", detail = "Unexpected indentation (0) (should be 4)"), + ) + ) + } } From 5e29d529bd9eaa4744019cdf20e5d328b0e45806 Mon Sep 17 00:00:00 2001 From: Paul Dingemans Date: Sun, 17 Jan 2021 12:53:54 +0100 Subject: [PATCH 5/7] Remove unused parameter isRoot --- .../main/kotlin/com/pinterest/ktlint/core/KtLint.kt | 10 +++++----- .../ktlint/core/internal/SupressedRegionLocator.kt | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt index 893189b654..939ba7a515 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt @@ -98,13 +98,13 @@ public object KtLint { // fixme: enforcing suppression based on node.startOffset is wrong // (not just because not all nodes are leaves but because rules are free to emit (and fix!) errors at any position) if ( - !preparedCode.suppressedRegionLocator(node.startOffset, fqRuleId, node === preparedCode.rootNode) + !preparedCode.suppressedRegionLocator(node.startOffset, fqRuleId) ) { try { rule.visit(node, false) { offset, errorMessage, canBeAutoCorrected -> // https://github.com/shyiko/ktlint/issues/158#issuecomment-462728189 if (node.startOffset != offset && - preparedCode.suppressedRegionLocator(offset, fqRuleId, node === preparedCode.rootNode) + preparedCode.suppressedRegionLocator(offset, fqRuleId) ) { return@visit } @@ -313,7 +313,7 @@ public object KtLint { // fixme: enforcing suppression based on node.startOffset is wrong // (not just because not all nodes are leaves but because rules are free to emit (and fix!) errors at any position) if ( - !preparedCode.suppressedRegionLocator(node.startOffset, fqRuleId, node === preparedCode.rootNode) + !preparedCode.suppressedRegionLocator(node.startOffset, fqRuleId) ) { try { rule.visit(node, true) { _, _, canBeAutoCorrected -> @@ -346,14 +346,14 @@ public object KtLint { // fixme: enforcing suppression based on node.startOffset is wrong // (not just because not all nodes are leaves but because rules are free to emit (and fix!) errors at any position) if ( - !preparedCode.suppressedRegionLocator(node.startOffset, fqRuleId, node === preparedCode.rootNode) + !preparedCode.suppressedRegionLocator(node.startOffset, fqRuleId) ) { try { rule.visit(node, false) { offset, errorMessage, canBeAutoCorrected -> // https://github.com/shyiko/ktlint/issues/158#issuecomment-462728189 if ( node.startOffset != offset && - preparedCode.suppressedRegionLocator(offset, fqRuleId, node === preparedCode.rootNode) + preparedCode.suppressedRegionLocator(offset, fqRuleId) ) { return@visit } diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt index 23a0bf745e..6f1f39f0e9 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt @@ -16,12 +16,12 @@ import org.jetbrains.kotlin.psi.psiUtil.startOffset /** * Detects if given `ruleId` at given `offset` is suppressed. */ -internal typealias SuppressionLocator = (offset: Int, ruleId: String, isRoot: Boolean) -> Boolean +internal typealias SuppressionLocator = (offset: Int, ruleId: String) -> Boolean /** * No suppression is detected. Always returns `false`. */ -internal val noSuppression: SuppressionLocator = { _, _, _ -> false } +internal val noSuppression: SuppressionLocator = { _, _ -> false } private val suppressAnnotationRuleMap = mapOf( "RemoveCurlyBracesFromTemplate" to "string-template" @@ -38,7 +38,7 @@ internal fun buildSuppressedRegionsLocator( val hintsList = collect(rootNode) return if (hintsList.isEmpty()) { noSuppression - } else { offset, ruleId, isRoot -> + } else { offset: Int, ruleId: String -> hintsList.any { hint -> ( hint.disabledRules.isEmpty() || From 3cb764989c8f311370df0c88e811123ec7aa0169 Mon Sep 17 00:00:00 2001 From: Paul Dingemans Date: Sun, 17 Jan 2021 12:58:51 +0100 Subject: [PATCH 6/7] Remove ktlint-disable directive on package statement The special disable directive on the package statement is no longer required because disabling via the block comment has been improved. The latter is more clear and consistent. --- .../core/internal/SupressedRegionLocator.kt | 10 +---- .../ktlint/core/ErrorSuppressionTest.kt | 40 ------------------- 2 files changed, 2 insertions(+), 48 deletions(-) diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt index 6f1f39f0e9..03388e6499 100644 --- a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt +++ b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SupressedRegionLocator.kt @@ -68,14 +68,8 @@ private fun collect( val text = node.getText() if (text.startsWith("//")) { val commentText = text.removePrefix("//").trim() - if (node.isDisabledOnPackageStatement()) { - parseHintArgs(commentText, "ktlint-disable") - ?.let { hints -> SuppressionHint(IntRange(0, Int.MAX_VALUE), hints) } - ?.let { suppressionHint -> open.add(suppressionHint) } - } else { - node.createLineDisableSupressionHint(commentText) - ?.let { suppressionHint -> result.add(suppressionHint) } - } + node.createLineDisableSupressionHint(commentText) + ?.let { suppressionHint -> result.add(suppressionHint) } } else { val commentText = text.removePrefix("/*").removeSuffix("*/").trim() node.createBlockDisableSuppressionHint(commentText) diff --git a/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt b/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt index d0d010bf3f..5c02062807 100644 --- a/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt +++ b/ktlint-core/src/test/kotlin/com/pinterest/ktlint/core/ErrorSuppressionTest.kt @@ -33,46 +33,6 @@ class ErrorSuppressionTest { ) } - @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( From dd31987ceca1cb306f4fb6a617e42722da75a968 Mon Sep 17 00:00:00 2001 From: Paul Dingemans Date: Sun, 17 Jan 2021 13:15:52 +0100 Subject: [PATCH 7/7] Clarify documentation regarding disabling of rules --- README.md | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 1c492d1f0d..4bcd5ad0d9 100644 --- a/README.md +++ b/README.md @@ -511,22 +511,38 @@ See [Creating A Ruleset](#creating-a-ruleset). to produce the correct result (please report any such instances using [GitHub Issues](https://github.com/pinterest/ktlint/issues)). To disable a specific rule you'll need to turn on the verbose mode (`ktlint --verbose ...`). At the end of each line -you'll see an error code. Use it as an argument for `ktlint-disable` directive (shown below). +you'll see the id of the rule. This id can be used as an argument for `ktlint-disable` directive (shown below). +A specific rule can be disabled for one specific line by adding the `ktlint-disable` directive, and the rule id as +follows: ```kotlin -import package.* // ktlint-disable no-wildcard-imports +import com.example.mypackage.* // ktlint-disable no-wildcard-imports +``` +A rule can be disabled for a block of statements by adding the `ktlint-disable` directive, and the rule id as follows: +``` /* ktlint-disable no-wildcard-imports */ -import package.a.* -import package.b.* +import com.example.mypackage1.* +import com.example.mypackage2.* /* ktlint-enable no-wildcard-imports */ ``` +Note: the `ktlint-enable` directive above can be omitted in case the rule should be disabled for the remainder of the +file. -To disable all checks: + +It is also possible to disable all checks for one single line, or a block of lines by not specifying the rule id after +the `ktlint-disable` directive: ```kotlin -import package.* // ktlint-disable +import com.example.mypackage.* // ktlint-disable + +/* ktlint-disable */ +import com.example.mypackage1.* +import com.example.mypackage2.* +/* ktlint-enable */ ``` +Note: the `ktlint-enable` directive above can be omitted in case all rules should be disabled for the remainder of the +file. ### How do I globally disable a rule? See the [EditorConfig section](https://github.com/pinterest/ktlint#editorconfig) for details on how to use the `disabled_rules` property.