From 3c4e688b73cce801761ea80f76ccfc5c7f057035 Mon Sep 17 00:00:00 2001 From: Alexander Drugakov Date: Mon, 25 Sep 2023 15:47:43 +0300 Subject: [PATCH] Fixed BracesInConditionalsAndLoopsRule (#1750) ### What's done: - Reworked `insertEmptyBlock()` function for adding empty braces blocks to conditions and `do-while` loop with empty bodies. - Added check for braces block existence to avoid duplicate braces blocks. - Added tests for conditions including tests for cases of `else if` and cases of empty `if` and `else` bodies. It's part of #1737 --- .../BracesInConditionalsAndLoopsRule.kt | 87 ++++++++++++++----- .../ruleset/chapter3/BracesRuleFixTest.kt | 3 - .../braces/IfElseBraces1Expected.kt | 80 ++++++++++++++++- .../paragraph3/braces/IfElseBraces1Test.kt | 50 +++++++++++ 4 files changed, 194 insertions(+), 26 deletions(-) diff --git a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter3/BracesInConditionalsAndLoopsRule.kt b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter3/BracesInConditionalsAndLoopsRule.kt index 34a5a7666c..0ae76fc30e 100644 --- a/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter3/BracesInConditionalsAndLoopsRule.kt +++ b/diktat-rules/src/main/kotlin/com/saveourtool/diktat/ruleset/rules/chapter3/BracesInConditionalsAndLoopsRule.kt @@ -9,14 +9,16 @@ import com.saveourtool.diktat.ruleset.utils.isSingleLineIfElse import com.saveourtool.diktat.ruleset.utils.loopType import com.saveourtool.diktat.ruleset.utils.prevSibling -import org.jetbrains.kotlin.KtNodeTypes import org.jetbrains.kotlin.KtNodeTypes.BLOCK +import org.jetbrains.kotlin.KtNodeTypes.BLOCK_CODE_FRAGMENT +import org.jetbrains.kotlin.KtNodeTypes.BODY import org.jetbrains.kotlin.KtNodeTypes.CALL_EXPRESSION import org.jetbrains.kotlin.KtNodeTypes.ELSE import org.jetbrains.kotlin.KtNodeTypes.IF import org.jetbrains.kotlin.KtNodeTypes.LAMBDA_EXPRESSION import org.jetbrains.kotlin.KtNodeTypes.REFERENCE_EXPRESSION import org.jetbrains.kotlin.KtNodeTypes.SAFE_ACCESS_EXPRESSION +import org.jetbrains.kotlin.KtNodeTypes.THEN import org.jetbrains.kotlin.KtNodeTypes.WHEN import org.jetbrains.kotlin.com.intellij.lang.ASTNode import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.CompositeElement @@ -84,8 +86,7 @@ class BracesInConditionalsAndLoopsRule(configRules: List) : DiktatR } } ?: run { - val nodeAfterCondition = ifPsi.rightParenthesis!!.node.treeNext - node.insertEmptyBlockBetweenChildren(nodeAfterCondition, nodeAfterCondition, indent) + node.insertEmptyBlockInsideThenNode(indent) } } } @@ -116,18 +117,53 @@ class BracesInConditionalsAndLoopsRule(configRules: List) : DiktatR } ?: run { // `else` can have empty body e.g. when there is a semicolon after: `else ;` - node.insertEmptyBlockBetweenChildren(elseKeyword.node.treeNext, null, indent) + node.insertEmptyBlockInsideElseNode(indent) } } } } + private fun ASTNode.insertEmptyBlockInsideThenNode(indent: Int) { + val ifPsi = psi as KtIfExpression + val elseKeyword = ifPsi.elseKeyword + val emptyThenNode = findChildByType(THEN) + + emptyThenNode?.findChildByType(BLOCK_CODE_FRAGMENT) ?: run { + val whiteSpacesAfterCondition = ifPsi.rightParenthesis?.node?.treeNext + + whiteSpacesAfterCondition?.let { + replaceChild(it, PsiWhiteSpaceImpl(" ")) + } + emptyThenNode?.insertEmptyBlock(indent) + elseKeyword?.let { + addChild(PsiWhiteSpaceImpl(" "), elseKeyword.node) + } + } + } + + private fun ASTNode.insertEmptyBlockInsideElseNode(indent: Int) { + val ifPsi = psi as KtIfExpression + val elseKeyword = ifPsi.elseKeyword + val emptyElseNode = findChildByType(ELSE) + + emptyElseNode?.findChildByType(BLOCK_CODE_FRAGMENT) ?: run { + val whiteSpacesAfterElseKeyword = elseKeyword?.node?.treeNext + + whiteSpacesAfterElseKeyword?.let { + replaceChild(it, PsiWhiteSpaceImpl(" ")) + } + emptyElseNode?.insertEmptyBlock(indent) + } + } + @Suppress("UnsafeCallOnNullableType") private fun checkLoop(node: ASTNode) { val loopBody = (node.psi as KtLoopExpression).body val loopBodyNode = loopBody?.node + if (loopBodyNode == null || loopBodyNode.elementType != BLOCK) { - NO_BRACES_IN_CONDITIONALS_AND_LOOPS.warnAndFix(configRules, emitWarn, isFixMode, node.elementType.toString(), node.startOffset, node) { + NO_BRACES_IN_CONDITIONALS_AND_LOOPS.warnAndFix(configRules, emitWarn, isFixMode, + node.elementType.toString(), node.startOffset, node) { // fixme proper way to calculate indent? or get step size (instead of hardcoded 4) val indent = node.findIndentBeforeNode() @@ -136,16 +172,29 @@ class BracesInConditionalsAndLoopsRule(configRules: List) : DiktatR } ?: run { // this corresponds to do-while with empty body - node.insertEmptyBlockBetweenChildren( - node.findChildByType(DO_KEYWORD)!!.treeNext, - node.findChildByType(WHILE_KEYWORD)!!.treePrev, - indent - ) + node.insertEmptyBlockInsideDoWhileNode(indent) } } } } + private fun ASTNode.insertEmptyBlockInsideDoWhileNode(indent: Int) { + findChildByType(BODY) ?: run { + val doKeyword = findChildByType(DO_KEYWORD) + val whileKeyword = findChildByType(WHILE_KEYWORD) + val whiteSpacesAfterDoKeyword = doKeyword?.treeNext + + addChild(CompositeElement(BODY), whileKeyword) + val emptyWhenNode = findChildByType(BODY) + + whiteSpacesAfterDoKeyword?.let { + replaceChild(it, PsiWhiteSpaceImpl(" ")) + } + emptyWhenNode?.insertEmptyBlock(indent) + addChild(PsiWhiteSpaceImpl(" "), whileKeyword) + } + } + private fun ASTNode.findIndentBeforeNode(): Int { val isElseIfStatement = treeParent.elementType == ELSE val primaryIfNode = if (isElseIfStatement) treeParent.treeParent else this @@ -175,7 +224,8 @@ class BracesInConditionalsAndLoopsRule(configRules: List) : DiktatR block.findChildrenMatching { it.isPartOfComment() }.isEmpty() } .forEach { - NO_BRACES_IN_CONDITIONALS_AND_LOOPS.warnAndFix(configRules, emitWarn, isFixMode, "WHEN", it.node.startOffset, it.node) { + NO_BRACES_IN_CONDITIONALS_AND_LOOPS.warnAndFix(configRules, emitWarn, isFixMode, + "WHEN", it.node.startOffset, it.node) { it.astReplace(it.firstStatement!!.node.psi) } } @@ -187,21 +237,14 @@ class BracesInConditionalsAndLoopsRule(configRules: List) : DiktatR )) } - private fun ASTNode.insertEmptyBlockBetweenChildren( - firstChild: ASTNode, - secondChild: ASTNode?, - indent: Int - ) { - val emptyBlock = CompositeElement(KtNodeTypes.BLOCK_CODE_FRAGMENT) - addChild(emptyBlock, firstChild) - addChild(PsiWhiteSpaceImpl(" "), emptyBlock) + private fun ASTNode.insertEmptyBlock(indent: Int) { + val emptyBlock = CompositeElement(BLOCK_CODE_FRAGMENT) + addChild(emptyBlock, null) emptyBlock.addChild(LeafPsiElement(LBRACE, "{")) emptyBlock.addChild(PsiWhiteSpaceImpl("\n${" ".repeat(indent)}")) emptyBlock.addChild(LeafPsiElement(RBRACE, "}")) - secondChild?.let { - replaceChild(it, PsiWhiteSpaceImpl(" ")) - } } + companion object { private const val INDENT_STEP = 4 const val NAME_ID = "races-rule" diff --git a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter3/BracesRuleFixTest.kt b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter3/BracesRuleFixTest.kt index 395c7b9725..f6114c7451 100644 --- a/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter3/BracesRuleFixTest.kt +++ b/diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter3/BracesRuleFixTest.kt @@ -4,14 +4,12 @@ import com.saveourtool.diktat.ruleset.rules.chapter3.BracesInConditionalsAndLoop import com.saveourtool.diktat.util.FixTestBase import generated.WarningNames -import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Tag import org.junit.jupiter.api.Test class BracesRuleFixTest : FixTestBase("test/paragraph3/braces", ::BracesInConditionalsAndLoopsRule) { @Test @Tag(WarningNames.NO_BRACES_IN_CONDITIONALS_AND_LOOPS) - @Disabled("https://github.com/saveourtool/diktat/issues/1737") fun `should add braces to if-else statements - 1`() { fixAndCompare("IfElseBraces1Expected.kt", "IfElseBraces1Test.kt") } @@ -36,7 +34,6 @@ class BracesRuleFixTest : FixTestBase("test/paragraph3/braces", ::BracesInCondit @Test @Tag(WarningNames.NO_BRACES_IN_CONDITIONALS_AND_LOOPS) - @Disabled("https://github.com/saveourtool/diktat/issues/1737") fun `should add braces to do-while loops with empty body`() { fixAndCompare("DoWhileBracesExpected.kt", "DoWhileBracesTest.kt") } diff --git a/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Expected.kt b/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Expected.kt index 847f22c8a4..82152f845c 100644 --- a/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Expected.kt +++ b/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Expected.kt @@ -27,7 +27,85 @@ fun foo3() { fun foo4() { if (x > 0) { } else { - } ; + }; +} + +fun foo5() { + if (x > 0) + { + foo() + } else + { + bar() + } +} + +fun foo6() { + if (x > 0) { + foo() + } else if (y > 0) { + abc() + } else { + bar() + } +} + +fun foo7() { + if (x > 0) + { + foo() + } else if (y > 0) + { + abc() + } else + { + bar() + } +} + +fun foo8() { + if (x > 0) { + if (y > 0) foo() else abc() + } else { + bar() + } +} + +fun foo9() { + if (x > 0) { + foo() + } else if (y > 0) { + abc() + } else { + bar() + } +} + +fun foo10() { + if (x > 0) { + foo() + } else if (z > 0) { + if (y > 0) abc() else qwe() + } else { + bar() + } +} + +fun foo11() { + if (x > 0) else bar() +} + +fun foo12() { + if (x > 0) { + foo() + } else { + }; +} + +fun foo13() { + if (x > 0) { + } else { + }; } fun foo() { diff --git a/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Test.kt b/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Test.kt index 98d639318d..6f01fd0fa9 100644 --- a/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Test.kt +++ b/diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Test.kt @@ -25,6 +25,56 @@ fun foo4() { else ; } +fun foo5() { + if (x > 0) + foo() + else + bar() +} + +fun foo6() { + if (x > 0) foo() + else if (y > 0) abc() + else bar() +} + +fun foo7() { + if (x > 0) + foo() + else if (y > 0) + abc() + else + bar() +} + +fun foo8() { + if (x > 0) if (y > 0) foo() else abc() + else bar() +} + +fun foo9() { + if (x > 0) foo() + else if (y > 0) abc() else bar() +} + +fun foo10() { + if (x > 0) foo() + else if (z > 0) if (y > 0) abc() else qwe() + else bar() +} + +fun foo11() { + if (x > 0) else bar() +} + +fun foo12() { + if (x > 0) foo() else ; +} + +fun foo13() { + if (x > 0) else ; +} + fun foo() { if (a) { bar()