diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index e135d9d7d453..edf8e46f9bd4 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -153,6 +153,10 @@ style: active: true singleLine: 'consistent' multiLine: 'consistent' + BracesOnWhenStatements: + active: false + singleLine: 'necessary' + multiLine: 'necessary' CanBeNonNullable: active: true CascadingCallWrapping: diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index cc91d595d754..307a1365ea64 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -507,7 +507,7 @@ style: multiLine: 'always' BracesOnWhenStatements: active: false - singleLine: 'consistent' + singleLine: 'necessary' multiLine: 'consistent' CanBeNonNullable: active: false diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt index 794a0aa9707c..58756288e29d 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt @@ -12,6 +12,7 @@ import io.gitlab.arturbosch.detekt.api.internal.Configuration import org.jetbrains.kotlin.com.intellij.psi.PsiElement import org.jetbrains.kotlin.psi.KtBlockExpression import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtWhenEntry import org.jetbrains.kotlin.psi.KtWhenExpression /** @@ -19,19 +20,19 @@ import org.jetbrains.kotlin.psi.KtWhenExpression * Keeping braces consistent will improve readability and avoid possible errors. * * Single-line `when` statement is: - * a `when` where each of the entries are single-line (has no line breaks `\n`). + * a `when` where each of the branches are single-line (has no line breaks `\n`). * * Multi-line `when` statement is: - * a `when` where at least one of the entries is multi-line (has a break line `\n`) - * or has multiple statements. + * a `when` where at least one of the branches is multi-line (has a break line `\n`). * * Available options are: - * * `never`: forces no braces on any entry. - _Tip_: this is very strict, it will force a simple expression, like a single function call / expression. Extracting a function for "complex" logic is one way to adhere to this policy. - * * `necessary`: forces no braces on any entry except where necessary for multi-statement entries. + * * `never`: forces no braces on any branch. + * _Tip_: this is very strict, it will force a simple expression, like a single function call / expression. + * Extracting a function for "complex" logic is one way to adhere to this policy. + * * `necessary`: forces no braces on any branch except where necessary for multi-statement branches. * * `consistent`: ensures that braces are consistent within `when` statement. - * If there are braces on one of the entries, all entries should have it. - * * `always`: forces braces on all entries. + * If there are braces on one of the branches, all branches should have it. + * * `always`: forces braces on all branches. * * * // singleLine = 'never' @@ -50,11 +51,15 @@ import org.jetbrains.kotlin.psi.KtWhenExpression * when (a) { * 1 -> { f1() } * 2 -> f2() + * 3 -> { f3(); f4() } * } * // multiLine = 'necessary' * when (a) { * 1 -> { f1() } - * 2 -> { f2(); f3() } + * 2 -> { + * f2() + * f3() + * } * } * * // singleLine = 'consistent' @@ -163,7 +168,7 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { ) @Configuration("single-line braces policy") - private val singleLine: BracePolicy by config("consistent") { BracePolicy.getValue(it) } + private val singleLine: BracePolicy by config("necessary") { BracePolicy.getValue(it) } @Configuration("multi-line braces policy") private val multiLine: BracePolicy by config("consistent") { BracePolicy.getValue(it) } @@ -171,33 +176,31 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { override fun visitWhenExpression(expression: KtWhenExpression) { super.visitWhenExpression(expression) - val branches: List = walk(expression) + val branches: List = walk(expression) validate(branches, policy(expression)) } - private fun walk(expression: KtWhenExpression): List { - return expression.entries.mapNotNull { it.expression } - } + private fun walk(expression: KtWhenExpression) = expression.entries - private fun validate(branches: List, policy: BracePolicy) { + private fun validate(branches: List, policy: BracePolicy) { val violators = when (policy) { BracePolicy.Always -> { - list.filter { !hasBraces(it) } + branches.filter { !hasBraces(it.expression) } } BracePolicy.Necessary -> { - list.filter { !isMultiStatement(it) && hasBraces(it) } + branches.filter { !isMultiStatement(it.expression) && hasBraces(it.expression) } } BracePolicy.Never -> { - list.filter { hasBraces(it) } + branches.filter { hasBraces(it.expression) } } BracePolicy.Consistent -> { - val braces = list.count { hasBraces(it) } - val noBraces = list.count { !hasBraces(it) } + val braces = branches.count { hasBraces(it.expression) } + val noBraces = branches.count { !hasBraces(it.expression) } if (braces != 0 && noBraces != 0) { - list.take(1) + branches.take(1) } else { emptyList() } @@ -206,31 +209,29 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { violators.forEach { report(it, policy) } } - private fun hasBraces(expression: KtExpression): Boolean = + private fun hasBraces(expression: KtExpression?): Boolean = expression is KtBlockExpression - private fun KtExpression.parentCandidate(): PsiElement? = - this.parent.parent + private fun KtWhenEntry.parentCandidate(): PsiElement? = + this.parent - private fun isMultiStatement(expression: KtExpression): Boolean = + private fun isMultiStatement(expression: KtExpression?): Boolean = expression is KtBlockExpression && expression.statements.size > 1 private fun policy(expression: KtWhenExpression): BracePolicy { val multiLineCandidate = expression.entries.firstOrNull { entry -> - entry.text.substringAfter("->").contains('\n') || - entry.expression?.let { isMultiStatement(it) } ?: false + entry.text.substringAfter("->").contains('\n') } return if (multiLineCandidate != null) multiLine else singleLine } - private fun report(violator: KtExpression, policy: BracePolicy) { + private fun report(violator: KtWhenEntry, policy: BracePolicy) { val parent = violator.parentCandidate() as KtWhenExpression - val entries = parent.entries.mapNotNull { it.expression } val reported = when { - violator in entries && policy == BracePolicy.Consistent -> parent.whenKeyword - violator in entries -> violator + violator in parent.entries && policy == BracePolicy.Consistent -> parent.whenKeyword + violator in parent.entries -> violator.arrow else -> error("Violating element (${violator.text}) is not part of this 'when' (${parent.text})") - } + } ?: return val message = when (policy) { BracePolicy.Always -> "Missing braces on this branch, add them." BracePolicy.Consistent -> "Inconsistent braces, make sure all branches either have or don't have braces." diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatementsSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatementsSpec.kt index b89570653b40..3a1723e6cb42 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatementsSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatementsSpec.kt @@ -60,7 +60,7 @@ class BracesOnWhenStatementsSpec { 2 -> { println() } } """.trimIndent(), - "println()"(1), + "->"(1), ), ) @@ -88,7 +88,7 @@ class BracesOnWhenStatementsSpec { else -> { println() } } """.trimIndent(), - "println()"(1), + "->"(1), ), ) @@ -102,7 +102,7 @@ class BracesOnWhenStatementsSpec { else -> println() } """.trimIndent(), - "println()"(3), + "->"(3), ), ) @@ -116,7 +116,7 @@ class BracesOnWhenStatementsSpec { else -> { println() } } """.trimIndent(), - "println()"(2), + "->"(2), ), ) } @@ -150,9 +150,9 @@ class BracesOnWhenStatementsSpec { else -> { println() } } """.trimIndent(), - "{ println() }"(1), - "{ println() }"(2), - "{ println() }"(3), + "->"(1), + "->"(2), + "->"(3), ), ) @@ -166,7 +166,7 @@ class BracesOnWhenStatementsSpec { else -> println() } """.trimIndent(), - "{ println() }"(1), + "->"(1), ), ) @@ -180,7 +180,7 @@ class BracesOnWhenStatementsSpec { else -> { println() } } """.trimIndent(), - "{ println() }"(1), + "->"(3), ), ) @@ -194,7 +194,7 @@ class BracesOnWhenStatementsSpec { else -> println() } """.trimIndent(), - "{ println() }"(1), + "->"(2), ), ) } @@ -212,7 +212,7 @@ class BracesOnWhenStatementsSpec { when (1) { 1 -> println() 2 -> println() - else -> println() + else -> { println(); println() } } """.trimIndent(), *NOTHING, @@ -229,9 +229,9 @@ class BracesOnWhenStatementsSpec { else -> { println() } } """.trimIndent(), - "{ println() }"(1), - "{ println() }"(2), - "{ println() }"(3), + "->"(1), + "->"(2), + "->"(3), ), ) @@ -242,10 +242,10 @@ class BracesOnWhenStatementsSpec { when (1) { 1 -> { println() } 2 -> println() - else -> println() + else -> { println(); println() } } """.trimIndent(), - "{ println() }"(1), + "->"(1), ), ) @@ -254,12 +254,12 @@ class BracesOnWhenStatementsSpec { flag( """ when (1) { - 1 -> println() + 1 -> { println(); println() } 2 -> println() else -> { println() } } """.trimIndent(), - "{ println() }"(1), + "->"(3), ), ) @@ -270,10 +270,10 @@ class BracesOnWhenStatementsSpec { when (1) { 1 -> println() 2 -> { println() } - else -> println() + else -> { println(); println() } } """.trimIndent(), - "{ println() }"(1), + "->"(2), ), ) } @@ -373,10 +373,13 @@ class BracesOnWhenStatementsSpec { 1 -> println() 2 -> { println() } - else -> { println(); println() } + else -> { + println() + println() + } } """.trimIndent(), - "println()"(1), + "->"(1), ), ) @@ -389,7 +392,10 @@ class BracesOnWhenStatementsSpec { println() } 2 -> { println() } - else -> { println(); println() } + else -> { + println() + println() + } } """.trimIndent(), *NOTHING, @@ -405,10 +411,13 @@ class BracesOnWhenStatementsSpec { println() } 2 -> println() - else -> { println(); println() } + else -> { + println() + println() + } } """.trimIndent(), - "println()"(2), + "->"(2), ), ) @@ -428,7 +437,7 @@ class BracesOnWhenStatementsSpec { println() } """.trimIndent(), - "println()"(4), + "->"(3), ), ) @@ -448,7 +457,76 @@ class BracesOnWhenStatementsSpec { } } """.trimIndent(), - "println()"(2), + "->"(2), + ), + ) + + @TestFactory + fun `nested when inside when case block`() = listOf( + flag( + """ + when (1) { + 1 -> { + when (2) { + 1 -> { + println() + } + else -> { + println() + } + } + } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `nested when inside when condition`() = listOf( + flag( + """ + when ( + when (2) { + 1 -> { + 1 + } + else -> { + 2 + } + } + ) { + 1 -> { + println() + } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `weird curly formatting for multiline whens`() = listOf( + flag( + """ + when (1) { + 1 -> + { + println() + } + 2 -> { println() + } + 3 -> { + println() } + else -> + { println() } + } + """.trimIndent(), + *NOTHING, ), ) } @@ -468,7 +546,7 @@ class BracesOnWhenStatementsSpec { println() 2 -> println() - else -> + else -> println() } """.trimIndent(), @@ -492,9 +570,9 @@ class BracesOnWhenStatementsSpec { } } """.trimIndent(), - "{\n println()\n }"(1), - "{\n println()\n }"(2), - "{\n println()\n }"(3), + "->"(1), + "->"(2), + "->"(3), ), ) @@ -513,7 +591,7 @@ class BracesOnWhenStatementsSpec { println() } """.trimIndent(), - "{\n println()\n }"(1), + "->"(1), ), ) @@ -528,10 +606,11 @@ class BracesOnWhenStatementsSpec { println() else -> { println() + println() } } """.trimIndent(), - "{\n println()\n }"(1), + "->"(3), ), ) @@ -545,13 +624,57 @@ class BracesOnWhenStatementsSpec { 2 -> { println() + println() } + else -> println() + } + """.trimIndent(), + "->"(2), + ), + ) + + @TestFactory + fun `nested when inside when case block`() = listOf( + flag( + """ + when (1) { + 1 -> + when (2) { + 1 -> + println() + else -> + println() + } + 2 -> println() + else -> println() + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `nested when inside when condition`() = listOf( + flag( + """ + when ( + when (2) { + 1 -> + 1 + else -> + 2 + } + ) { + 1 -> + println() + else -> + println() } """.trimIndent(), - "{\n println()\n }"(1), + *NOTHING, ), ) } @@ -595,9 +718,9 @@ class BracesOnWhenStatementsSpec { } } """.trimIndent(), - "{\n println()\n }"(1), - "{\n println()\n }"(2), - "{\n println()\n }"(3), + "->"(1), + "->"(2), + "->"(3), ), ) @@ -619,7 +742,7 @@ class BracesOnWhenStatementsSpec { } } """.trimIndent(), - "{\n println()\n }"(1), + "->"(1), ), ) @@ -641,7 +764,7 @@ class BracesOnWhenStatementsSpec { } } """.trimIndent(), - "{\n println()\n }"(1), + "->"(3), ), ) @@ -662,7 +785,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), - "{\n println()\n }"(1), + "->"(2), ), ) @@ -786,6 +909,71 @@ class BracesOnWhenStatementsSpec { "when"(1), ), ) + + @TestFactory + fun `nested when inside when case block`() = listOf( + flag( + """ + when (1) { + 1 -> { + when (2) { + 1 -> + println() + else -> + println() + } + } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `nested when inside when condition`() = listOf( + flag( + """ + when ( + when (2) { + 1 -> + 1 + else -> + 2 + } + ) { + 1 -> { + println() + } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `weird curly formatting for multiline whens`() = listOf( + flag( + """ + when (1) { + 1 -> + { + println() + } + 2 -> { println() + } + 3 -> { + println() } + else -> + { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) } }