Skip to content

Commit

Permalink
Refactor life cycle hooks on Rule (#1547)
Browse files Browse the repository at this point in the history
Up until ktlint 0.46 the Rule class provided only one life cycle hook. This "visit" hook was called in a depth-first-approach on all nodes in the file. A rule like the IndentationRule used the RunOnRootOnly visitor modifier to call this lifecycle hook for the root node only in combination with an alternative way of traversing the ASTNodes. Downside of this approach was that suppression of the rule on blocks inside a file was not possible (#631). More generically, this applied to all rules, applying alternative traversals of the AST.

The Rule class now offers new life cycle hooks:

beforeFirstNode: This method is called once before the first node is visited. It can be used to initialize the state of the rule before processing of nodes starts. The ".editorconfig" properties (including overrides) are provided as parameter.
beforeVisitChildNodes: This method is called on a node in AST before visiting its child nodes. This is repeated recursively for the child nodes resulting in a depth first traversal of the AST. This method is the equivalent of the "visit" life cycle hooks. However, note that in KtLint 0.48, the UserData of the rootnode no longer provides access to the ".editorconfig" properties. This method can be used to emit Lint Violations and to autocorrect if applicable.
afterVisitChildNodes: This method is called on a node in AST after all its child nodes have been visited. This method can be used to emit Lint Violations and to autocorrect if applicable.
afterLastNode: This method is called once after the last node in the AST is visited. It can be used for teardown of the state of the rule.
The "visit" life cycle hook will be removed in Ktlint 0.48. In KtLint 0.47 the "visit" life cycle hook will be called only when hook "beforeVisitChildNodes" is not overridden. It is recommended to migrate to the new lifecycle hooks in KtLint 0.47. Please create an issue, in case you need additional assistence to implement the new life cycle hooks in your rules.

Closes #631

Highlight of changes included:
* Extend Rule hooks with beforeFirstNode, beforeVisitChildNodes, afterVisitChildNodes and afterLastNode and deprecating visit.
* Simplification of logic
  - Remove node parameter from VisitorProvider result to simplify code
  - Simplify condition to rebuild supressed region locator
  - Checking for a suppressed region on the root node is not necessary. By default, the root node (e.g. the node with ElementType FILE) can not be suppressed. A '@file:Suppress' annotation on top of the file is already contained in a non-root node.
  - Remove indentation logic of complex arguments in ArgumentListWrappingRule and ParameterListWrappingRule as this is the responsibility of the IndentationRule
* Remove concurrent visitor provider: Despite the name, the concurrent visitor is not faster than the sequential visitor as each file is processed on a single thread. There seem to be no other benefits for having two different visitor providers. Inlined the sequentialVisitor.
* Fix early return when no rules have to be run
* Extract PreparedCode and move PsiFileFactory to this vlass
* Move logic to check whether to run on root node only or on all nodes from VisitorProvider to Ktlint. The visitor provider is only responsible for running the relevant rules in the correct order. On which node is to be executed is not relevant.
* Deprecate the RunOnRootNodeOnly visitor modifier: This visitor modifier was typically used to run a custom node visit algorithm using the root node and the generic "package.visit" methods. The "package.visit" method however did not support rule suppression. With the new lifecycle hooks on the Rule class the visit modifier has become
obsolete.
* Refactor rules to comply with new life cycle hooks. Some rules needed a bigger refactoring to achieve this.
* Provide EditorConfigProperties directly instead of via ASTNode UserData: Prepare to remove ".editorconfig" properties from the UserData in ktlint 0.48. Those properties are only required on the root node but the "getUserData" method can be called on any node. The new Rule lifecycle hooks "beforeFirstNode" provides the ".editorconfig" properties directly to the rule. This contract is cleaner and more explicit.
  • Loading branch information
paul-dingemans committed Jul 17, 2022
1 parent 0949748 commit df43622
Show file tree
Hide file tree
Showing 81 changed files with 1,999 additions and 1,584 deletions.
18 changes: 18 additions & 0 deletions CHANGELOG.md
Expand Up @@ -4,11 +4,29 @@ This project adheres to [Semantic Versioning](https://semver.org/).

## Unreleased

### API Changes & RuleSet providers

If you are not an API user nor a RuleSet provider, then you can safely skip this section. Otherwise, please read below carefully and upgrade your usage of ktlint. In this and coming releases, we are changing and adapting important parts of our API in order to increase maintainability and flexibility for future changes. Please avoid skipping a releases as that will make it harder to migrate.

#### Rule lifecycle hooks / deprecate RunOnRootOnly visitor modifier

Up until ktlint 0.46 the Rule class provided only one life cycle hook. This "visit" hook was called in a depth-first-approach on all nodes in the file. A rule like the IndentationRule used the RunOnRootOnly visitor modifier to call this lifecycle hook for the root node only in combination with an alternative way of traversing the ASTNodes. Downside of this approach was that suppression of the rule on blocks inside a file was not possible ([#631](https://github.com/pinterest/ktlint/issues/631)). More generically, this applied to all rules, applying alternative traversals of the AST.

The Rule class now offers new life cycle hooks:
* beforeFirstNode: This method is called once before the first node is visited. It can be used to initialize the state of the rule before processing of nodes starts. The ".editorconfig" properties (including overrides) are provided as parameter.
* beforeVisitChildNodes: This method is called on a node in AST before visiting its child nodes. This is repeated recursively for the child nodes resulting in a depth first traversal of the AST. This method is the equivalent of the "visit" life cycle hooks. However, note that in KtLint 0.48, the UserData of the rootnode no longer provides access to the ".editorconfig" properties. This method can be used to emit Lint Violations and to autocorrect if applicable.
* afterVisitChildNodes: This method is called on a node in AST after all its child nodes have been visited. This method can be used to emit Lint Violations and to autocorrect if applicable.
* afterLastNode: This method is called once after the last node in the AST is visited. It can be used for teardown of the state of the rule.

The "visit" life cycle hook will be removed in Ktlint 0.48. In KtLint 0.47 the "visit" life cycle hook will be called *only* when hook "beforeVisitChildNodes" is not overridden. It is recommended to migrate to the new lifecycle hooks in KtLint 0.47. Please create an issue, in case you need additional assistence to implement the new life cycle hooks in your rules.


### Added

### Fixed

* Fix cli argument "--disabled_rules" ([#1520](https://github.com/pinterest/ktlint/issue/1520)).
* Disable/enable IndentationRule on blocks in middle of file. (`indent`) [#631](https://github.com/pinterest/ktlint/issues/631)

### Changed

Expand Down
271 changes: 90 additions & 181 deletions ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt

Large diffs are not rendered by default.

64 changes: 61 additions & 3 deletions ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/Rule.kt
@@ -1,5 +1,6 @@
package com.pinterest.ktlint.core

import com.pinterest.ktlint.core.api.EditorConfigProperties
import com.pinterest.ktlint.core.internal.IdNamingPolicy
import org.jetbrains.kotlin.com.intellij.lang.ASTNode

Expand All @@ -15,7 +16,7 @@ import org.jetbrains.kotlin.com.intellij.lang.ASTNode
*
* @see RuleSet
*/
abstract class Rule(
public open class Rule(
val id: String,
public val visitorModifiers: Set<VisitorModifier> = emptySet()
) {
Expand All @@ -24,17 +25,68 @@ abstract class Rule(
}

/**
* This method is going to be executed for each node in AST (in DFS fashion).
* This method is called once before the first node is visited. It can be used to initialize the state of the rule
* before processing of nodes starts.
*/
@Suppress("UNUSED_PARAMETER")
public open fun beforeFirstNode(editorConfigProperties: EditorConfigProperties) {}

/**
* This method is called on a node in AST before visiting the child nodes. This is repeated recursively for the
* child nodes resulting in a depth first traversal of the AST.
*
* @param node AST node
* @param autoCorrect indicates whether rule should attempt auto-correction
* @param emit a way for rule to notify about a violation (lint error)
*/
abstract fun visit(
public open fun beforeVisitChildNodes(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) =
/**
* For backwards compatibility with ktlint 0.46.x or before, call [visit] when not implemented on node.
* Add abstract function modifier and remove function body after removal of deprecated [visit] method to enforce
* explicit implementation by rule developer.
*/
visit(node, autoCorrect, emit)

/**
* Rules that override method [visit] should rename that method to [beforeVisitChildNodes]. For backwards
* compatibility reasons (in KtLint 0.47 only), this method is called via the default implementation of
* [beforeVisitChildNodes]. Whenever [beforeVisitChildNodes] is overridden with a custom implementation, this method
* will no longer be called.
*
* @param node AST node
* @param autoCorrect indicates whether rule should attempt auto-correction
* @param emit a way for rule to notify about a violation (lint error)
*/
@Deprecated(
message = "Marked for deletion in ktlint 0.48.0",
replaceWith = ReplaceWith("beforeVisitChildNodes(node, autocorrect, emit")
)
@Suppress("UNUSED_PARAMETER")
public open fun visit(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {}

/**
* This method is called on a node in AST after all its child nodes have been visited.
*/
@Suppress("unused", "UNUSED_PARAMETER")
public open fun afterVisitChildNodes(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
) {}

/**
* This method is called once after the last node in the AST is visited. It can be used for teardown of the state
* of the rule.
*/
public open fun afterLastNode() {}

sealed class VisitorModifier {

Expand All @@ -56,6 +108,12 @@ abstract class Rule(

object RunAsLateAsPossible : VisitorModifier()

@Deprecated(
"""
Marked for removal in Ktlint 0.48. This modifier blocks the ability to suppress ktlint rules. See
changelog Ktlint 0.47 for details on how to modify a rule using this modifier.
"""
)
object RunOnRootNodeOnly : VisitorModifier()
}
}
Expand Up @@ -5,6 +5,7 @@ import com.pinterest.ktlint.core.KtLint
import com.pinterest.ktlint.core.api.DefaultEditorConfigProperties.CodeStyleValue
import com.pinterest.ktlint.core.api.DefaultEditorConfigProperties.CodeStyleValue.android
import com.pinterest.ktlint.core.api.DefaultEditorConfigProperties.CodeStyleValue.official
import com.pinterest.ktlint.core.api.DefaultEditorConfigProperties.codeStyleSetProperty
import com.pinterest.ktlint.core.initKtLintKLogger
import mu.KotlinLogging
import org.ec4j.core.model.Property
Expand Down Expand Up @@ -42,18 +43,31 @@ public interface UsesEditorConfigProperties {
* Get the value of [EditorConfigProperty] based on loaded [EditorConfigProperties] content for the current
* [ASTNode].
*/
public fun <T> EditorConfigProperties.getEditorConfigValue(editorConfigProperty: EditorConfigProperty<T>): T {
require(editorConfigProperties.contains(editorConfigProperty)) {
"EditorConfigProperty '${editorConfigProperty.type.name}' may only be retrieved when it is registered in the editorConfigProperties."
}
val codeStyle = getEditorConfigValue(codeStyleSetProperty, official)
return getEditorConfigValue(editorConfigProperty, codeStyle)
}

/**
* Get the value of [EditorConfigProperty] based on loaded [EditorConfigProperties] content for the current
* [ASTNode].
*/
@Deprecated(message = "Marked for deletion in Ktlint 0.48. EditorConfigProperties are now supplied to Rule via call on method beforeFirstNode")
public fun <T> ASTNode.getEditorConfigValue(editorConfigProperty: EditorConfigProperty<T>): T {
require(editorConfigProperties.contains(editorConfigProperty)) {
"EditorConfigProperty '${editorConfigProperty.type.name}' may only be retrieved when it is registered in the editorConfigProperties."
}
val editorConfigPropertyValues = getUserData(KtLint.EDITOR_CONFIG_PROPERTIES_USER_DATA_KEY)!!
val codeStyle = editorConfigPropertyValues.getEditorConfigValue(DefaultEditorConfigProperties.codeStyleSetProperty)
val codeStyle = editorConfigPropertyValues.getEditorConfigValue(codeStyleSetProperty, official)
return editorConfigPropertyValues.getEditorConfigValue(editorConfigProperty, codeStyle)
}

private fun <T> EditorConfigProperties.getEditorConfigValue(
editorConfigProperty: EditorConfigProperty<T>,
codeStyleValue: CodeStyleValue = official
codeStyleValue: CodeStyleValue
): T {
val property = get(editorConfigProperty.type.name)

Expand Down
Expand Up @@ -232,11 +232,13 @@ fun LeafElement.upsertWhitespaceAfterMe(text: String): LeafElement {
}
}

@Deprecated(message = "Marked for removal in Ktlint 0.48. See Ktlint 0.47.0 changelog for more information.")
fun ASTNode.visit(enter: (node: ASTNode) -> Unit) {
enter(this)
this.getChildren(null).forEach { it.visit(enter) }
}

@Deprecated(message = "Marked for removal in Ktlint 0.48. See Ktlint 0.47.0 changelog for more information.")
fun ASTNode.visit(enter: (node: ASTNode) -> Unit, exit: (node: ASTNode) -> Unit) {
enter(this)
this.getChildren(null).forEach { it.visit(enter, exit) }
Expand Down
@@ -0,0 +1,91 @@
package com.pinterest.ktlint.core.internal

import com.pinterest.ktlint.core.KtLint
import com.pinterest.ktlint.core.ParseException
import com.pinterest.ktlint.core.api.EditorConfigProperties
import java.nio.file.Paths
import org.jetbrains.kotlin.com.intellij.lang.FileASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.com.intellij.psi.PsiErrorElement
import org.jetbrains.kotlin.idea.KotlinLanguage
import org.jetbrains.kotlin.psi.KtFile

private val kotlinPsiFileFactoryProvider = KotlinPsiFileFactoryProvider()

internal class PreparedCode(
val rootNode: FileASTNode,
val editorConfigProperties: EditorConfigProperties,
val positionInTextLocator: (offset: Int) -> LineAndColumn,
var suppressedRegionLocator: SuppressionLocator
)

internal fun prepareCodeForLinting(params: KtLint.ExperimentalParams): PreparedCode {
val psiFileFactory = kotlinPsiFileFactoryProvider.getKotlinPsiFileFactory(params.isInvokedFromCli)
val normalizedText = normalizeText(params.text)
val positionInTextLocator = buildPositionInTextLocator(normalizedText)

val psiFileName = if (params.script) {
"file.kts"
} else {
"file.kt"
}
val psiFile = psiFileFactory.createFileFromText(
psiFileName,
KotlinLanguage.INSTANCE,
normalizedText
) as KtFile

val errorElement = psiFile.findErrorElement()
if (errorElement != null) {
val (line, col) = positionInTextLocator(errorElement.textOffset)
throw ParseException(line, col, errorElement.errorDescription)
}

val rootNode = psiFile.node

val editorConfigProperties = KtLint.editorConfigLoader.loadPropertiesForFile(
params.normalizedFilePath,
params.isStdIn,
params.editorConfigPath?.let { Paths.get(it) },
params.rules,
params.editorConfigOverride,
params.debug
)

if (!params.isStdIn) {
rootNode.putUserData(KtLint.FILE_PATH_USER_DATA_KEY, params.normalizedFilePath.toString())
}

// Keep for backwards compatibility in Ktlint 0.47.0 until ASTNode.getEditorConfigValue in UsesEditorConfigProperties
// is removed
rootNode.putUserData(KtLint.EDITOR_CONFIG_PROPERTIES_USER_DATA_KEY, editorConfigProperties)

val suppressedRegionLocator = SuppressionLocatorBuilder.buildSuppressedRegionsLocator(rootNode)

return PreparedCode(
rootNode,
editorConfigProperties,
positionInTextLocator,
suppressedRegionLocator
)
}

private fun normalizeText(text: String): String {
return text
.replace("\r\n", "\n")
.replace("\r", "\n")
.replaceFirst(KtLint.UTF8_BOM, "")
}

private fun PsiElement.findErrorElement(): PsiErrorElement? {
if (this is PsiErrorElement) {
return this
}
this.children.forEach { child ->
val errorElement = child.findErrorElement()
if (errorElement != null) {
return errorElement
}
}
return null
}
@@ -1,7 +1,6 @@
package com.pinterest.ktlint.core.internal

import com.pinterest.ktlint.core.ast.prevLeaf
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.PsiWhiteSpace
Expand Down Expand Up @@ -60,7 +59,7 @@ internal object SuppressionLocatorBuilder {
): List<SuppressionHint> {
val result = ArrayList<SuppressionHint>()
val open = ArrayList<SuppressionHint>()
rootNode.visit { node ->
rootNode.collect { node ->
if (node is PsiComment) {
val text = node.getText()
if (text.startsWith("//")) {
Expand All @@ -85,7 +84,7 @@ internal object SuppressionLocatorBuilder {
val openingHint = open.removeAt(openHintIndex)
result.add(
SuppressionHint(
IntRange(openingHint.range.first, node.startOffset),
IntRange(openingHint.range.first, node.startOffset - 1),
disabledRules
)
)
Expand All @@ -110,6 +109,13 @@ internal object SuppressionLocatorBuilder {
return result
}

private fun ASTNode.collect(block: (node: ASTNode) -> Unit) {
block(this)
this
.getChildren(null)
.forEach { it.collect(block) }
}

private fun parseHintArgs(
commentText: String,
key: String
Expand Down

0 comments on commit df43622

Please sign in to comment.