Add missing whitespace when else is on same line as true condition (#…

…1565) * Add missing whitespace when else is on same line as true condition `multiline-if-else` Closes #1560 * Fix multiline if statement Closes #828
pinterest · Aug 3, 2022 · 49efdba · 49efdba
1 parent bf1443e
commit 49efdba
Show file tree

Hide file tree

Showing 5 changed files with 169 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -126,6 +126,8 @@ The callback function provided as parameter to the format function is now called
 * When a glob is specified then ensure that it matches files in the current directory and not only in subdirectories of the current directory ([#1533](https://github.com/pinterest/ktlint/issue/1533)).
 * Execute `ktlint` cli on default kotlin extensions only when an (existing) path to a directory is given. ([#917](https://github.com/pinterest/ktlint/issue/917)).
 * Invoke callback on `format` function for all errors including errors that are autocorrected ([#1491](https://github.com/pinterest/ktlint/issues/1491))
+* Add missing whitespace when else is on same line as true condition `multiline-if-else` ([#1560](https://github.com/pinterest/ktlint/issues/1560))
+* Fix multiline if-statements `multiline-if-else` ([#828](https://github.com/pinterest/ktlint/issues/828))
 * Prevent class cast exception on ".editorconfig" property `ktlint_code_style`  ([#1559](https://github.com/pinterest/ktlint/issues/1559))
 * Handle trailing comma in enums `trailing-comma` ([#1542](https://github.com/pinterest/ktlint/pull/1542))
 

diff --git a/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SuppressionLocatorBuilder.kt b/ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/internal/SuppressionLocatorBuilder.kt
@@ -38,12 +38,17 @@ internal object SuppressionLocatorBuilder {
         val hintsList = collect(rootNode)
         return if (hintsList.isEmpty()) {
             noSuppression
-        } else { offset, ruleId, isRoot ->
+        } else {
+            toSuppressedRegionsLocator(hintsList)
+        }
+    }
+
+    private fun toSuppressedRegionsLocator(hintsList: List<SuppressionHint>): SuppressionLocator =
+        { offset, ruleId, isRoot ->
             hintsList
                 .filter { offset in it.range }
                 .any { hint -> hint.disabledRules.isEmpty() || hint.disabledRules.contains(ruleId) }
         }
-    }
 
     /**
      * @param range zero-based range of lines where lint errors should be suppressed

diff --git a/...t-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/AnnotationSpacingRule.kt b/...t-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/AnnotationSpacingRule.kt
@@ -73,7 +73,9 @@ class AnnotationSpacingRule : Rule("annotation-spacing") {
                     val s = it.text
                     // Ensure at least one occurrence of two line breaks
                     s.indexOf("\n") != s.lastIndexOf("\n")
-                } else it.isPartOfComment() && !it.isCommentOnSameLineAsPrevLeaf()
+                } else {
+                    it.isPartOfComment() && !it.isCommentOnSameLineAsPrevLeaf()
+                }
             }
         )
         if (next != null) {

diff --git a/...set-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/MultiLineIfElseRule.kt b/...set-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/MultiLineIfElseRule.kt
@@ -1,17 +1,26 @@
 package com.pinterest.ktlint.ruleset.standard
 
+import com.pinterest.ktlint.core.IndentConfig
 import com.pinterest.ktlint.core.Rule
+import com.pinterest.ktlint.core.api.DefaultEditorConfigProperties
+import com.pinterest.ktlint.core.api.EditorConfigProperties
+import com.pinterest.ktlint.core.api.UsesEditorConfigProperties
+import com.pinterest.ktlint.core.ast.ElementType.BLOCK
 import com.pinterest.ktlint.core.ast.ElementType.ELSE
 import com.pinterest.ktlint.core.ast.ElementType.ELSE_KEYWORD
+import com.pinterest.ktlint.core.ast.ElementType.IF
 import com.pinterest.ktlint.core.ast.ElementType.LBRACE
 import com.pinterest.ktlint.core.ast.ElementType.RBRACE
 import com.pinterest.ktlint.core.ast.ElementType.RPAR
 import com.pinterest.ktlint.core.ast.ElementType.THEN
+import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
 import com.pinterest.ktlint.core.ast.isPartOfComment
+import com.pinterest.ktlint.core.ast.isWhiteSpace
 import com.pinterest.ktlint.core.ast.isWhiteSpaceWithoutNewline
-import com.pinterest.ktlint.core.ast.prevLeaf
+import com.pinterest.ktlint.core.ast.lineIndent
+import com.pinterest.ktlint.core.ast.nextSibling
+import com.pinterest.ktlint.core.ast.upsertWhitespaceBeforeMe
 import org.jetbrains.kotlin.com.intellij.lang.ASTNode
-import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace
 import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement
 import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl
 import org.jetbrains.kotlin.psi.KtBlockExpression
@@ -20,50 +29,100 @@ import org.jetbrains.kotlin.psi.psiUtil.leaves
 /**
  * https://kotlinlang.org/docs/reference/coding-conventions.html#formatting-control-flow-statements
  */
-class MultiLineIfElseRule : Rule("multiline-if-else") {
+class MultiLineIfElseRule :
+    Rule("multiline-if-else"),
+    UsesEditorConfigProperties {
+    override val editorConfigProperties: List<UsesEditorConfigProperties.EditorConfigProperty<*>> =
+        listOf(
+            DefaultEditorConfigProperties.indentSizeProperty,
+            DefaultEditorConfigProperties.indentStyleProperty
+        )
+    private var indentConfig = IndentConfig.DEFAULT_INDENT_CONFIG
+
+    override fun beforeFirstNode(editorConfigProperties: EditorConfigProperties) {
+        indentConfig = IndentConfig(
+            indentStyle = editorConfigProperties.getEditorConfigValue(DefaultEditorConfigProperties.indentStyleProperty),
+            tabWidth = editorConfigProperties.getEditorConfigValue(DefaultEditorConfigProperties.indentSizeProperty)
+        )
+    }
+
     override fun beforeVisitChildNodes(
         node: ASTNode,
         autoCorrect: Boolean,
         emit: (offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit
     ) {
         if (node.elementType == THEN || node.elementType == ELSE) {
-            if (!node.treePrev.textContains('\n')) { // if (...) <statement>
+            if (node.firstChildNode?.elementType == BLOCK) {
                 return
             }
 
-            if (node.firstChildNode?.firstChildNode?.elementType != LBRACE) {
-                emit(node.firstChildNode.startOffset, "Missing { ... }", true)
-                if (autoCorrect) {
-                    autocorrect(node)
+            if (!node.treePrev.textContains('\n')) {
+                if (node.firstChildNode.elementType == IF) {
+                    // Allow single line for:
+                    // else if (...)
+                    return
+                }
+                if (!node.treeParent.textContains('\n')) {
+                    // Allow single line if statements as long as they are really simple (e.g. do not contain newlines)
+                    //    if (...) <statement> // no else statement
+                    //    if (...) <statement> else <statement>
+                    return
                 }
             }
+
+            emit(node.firstChildNode.startOffset, "Missing { ... }", true)
+            if (autoCorrect) {
+                autocorrect(node)
+            }
         }
     }
 
     private fun autocorrect(node: ASTNode) {
         val prevLeaves =
-            node.leaves(forward = false).takeWhile { it.elementType !in listOf(RPAR, ELSE_KEYWORD) }.toList().reversed()
+            node
+                .leaves(forward = false)
+                .takeWhile { it.elementType !in listOf(RPAR, ELSE_KEYWORD) }
+                .toList()
+                .reversed()
         val nextLeaves =
-            node.leaves(forward = true).takeWhile { it.isWhiteSpaceWithoutNewline() || it.isPartOfComment() }.toList()
-        val rightBraceIndent = node.treeParent
-            .prevLeaf { it is PsiWhiteSpace && it.textContains('\n') }?.text.orEmpty()
-            .let { "\n${it.substringAfterLast("\n")}" }
+            node
+                .leaves(forward = true)
+                .takeWhile { it.isWhiteSpaceWithoutNewline() || it.isPartOfComment() }
+                .toList()
+                .dropLastWhile { it.isWhiteSpaceWithoutNewline() }
 
-        (node.treePrev as LeafPsiElement).rawReplaceWithText(" ")
+        prevLeaves
+            .firstOrNull()
+            .takeIf { it.isWhiteSpace() }
+            ?.let {
+                (it as LeafPsiElement).rawReplaceWithText(" ")
+            }
         KtBlockExpression(null).apply {
             val previousChild = node.firstChildNode
             node.replaceChild(node.firstChildNode, this)
             addChild(LeafPsiElement(LBRACE, "{"))
-            prevLeaves.forEach(::addChild)
+            addChild(PsiWhiteSpaceImpl("\n" + node.lineIndent() + indentConfig.indent))
+            prevLeaves
+                .dropWhile { it.isWhiteSpace() }
+                .forEach(::addChild)
             addChild(previousChild)
             nextLeaves.forEach(::addChild)
-            addChild(PsiWhiteSpaceImpl(rightBraceIndent))
+            addChild(PsiWhiteSpaceImpl("\n" + node.lineIndent()))
             addChild(LeafPsiElement(RBRACE, "}"))
         }
 
         // Make sure else starts on same line as newly inserted right brace
-        if (node.elementType == THEN && node.treeNext?.treeNext?.elementType == ELSE_KEYWORD) {
-            node.treeParent.replaceChild(node.treeNext, PsiWhiteSpaceImpl(" "))
+        if (node.elementType == THEN) {
+            node
+                .nextSibling { !it.isPartOfComment() }
+                ?.let { nextSibling ->
+                    if (nextSibling.elementType == ELSE_KEYWORD) {
+                        (nextSibling as LeafPsiElement).upsertWhitespaceBeforeMe(" ")
+                    }
+                    if (nextSibling.elementType == WHITE_SPACE && nextSibling.text != " ") {
+                        (nextSibling as LeafPsiElement).rawReplaceWithText(" ")
+                    }
+                }
         }
     }
 }
diff --git a/...standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/MultiLineIfElseRuleTest.kt b/...standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/MultiLineIfElseRuleTest.kt
@@ -480,16 +480,93 @@ class MultiLineIfElseRuleTest {
             fun test(a: Int, b: Int, c: Int, d: Int, bar: Boolean) {
                 foo(
                     a,
-                    if (bar) b else {
+                    if (bar) {
+                        b
+                    } else {
                         c
                     },
                     d
                 )
             }
             """.trimIndent()
         multiLineIfElseRuleAssertThat(code)
-            // TODO: It is not consistent that argument "b" is not wrapped in a block while argument "c" is wrapped
-            .hasLintViolation(6, 13, "Missing { ... }")
+            .hasLintViolations(
+                LintViolation(5, 18, "Missing { ... }"),
+                LintViolation(6, 13, "Missing { ... }")
+            ).isFormattedAs(formattedCode)
+    }
+
+    @Test
+    fun `Issue 1560 - Given an if statement with else keyword on same line as true branch`() {
+        val code =
+            """
+            fun foo() = if (bar())
+                "a" else
+                "b"
+            """.trimIndent()
+        val formattedCode =
+            """
+            fun foo() = if (bar()) {
+                "a"
+            } else {
+                "b"
+            }
+            """.trimIndent()
+        multiLineIfElseRuleAssertThat(code)
+            .hasLintViolations(
+                LintViolation(2, 5, "Missing { ... }"),
+                LintViolation(3, 5, "Missing { ... }")
+            ).isFormattedAs(formattedCode)
+    }
+
+    @Test
+    fun `Issue 828 - Given an if statement with multiline statement starting on same line as if`() {
+        val code =
+            """
+            fun foo() {
+                if (true) 50
+                  .toString()
+            }
+            """.trimIndent()
+        val formattedCode =
+            """
+            fun foo() {
+                if (true) {
+                    50
+                        .toString()
+                }
+            }
+            """.trimIndent()
+        multiLineIfElseRuleAssertThat(code)
+            .addAdditionalRuleProvider { IndentationRule() }
+            .hasLintViolation(2, 15, "Missing { ... }")
             .isFormattedAs(formattedCode)
     }
+
+    @Test
+    fun `Issue 828 - Given an if statement with simple branches but the else branch is on a separate line`() {
+        val code =
+            """
+            fun foo() {
+                if (true) 50
+                else 55
+            }
+            """.trimIndent()
+        val formattedCode =
+            """
+            fun foo() {
+                if (true) {
+                    50
+                } else {
+                    55
+                }
+            }
+            """.trimIndent()
+        multiLineIfElseRuleAssertThat(code)
+            .addAdditionalRuleProvider { IndentationRule() }
+            .hasLintViolations(
+                LintViolation(2, 15, "Missing { ... }"),
+                LintViolation(3, 10, "Missing { ... }")
+            ).isFormattedAs(formattedCode)
+    }
 }