From 740dfcf01263c7463d8619d3bec2d57601f897da Mon Sep 17 00:00:00 2001 From: Dmitry Suzdalev Date: Fri, 27 May 2022 12:56:56 +0200 Subject: [PATCH] Improve ModifierParameterPosition Make it better follow compose guidelines Closes #1 --- CHANGELOG.md | 5 + gradle.properties | 2 +- .../rule/compose/ModifierParameterPosition.kt | 71 +++++- .../compose/ModifierParameterPositionTest.kt | 219 +++++++++++++++++- 4 files changed, 280 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c538c53..ff60323 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 1.0.1 - 2022-05-26 + +* Update `ModifierParameterPosition` rule to better follow Compose style: `modifier` parameter should be a first _optional_ parameter, i.e. it should come after _required_ parameters and before _optional_ parameters + + ## 1.0.0 - 2022-05-19 * Initial release diff --git a/gradle.properties b/gradle.properties index 4e43452..ff17178 100644 --- a/gradle.properties +++ b/gradle.properties @@ -2,7 +2,7 @@ org.gradle.jvmargs=-XX:MaxMetaspaceSize=1g kotlin.code.style=official -versionName=1.0.0 +versionName=1.0.1 pomGroupId=ru.kode pomDescription=Detekt rules for Jetpack Compose pomUrl=https://github.com/appKODE/detekt-rules-compose diff --git a/src/main/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPosition.kt b/src/main/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPosition.kt index 91c6057..5d1d0b3 100644 --- a/src/main/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPosition.kt +++ b/src/main/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPosition.kt @@ -20,20 +20,41 @@ import java.util.Collections import java.util.IdentityHashMap /** - * Checks that "modifier" argument for Composable function is passed as a first parameter. + * Checks that "modifier" argument for Composable function is passed as a first optional parameter. * * Wrong: * ``` * fun Button( - * arrangement = Vertical, - * modifier = Modifier, + * arrangement: Arrangement = Arrangement.spacedBy(12.dp), + * modifier: Modifier = Modifier.height(16.dp), * ) * ``` * Correct: * ``` * fun Button( - * modifier = Modifier, - * arrangement = Vertical, + * modifier: Modifier = Modifier.height(16.dp), + * arrangement: Arrangement = Arrangement.spacedBy(12.dp), + * ) + * ``` + * + * When required and optional parameters are present, `modifier` needs to be the first among the optional parameters: + * + * Wrong: + * ``` + * fun Button( + * text: String, + * onClick: () -> Unit, + * arrangement: Arrangement = Arrangement.spacedBy(12.dp), + * modifier: Modifier = Modifier.height(16.dp), + * ) + * ``` + * Correct: + * ``` + * fun Button( + * text: String, + * onClick: () -> Unit, + * modifier: Modifier = Modifier.height(16.dp), + * arrangement: Arrangement = Arrangement.spacedBy(12.dp), * ) * ``` */ @@ -54,9 +75,26 @@ class ModifierParameterPosition(config: Config = Config.empty) : Rule(config) { } private fun checkFunction(function: KtNamedFunction) { - val position = function.valueParameters.indexOfFirst { it.isModifierParameter() } - if (position > 0) { - incorrectPositions.add(function) + if (function.valueParameters.any { it.isComposableSlot() && !it.hasDefaultValue() }) { + // there's no point in enforcing modifier-after-last-required in presence of required composable lambda slots: + // putting modifier after "content" slot would break "trailing lambda" syntax, and even if we would make + // an exception and require to put modifier before the slot, it still accomplishes nothing, because slot is + // a required-argument and so named arguments syntax would be needed anyway + return + } + val valueParameters = function.valueParameters + val modifierPosition = valueParameters.indexOfFirst { it.isModifierParameter() } + if (modifierPosition >= 0) { + val firstOptionalPosition = valueParameters.indexOfFirst { it.hasDefaultValue() && !it.isModifierParameter() } + val lastRequiredPosition = valueParameters.indexOfLast { !it.hasDefaultValue() && !it.isModifierParameter() } + when { + lastRequiredPosition >= 0 && modifierPosition != lastRequiredPosition + 1 -> { + incorrectPositions.add(function) + } + firstOptionalPosition >= 0 && modifierPosition != firstOptionalPosition - 1 -> { + incorrectPositions.add(function) + } + } } } @@ -66,15 +104,28 @@ class ModifierParameterPosition(config: Config = Config.empty) : Rule(config) { override fun postVisit(root: KtFile) { incorrectPositions.forEach { node -> + val valueParameters = node.valueParameters + val firstOptional = valueParameters.firstOrNull { it.hasDefaultValue() } + val lastRequired = valueParameters + .filterNot { it.isModifierParameter() }.lastOrNull { !it.hasDefaultValue() } report( CodeSmell( issue, - Entity.from(node, Location.from(node.valueParameters.first { it.isModifierParameter() })), - "Modifier parameter of composable functions must always be first" + Entity.from(node, Location.from(valueParameters.first { it.isModifierParameter() })), + if (firstOptional != null && lastRequired == null) { + "Modifier parameter should be the first optional parameter" + + " (put it before \"${firstOptional.identifierName()}\")" + } else if (lastRequired != null) { + "Modifier parameter should be the first parameter after required parameters" + + " (put it after \"${lastRequired.identifierName()}\")" + } else { + "Modifier parameter must be a first optional parameter" + } ) ) } } private fun KtParameter.isModifierParameter() = identifierName() == "modifier" + private fun KtParameter.isComposableSlot() = this.typeReference?.hasAnnotation("Composable") == true } diff --git a/src/test/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPositionTest.kt b/src/test/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPositionTest.kt index 63540e8..45f5d5f 100644 --- a/src/test/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPositionTest.kt +++ b/src/test/kotlin/ru/kode/detekt/rule/compose/ModifierParameterPositionTest.kt @@ -7,15 +7,17 @@ import io.gitlab.arturbosch.detekt.test.lint import io.kotest.core.spec.style.ShouldSpec import io.kotest.matchers.collections.shouldBeEmpty import io.kotest.matchers.collections.shouldHaveSize +import io.kotest.matchers.string.shouldContain class ModifierParameterPositionTest : ShouldSpec({ - should("report when incorrect position") { + should("report when incorrect position with all optional parameters") { // language=kotlin val code = """ @Composable fun Test( - verticalAlignment = Alignment.CenterVertically, - modifier = modifier.height(24.dp), + verticalAlignment: Alignment = Alignment.CenterVertically, + modifier: Modifier = Modifier.height(24.dp), + enabled: Boolean = false, ) { Text(text = props.title) } @@ -24,15 +26,16 @@ class ModifierParameterPositionTest : ShouldSpec({ val findings = ModifierParameterPosition().lint(code) findings shouldHaveSize 1 + findings.first().message shouldContain "before \"verticalAlignment\"" } - should("not report when in first position") { + should("not report when in first position with all optional parameters") { // language=kotlin val code = """ @Composable fun Test( - modifier = modifier.height(24.dp), - verticalAlignment = Alignment.CenterVertically, + modifier: Modifier = Modifier.height(24.dp), + verticalAlignment: Alignment = Alignment.CenterVertically, ) { Text(text = props.title) } @@ -42,4 +45,208 @@ class ModifierParameterPositionTest : ShouldSpec({ findings.shouldBeEmpty() } + + should("not report when it is the single parameter") { + // language=kotlin + val code = """ + @Composable + fun Test( + modifier: Modifier = Modifier.height(24.dp), + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings.shouldBeEmpty() + } + + should("report incorrect position with a mix of required and optional parameters") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + verticalAlignment: Alignment = Alignment.CenterVertically, + modifier: Modifier = Modifier.height(24.dp), + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings shouldHaveSize 1 + findings.first().message shouldContain "after \"text\"" + } + + should("not report when modifier is optional parameter") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + modifier: Modifier = Modifier.height(24.dp), + verticalAlignment: Alignment = Alignment.CenterVertically, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings.shouldBeEmpty() + } + + should("not report when modifier is last and no optional parameters") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + modifier: Modifier = Modifier.height(24.dp), + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings.shouldBeEmpty() + } + + should("report incorrect position when no optional parameters and modifier is not optional") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + modifier: Modifier, + verticalAlignment: Alignment, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings shouldHaveSize 1 + findings.first().message shouldContain "after \"verticalAlignment\"" + } + + should("not report incorrect position when no optional parameters non-optional modifier is last") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + verticalAlignment: Alignment, + modifier: Modifier, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings.shouldBeEmpty() + } + + should("report incorrect position when optional parameters exist and modifier is not optional") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + verticalAlignment: Alignment = Alignment.Center, + modifier: Modifier, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings shouldHaveSize 1 + findings.first().message shouldContain "after \"text\"" + } + + should("not report when optional parameters and event handlers and modifier is before first optional") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + onClick: () -> Unit, + modifier: Modifier, + verticalAlignment: Alignment = Alignment.Center, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings.shouldBeEmpty() + } + + should("report incorrect position when optional parameters interspersed with required parameters") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + modifier: Modifier, + isEnabled: Boolean = false, + verticalAlignment: Alignment, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings shouldHaveSize 1 + findings.first().message shouldContain "after \"verticalAlignment\"" + } + + should("not report incorrect position when last parameter is a required composable lambda") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + modifier: Modifier, + verticalAlignment: Alignment, + content: @Composable () -> Unit, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings.shouldBeEmpty() + } + + should("report incorrect position when incorrect position with composable lambda with default value") { + // language=kotlin + val code = """ + @Composable + fun Test( + text: String, + modifier: Modifier, + verticalAlignment: Alignment, + content: @Composable () -> Unit = {}, + ) { + Text(text = props.title) + } + """.trimIndent() + + val findings = ModifierParameterPosition().lint(code) + + findings shouldHaveSize 1 + findings.first().message shouldContain "after \"verticalAlignment\"" + } })