From e003b8afe17915697c99e7586ea75c2016156091 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Sat, 25 Feb 2023 18:03:03 -0400 Subject: [PATCH 01/11] Add "BracesOnWhenStatements" rule --- .../main/resources/default-detekt-config.yml | 4 + .../rules/style/BracesOnWhenStatements.kt | 254 +++++ .../detekt/rules/style/StyleGuideProvider.kt | 1 + .../rules/style/BracesOnWhenStatementsSpec.kt | 904 ++++++++++++++++++ 4 files changed, 1163 insertions(+) create mode 100644 detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt create mode 100644 detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatementsSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 669ada152b5..74e6a5c984d 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -510,6 +510,10 @@ style: active: false singleLine: 'never' multiLine: 'always' + BracesOnWhenStatements: + active: false + singleLine: 'consistent' + multiLine: 'consistent' CanBeNonNullable: active: false CascadingCallWrapping: 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 new file mode 100644 index 00000000000..acee204e7ec --- /dev/null +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt @@ -0,0 +1,254 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import io.gitlab.arturbosch.detekt.api.config +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.KtWhenExpression + +/** + * This rule detects `when` statements which do not comply with the specified policy. + * 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`). + * + * 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. + * + * Available options are: + * * `never`: forces no braces on any entry. + * * `necessary`: forces no braces on any entry except where necessary for multi-statement entries. + * * `consistent`: ensures that braces are consistent within `when` statement. + * If there are braces on one of the entry, all entries should have it. + * * `always`: forces braces on all entries. + * + * + * // singleLine = 'never' + * when (a) { + * 1 -> { f1() } + * 2 -> f2() + * } + * // multiLine = 'never' + * when (a) { + * 1 -> { + * f1() + * } + * 2 -> f2() + * } + * // singleLine = 'necessary' + * when (a) { + * 1 -> { f1() } + * 2 -> f2() + * } + * // multiLine = 'necessary' + * when (a) { + * 1 -> { f1() } + * 2 -> { f2(); f3() } + * } + * + * // singleLine = 'consistent' + * when (a) { + * 1 -> { f1() } + * 2 -> f2() + * } + * // multiLine = 'consistent' + * when (a) { + * 1 -> + * f1() + * 2 -> { + * f2() + * f3() + * } + * } + * + * // singleLine = 'always' + * when (a) { + * 1 -> { f1() } + * 2 -> f2() + * } + * // multiLine = 'always' + * when (a) { + * 1 -> + * f1() + * 2 -> { + * f2() + * f3() + * } + * } + * + * + * + * + * // singleLine = 'never' + * when (a) { + * 1 -> f1() + * 2 -> f2() + * } + * // multiLine = 'never' + * when (a) { + * 1 -> + * f1() + * 2 -> f2() + * } + * // singleLine = 'necessary' + * when (a) { + * 1 -> f1() + * 2 -> f2() + * } + * // multiLine = 'necessary' + * when (a) { + * 1 -> + * f1() + * 2 -> { + * f2() + * f3() + * } + * } + * + * // singleLine = 'consistent' + * when (a) { + * 1 -> { f1() } + * 2 -> { f2() } + * } + * when (a) { + * 1 -> f1() + * 2 -> f2() + * } + * // multiLine = 'consistent' + * when (a) { + * 1 -> { + * f1() + * } + * 2 -> { + * f2() + * f3() + * } + * } + * + * // singleLine = 'always' + * when (a) { + * 1 -> { f1() } + * 2 -> { f2() } + * } + * // multiLine = 'always' + * when (a) { + * 1 -> { + * f1() + * } + * 2 -> { + * f2() + * f3() + * } + * } + * + * + */ +class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { + override val issue = Issue( + javaClass.simpleName, + Severity.Style, + "Braces do not comply with the specified policy", + Debt.FIVE_MINS + ) + + @Configuration("single-line braces policy") + private val singleLine: BracePolicy by config("consistent") { BracePolicy.getValue(it) } + + @Configuration("multi-line braces policy") + private val multiLine: BracePolicy by config("consistent") { BracePolicy.getValue(it) } + + override fun visitWhenExpression(expression: KtWhenExpression) { + super.visitWhenExpression(expression) + + val branches: List = walk(expression) + validate(branches, policy(expression)) + } + + private fun walk(expression: KtWhenExpression): List { + return expression.entries.mapNotNull { it.expression } + } + + private fun validate(list: List, policy: BracePolicy) { + val violators = when (policy) { + BracePolicy.Always -> { + list.filter { !hasBraces(it) } + } + + BracePolicy.Necessary -> { + list.filter { !isMultiStatement(it) && hasBraces(it) } + } + + BracePolicy.Never -> { + list.filter { hasBraces(it) } + } + + BracePolicy.Consistent -> { + val braces = list.count { hasBraces(it) } + val noBraces = list.count { !hasBraces(it) } + if (braces != 0 && noBraces != 0) { + list.take(1) + } else { + emptyList() + } + } + } + violators.forEach { report(it, policy) } + } + + private fun hasBraces(expression: KtExpression): Boolean = + expression is KtBlockExpression + + private fun KtExpression.parentCandidate(): PsiElement? = + this.parent.parent + + 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 + } + return if (multiLineCandidate != null) multiLine else singleLine + } + + private fun report(violator: KtExpression, 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 + else -> error("Violating element (${violator.text}) is not part of this 'when' (${parent.text})") + } + 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." + BracePolicy.Necessary -> "Extra braces exist on this branch, remove them (ignore multi-statement)." + BracePolicy.Never -> "Extra braces exist on this branch, remove them." + } + report(CodeSmell(issue, Entity.from(reported), message)) + } + + enum class BracePolicy(val config: String) { + Always("always"), + Consistent("consistent"), + Necessary("necessary"), + Never("never"); + + companion object { + fun getValue(arg: String): BracePolicy = + values().singleOrNull { it.config == arg } + ?: error("Unknown value $arg, allowed values are: ${values().joinToString("|")}") + } + } +} diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt index 02a5ae4374f..7cbf54fd2b7 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/StyleGuideProvider.kt @@ -78,6 +78,7 @@ class StyleGuideProvider : DefaultRuleSetProvider { MayBeConst(config), PreferToOverPairSyntax(config), BracesOnIfStatements(config), + BracesOnWhenStatements(config), MandatoryBracesLoops(config), NullableBooleanCheck(config), VarCouldBeVal(config), 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 new file mode 100644 index 00000000000..ea0700788cd --- /dev/null +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatementsSpec.kt @@ -0,0 +1,904 @@ +package io.gitlab.arturbosch.detekt.rules.style + +import io.gitlab.arturbosch.detekt.api.TextLocation +import io.gitlab.arturbosch.detekt.rules.style.BracesOnWhenStatements.BracePolicy +import io.gitlab.arturbosch.detekt.rules.style.BracesOnWhenStatementsSpec.Companion.NOT_RELEVANT +import io.gitlab.arturbosch.detekt.rules.style.BracesOnWhenStatementsSpec.Companion.test +import io.gitlab.arturbosch.detekt.test.TestConfig +import io.gitlab.arturbosch.detekt.test.assertThat +import io.gitlab.arturbosch.detekt.test.compileAndLint +import io.gitlab.arturbosch.detekt.test.lint +import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.assertThatCode +import org.junit.jupiter.api.DynamicContainer.dynamicContainer +import org.junit.jupiter.api.DynamicNode +import org.junit.jupiter.api.DynamicTest.dynamicTest +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.TestFactory + +/** + * Note: this class makes extensive use of dynamic tests and containers, few tips for maintenance: + * * Numbers are made relative to the code snippet rather than the whole code passed to Kotlin compiler. [test]. + * * To debug a specific test case, remove all other test cases from the same method. + * * Test coverage is added for all possible configuration options, see [NOT_RELEVANT]. + */ +@Suppress( + "ClassName", + "LongMethod", + "CommentOverPrivateProperty" +) +class BracesOnWhenStatementsSpec { + + @Test + fun `validate behavior of occurrence function`() { + val code = "fun f() { if (true) else if (true) if (true) true }" + assertThat("if"(1)(code)).isEqualTo(10 to 12) + assertThat("if"(2)(code)).isEqualTo(25 to 27) + assertThat("if"(3)(code)).isEqualTo(35 to 37) + assertThat("else"(1)(code)).isEqualTo(20 to 24) + assertThatCode { "if"(4)(code) }.isInstanceOf(IllegalArgumentException::class.java) + assertThatCode { "if"(0)(code) }.isInstanceOf(IllegalArgumentException::class.java) + assertThatCode { "else"(2)(code) }.isInstanceOf(IllegalArgumentException::class.java) + } + + @Nested + inner class singleLine { + + @Nested + inner class `=always` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(BracePolicy.Always.config, NOT_RELEVANT, code, *locations) + + @TestFactory + fun `missing braces are flagged`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> { println() } + } + """.trimIndent(), + "println()"(1), + ), + ) + + @TestFactory + fun `existing braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `partially missing braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + "println()"(1), + ), + ) + + @TestFactory + fun `partially missing braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> { println() } + else -> println() + } + """.trimIndent(), + "println()"(3), + ), + ) + + @TestFactory + fun `partially missing braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> println() + else -> { println() } + } + """.trimIndent(), + "println()"(2), + ), + ) + } + + @Nested + inner class `=never` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(BracePolicy.Never.config, NOT_RELEVANT, code, *locations) + + @TestFactory fun `no braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> println() + else -> println() + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `existing braces are flagged`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + "{ println() }"(1), + "{ println() }"(2), + "{ println() }"(3), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> println() + else -> println() + } + """.trimIndent(), + "{ println() }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> println() + else -> { println() } + } + """.trimIndent(), + "{ println() }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> { println() } + else -> println() + } + """.trimIndent(), + "{ println() }"(1), + ), + ) + } + + @Nested + inner class `=necessary` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(BracePolicy.Necessary.config, NOT_RELEVANT, code, *locations) + + @TestFactory + fun `no braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> println() + else -> println() + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `existing braces are flagged`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + "{ println() }"(1), + "{ println() }"(2), + "{ println() }"(3), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> println() + else -> println() + } + """.trimIndent(), + "{ println() }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> println() + else -> { println() } + } + """.trimIndent(), + "{ println() }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> { println() } + else -> println() + } + """.trimIndent(), + "{ println() }"(1), + ), + ) + } + + @Nested + inner class `=consistent` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(BracePolicy.Consistent.config, NOT_RELEVANT, code, *locations) + + @TestFactory + fun `no braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> println() + else -> println() + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `existing braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `partial braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> println() + 2 -> { println() } + else -> { println() } + } + """.trimIndent(), + "when"(1), + ), + ) + + @TestFactory + fun `partial braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> { println() } + else -> println() + } + """.trimIndent(), + "when"(1), + ), + ) + + @TestFactory + fun `partial braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> { println() } + 2 -> println() + else -> { println() } + } + """.trimIndent(), + "when"(1), + ), + ) + } + } + + @Nested + inner class multiLine { + + @Nested + inner class `=always` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(NOT_RELEVANT, BracePolicy.Always.config, code, *locations) + + @TestFactory + fun `missing braces are flagged`() = listOf( + flag( + """ + when (1) { + 1 -> + println() + 2 -> { println() } + else -> { println(); println() } + } + """.trimIndent(), + "println()"(1), + ), + ) + + @TestFactory + fun `existing braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { println() } + else -> { println(); println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `partially missing braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> println() + else -> { println(); println() } + } + """.trimIndent(), + "println()"(2), + ), + ) + + @TestFactory + fun `partially missing braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { + println() + println() + } + else -> + println() + } + """.trimIndent(), + "println()"(4), + ), + ) + + @TestFactory + fun `partially missing braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> + println() + else -> { + println() + println() + } + } + """.trimIndent(), + "println()"(2), + ), + ) + } + + @Nested + inner class `=never` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(NOT_RELEVANT, BracePolicy.Never.config, code, *locations) + + @TestFactory + fun `no braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> + println() + 2 -> + println() + else -> + println() + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `existing braces are flagged`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { + println() + } + else -> { + println() + } + } + """.trimIndent(), + "{\n println()\n }"(1), + "{\n println()\n }"(2), + "{\n println()\n }"(3), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> + println() + + else -> + println() + } + """.trimIndent(), + "{\n println()\n }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> + println() + 2 -> + println() + else -> { + println() + } + } + """.trimIndent(), + "{\n println()\n }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> + println() + + 2 -> { + println() + } + else -> + println() + + } + """.trimIndent(), + "{\n println()\n }"(1), + ), + ) + } + + @Nested + inner class `=necessary` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(NOT_RELEVANT, BracePolicy.Necessary.config, code, *locations) + + @TestFactory + fun `no braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> + println() + 2 -> + println() + else -> + println() + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `existing braces are flagged`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { + println() + } + else -> { + println() + } + } + """.trimIndent(), + "{\n println()\n }"(1), + "{\n println()\n }"(2), + "{\n println()\n }"(3), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { + println() + println() + } + else -> { + println() + println() + } + } + """.trimIndent(), + "{\n println()\n }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + println() + } + 2 -> { + println() + println() + } + else -> { + println() + } + } + """.trimIndent(), + "{\n println()\n }"(1), + ), + ) + + @TestFactory + fun `partially extra braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + println() + } + 2 -> { + println() + } + else -> + println() + + } + """.trimIndent(), + "{\n println()\n }"(1), + ), + ) + + @TestFactory + fun `existing braces are not flagged when necessary`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + println() + } + 2 -> { + println() + println() + } + else -> { + println() + println() + } + } + """.trimIndent(), + *NOTHING, + ), + ) + } + + @Nested + inner class `=consistent` { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(NOT_RELEVANT, BracePolicy.Consistent.config, code, *locations) + + @TestFactory + fun `no braces are accepted`() = listOf( + flag( + """ + when (1) { + 1 -> + println() + 2 -> + println() + else -> + println() + } + """.trimIndent(), + *NOTHING, + ), + ) + + @TestFactory + fun `existing braces are flagged`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { + println() + } + else -> + println() + } + """.trimIndent(), + "when"(1), + ), + ) + + @TestFactory + fun `partial braces are flagged (first branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> + println() + else -> + println() + } + """.trimIndent(), + "when"(1), + ), + ) + + @TestFactory + fun `partial braces are flagged (last branch)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { + println() + } + else -> + println() + } + """.trimIndent(), + "when"(1), + ), + ) + + @TestFactory + fun `partial braces are flagged (middle branches)`() = listOf( + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> println() + else -> { + println() + } + } + """.trimIndent(), + "when"(1), + ), + ) + } + } + + companion object { + + /** + * Not relevant means, that it should be covered, but for that specific test case the value doesn't matter. + * This needs to be covered still, to make sure it never becomes relevant. + * In this rule, configuration options should be separate for each config, not coupled with each other. + */ + private const val NOT_RELEVANT: String = "*" + + /** + * Nothing is expected to be flagged as a finding. + */ + private val NOTHING: Array<(String) -> Pair> = emptyArray() + + private fun createSubject(singleLine: String, multiLine: String): BracesOnWhenStatements { + val config = TestConfig( + mapOf( + "singleLine" to singleLine, + "multiLine" to multiLine + ) + ) + return BracesOnWhenStatements(config) + } + + private fun options(option: String): List = + if (option == NOT_RELEVANT) { + BracePolicy.values().map { it.config } + } else { + listOf(option) + } + + private fun testCombinations( + singleLine: String, + multiLine: String, + code: String, + vararg locations: (String) -> Pair + ): DynamicNode { + val codeLocation = locations.map { it(code) }.toTypedArray() + // Separately compile the code because otherwise all the combinations would compile them again and again. + val compileTest = dynamicTest("Compiles: $code") { + BracesOnWhenStatements().compileAndLint(code) + } + val validationTests = createBraceTests(singleLine, multiLine) { rule -> + rule.test(code, *codeLocation) + } + val locationString = if (NOTHING.contentEquals(codeLocation)) { + "nothing" + } else { + codeLocation.map { TextLocation(it.first, it.second) }.toString() + } + return dynamicContainer("flags $locationString in `$code`", validationTests + compileTest) + } + + private fun BracesOnWhenStatements.test(code: String, vararg locations: Pair) { + // This creates a 10 character prefix (signature/9, space/1) for every code example. + // Note: not compileAndLint for performance reasons, compilation is in a separate test. + val findings = lint("fun f() { $code }") + // Offset text locations by the above prefix, it results in 0-indexed locations. + val offset = 10 + assertThat(findings) + .hasTextLocations( + *(locations.map { it.first + offset to it.second + offset }).toTypedArray() + ) + } + + /** + * Generates a list of tests for the given brace policy combinations. + * The expectations in the test will be the same for all combinations. + * + * @see options for how the arguments are interpreted. + */ + private fun createBraceTests( + singleLine: String, + multiLine: String, + test: (BracesOnWhenStatements) -> Unit + ): List { + val singleOptions = options(singleLine) + val multiOptions = options(multiLine) + require(singleOptions.isNotEmpty() && multiOptions.isNotEmpty()) { + "No options to test: $singleLine -> $singleOptions, $multiLine -> $multiOptions" + } + return singleOptions.flatMap { singleLineOption -> + multiOptions.map { multiLineOption -> + val trace = Exception("Async stack trace of TestFactory") + dynamicTest("singleLine=$singleLineOption, multiLine=$multiLineOption") { + try { + // Note: if you jumped here from a failed test's stack trace, + // select the "Async stack trace of TestFactory" cause's + // last relevant stack line to jump to the actual test code. + test(createSubject(singleLineOption, multiLineOption)) + } catch (e: Throwable) { + generateSequence(e) { it.cause }.last().initCause(trace) + throw e + } + } + } + } + } + + operator fun String.invoke(ordinal: Int): (String) -> Pair = + { code -> + fun String.next(string: String, start: Int): Int? = indexOf(string, start).takeIf { it != -1 } + + val indices = generateSequence(code.next(this, 0)) { startIndex -> + code.next(this, startIndex + 1) + } + val index = requireNotNull(indices.elementAtOrNull(ordinal - 1)) { + "There's no $ordinal. occurrence of '$this' in '$code'" + } + index to index + this.length + } + } +} From acd5baf4fbf5121a8f3eb6ac75dc5bf2fde012c2 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Sat, 25 Feb 2023 18:45:22 -0400 Subject: [PATCH 02/11] Fix after merge --- .../detekt/rules/style/BracesOnWhenStatementsSpec.kt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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 ea0700788cd..b89570653b4 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 @@ -805,10 +805,8 @@ class BracesOnWhenStatementsSpec { private fun createSubject(singleLine: String, multiLine: String): BracesOnWhenStatements { val config = TestConfig( - mapOf( - "singleLine" to singleLine, - "multiLine" to multiLine - ) + "singleLine" to singleLine, + "multiLine" to multiLine ) return BracesOnWhenStatements(config) } From 0429826d0f56b0736e1f337ea8de21c4f16fc9d5 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Wed, 8 Mar 2023 01:06:29 +0300 Subject: [PATCH 03/11] Update detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Róbert Papp --- .../arturbosch/detekt/rules/style/BracesOnWhenStatements.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 acee204e7ec..19c1337b84e 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 @@ -178,7 +178,7 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { return expression.entries.mapNotNull { it.expression } } - private fun validate(list: List, policy: BracePolicy) { + private fun validate(branches: List, policy: BracePolicy) { val violators = when (policy) { BracePolicy.Always -> { list.filter { !hasBraces(it) } From c1a1f855b3ec31e7f424b52e958d30dc404b4a7a Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Wed, 8 Mar 2023 01:14:23 +0300 Subject: [PATCH 04/11] Update detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Róbert Papp --- .../arturbosch/detekt/rules/style/BracesOnWhenStatements.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 19c1337b84e..06ba3c2fea9 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 @@ -26,7 +26,8 @@ import org.jetbrains.kotlin.psi.KtWhenExpression * or has multiple statements. * * Available options are: - * * `never`: forces no braces on any entry. + * * `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. * * `consistent`: ensures that braces are consistent within `when` statement. * If there are braces on one of the entry, all entries should have it. From 003cfdb70974d11a607db0d7162fad379f45e4b8 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Wed, 8 Mar 2023 01:18:53 +0300 Subject: [PATCH 05/11] Update detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Róbert Papp --- .../arturbosch/detekt/rules/style/BracesOnWhenStatements.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 06ba3c2fea9..2ecce965ca1 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 @@ -30,7 +30,7 @@ import org.jetbrains.kotlin.psi.KtWhenExpression _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. * * `consistent`: ensures that braces are consistent within `when` statement. - * If there are braces on one of the entry, all entries should have it. + * If there are braces on one of the entries, all entries should have it. * * `always`: forces braces on all entries. * * From 40647b845320410aa14fe91a6219735fe377f4e3 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Wed, 8 Mar 2023 01:27:44 +0300 Subject: [PATCH 06/11] Update detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Róbert Papp --- .../arturbosch/detekt/rules/style/BracesOnWhenStatements.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 2ecce965ca1..794a0aa9707 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 @@ -234,7 +234,7 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { 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." - BracePolicy.Necessary -> "Extra braces exist on this branch, remove them (ignore multi-statement)." + BracePolicy.Necessary -> "Extra braces exist on this branch, remove them." BracePolicy.Never -> "Extra braces exist on this branch, remove them." } report(CodeSmell(issue, Entity.from(reported), message)) From 99ade130e2eb9bff79b8d51b5b8ef04d55afc702 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Wed, 8 Mar 2023 01:32:37 +0300 Subject: [PATCH 07/11] Fix tests and bugs --- config/detekt/detekt.yml | 4 + .../main/resources/default-detekt-config.yml | 2 +- .../rules/style/BracesOnWhenStatements.kt | 70 +++-- .../rules/style/BracesOnWhenStatementsSpec.kt | 268 +++++++++++++++--- 4 files changed, 267 insertions(+), 77 deletions(-) diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index e135d9d7d45..edf8e46f9bd 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 cc91d595d75..307a1365ea6 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 794a0aa9707..2337102c71b 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 @@ -11,7 +11,7 @@ import io.gitlab.arturbosch.detekt.api.config 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 +19,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 +50,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 +167,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 +175,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 { !it.hasBraces() } } BracePolicy.Necessary -> { - list.filter { !isMultiStatement(it) && hasBraces(it) } + branches.filter { !it.isMultiStatement() && it.hasBraces() } } BracePolicy.Never -> { - list.filter { hasBraces(it) } + branches.filter { it.hasBraces() } } BracePolicy.Consistent -> { - val braces = list.count { hasBraces(it) } - val noBraces = list.count { !hasBraces(it) } + val braces = branches.count { it.hasBraces() } + val noBraces = branches.count { !it.hasBraces() } if (braces != 0 && noBraces != 0) { - list.take(1) + branches.take(1) } else { emptyList() } @@ -206,31 +208,27 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { violators.forEach { report(it, policy) } } - private fun hasBraces(expression: KtExpression): Boolean = - expression is KtBlockExpression + private fun KtWhenEntry.hasBraces(): 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 = - expression is KtBlockExpression && expression.statements.size > 1 + private fun KtWhenEntry.isMultiStatement(): Boolean = + expression.let { (it is KtBlockExpression) && (it.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 + val multiLineCandidate = expression.entries.firstOrNull { branch -> + branch.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 b89570653b4..3a1723e6cb4 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, + ), + ) } } From 191644c9ebe4533e3f4ef0f739bee082e0698171 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Sun, 19 Mar 2023 11:50:38 -0300 Subject: [PATCH 08/11] Fix tests and bugs --- config/detekt/detekt.yml | 4 - .../rules/style/BracesOnWhenStatements.kt | 28 +- .../rules/style/BracesOnWhenStatementsSpec.kt | 277 ++++++------------ 3 files changed, 104 insertions(+), 205 deletions(-) diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index edf8e46f9bd..e135d9d7d45 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -153,10 +153,6 @@ style: active: true singleLine: 'consistent' multiLine: 'consistent' - BracesOnWhenStatements: - active: false - singleLine: 'necessary' - multiLine: 'necessary' CanBeNonNullable: active: true CascadingCallWrapping: 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 2337102c71b..a3b8296d830 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 @@ -9,10 +9,10 @@ import io.gitlab.arturbosch.detekt.api.Rule import io.gitlab.arturbosch.detekt.api.Severity import io.gitlab.arturbosch.detekt.api.config 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.KtWhenEntry import org.jetbrains.kotlin.psi.KtWhenExpression +import org.jetbrains.kotlin.psi.psiUtil.siblings /** * This rule detects `when` statements which do not comply with the specified policy. @@ -49,15 +49,11 @@ import org.jetbrains.kotlin.psi.KtWhenExpression * // singleLine = 'necessary' * when (a) { * 1 -> { f1() } - * 2 -> f2() - * 3 -> { f3(); f4() } * } * // multiLine = 'necessary' * when (a) { - * 1 -> { f1() } - * 2 -> { - * f2() - * f3() + * 1 -> { + * f1() * } * } * @@ -175,12 +171,9 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { override fun visitWhenExpression(expression: KtWhenExpression) { super.visitWhenExpression(expression) - val branches: List = walk(expression) - validate(branches, policy(expression)) + validate(expression.entries, policy(expression)) } - private fun walk(expression: KtWhenExpression) = expression.entries - private fun validate(branches: List, policy: BracePolicy) { val violators = when (policy) { BracePolicy.Always -> { @@ -210,20 +203,21 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { private fun KtWhenEntry.hasBraces(): Boolean = expression is KtBlockExpression - private fun KtWhenEntry.parentCandidate(): PsiElement? = this.parent - private fun KtWhenEntry.isMultiStatement(): Boolean = expression.let { (it is KtBlockExpression) && (it.statements.size > 1) } private fun policy(expression: KtWhenExpression): BracePolicy { - val multiLineCandidate = expression.entries.firstOrNull { branch -> - branch.text.substringAfter("->", "").contains('\n') + val isMultiLine = expression.entries.any { branch -> + branch.arrow + ?.siblings(forward = true, withItself = false) + ?.any { it.textContains('\n') } + ?: false } - return if (multiLineCandidate != null) multiLine else singleLine + return if (isMultiLine) multiLine else singleLine } private fun report(violator: KtWhenEntry, policy: BracePolicy) { - val parent = violator.parentCandidate() as KtWhenExpression + val parent = violator.parent as KtWhenExpression val reported = when { violator in parent.entries && policy == BracePolicy.Consistent -> parent.whenKeyword violator in parent.entries -> violator.arrow 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 3a1723e6cb4..dc2c0517d0d 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 @@ -42,6 +42,92 @@ class BracesOnWhenStatementsSpec { assertThatCode { "else"(2)(code) }.isInstanceOf(IllegalArgumentException::class.java) } + @Nested + inner class specialTestCases { + + private fun flag(code: String, vararg locations: (String) -> Pair) = + testCombinations(BracePolicy.Never.config, BracePolicy.Always.config, code, *locations) + + @TestFactory + fun `nested when inside when case block`() = listOf( + flag( + """ + when (1) { + 1 -> { + when (2) { 1 -> println(); else -> { println() } } + } + 2 -> println() + else -> { println() } + } + """.trimIndent(), + "->"(3), + "->"(4), + ), + ) + + @TestFactory + fun `nested when inside when subject`() = listOf( + flag( + """ + when (when (2) { 1 -> { 1 }; else -> { 2 } }) { + 1 -> { + println() + } + 2 -> println() + else -> { println() } + } + """.trimIndent(), + "->"(1), + "->"(2), + "->"(4), + ), + ) + + @TestFactory + fun `nested when inside when condition`() = listOf( + flag( + """ + fun f(s: String?) { + when { + when { s != null -> { s.length }; else -> { 0 } } > 5 -> { + true + } + when(s) { "foo" -> { 1 }; "bar" -> { 1 }; else -> { 2 } } == 1 -> false + else -> { null } + } + } + """.trimIndent(), + "->"(1), + "->"(2), + "->"(4), + "->"(5), + "->"(6), + "->"(7), + ), + ) + + @TestFactory + fun `weird curly formatting for multiline whens`() = listOf( + flag( + """ + when (1) { + 1 -> + { + println() + } + 2 -> { println() + } + 3 -> { + println() } + else -> + { println() } + } + """.trimIndent(), + *NOTHING, + ), + ) + } + @Nested inner class singleLine { @@ -57,7 +143,7 @@ class BracesOnWhenStatementsSpec { """ when (1) { 1 -> println() - 2 -> { println() } + 2 -> { println(); println() } } """.trimIndent(), "->"(1), @@ -71,7 +157,7 @@ class BracesOnWhenStatementsSpec { when (1) { 1 -> { println() } 2 -> { println() } - else -> { println() } + else -> { println(); println() } } """.trimIndent(), *NOTHING, @@ -85,7 +171,7 @@ class BracesOnWhenStatementsSpec { when (1) { 1 -> println() 2 -> { println() } - else -> { println() } + else -> { println(); println() } } """.trimIndent(), "->"(1), @@ -98,7 +184,7 @@ class BracesOnWhenStatementsSpec { """ when (1) { 1 -> { println() } - 2 -> { println() } + 2 -> { println(); println() } else -> println() } """.trimIndent(), @@ -111,7 +197,7 @@ class BracesOnWhenStatementsSpec { flag( """ when (1) { - 1 -> { println() } + 1 -> { println(); println() } 2 -> println() else -> { println() } } @@ -147,7 +233,7 @@ class BracesOnWhenStatementsSpec { when (1) { 1 -> { println() } 2 -> { println() } - else -> { println() } + else -> { println(); println() } } """.trimIndent(), "->"(1), @@ -161,7 +247,7 @@ class BracesOnWhenStatementsSpec { flag( """ when (1) { - 1 -> { println() } + 1 -> { println(); println() } 2 -> println() else -> println() } @@ -460,75 +546,6 @@ class BracesOnWhenStatementsSpec { "->"(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, - ), - ) } @Nested @@ -634,49 +651,6 @@ class BracesOnWhenStatementsSpec { "->"(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(), - *NOTHING, - ), - ) } @Nested @@ -909,71 +883,6 @@ 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, - ), - ) } } From 457c337343e0da7c26cb43abe7e5665096be7f86 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Fri, 7 Apr 2023 20:54:46 -0300 Subject: [PATCH 09/11] Add various improvements --- .../rules/style/BracesOnWhenStatements.kt | 34 +-- .../rules/style/BracesOnWhenStatementsSpec.kt | 277 +++++++++--------- 2 files changed, 158 insertions(+), 153 deletions(-) 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 a3b8296d830..07b891a93e4 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 @@ -48,13 +48,15 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings * } * // singleLine = 'necessary' * when (a) { - * 1 -> { f1() } + * 1 -> { f1() } // unnecessary braces + * 2 -> f2() * } * // multiLine = 'necessary' * when (a) { - * 1 -> { + * 1 -> { // unnecessary braces * f1() * } + * 2 -> f2() * } * * // singleLine = 'consistent' @@ -104,13 +106,13 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings * // singleLine = 'necessary' * when (a) { * 1 -> f1() - * 2 -> f2() + * 2 -> { f2(); f3() } // necessary braces because of multiple statements * } * // multiLine = 'necessary' * when (a) { * 1 -> * f1() - * 2 -> { + * 2 -> { // // necessary braces because of multiple statements * f2() * f3() * } @@ -204,7 +206,7 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { private fun KtWhenEntry.hasBraces(): Boolean = expression is KtBlockExpression private fun KtWhenEntry.isMultiStatement(): Boolean = - expression.let { (it is KtBlockExpression) && (it.statements.size > 1) } + expression.let { it is KtBlockExpression && it.statements.size > 1 } private fun policy(expression: KtWhenExpression): BracePolicy { val isMultiLine = expression.entries.any { branch -> @@ -220,23 +222,19 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { val parent = violator.parent as KtWhenExpression val reported = when { violator in parent.entries && policy == BracePolicy.Consistent -> parent.whenKeyword - violator in parent.entries -> violator.arrow + violator in parent.entries -> requireNotNull(violator.arrow) { + "When branch ${violator.text} has no 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." - BracePolicy.Necessary -> "Extra braces exist on this branch, remove them." - BracePolicy.Never -> "Extra braces exist on this branch, remove them." } - report(CodeSmell(issue, Entity.from(reported), message)) + report(CodeSmell(issue, Entity.from(reported), policy.message)) } - enum class BracePolicy(val config: String) { - Always("always"), - Consistent("consistent"), - Necessary("necessary"), - Never("never"); + enum class BracePolicy(val config: String, val message: String) { + Always("always", "Missing braces on this branch, add them."), + Consistent("consistent", "Inconsistent braces, make sure all branches either have or don't have braces."), + Necessary("necessary", "Extra braces exist on this branch, remove them."), + Never("never", "Extra braces exist on this branch, remove them."); companion object { fun getValue(arg: String): BracePolicy = 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 dc2c0517d0d..58485331a98 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 @@ -44,12 +44,46 @@ class BracesOnWhenStatementsSpec { @Nested inner class specialTestCases { + @TestFactory + fun `special when conditions`() = flag( + """ + when (0) { + 1, 2 -> { println() } + in 3..4 -> { println() } + is Int -> { println() } + 5, 6, in 7..8, is Number -> { println() } + else -> { println() } + } + """.trimIndent(), + "->"(1), + "->"(2), + "->"(3), + "->"(4), + "->"(5), + ) private fun flag(code: String, vararg locations: (String) -> Pair) = testCombinations(BracePolicy.Never.config, BracePolicy.Always.config, code, *locations) @TestFactory - fun `nested when inside when case block`() = listOf( + fun `extra line break between branches`() = + flag( + """ + when (1) { + + 1 -> println() + + 2 -> println() + + else -> println() + + } + """.trimIndent(), + *NOTHING + ) + + @TestFactory + fun `nested when inside when case block`() = flag( """ when (1) { @@ -62,11 +96,10 @@ class BracesOnWhenStatementsSpec { """.trimIndent(), "->"(3), "->"(4), - ), - ) + ) @TestFactory - fun `nested when inside when subject`() = listOf( + fun `nested when inside when subject`() = flag( """ when (when (2) { 1 -> { 1 }; else -> { 2 } }) { @@ -80,11 +113,10 @@ class BracesOnWhenStatementsSpec { "->"(1), "->"(2), "->"(4), - ), - ) + ) @TestFactory - fun `nested when inside when condition`() = listOf( + fun `nested when inside when condition`() = flag( """ fun f(s: String?) { @@ -103,11 +135,10 @@ class BracesOnWhenStatementsSpec { "->"(5), "->"(6), "->"(7), - ), - ) + ) @TestFactory - fun `weird curly formatting for multiline whens`() = listOf( + fun `weird curly formatting for multiline whens`() = flag( """ when (1) { @@ -124,8 +155,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) } @Nested @@ -138,7 +168,7 @@ class BracesOnWhenStatementsSpec { testCombinations(BracePolicy.Always.config, NOT_RELEVANT, code, *locations) @TestFactory - fun `missing braces are flagged`() = listOf( + fun `missing braces are flagged`() = flag( """ when (1) { @@ -147,11 +177,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(1), - ), - ) + ) @TestFactory - fun `existing braces are accepted`() = listOf( + fun `existing braces are accepted`() = flag( """ when (1) { @@ -161,11 +190,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `partially missing braces are flagged (first branch)`() = listOf( + fun `partially missing braces are flagged (first branch)`() = flag( """ when (1) { @@ -175,11 +203,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(1), - ), - ) + ) @TestFactory - fun `partially missing braces are flagged (last branch)`() = listOf( + fun `partially missing braces are flagged (last branch)`() = flag( """ when (1) { @@ -189,11 +216,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(3), - ), - ) + ) @TestFactory - fun `partially missing braces are flagged (middle branches)`() = listOf( + fun `partially missing braces are flagged (middle branches)`() = flag( """ when (1) { @@ -203,8 +229,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(2), - ), - ) + ) } @Nested @@ -213,7 +238,7 @@ class BracesOnWhenStatementsSpec { private fun flag(code: String, vararg locations: (String) -> Pair) = testCombinations(BracePolicy.Never.config, NOT_RELEVANT, code, *locations) - @TestFactory fun `no braces are accepted`() = listOf( + @TestFactory fun `no braces are accepted`() = flag( """ when (1) { @@ -223,11 +248,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `existing braces are flagged`() = listOf( + fun `existing braces are flagged`() = flag( """ when (1) { @@ -239,11 +263,10 @@ class BracesOnWhenStatementsSpec { "->"(1), "->"(2), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (first branch)`() = listOf( + fun `partially extra braces are flagged (first branch)`() = flag( """ when (1) { @@ -253,11 +276,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(1), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (last branch)`() = listOf( + fun `partially extra braces are flagged (last branch)`() = flag( """ when (1) { @@ -267,11 +289,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (middle branches)`() = listOf( + fun `partially extra braces are flagged (middle branches)`() = flag( """ when (1) { @@ -281,8 +302,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(2), - ), - ) + ) } @Nested @@ -292,7 +312,7 @@ class BracesOnWhenStatementsSpec { testCombinations(BracePolicy.Necessary.config, NOT_RELEVANT, code, *locations) @TestFactory - fun `no braces are accepted`() = listOf( + fun `no braces are accepted`() = flag( """ when (1) { @@ -302,11 +322,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `existing braces are flagged`() = listOf( + fun `existing braces are flagged`() = flag( """ when (1) { @@ -318,11 +337,10 @@ class BracesOnWhenStatementsSpec { "->"(1), "->"(2), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (first branch)`() = listOf( + fun `partially extra braces are flagged (first branch)`() = flag( """ when (1) { @@ -332,11 +350,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(1), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (last branch)`() = listOf( + fun `partially extra braces are flagged (last branch)`() = flag( """ when (1) { @@ -346,11 +363,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (middle branches)`() = listOf( + fun `partially extra braces are flagged (middle branches)`() = flag( """ when (1) { @@ -360,8 +376,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(2), - ), - ) + ) } @Nested @@ -371,7 +386,7 @@ class BracesOnWhenStatementsSpec { testCombinations(BracePolicy.Consistent.config, NOT_RELEVANT, code, *locations) @TestFactory - fun `no braces are accepted`() = listOf( + fun `no braces are accepted`() = flag( """ when (1) { @@ -381,11 +396,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `existing braces are accepted`() = listOf( + fun `existing braces are accepted`() = flag( """ when (1) { @@ -395,11 +409,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `partial braces are flagged (first branch)`() = listOf( + fun `partial braces are flagged (first branch)`() = flag( """ when (1) { @@ -409,11 +422,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "when"(1), - ), - ) + ) @TestFactory - fun `partial braces are flagged (last branch)`() = listOf( + fun `partial braces are flagged (last branch)`() = flag( """ when (1) { @@ -423,11 +435,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "when"(1), - ), - ) + ) @TestFactory - fun `partial braces are flagged (middle branches)`() = listOf( + fun `partial braces are flagged (middle branches)`() = flag( """ when (1) { @@ -437,8 +448,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "when"(1), - ), - ) + ) } } @@ -452,7 +462,7 @@ class BracesOnWhenStatementsSpec { testCombinations(NOT_RELEVANT, BracePolicy.Always.config, code, *locations) @TestFactory - fun `missing braces are flagged`() = listOf( + fun `missing braces are flagged`() = flag( """ when (1) { @@ -466,11 +476,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(1), - ), - ) + ) @TestFactory - fun `existing braces are accepted`() = listOf( + fun `existing braces are accepted`() = flag( """ when (1) { @@ -485,11 +494,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `partially missing braces are flagged (first branch)`() = listOf( + fun `partially missing braces are flagged (first branch)`() = flag( """ when (1) { @@ -504,11 +512,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(2), - ), - ) + ) @TestFactory - fun `partially missing braces are flagged (last branch)`() = listOf( + fun `partially missing braces are flagged (last branch)`() = flag( """ when (1) { @@ -524,11 +531,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(3), - ), - ) + ) @TestFactory - fun `partially missing braces are flagged (middle branches)`() = listOf( + fun `partially missing braces are flagged (middle branches)`() = flag( """ when (1) { @@ -544,8 +550,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(2), - ), - ) + ) } @Nested @@ -555,7 +560,7 @@ class BracesOnWhenStatementsSpec { testCombinations(NOT_RELEVANT, BracePolicy.Never.config, code, *locations) @TestFactory - fun `no braces are accepted`() = listOf( + fun `no braces are accepted`() = flag( """ when (1) { @@ -568,11 +573,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `existing braces are flagged`() = listOf( + fun `existing braces are flagged`() = flag( """ when (1) { @@ -590,11 +594,10 @@ class BracesOnWhenStatementsSpec { "->"(1), "->"(2), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (first branch)`() = listOf( + fun `partially extra braces are flagged (first branch)`() = flag( """ when (1) { @@ -609,11 +612,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(1), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (last branch)`() = listOf( + fun `partially extra braces are flagged (last branch)`() = flag( """ when (1) { @@ -628,11 +630,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (middle branches)`() = listOf( + fun `partially extra braces are flagged (middle branches)`() = flag( """ when (1) { @@ -649,8 +650,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(2), - ), - ) + ) } @Nested @@ -660,7 +660,7 @@ class BracesOnWhenStatementsSpec { testCombinations(NOT_RELEVANT, BracePolicy.Necessary.config, code, *locations) @TestFactory - fun `no braces are accepted`() = listOf( + fun `no braces are accepted`() = flag( """ when (1) { @@ -673,11 +673,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) @TestFactory - fun `existing braces are flagged`() = listOf( + fun `existing braces are flagged`() = flag( """ when (1) { @@ -695,11 +694,10 @@ class BracesOnWhenStatementsSpec { "->"(1), "->"(2), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (first branch)`() = listOf( + fun `partially extra braces are flagged (first branch)`() = flag( """ when (1) { @@ -717,11 +715,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(1), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (last branch)`() = listOf( + fun `partially extra braces are flagged (last branch)`() = flag( """ when (1) { @@ -739,11 +736,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(3), - ), - ) + ) @TestFactory - fun `partially extra braces are flagged (middle branches)`() = listOf( + fun `partially extra braces are flagged (middle branches)`() = flag( """ when (1) { @@ -760,11 +756,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "->"(2), - ), - ) + ) @TestFactory - fun `existing braces are not flagged when necessary`() = listOf( + fun `existing braces are not flagged when necessary`() = flag( """ when (1) { @@ -783,8 +778,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) } @Nested @@ -794,7 +788,7 @@ class BracesOnWhenStatementsSpec { testCombinations(NOT_RELEVANT, BracePolicy.Consistent.config, code, *locations) @TestFactory - fun `no braces are accepted`() = listOf( + fun `no braces are accepted`() = flag( """ when (1) { @@ -807,11 +801,28 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), *NOTHING, - ), - ) + ) + + @TestFactory + fun `existing braces are accepted`() = + flag( + """ + when (1) { + 1 -> { + println() + } + 2 -> { println() } + else -> { + println() + println() + } + } + """.trimIndent(), + *NOTHING, + ) @TestFactory - fun `existing braces are flagged`() = listOf( + fun `inconsistent braces are flagged`() = flag( """ when (1) { @@ -826,11 +837,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "when"(1), - ), - ) + ) @TestFactory - fun `partial braces are flagged (first branch)`() = listOf( + fun `inconsistent braces are flagged (first branch)`() = flag( """ when (1) { @@ -844,11 +854,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "when"(1), - ), - ) + ) @TestFactory - fun `partial braces are flagged (last branch)`() = listOf( + fun `inconsistent braces are flagged (last branch)`() = flag( """ when (1) { @@ -863,11 +872,10 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "when"(1), - ), - ) + ) @TestFactory - fun `partial braces are flagged (middle branches)`() = listOf( + fun `inconsistent braces are flagged (middle branches)`() = flag( """ when (1) { @@ -881,8 +889,7 @@ class BracesOnWhenStatementsSpec { } """.trimIndent(), "when"(1), - ), - ) + ) } } From 0e45543692e461e63a0d3bc2c6631d94fad0852f Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Thu, 13 Apr 2023 12:00:35 -0300 Subject: [PATCH 10/11] Add various improvements --- .../rules/style/BracesOnWhenStatements.kt | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) 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 07b891a93e4..918bfd440de 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 @@ -36,24 +36,24 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings * * // singleLine = 'never' * when (a) { - * 1 -> { f1() } + * 1 -> { f1() } // Not allowed. * 2 -> f2() * } * // multiLine = 'never' * when (a) { - * 1 -> { + * 1 -> { // Not allowed. * f1() * } * 2 -> f2() * } * // singleLine = 'necessary' * when (a) { - * 1 -> { f1() } // unnecessary braces + * 1 -> { f1() } // Unnecessary braces. * 2 -> f2() * } * // multiLine = 'necessary' * when (a) { - * 1 -> { // unnecessary braces + * 1 -> { // Unnecessary braces. * f1() * } * 2 -> f2() @@ -67,7 +67,7 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings * // multiLine = 'consistent' * when (a) { * 1 -> - * f1() + * f1() // Missing braces. * 2 -> { * f2() * f3() @@ -77,12 +77,12 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings * // singleLine = 'always' * when (a) { * 1 -> { f1() } - * 2 -> f2() + * 2 -> f2() // Missing braces. * } * // multiLine = 'always' * when (a) { * 1 -> - * f1() + * f1() // Missing braces. * 2 -> { * f2() * f3() @@ -106,13 +106,13 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings * // singleLine = 'necessary' * when (a) { * 1 -> f1() - * 2 -> { f2(); f3() } // necessary braces because of multiple statements + * 2 -> { f2(); f3() } // Necessary braces because of multiple statements. * } * // multiLine = 'necessary' * when (a) { * 1 -> * f1() - * 2 -> { // // necessary braces because of multiple statements + * 2 -> { // // Necessary braces because of multiple statements. * f2() * f3() * } @@ -210,22 +210,19 @@ class BracesOnWhenStatements(config: Config = Config.empty) : Rule(config) { private fun policy(expression: KtWhenExpression): BracePolicy { val isMultiLine = expression.entries.any { branch -> - branch.arrow - ?.siblings(forward = true, withItself = false) - ?.any { it.textContains('\n') } - ?: false + requireNotNull(branch.arrow) { "When branch ${branch.text} has no arrow!" } + .siblings(forward = true, withItself = false) + .any { it.textContains('\n') } } return if (isMultiLine) multiLine else singleLine } private fun report(violator: KtWhenEntry, policy: BracePolicy) { - val parent = violator.parent as KtWhenExpression - val reported = when { - violator in parent.entries && policy == BracePolicy.Consistent -> parent.whenKeyword - violator in parent.entries -> requireNotNull(violator.arrow) { - "When branch ${violator.text} has no arrow!" - } - else -> error("Violating element (${violator.text}) is not part of this 'when' (${parent.text})") + val reported = when (policy) { + BracePolicy.Consistent -> (violator.parent as KtWhenExpression).whenKeyword + BracePolicy.Always, + BracePolicy.Necessary, + BracePolicy.Never -> requireNotNull(violator.arrow) { "When branch ${violator.text} has no arrow!" } } report(CodeSmell(issue, Entity.from(reported), policy.message)) } From 70183dea1c9658fa781570beee820a8533ecb412 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Thu, 13 Apr 2023 18:05:18 +0300 Subject: [PATCH 11/11] Update detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/BracesOnWhenStatements.kt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Róbert Papp --- .../arturbosch/detekt/rules/style/BracesOnWhenStatements.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 918bfd440de..cf97dc368fc 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 @@ -112,7 +112,7 @@ import org.jetbrains.kotlin.psi.psiUtil.siblings * when (a) { * 1 -> * f1() - * 2 -> { // // Necessary braces because of multiple statements. + * 2 -> { // Necessary braces because of multiple statements. * f2() * f3() * }