Skip to content

Commit

Permalink
Merge pull request #1230 from paul-dingemans/conditional-ordering-of-…
Browse files Browse the repository at this point in the history 
…rules

Conditional ordering of rules
  • Loading branch information
@paul-dingemans
paul-dingemans authored Jan 22, 2022
2 parents 77c60e5 + bdd027d commit 7bb1d47
Show file tree
Hide file tree
Showing 18 changed files with 1,416 additions and 208 deletions.
214 changes: 99 additions & 115 deletions ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package com.pinterest.ktlint.core
import com.pinterest.ktlint.core.api.EditorConfigProperties
import com.pinterest.ktlint.core.api.FeatureInAlphaState
import com.pinterest.ktlint.core.api.UsesEditorConfigProperties
import com.pinterest.ktlint.core.ast.visit
import com.pinterest.ktlint.core.internal.EditorConfigGenerator
import com.pinterest.ktlint.core.internal.EditorConfigLoader
import com.pinterest.ktlint.core.internal.EditorConfigLoader.Companion.convertToRawValues
Expand Down Expand Up @@ -133,9 +132,18 @@ public object KtLint {
* @throws ParseException if text is not a valid Kotlin code
* @throws RuleExecutionException in case of internal failure caused by a bug in rule implementation
*/
// TODO: Shouldn't this method be moved to module ktlint-test as it is called from unit tests only? It will be a
// breaking change for custom rule set implementations.
@OptIn(FeatureInAlphaState::class)
public fun lint(params: Params) {
lint(toExperimentalParams(params))
lint(
toExperimentalParams(params),
VisitorProvider(
ruleSets = params.ruleSets,
debug = params.debug,
isUnitTestContext = true
)
)
}

/**
Expand All @@ -144,35 +152,43 @@ public object KtLint {
* @throws ParseException if text is not a valid Kotlin code
* @throws RuleExecutionException in case of internal failure caused by a bug in rule implementation
*/
// TODO: Shouldn't this method be moved to module ktlint-test as it is called from unit tests only? It will be a
// breaking change for custom rule set implementations.
@FeatureInAlphaState
public fun lint(params: ExperimentalParams) {
public fun lint(
params: ExperimentalParams,
visitorProvider: VisitorProvider
) {
val psiFileFactory = kotlinPsiFileFactoryProvider.getKotlinPsiFileFactory(params.isInvokedFromCli)
val preparedCode = prepareCodeForLinting(psiFileFactory, params)
val errors = mutableListOf<LintError>()

visitor(preparedCode.rootNode, params.ruleSets).invoke { node, rule, fqRuleId ->
// 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)
) {
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)
) {
return@visit
visitorProvider
.visitor(params.ruleSets, preparedCode.rootNode)
.invoke { node, rule, fqRuleId ->
// 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)
) {
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)
) {
return@visit
}
val (line, col) = preparedCode.positionInTextLocator(offset)
errors.add(LintError(line, col, fqRuleId, errorMessage, canBeAutoCorrected))
}
val (line, col) = preparedCode.positionInTextLocator(offset)
errors.add(LintError(line, col, fqRuleId, errorMessage, canBeAutoCorrected))
} catch (e: Exception) {
val (line, col) = preparedCode.positionInTextLocator(node.startOffset)

throw RuleExecutionException(line, col, fqRuleId, e)
}
} catch (e: Exception) {
val (line, col) = preparedCode.positionInTextLocator(node.startOffset)
throw RuleExecutionException(line, col, fqRuleId, e)
}
}
}

errors
.sortedWith { l, r -> if (l.line != r.line) l.line - r.line else l.col - r.col }
Expand Down Expand Up @@ -270,75 +286,6 @@ public object KtLint {
)
}

private fun visitor(
rootNode: ASTNode,
ruleSets: Iterable<RuleSet>,
concurrent: Boolean = true,
filter: (rootNode: ASTNode, fqRuleId: String) -> Boolean = this::filterDisabledRules

): ((node: ASTNode, rule: Rule, fqRuleId: String) -> Unit) -> Unit {
val fqrsRestrictedToRoot = mutableListOf<Pair<String, Rule>>()
val fqrs = mutableListOf<Pair<String, Rule>>()
val fqrsExpectedToBeExecutedLastOnRoot = mutableListOf<Pair<String, Rule>>()
val fqrsExpectedToBeExecutedLast = mutableListOf<Pair<String, Rule>>()
for (ruleSet in ruleSets) {
val prefix = if (ruleSet.id === "standard") "" else "${ruleSet.id}:"
for (rule in ruleSet) {
val fqRuleId = "$prefix${rule.id}"
if (!filter(rootNode, fqRuleId)) {
continue
}
val fqr = fqRuleId to rule
when (rule) {
is Rule.Modifier.Last -> fqrsExpectedToBeExecutedLast.add(fqr)
is Rule.Modifier.RestrictToRootLast -> fqrsExpectedToBeExecutedLastOnRoot.add(fqr)
is Rule.Modifier.RestrictToRoot -> fqrsRestrictedToRoot.add(fqr)
else -> fqrs.add(fqr)
}
}
}
return { visit ->
for ((fqRuleId, rule) in fqrsRestrictedToRoot) {
visit(rootNode, rule, fqRuleId)
}
if (concurrent) {
rootNode.visit { node ->
for ((fqRuleId, rule) in fqrs) {
visit(node, rule, fqRuleId)
}
}
} else {
for ((fqRuleId, rule) in fqrs) {
rootNode.visit { node ->
visit(node, rule, fqRuleId)
}
}
}
for ((fqRuleId, rule) in fqrsExpectedToBeExecutedLastOnRoot) {
visit(rootNode, rule, fqRuleId)
}
if (fqrsExpectedToBeExecutedLast.isNotEmpty()) {
if (concurrent) {
rootNode.visit { node ->
for ((fqRuleId, rule) in fqrsExpectedToBeExecutedLast) {
visit(node, rule, fqRuleId)
}
}
} else {
for ((fqRuleId, rule) in fqrsExpectedToBeExecutedLast) {
rootNode.visit { node ->
visit(node, rule, fqRuleId)
}
}
}
}
}
}

private fun filterDisabledRules(rootNode: ASTNode, fqRuleId: String): Boolean {
return rootNode.getUserData(DISABLED_RULES)?.contains(fqRuleId) == false
}

@Deprecated(
message = "Should not be a part of public api. Will be removed in future release.",
level = DeprecationLevel.WARNING
Expand All @@ -355,8 +302,21 @@ public object KtLint {
* @throws ParseException if text is not a valid Kotlin code
* @throws RuleExecutionException in case of internal failure caused by a bug in rule implementation
*/
// TODO: Shouldn't this method be moved to module ktlint-test as it is called from unit tests only? It will be a
// breaking change for custom rule set implementations.
@OptIn(FeatureInAlphaState::class)
public fun format(params: Params): String = format(toExperimentalParams(params))
public fun format(params: Params): String {
val toExperimentalParams = toExperimentalParams(params)
return format(
toExperimentalParams,
toExperimentalParams.ruleSets,
VisitorProvider(
ruleSets = toExperimentalParams.ruleSets,
debug = toExperimentalParams.debug,
isUnitTestContext = true
)
)
}

/**
* Fix style violations.
Expand All @@ -365,15 +325,23 @@ public object KtLint {
* @throws RuleExecutionException in case of internal failure caused by a bug in rule implementation
*/
@FeatureInAlphaState
public fun format(params: ExperimentalParams): String {
public fun format(
params: ExperimentalParams,
ruleSets: Iterable<RuleSet>,
visitorProvider: VisitorProvider
): String {
val hasUTF8BOM = params.text.startsWith(UTF8_BOM)
val psiFileFactory = kotlinPsiFileFactoryProvider.getKotlinPsiFileFactory(params.isInvokedFromCli)
val preparedCode = prepareCodeForLinting(psiFileFactory, params)

var tripped = false
var mutated = false
visitor(preparedCode.rootNode, params.ruleSets, concurrent = false)
.invoke { node, rule, fqRuleId ->
visitorProvider
.visitor(
ruleSets,
preparedCode.rootNode,
concurrent = false
).invoke { node, rule, fqRuleId ->
// 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 (
Expand All @@ -400,30 +368,46 @@ public object KtLint {
}
if (tripped) {
val errors = mutableListOf<Pair<LintError, Boolean>>()
visitor(preparedCode.rootNode, params.ruleSets).invoke { node, rule, fqRuleId ->
// 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)
) {
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)
) {
return@visit
visitorProvider
.visitor(ruleSets, preparedCode.rootNode)
.invoke { node, rule, fqRuleId ->
// 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
)
) {
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
)
) {
return@visit
}
val (line, col) = preparedCode.positionInTextLocator(offset)
errors.add(
Pair(
LintError(line, col, fqRuleId, errorMessage, canBeAutoCorrected),
false
)
)
}
val (line, col) = preparedCode.positionInTextLocator(offset)
errors.add(Pair(LintError(line, col, fqRuleId, errorMessage, canBeAutoCorrected), false))
} catch (e: Exception) {
val (line, col) = preparedCode.positionInTextLocator(node.startOffset)

throw RuleExecutionException(line, col, fqRuleId, e)
}
} catch (e: Exception) {
val (line, col) = preparedCode.positionInTextLocator(node.startOffset)
throw RuleExecutionException(line, col, fqRuleId, e)
}
}
}

errors
.sortedWith { (l), (r) -> if (l.line != r.line) l.line - r.line else l.col - r.col }
Expand Down
55 changes: 42 additions & 13 deletions ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/Rule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.pinterest.ktlint.core

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

/**
* A rule contract.
Expand All @@ -11,11 +10,15 @@ import org.jetbrains.kotlin.com.intellij.lang.FileASTNode
* (provided [RuleSetProvider] creates a new instance on each `get()` call).
*
* @param id must be unique within the ruleset
* @param visitorModifiers: set of modifiers of the visitor. Preferably a rule has no modifiers at all, meaning that it
* is completely independent of all other rules.
*
* @see RuleSet
*/
abstract class Rule(val id: String) {

abstract class Rule(
val id: String,
public val visitorModifiers: Set<VisitorModifier> = emptySet()
) {
init {
IdNamingPolicy.enforceNaming(id)
}
Expand All @@ -34,19 +37,45 @@ abstract class Rule(val id: String) {
)

object Modifier {
/**
* Any rule implementing this interface will be given root ([FileASTNode]) node only
* (in other words, [visit] will be called on [FileASTNode] but not on [FileASTNode] children).
*/
@Deprecated(
message = "Marked for deletion in a future version. Add 'VisitorModifier.RunOnRootNodeOnly' to the rule.",
level = DeprecationLevel.WARNING
)
interface RestrictToRoot

/**
* Marker interface to indicate that rule must be executed after all other rules (order among multiple
* [RestrictToRootLast] rules is not defined and should be assumed to be random).
*
* Note that [RestrictToRootLast] implements [RestrictToRoot].
*/
@Deprecated(
message = "Marked for deletion in a future version. Add 'VisitorModifier.RunOnRootNodeOnly' and 'VisitorModifier.RunAsLateAsPossible' to the rule.",
level = DeprecationLevel.WARNING
)
interface RestrictToRootLast : RestrictToRoot

@Deprecated(
message = "Marked for deletion in a future version. Add 'VisitorModifier.RunAsLateAsPossible' to the rule.",
level = DeprecationLevel.WARNING
)
interface Last
}

sealed class VisitorModifier {

data class RunAfterRule(
/**
* Qualified ruleId in format "ruleSetId:ruleId". For a rule in the standard rule set it suffices to specify
* the ruleId only.
*/
val ruleId: String,
/**
* The annotated rule will only be loaded in case the other rule is loaded as well.
*/
val loadOnlyWhenOtherRuleIsLoaded: Boolean = false,
/**
* The annotated rule will only be run in case the other rule is enabled.
*/
val runOnlyWhenOtherRuleIsEnabled: Boolean = false
) : VisitorModifier()

object RunAsLateAsPossible : VisitorModifier()

object RunOnRootNodeOnly : VisitorModifier()
}
}
Loading

0 comments on commit 7bb1d47

Please sign in to comment.