Improve ModifierParameterPosition

Make it better follow compose guidelines

Closes #1

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

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

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
 }

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\""
+  }
 })