Skip to content

Commit

Permalink
Merge pull request #2132 from pinterest/2130-upsert-whitespace
Browse files Browse the repository at this point in the history 
Fix solving problems in 3 consecutive runs
  • Loading branch information
@paul-dingemans
paul-dingemans authored Jul 15, 2023
2 parents 7d8a509 + 5c28144 commit 76016af
Show file tree
Hide file tree
Showing 6 changed files with 219 additions and 83 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
* Allow to disable ktlint in `.editorconfig` for a glob ([#2100](https://github.com/pinterest/ktlint/issues/2100))
* Fix wrapping of nested function literals `wrapping` ([#2106](https://github.com/pinterest/ktlint/issues/2106))
* Do not indent class body for classes having a long super type list in code style `ktlint_official` as it is inconsistent compared to other class bodies `indent` [#2115](https://github.com/pinterest/ktlint/issues/2115)
* Log message `Format was not able to resolve all violations which (theoretically) can be autocorrected in file ... in 3 consecutive runs of format` is now only displayed in case a new ktlint rule is actually needed. [#2129](https://github.com/pinterest/ktlint/issues/2129)
* Fix wrapping of function signature in case the opening brace of the function body block exceeds the maximum line length. Fix upsert of whitespace into composite nodes. `function-signature` [#2130](https://github.com/pinterest/ktlint/issues/2130)
* Fix spacing around colon in annotations `spacing-around-colon` ([#2093](https://github.com/pinterest/ktlint/issues/2093))

### Changed
Expand Down
38 changes: 33 additions & 5 deletions ...engine-core/src/main/kotlin/com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtension.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,23 @@ public fun ASTNode.upsertWhitespaceBeforeMe(text: String) {
}
}
} else {
val prevLeaf =
requireNotNull(prevLeaf()) {
"Can not upsert a whitespace if the first node is a non-leaf node"
when (val prevSibling = prevSibling()) {
null -> {
// Never insert a whitespace element as first child node in a composite node. Instead, upsert just before the composite node
treeParent.upsertWhitespaceBeforeMe(text)
}
prevLeaf.upsertWhitespaceAfterMe(text)

is LeafElement -> {
prevSibling.upsertWhitespaceAfterMe(text)
}

else -> {
// Insert in between two composite nodes
PsiWhiteSpaceImpl(text).also { psiWhiteSpace ->
treeParent.addChild(psiWhiteSpace.node, this)
}
}
}
}
}

Expand Down Expand Up @@ -279,7 +291,23 @@ public fun ASTNode.upsertWhitespaceAfterMe(text: String) {
}
}
} else {
lastChildLeafOrSelf().upsertWhitespaceAfterMe(text)
when (val nextSibling = nextSibling()) {
null -> {
// Never insert a whitespace element as last child node in a composite node. Instead, upsert just after the composite node
treeParent.upsertWhitespaceAfterMe(text)
}

is LeafElement -> {
nextSibling.upsertWhitespaceBeforeMe(text)
}

else -> {
// Insert in between two composite nodes
PsiWhiteSpaceImpl(text).also { psiWhiteSpace ->
treeParent.addChild(psiWhiteSpace.node, nextSibling)
}
}
}
}
}

Expand Down
137 changes: 108 additions & 29 deletions ...ne-core/src/test/kotlin/com/pinterest/ktlint/rule/engine/core/api/ASTNodeExtensionTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ package com.pinterest.ktlint.rule.engine.core.api

import com.pinterest.ktlint.rule.engine.api.Code
import com.pinterest.ktlint.rule.engine.api.KtLintRuleEngine
import com.pinterest.ktlint.rule.engine.core.api.ElementType.ANNOTATION_ENTRY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.CLASS
import com.pinterest.ktlint.rule.engine.core.api.ElementType.CLASS_BODY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.ENUM_ENTRY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.FUN
import com.pinterest.ktlint.rule.engine.core.api.ElementType.IDENTIFIER
import com.pinterest.ktlint.rule.engine.core.api.ElementType.LPAR
import com.pinterest.ktlint.rule.engine.core.api.ElementType.MODIFIER_LIST
import com.pinterest.ktlint.rule.engine.core.api.ElementType.RPAR
import com.pinterest.ktlint.rule.engine.core.api.ElementType.TYPE_REFERENCE
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_PARAMETER
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VALUE_PARAMETER_LIST
import com.pinterest.ktlint.rule.engine.core.api.ElementType.WHITE_SPACE
Expand Down Expand Up @@ -303,7 +306,7 @@ class ASTNodeExtensionTest {
@Nested
inner class UpsertWhitespaceBeforeMe {
@Test
fun `Given a whitespace node and upsert a whitespace before the node (RPAR) then replace the current whitespace element`() {
fun `Given an upsert of a whitespace based before another whitespace node then replace the existing whitespace element`() {
val code =
"""
fun foo( ) = 42
Expand All @@ -327,10 +330,10 @@ class ASTNodeExtensionTest {
}

@Test
fun `Given a node (RPAR) which is preceded by a non-whitespace leaf element (LPAR) and upsert a whitespace before the node (RPAR) then create a new whitespace element before the node (RPAR)`() {
fun `Given an upsert of a whitespace before a non-whitespace node (RPAR) which is preceded by a whitespace leaf element (LPAR) then then replace the whitespace element`() {
val code =
"""
fun foo() = 42
fun foo( ) = 42
""".trimIndent()
val formattedCode =
"""
Expand All @@ -350,10 +353,10 @@ class ASTNodeExtensionTest {
}

@Test
fun `Given a node (RPAR) which is preceded by a whitespace leaf element and upsert a whitespace before the node (RPAR) then replace the whitespace element before the node (RPAR)`() {
fun `Given an upsert of a whitespace before a non-whitespace node (RPAR) which is preceded by another non-whitespace leaf element (LPAR) then create a new whitespace element`() {
val code =
"""
fun foo( ) = 42
fun foo() = 42
""".trimIndent()
val formattedCode =
"""
Expand All @@ -373,7 +376,33 @@ class ASTNodeExtensionTest {
}

@Test
fun `Given a node (VALUE_PARAMETER) which is preceded by a non-whitespace leaf element (LPAR) and upsert a whitespace before the node (VALUE_PARAMETER) then create a new whitespace element before the node (VALUE_PARAMETER)`() {
fun `Given an upsert of a whitespace before a composite node (ANNOTATION_ENTRY) which is the first child of another composite element then create a new whitespace before (VALUE_PARAMETER)`() {
val code =
"""
fun foo(@Bar string: String) = 42
""".trimIndent()
val formattedCode =
"""
fun foo(
@Bar string: String) = 42
""".trimIndent()

val actual =
code
.transformAst {
findChildByType(FUN)
?.findChildByType(VALUE_PARAMETER_LIST)
?.findChildByType(VALUE_PARAMETER)
?.findChildByType(MODIFIER_LIST)
?.findChildByType(ANNOTATION_ENTRY)
?.upsertWhitespaceBeforeMe("\n ")
}.text

assertThat(actual).isEqualTo(formattedCode)
}

@Test
fun `Given an upsert of a whitespace before a composite node (VALUE_PARAMETER) which is preceded by a non-whitespace leaf element (LPAR) then create a new whitespace element before the node (VALUE_PARAMETER)`() {
val code =
"""
fun foo(string: String) = 42
Expand All @@ -397,25 +426,26 @@ class ASTNodeExtensionTest {
}

@Test
fun `Given a node (FUN bar) which is preceded by a composite element (FUN foo) and upsert a whitespace before the node (FUN bar) then create a new whitespace element before the node (FUN bar)`() {
fun `Given an upsert of a whitespace before a composite node (ANNOTATION_ENTRY) which is preceded by another composite node (ANNOTATION_ENTRY) then create a new whitespace between the ANNOTATION_ENTRIES`() {
val code =
"""
fun foo() = "foo"
fun bar() = "bar"
fun foo(@Bar@Foo string: String) = 42
""".trimIndent()
val formattedCode =
"""
fun foo() = "foo"
fun bar() = "bar"
fun foo(@Bar @Foo string: String) = 42
""".trimIndent()

val actual =
code
.transformAst {
children()
.last { it.elementType == FUN }
.upsertWhitespaceBeforeMe("\n\n")
findChildByType(FUN)
?.findChildByType(VALUE_PARAMETER_LIST)
?.findChildByType(VALUE_PARAMETER)
?.findChildByType(MODIFIER_LIST)
?.findChildByType(ANNOTATION_ENTRY)
?.nextSibling()
?.upsertWhitespaceBeforeMe(" ")
}.text

assertThat(actual).isEqualTo(formattedCode)
Expand All @@ -425,10 +455,34 @@ class ASTNodeExtensionTest {
@Nested
inner class UpsertWhitespaceAfterMe {
@Test
fun `Given a node (LPAR) which is followed by a non-whitespace leaf element (RPAR) and upsert a whitespace after the node (LPAR) then create a new whitespace element after the node (LPAR)`() {
fun `Given an upsert of a whitespace based after another whitespace node then replace the existing whitespace element`() {
val code =
"""
fun foo() = 42
fun foo( ) = 42
""".trimIndent()
val formattedCode =
"""
fun foo(
) = 42
""".trimIndent()

val actual =
code
.transformAst {
findChildByType(FUN)
?.findChildByType(VALUE_PARAMETER_LIST)
?.findChildByType(WHITE_SPACE)
?.upsertWhitespaceAfterMe("\n")
}.text

assertThat(actual).isEqualTo(formattedCode)
}

@Test
fun `Given an upsert of a whitespace after a non-whitespace node (LPAR) which is followed by a whitespace leaf element (RPAR) then then replace the whitespace element`() {
val code =
"""
fun foo( ) = 42
""".trimIndent()
val formattedCode =
"""
Expand All @@ -448,10 +502,10 @@ class ASTNodeExtensionTest {
}

@Test
fun `Given a node (LPAR) which is followed by a whitespace leaf element and upsert a whitespace after the node (LPAR) then replace the whitespace element after the node (LPAR)`() {
fun `Given an upsert of a whitespace after a non-whitespace node (LPAR) which is followed by another non-whitespace leaf element (RPAR) then create a new whitespace element`() {
val code =
"""
fun foo( ) = 42
fun foo() = 42
""".trimIndent()
val formattedCode =
"""
Expand All @@ -471,7 +525,32 @@ class ASTNodeExtensionTest {
}

@Test
fun `Given a node (VALUE_PARAMETER) which is followed by a non-whitespace leaf element (RPAR) and upsert a whitespace after the node (VALUE_PARAMETER) then create a new whitespace element after the node (VALUE_PARAMETER)`() {
fun `Given an upsert of a whitespace after a composite node (ANNOTATION_ENTRY) which is the last child of another composite element then create a new whitespace after (VALUE_PARAMETER)`() {
val code =
"""
fun foo(@Bar string: String) = 42
""".trimIndent()
val formattedCode =
"""
fun foo(@Bar string: String
) = 42
""".trimIndent()

val actual =
code
.transformAst {
findChildByType(FUN)
?.findChildByType(VALUE_PARAMETER_LIST)
?.findChildByType(VALUE_PARAMETER)
?.findChildByType(TYPE_REFERENCE)
?.upsertWhitespaceAfterMe("\n")
}.text

assertThat(actual).isEqualTo(formattedCode)
}

@Test
fun `Given an upsert of a whitespace after a composite node (VALUE_PARAMETER) which is followed by a non-whitespace leaf element (RPAR) then create a new whitespace element after the node (VALUE_PARAMETER)`() {
val code =
"""
fun foo(string: String) = 42
Expand All @@ -495,25 +574,25 @@ class ASTNodeExtensionTest {
}

@Test
fun `Given a node (FUN foo) which is followed by a composite element (FUN bar) and upsert a whitespace after the node (FUN foo) then create a new whitespace element after the node (FUN foo)`() {
fun `Given an upsert of a whitespace after a composite node (ANNOTATION_ENTRY) which is followed by another composite node (ANNOTATION_ENTRY) then create a new whitespace between the ANNOTATION_ENTRIES`() {
val code =
"""
fun foo() = "foo"
fun bar() = "bar"
fun foo(@Bar@Foo string: String) = 42
""".trimIndent()
val formattedCode =
"""
fun foo() = "foo"
fun bar() = "bar"
fun foo(@Bar @Foo string: String) = 42
""".trimIndent()

val actual =
code
.transformAst {
children()
.first { it.elementType == FUN }
.upsertWhitespaceAfterMe("\n\n")
findChildByType(FUN)
?.findChildByType(VALUE_PARAMETER_LIST)
?.findChildByType(VALUE_PARAMETER)
?.findChildByType(MODIFIER_LIST)
?.findChildByType(ANNOTATION_ENTRY)
?.upsertWhitespaceAfterMe(" ")
}.text

assertThat(actual).isEqualTo(formattedCode)
Expand Down
27 changes: 24 additions & 3 deletions ktlint-rule-engine/src/main/kotlin/com/pinterest/ktlint/rule/engine/api/KtLintRuleEngine.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,30 @@ public class KtLintRuleEngine(
}
} while (mutated && formatRunCount < MAX_FORMAT_RUNS_PER_FILE)
if (formatRunCount == MAX_FORMAT_RUNS_PER_FILE && mutated) {
LOGGER.warn {
"Format was not able to resolve all violations which (theoretically) can be autocorrected in file " +
"${code.filePathOrStdin()} in $MAX_FORMAT_RUNS_PER_FILE consecutive runs of format."
// It is unknown if the last format run introduces new lint violations which can be autocorrected. So run lint once more so that
// the user can be informed about this correctly.
var hasErrorsWhichCanBeAutocorrected = false
visitorProvider
.visitor()
.invoke { rule ->
ruleExecutionContext.executeRule(rule, false) { offset, message, canBeAutoCorrected ->
if (canBeAutoCorrected) {
ruleExecutionContext.rebuildSuppressionLocator()
val formattedCode =
ruleExecutionContext
.rootNode
.text
.replace("\n", ruleExecutionContext.determineLineSeparator(code.content))
val (line, col) = ruleExecutionContext.positionInTextLocator(offset)
hasErrorsWhichCanBeAutocorrected = true
}
}
}
if (hasErrorsWhichCanBeAutocorrected) {
LOGGER.warn {
"Format was not able to resolve all violations which (theoretically) can be autocorrected in file " +
"${code.filePathOrStdin()} in $MAX_FORMAT_RUNS_PER_FILE consecutive runs of format."
}
}
}

Expand Down
Loading

0 comments on commit 76016af

Please sign in to comment.