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.

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

Let API Consumer decide whether a LintError has to be autocorrected, or not #2671

Merged
merged 33 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
9b57c23
Add RuleAutocorrectApproveHandler interface
paul-dingemans May 15, 2024
dbb21c5
Extract CodeLinter and CodeFormatter from KtLintRuleEngine
paul-dingemans May 18, 2024
8710723
Make interactiveFormat actually work
paul-dingemans May 19, 2024
faef0a4
Simplifly SuppressHandler
paul-dingemans May 19, 2024
a930246
Update api contract
paul-dingemans May 19, 2024
8187494
Refactor and remove SuppressHandler
paul-dingemans May 19, 2024
358087d
Refactor AutoCorrectOffsetRangeHandler to RangeAutoCorrectHandler so …
paul-dingemans May 19, 2024
f27e4ff
Disable autocorrect in rules that do not implement the RuleApproveAut…
paul-dingemans May 22, 2024
b5cc08b
Ensure backward compatibility by adding parameter used to set the def…
paul-dingemans May 23, 2024
8084c01
Add FormatDecision as return result from callback used in format func…
paul-dingemans May 23, 2024
38f27d2
Update documentation
paul-dingemans May 24, 2024
998bc31
Refactor FormatDecision to AutocorrectDecision
paul-dingemans May 24, 2024
c8c9f51
Update ktlint-api-consumer
paul-dingemans May 25, 2024
913d259
Fix all rules
paul-dingemans May 25, 2024
171e9d2
Fix "cannot be auto-corrected" in lint error log
paul-dingemans May 26, 2024
1db5cae
Use new format function in ktlint CLI
paul-dingemans May 26, 2024
4b3c5ea
Rename variable
paul-dingemans May 26, 2024
fe6e0fd
Fix documentation
paul-dingemans May 26, 2024
a89d9ab
Add deprecation notice
paul-dingemans May 26, 2024
10c2236
Improve API documentation
paul-dingemans May 26, 2024
8aa7bf0
Update API contract
paul-dingemans May 26, 2024
a54218e
Update documentation
paul-dingemans May 27, 2024
c3504d0
Remove CodeLinter
paul-dingemans May 27, 2024
dede1d4
Update log message
paul-dingemans May 28, 2024
22b0efc
Inline extension methods
paul-dingemans May 28, 2024
ec0b87f
Rename methods
paul-dingemans May 28, 2024
004c7a7
Try to fix test on Windows
paul-dingemans May 28, 2024
9363bda
Try to fix test on Windows
paul-dingemans May 28, 2024
78cb2d8
Replace call to deprecated "format", except the one that need to test…
paul-dingemans May 28, 2024
036fcbb
Try to fix test Windows OS
paul-dingemans May 28, 2024
f9f7847
Try to fix test Windows OS
paul-dingemans May 28, 2024
ab72b63
Try to fix test Windows OS
paul-dingemans May 28, 2024
3460464
Try to fix test Windows OS
paul-dingemans May 28, 2024
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
Prev Previous commit
Next Next commit
Make interactiveFormat actually work
  • Loading branch information
paul-dingemans committed May 19, 2024
commit 8710723372d963faca7a2e29e55c003cceff9d85
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import com.pinterest.ktlint.rule.engine.api.KtLintRuleEngine
import com.pinterest.ktlint.rule.engine.api.LintError
import com.pinterest.ktlint.rule.engine.core.api.ElementType
import com.pinterest.ktlint.rule.engine.core.api.Rule
import com.pinterest.ktlint.rule.engine.core.api.RuleAutocorrectApproveHandler
import com.pinterest.ktlint.rule.engine.core.api.RuleId
import com.pinterest.ktlint.rule.engine.core.api.RuleProvider
import com.pinterest.ktlint.rule.engine.core.api.editorconfig.EXPERIMENTAL_RULES_EXECUTION_PROPERTY
Expand Down Expand Up @@ -313,6 +314,42 @@ class KtLintRuleEngineTest {
""".trimIndent(),
)
}

@Test
fun `Given a kotlin code snippet that does contain multiple indentation errors then only format the lint error at specific offset and message`() {
val ktLintRuleEngine =
KtLintRuleEngine(
ruleProviders =
setOf(
RuleProvider { IndentationRule() },
),
fileSystem = ktlintTestFileSystem.fileSystem,
)

val originalCode =
"""
fun main() {
println("Hello world!")
println("Hello world!")
println("Hello world!")
}
""".trimIndent()
val actual =
ktLintRuleEngine
.interactiveFormat(Code.fromSnippet(originalCode)) { lintError ->
lintError.line == 3 && lintError.detail == "Unexpected indentation (0) (should be 4)"
}

assertThat(actual).isEqualTo(
"""
fun main() {
println("Hello world!")
println("Hello world!")
println("Hello world!")
}
""".trimIndent(),
)
}
}

@Test
Expand Down Expand Up @@ -358,14 +395,14 @@ class KtLintRuleEngineTest {
ruleId = NO_VAR_RULE_ID,
about = About(),
),
RuleAutocorrectApproveHandler,
Rule.Experimental {
override fun beforeVisitChildNodes(
node: ASTNode,
autoCorrect: Boolean,
emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit,
emitAndApprove: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Boolean,
) {
if (node.elementType == ElementType.VAR_KEYWORD) {
emit(node.startOffset, "Unexpected var, use val instead", false)
emitAndApprove(node.startOffset, "Unexpected var, use val instead", false)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import com.pinterest.ktlint.rule.engine.internal.EditorConfigFinder
import com.pinterest.ktlint.rule.engine.internal.EditorConfigGenerator
import com.pinterest.ktlint.rule.engine.internal.EditorConfigLoader
import com.pinterest.ktlint.rule.engine.internal.EditorConfigLoaderEc4j
import com.pinterest.ktlint.rule.engine.internal.LintErrorAutoCorrectHandler
import com.pinterest.ktlint.rule.engine.internal.RuleExecutionContext.Companion.createRuleExecutionContext
import com.pinterest.ktlint.rule.engine.internal.ThreadSafeEditorConfigCache.Companion.THREAD_SAFE_EDITOR_CONFIG_CACHE
import org.ec4j.core.Resource
Expand Down Expand Up @@ -95,6 +96,9 @@ public class KtLintRuleEngine(
* If [code] contains lint errors which have been autocorrected, then the resulting code is formatted again (up until
* [MAX_FORMAT_RUNS_PER_FILE] times) in order to fix lint errors that might result from the previous formatting run.
*
* [callback] is invoked once for each unique [LintError] found during all runs. Note that [callback] is only invoked once all format
* runs have been completed.
*
* @throws KtLintParseException if text is not a valid Kotlin code
* @throws KtLintRuleException in case of internal failure caused by a bug in rule implementation
*/
Expand Down Expand Up @@ -169,8 +173,7 @@ public class KtLintRuleEngine(
): String =
codeFormatter.format(
code = code,
autoCorrectHandler = AutoCorrectEnabledHandler,
callback = { lintError, _ -> callback(lintError) },
autoCorrectHandler = LintErrorAutoCorrectHandler(callback),
maxFormatRunsPerFile = 1,
)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,41 @@
package com.pinterest.ktlint.rule.engine.internal

import com.pinterest.ktlint.rule.engine.api.LintError

/**
* Handler which determines whether autocorrect should be enabled or disabled for the given offset.
*/
internal sealed interface AutoCorrectHandler {
fun autoCorrect(offset: Int): Boolean

fun autoCorrect(lintError: LintError): Boolean
}

internal data object AutoCorrectDisabledHandler : AutoCorrectHandler {
override fun autoCorrect(offset: Int) = false

override fun autoCorrect(lintError: LintError) = false
}

internal data object AutoCorrectEnabledHandler : AutoCorrectHandler {
override fun autoCorrect(offset: Int) = true

override fun autoCorrect(lintError: LintError) = true
}

@Deprecated(message = "Replace with LintErrorAutocorrectHandler")
internal class AutoCorrectOffsetRangeHandler(
private val autoCorrectOffsetRange: IntRange,
) : AutoCorrectHandler {
override fun autoCorrect(offset: Int) = offset in autoCorrectOffsetRange

override fun autoCorrect(lintError: LintError) = throw IllegalStateException()
}

internal class LintErrorAutoCorrectHandler(
private val callback: (LintError) -> Boolean,
) : AutoCorrectHandler {
override fun autoCorrect(offset: Int) = true

override fun autoCorrect(lintError: LintError) = callback(lintError)
}
Original file line number Diff line number Diff line change
Expand Up @@ -112,30 +112,37 @@ internal class CodeFormatter(
): Set<Pair<LintError, Boolean>> {
val errors = mutableSetOf<Pair<LintError, Boolean>>()
executeRule(rule, autoCorrectHandler) { offset, errorMessage, canBeAutoCorrected ->
if (canBeAutoCorrected) {
// TODO: send LintError to external approve handler
val (line, col) = positionInTextLocator(offset)
val lintError = LintError(line, col, rule.ruleId, errorMessage, canBeAutoCorrected)
val autoCorrect =
if (canBeAutoCorrected) {
if (autoCorrectHandler is AutoCorrectOffsetRangeHandler) {
autoCorrectHandler.autoCorrect(offset)
} else {
autoCorrectHandler.autoCorrect(lintError)
}
} else {
false
}
if (autoCorrect) {
/*
* Rebuild the suppression locator after each change in the AST as the offsets of the suppression hints might
* have changed.
*/
rebuildSuppressionLocator()
}
val (line, col) = positionInTextLocator(offset)
LintError(line, col, rule.ruleId, errorMessage, canBeAutoCorrected)
.let { lintError ->
errors.add(
Pair(
lintError,
// It is assumed that a rule that emits that an error can be autocorrected, also does correct the error.
canBeAutoCorrected,
),
)
// In trace mode report the violation immediately. The order in which violations are actually found might be
// different from the order in which they are reported. For debugging purposes it can be helpful to know the
// exact order in which violations are being solved.
LOGGER.trace { "Format violation: ${lintError.logMessage(code)}" }
}
canBeAutoCorrected
errors.add(
Pair(
lintError,
// It is assumed that a rule that asks for autocorrect approval, also does correct the error.
autoCorrect,
),
)
// In trace mode report the violation immediately. The order in which violations are actually found might be
// different from the order in which they are reported. For debugging purposes it can be helpful to know the
// exact order in which violations are being solved.
LOGGER.trace { "Format violation: ${lintError.logMessage(code)}" }
autoCorrect
}
return errors
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,24 +91,24 @@ internal class RuleExecutionContext private constructor(
val suppressHandler = SuppressHandler(suppressionLocator, autoCorrectHandler, emitAndApprove)
if (rule.shouldContinueTraversalOfAST()) {
try {
executeRuleOnNodeRecursively(node, rule, autoCorrectHandler, suppressHandler)
executeRuleOnNodeRecursively(node, rule, autoCorrectHandler, suppressHandler, emitAndApprove)
} catch (e: Exception) {
if (autoCorrectHandler.autoCorrect(node.startOffset)) {
// line/col cannot be reliably mapped as exception might originate from a node not present in the
// original AST
if (autoCorrectHandler is AutoCorrectDisabledHandler) {
val (line, col) = positionInTextLocator(node.startOffset)
throw RuleExecutionException(
rule,
0,
0,
line,
col,
// Prevent extreme long stack trace caused by recursive call and only pass root cause
e.cause ?: e,
)
} else {
val (line, col) = positionInTextLocator(node.startOffset)
// line/col cannot be reliably mapped as exception might originate from a node not present in the
// original AST
throw RuleExecutionException(
rule,
line,
col,
0,
0,
// Prevent extreme long stack trace caused by recursive call and only pass root cause
e.cause ?: e,
)
Expand All @@ -122,6 +122,7 @@ internal class RuleExecutionContext private constructor(
rule: Rule,
autoCorrectHandler: AutoCorrectHandler,
suppressHandler: SuppressHandler,
emitAndApprove: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Boolean,
) {
if (rule is RuleAutocorrectApproveHandler) {
suppressHandler.handle(node, rule.ruleId) { emitAndApprove ->
Expand All @@ -139,14 +140,12 @@ internal class RuleExecutionContext private constructor(
node
.getChildren(null)
.forEach { childNode ->
suppressHandler.handle(childNode, rule.ruleId) { emitAndApprove ->
this.executeRuleOnNodeRecursively(
childNode,
rule,
autoCorrectHandler,
emitAndApprove,
)
}
this.executeRuleOnNodeRecursively(
childNode,
rule,
autoCorrectHandler,
emitAndApprove,
)
}
}
if (rule is RuleAutocorrectApproveHandler) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ internal class SuppressHandler(
node.startOffset,
ruleId,
)
val autoCorrect = this.autoCorrectHandler.autoCorrect(node.startOffset) && !suppress
val autoCorrect = this.autoCorrectHandler !is AutoCorrectDisabledHandler && !suppress
val emit =
if (suppress) {
SUPPRESS_EMIT
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,42 +120,41 @@ internal object SuppressionLocatorBuilder {
private fun ASTNode.createSuppressionHintFromEolComment(formatterTags: FormatterTags): CommentSuppressionHint? =
text
.removePrefix("//")
.trim()
.split(" ")
.takeIf { it.isNotEmpty() }
?.takeIf { it[0] == formatterTags.formatterTagOff }
?.let { parts ->
CommentSuppressionHint(
this,
HashSet(parts.tail()),
EOL,
)
}
.let { createCommentSuppressionHint(it, formatterTags) }

private fun ASTNode.createSuppressionHintFromBlockComment(formatterTags: FormatterTags): CommentSuppressionHint? =
text
.removePrefix("/*")
.removeSuffix("*/")
.trim()
.split(" ")
.takeIf { it.isNotEmpty() }
?.let { parts ->
if (parts[0] == formatterTags.formatterTagOff) {
.let { createCommentSuppressionHint(it, formatterTags) }

private fun ASTNode.createCommentSuppressionHint(
comment: String,
formatterTags: FormatterTags,
) = comment
.trim()
.split(" ")
.takeIf { it.isNotEmpty() }
?.let { parts ->
when (parts[0]) {
formatterTags.formatterTagOff ->
CommentSuppressionHint(
this,
HashSet(parts.tail()),
BLOCK_START,
)
} else if (parts[0] == formatterTags.formatterTagOn) {

formatterTags.formatterTagOn ->
CommentSuppressionHint(
this,
HashSet(parts.tail()),
BLOCK_END,
)
} else {

else ->
null
}
}
}

private fun MutableList<CommentSuppressionHint>.toSuppressionHints(rootNode: ASTNode): MutableList<SuppressionHint> {
val suppressionHints = mutableListOf<SuppressionHint>()
Expand Down
Loading