Skip to content

Commit

Permalink
Fixed BracesInConditionalsAndLoopsRule (#1750)
Browse files Browse the repository at this point in the history 
### 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
  • Loading branch information
@DrAlexD
DrAlexD authored Sep 25, 2023
1 parent 17d4bcc commit 3c4e688
Show file tree
Hide file tree
Showing 4 changed files with 194 additions and 26 deletions.
87 changes: 65 additions & 22 deletions .../kotlin/com/saveourtool/diktat/ruleset/rules/chapter3/BracesInConditionalsAndLoopsRule.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -84,8 +86,7 @@ class BracesInConditionalsAndLoopsRule(configRules: List<RulesConfig>) : DiktatR
}
}
?: run {
val nodeAfterCondition = ifPsi.rightParenthesis!!.node.treeNext
node.insertEmptyBlockBetweenChildren(nodeAfterCondition, nodeAfterCondition, indent)
node.insertEmptyBlockInsideThenNode(indent)
}
}
}
Expand Down Expand Up @@ -116,18 +117,53 @@ class BracesInConditionalsAndLoopsRule(configRules: List<RulesConfig>) : 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()

Expand All @@ -136,16 +172,29 @@ class BracesInConditionalsAndLoopsRule(configRules: List<RulesConfig>) : 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
Expand Down Expand Up @@ -175,7 +224,8 @@ class BracesInConditionalsAndLoopsRule(configRules: List<RulesConfig>) : 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)
}
}
Expand All @@ -187,21 +237,14 @@ class BracesInConditionalsAndLoopsRule(configRules: List<RulesConfig>) : 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"
Expand Down
3 changes: 0 additions & 3 deletions diktat-rules/src/test/kotlin/com/saveourtool/diktat/ruleset/chapter3/BracesRuleFixTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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")
}
Expand Down
80 changes: 79 additions & 1 deletion diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Expected.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
50 changes: 50 additions & 0 deletions diktat-rules/src/test/resources/test/paragraph3/braces/IfElseBraces1Test.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit 3c4e688

Please sign in to comment.