Skip to content

Commit

Permalink
Keep formatting of for-loop in sync with default IntelliJ formatter u…
Browse files Browse the repository at this point in the history 
…ntil https://youtrack.jetbrains.com/issue/IDEA-293691/Format-Kotlin-for-loop is fixed. The wrapping rule no longer forces the for-statement to be wrapped if only the expression contains a newline.

Closes pinterest#1350
  • Loading branch information
= committed May 8, 2022
1 parent 504162c commit 02dce0a
Show file tree
Hide file tree
Showing 6 changed files with 207 additions and 31 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ An AssertJ style API for testing KtLint rules ([#1444](https://github.com/pinter
### Fixed
- Fix check of spacing in the receiver type of an anonymous function ([#1440](https://github.com/pinterest/ktlint/issues/1440))
- Allow comment on same line as super class in class declaration `wrapping` ([#1457](https://github.com/pinterest/ktlint/pull/1457))
- Keep formatting of for-loop in sync with default IntelliJ formatter (`indent`) and a newline in the expression in a for-statement should not force to wrap it `wrapping` ([#1350](https://github.com/pinterest/ktlint/issues/1350))

### Changed
* Set Kotlin development version to `1.7.0-Beta` and Kotlin version to `1.6.21`.
Expand Down
54 changes: 48 additions & 6 deletions ...ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.pinterest.ktlint.core.ast.ElementType.ELSE
import com.pinterest.ktlint.core.ast.ElementType.ELVIS
import com.pinterest.ktlint.core.ast.ElementType.EOL_COMMENT
import com.pinterest.ktlint.core.ast.ElementType.EQ
import com.pinterest.ktlint.core.ast.ElementType.FOR
import com.pinterest.ktlint.core.ast.ElementType.FUN
import com.pinterest.ktlint.core.ast.ElementType.FUNCTION_LITERAL
import com.pinterest.ktlint.core.ast.ElementType.GT
Expand Down Expand Up @@ -238,11 +239,18 @@ public class IndentationRule :
)
}
else -> {
expectedIndent++
logger.trace { "$line: block starting with ${n.text} -> Increase to $expectedIndent" }
ctx.blockStack.push(
Block(n.elementType, line, REGULAR)
)
if (n.isPartOfForLoopConditionWithMultilineExpression()) {
logger.trace { "$line: block starting with ${n.text} -> Keep at $expectedIndent" }
ctx.blockStack.push(
Block(n.elementType, line, SAME_AS_PREVIOUS_BLOCK)
)
} else {
expectedIndent++
logger.trace { "$line: block starting with ${n.text} -> Increase to $expectedIndent" }
ctx.blockStack.push(
Block(n.elementType, line, REGULAR)
)
}
}
}
logger.trace {
Expand Down Expand Up @@ -765,7 +773,12 @@ public class IndentationRule :
nextLeafElementType == GT &&
node.treeParent?.elementType.let { it == TYPE_PARAMETER_LIST || it == TYPE_ARGUMENT_LIST } ->
0
nextLeafElementType in rTokenSet -> -1
nextLeafElementType in rTokenSet ->
if (node.isPartOfForLoopConditionWithMultilineExpression()) {
0
} else {
-1
}
// IDEA quirk:
// val i: Int
// by lazy { 1 }
Expand Down Expand Up @@ -1032,3 +1045,32 @@ private fun KtStringTemplateExpression.isFollowedByTrimMargin() = isFollowedBy("
private fun KtStringTemplateExpression.isFollowedBy(callExpressionName: String) =
this.node.nextSibling { it.elementType != DOT }
.let { it?.elementType == CALL_EXPRESSION && it.text == callExpressionName }

/**
* A for-loop for which the condition contains a sibling node containing a newline is not correctly formatted by the
* default formatter of IntelliJ IDEA (https://youtrack.jetbrains.com/issue/IDEA-293691/Format-Kotlin-for-loop). When
* using the correct indentation level, it conflicts with the IntelliJ IDEA formatting, so until the aforementioned bug
* is resolved, ktlint will produce the same format as IntelliJ default formatter.
*/
private fun ASTNode.isPartOfForLoopConditionWithMultilineExpression(): Boolean {
if (treeParent.elementType != FOR) {
return false
}
if (this.elementType != LPAR) {
return treeParent.findChildByType(LPAR)!!.isPartOfForLoopConditionWithMultilineExpression()
}
require(elementType == LPAR) {
"Node should be the LPAR of the FOR loop"
}

// Iterate all sibling node until RPAR to check whether the node contains a newline. Note that it does not matter
// whether is code sibling contains a newline.
var node: ASTNode? = this
while (node != null && node.elementType != RPAR) {
if (node.isWhiteSpaceWithNewline()) {
return true
}
node = node.nextSibling { true }
}
return false
}
51 changes: 51 additions & 0 deletions ...nt-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/WrappingRule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.pinterest.ktlint.core.EditorConfig.Companion.loadEditorConfig
import com.pinterest.ktlint.core.EditorConfig.Companion.loadIndentConfig
import com.pinterest.ktlint.core.IndentConfig
import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType
import com.pinterest.ktlint.core.ast.ElementType.ANNOTATION
import com.pinterest.ktlint.core.ast.ElementType.ARROW
import com.pinterest.ktlint.core.ast.ElementType.CALL_EXPRESSION
Expand Down Expand Up @@ -145,6 +146,21 @@ public class WrappingRule : Rule(
) {
return
}
if (node.isPartOfForLoopConditionWithMultilineExpression()) {
// keep:
// for (foo in listOf(
// "foo-1",
// "foo-2"
// )) { ... }
// but reject:
// for (
// foo in listOf(
// "foo-1",
// "foo-2"
// )
// ) { ... }
return
}
if (!node.nextCodeLeaf()?.prevLeaf {
// Skip comments, whitespace, and empty nodes
!it.isPartOfComment() &&
Expand Down Expand Up @@ -444,4 +460,39 @@ public class WrappingRule : Rule(
private fun KtStringTemplateExpression.isFollowedBy(callExpressionName: String) =
this.node.nextSibling { it.elementType != DOT }
.let { it?.elementType == CALL_EXPRESSION && it.text == callExpressionName }

/**
* Allow for-statement in which only the expression contains a newline:
* for (foo in listOf(
* "foo-1",
* "foo-2"
* )) { ... }
* but reject:
* for (
* foo in listOf(
* "foo-1",
* "foo-2"
* )
* ) { ... }
*/
private fun ASTNode.isPartOfForLoopConditionWithMultilineExpression(): Boolean {
if (treeParent.elementType != ElementType.FOR) {
return false
}
if (this.elementType != LPAR) {
return treeParent.findChildByType(LPAR)!!.isPartOfForLoopConditionWithMultilineExpression()
}
require(elementType == LPAR) {
"Node should be the LPAR of the FOR loop"
}

var node: ASTNode? = this
while (node != null && node.elementType != RPAR) {
if (node.isWhiteSpaceWithNewline()) {
return false
}
node = node.nextSibling { true }
}
return true
}
}
46 changes: 21 additions & 25 deletions ...uleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/FilenameRuleTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,18 @@ class FilenameRuleTest {

@Test
fun testMatchingSingleClassName() {
for (
src in listOf(
"class A",
"class `A`",
"data class A(val v: Int)",
"sealed class A",
"interface A",
"object A",
"enum class A {A}",
"typealias A = Set<Network.Node>",
// >1 declaration case
"class B\nfun A.f() {}"
)
) {
for (src in listOf(
"class A",
"class `A`",
"data class A(val v: Int)",
"sealed class A",
"interface A",
"object A",
"enum class A {A}",
"typealias A = Set<Network.Node>",
// >1 declaration case
"class B\nfun A.f() {}"
)) {
val code =
"""
/*
Expand All @@ -41,17 +39,15 @@ class FilenameRuleTest {

@Test
fun testNonMatchingSingleClassName() {
for (
src in mapOf(
"class A" to "class",
"data class A(val v: Int)" to "class",
"sealed class A" to "class",
"interface A" to "interface",
"object A" to "object",
"enum class A {A}" to "class",
"typealias A = Set<Network.Node>" to "typealias"
)
) {
for (src in mapOf(
"class A" to "class",
"data class A(val v: Int)" to "class",
"sealed class A" to "class",
"interface A" to "interface",
"object A" to "object",
"enum class A {A}" to "class",
"typealias A = Set<Network.Node>" to "typealias"
)) {
val code =
"""
/*
Expand Down
70 changes: 70 additions & 0 deletions ...set-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3720,6 +3720,76 @@ internal class IndentationRuleTest {
indentationRuleAssertThat(code).hasNoLintViolations()
}

@Nested
inner class Issue1350 {
@Test
fun `Issue 1350 - Given a for-loop containing a newline before the declaration then do not indent it to keep it in sync with IntelliJ default formatting`() {
val code =
"""
fun foo() {
for (
item in listOf(
"a",
"b"
)) {
println(item)
}
}
""".trimIndent()
indentationRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Issue 1350 - Given a for-loop containing a newline before the 'in' operator then do not indent it to keep it in sync with IntelliJ default formatting`() {
val code =
"""
fun foo() {
for (item
in listOf(
"a",
"b"
)) {
println(item)
}
}
""".trimIndent()
indentationRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Issue 1350 - Given a for-loop containing a newline before the expression then do not indent it to keep it in sync with IntelliJ default formatting`() {
val code =
"""
fun foo() {
for (item in
listOf(
"a",
"b"
)) {
println(item)
}
}
""".trimIndent()
indentationRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Issue 1350 - Given a for-loop with a multiline expression then indent that expression normally`() {
val code =
"""
fun foo() {
for (item in listOf(
"a",
"b"
)) {
println(item)
}
}
""".trimIndent()
indentationRuleAssertThat(code).hasNoLintViolations()
}
}

private companion object {
val INDENT_STYLE_TAB = indentStyleProperty to PropertyType.IndentStyleValue.tab
}
Expand Down
16 changes: 16 additions & 0 deletions ...uleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/WrappingRuleTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1631,6 +1631,22 @@ internal class WrappingRuleTest {
assertThat(WrappingRule().format(code)).isEqualTo(formattedCode)
}

@Test
fun `Issue 1350 - Given a for-statement with a newline in the expression only then do not wrap`() {
val code =
"""
fun foo() {
for (item in listOf(
"a",
"b"
)) {
println(item)
}
}
""".trimIndent()
wrappingRuleAssertThat(code).hasNoLintViolations()
}

private companion object {
const val MULTILINE_STRING_QUOTE = "${'"'}${'"'}${'"'}"
const val TAB = "${'\t'}"
Expand Down

0 comments on commit 02dce0a

Please sign in to comment.