Take out MutableStateParameter rule from MutableParameters rule (#159)

mrmans0n · Dec 13, 2023 · d1dfd06 · d1dfd06
1 parent f1ddfdf
commit d1dfd06
Show file tree

Hide file tree

Showing 13 changed files with 190 additions and 23 deletions.
diff --git a/core-common/src/main/kotlin/io/nlopez/rules/core/util/KtCallableDeclarations.kt b/core-common/src/main/kotlin/io/nlopez/rules/core/util/KtCallableDeclarations.kt
@@ -25,8 +25,6 @@ private val KnownMutableCommonTypesRegex = sequenceOf(
     "MutableMap<.*>\\??",
     "HashMap<.*>\\??",
     "Hashtable<.*>\\??",
-    // Compose
-    "MutableState<.*>\\??",
     // Flow
     "MutableStateFlow<.*>\\??",
     "MutableSharedFlow<.*>\\??",

diff --git a/docs/detekt.md b/docs/detekt.md
@@ -22,6 +22,12 @@ For the rules to be picked up, you will need to enable them in your `detekt.yml`
 Compose:
   ComposableAnnotationNaming:
     active: true
+  ComposableNaming:
+    active: true
+    # You can optionally disable the checks in this rule for regex matches against the composable name (e.g. molecule presenters)
+    # allowedComposableFunctionNames: .*Presenter,.*MoleculePresenter
+  ComposableParamOrder:
+    active: true
   CompositionLocalAllowlist:
     active: true
     # You can optionally define a list of CompositionLocals that are allowed here
@@ -59,11 +65,7 @@ Compose:
     # contentEmitters: MyComposable,MyOtherComposable
   MutableParams:
     active: true
-  ComposableNaming:
-    active: true
-    # You can optionally disable the checks in this rule for regex matches against the composable name (e.g. molecule presenters)
-    # allowedComposableFunctionNames: .*Presenter,.*MoleculePresenter
-  ComposableParamOrder:
+  MutableStateParam:
     active: true
   PreviewAnnotationNaming:
     active: true

diff --git a/docs/rules.md b/docs/rules.md
@@ -7,7 +7,8 @@ Compose is built upon the idea of a [unidirectional data flow](https://developer
 In practice, there are a few common things to look out for:
 
 - Do not pass ViewModels (or objects from DI) down.
-- Do not pass `State<Foo>` or `MutableState<Bar>` instances down.
+- Do not pass `MutableState<Bar>` instances down.
+- Do not pass inherently mutable types, that can't be observed types, down.
 
 Instead pass down the relevant data to the function, and optional lambdas for callbacks.
 
@@ -71,10 +72,22 @@ This is an anti-pattern though as it breaks the pattern of state flowing down, a
 
 There are a few reasons for this, but the main one is that it is very easy to use a mutable object which does not trigger recomposition. Without triggering recomposition, your composables will not automatically update to reflect the updated value.
 
-Passing `ArrayList<T>`, `MutableState<T>`, `ViewModel` are common examples of this (but not limited to those types).
+Passing `ArrayList<T>` or `ViewModel` are common examples of this (but not limited to those types).
 
 Related rule: [compose:mutable-params-check](https://github.com/mrmans0n/compose-rules/blob/main/rules/common/src/main/kotlin/io/nlopez/compose/rules/MutableParameters.kt)
 
+### Do not use MutableState as a parameter
+
+This practice also follows on from the 'Hoist all the things' item above. When using `MutableState<T>` in a @Composable function signature as a parameter, this is promoting joint ownership over a state between a component and its user.
+
+Instead, if possible, consider making the component stateless and concede the state change to the caller. If mutation of the parent’s owned property is required in the component, consider creating a ComponentState class with the domain specific meaningful field that is backed by `mutableStateOf(...)`.
+
+When a component accepts MutableState as a parameter, it gains the ability to change it. This results in the split ownership of the state, and the usage side that owns the state now has no control over how and when it will be changed from within the component’s implementation.
+
+More info: [Compose API guidelines](https://android.googlesource.com/platform/frameworks/support/+/androidx-main/compose/docs/compose-component-api-guidelines.md#mutablestate_t_as-a-parameter)
+
+Related rule: [compose:mutable-state-param-check](https://github.com/mrmans0n/compose-rules/blob/main/rules/common/src/main/kotlin/io/nlopez/compose/rules/MutableStateParameter.kt)
+
 ### Do not emit content and return a result
 
 Composable functions should either emit layout content, or return a value, but not both.

diff --git a/rules/common/src/main/kotlin/io/nlopez/compose/rules/MutableStateParameter.kt b/rules/common/src/main/kotlin/io/nlopez/compose/rules/MutableStateParameter.kt
@@ -0,0 +1,35 @@
+// Copyright 2023 Nacho Lopez
+// SPDX-License-Identifier: Apache-2.0
+package io.nlopez.compose.rules
+
+import io.nlopez.rules.core.ComposeKtConfig
+import io.nlopez.rules.core.ComposeKtVisitor
+import io.nlopez.rules.core.Emitter
+import io.nlopez.rules.core.report
+import org.jetbrains.kotlin.psi.KtFunction
+
+class MutableStateParameter : ComposeKtVisitor {
+
+    override fun visitComposable(
+        function: KtFunction,
+        autoCorrect: Boolean,
+        emitter: Emitter,
+        config: ComposeKtConfig,
+    ) {
+        function.valueParameters
+            .filter { it.typeReference?.text?.matches(MutableStateRegex) == true }
+            .forEach { emitter.report(it, MutableStateParameterInCompose) }
+    }
+
+    companion object {
+        private val MutableStateRegex = "MutableState<.*>\\??".toRegex()
+
+        val MutableStateParameterInCompose = """
+            MutableState shouldn't be used as a parameter in a @Composable function, as it promotes joint ownership over a state between a component and its user.
+
+            If possible, consider making the component stateless and concede the state change to the caller. If mutation of the parent’s owned property is required in the component, consider creating a ComponentState class with the domain specific meaningful field that is backed by mutableStateOf().
+
+            See https://mrmans0n.github.io/compose-rules/rules/#do-not-use-mutablestate-as-a-parameter for more information.
+        """.trimIndent()
+    }
+}
diff --git a/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ComposeRuleSetProvider.kt b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ComposeRuleSetProvider.kt
@@ -26,6 +26,7 @@ class ComposeRuleSetProvider : RuleSetProvider {
             ModifierWithoutDefaultCheck(config),
             MultipleContentEmittersCheck(config),
             MutableParametersCheck(config),
+            MutableStateParameterCheck(config),
             NamingCheck(config),
             ParameterOrderCheck(config),
             PreviewAnnotationNamingCheck(config),

diff --git a/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/MutableStateParameterCheck.kt b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/MutableStateParameterCheck.kt
@@ -0,0 +1,22 @@
+// Copyright 2023 Nacho Lopez
+// SPDX-License-Identifier: Apache-2.0
+package io.nlopez.compose.rules.detekt
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.Debt
+import io.gitlab.arturbosch.detekt.api.Issue
+import io.gitlab.arturbosch.detekt.api.Severity
+import io.nlopez.compose.rules.MutableStateParameter
+import io.nlopez.rules.core.ComposeKtVisitor
+import io.nlopez.rules.core.detekt.DetektRule
+
+class MutableStateParameterCheck(config: Config) :
+    DetektRule(config),
+    ComposeKtVisitor by MutableStateParameter() {
+    override val issue: Issue = Issue(
+        id = "MutableStateParam",
+        severity = Severity.Defect,
+        description = MutableStateParameter.MutableStateParameterInCompose,
+        debt = Debt.TWENTY_MINS,
+    )
+}
diff --git a/rules/detekt/src/main/resources/config/config.yml b/rules/detekt/src/main/resources/config/config.yml
@@ -1,6 +1,10 @@
 Compose:
   ComposableAnnotationNaming:
     active: true
+  ComposableNaming:
+    active: true
+  ComposableParamOrder:
+    active: true
   CompositionLocalAllowlist:
     active: true
   CompositionLocalNaming:
@@ -27,9 +31,7 @@ Compose:
     active: true
   MutableParams:
     active: true
-  ComposableNaming:
-    active: true
-  ComposableParamOrder:
+  MutableStateParam:
     active: true
   PreviewAnnotationNaming:
     active: true

diff --git a/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/MutableParametersCheckTest.kt b/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/MutableParametersCheckTest.kt
@@ -19,8 +19,6 @@ class MutableParametersCheckTest {
         @Language("kotlin")
         val code =
             """
-                @Composable
-                fun Something(a: MutableState<String>) {}
                 @Composable
                 fun Something(a: ArrayList<String>) {}
                 @Composable
@@ -29,12 +27,11 @@ class MutableParametersCheckTest {
                 fun Something(a: MutableMap<String, String>) {}
             """.trimIndent()
         val errors = rule.lint(code)
-        assertThat(errors).hasSize(4)
+        assertThat(errors)
             .hasStartSourceLocations(
                 SourceLocation(2, 15),
                 SourceLocation(4, 15),
                 SourceLocation(6, 15),
-                SourceLocation(8, 15),
             )
         for (error in errors) {
             assertThat(error).hasMessage(MutableParameters.MutableParameterInCompose)

diff --git a/...s/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/MutableStateParameterCheckTest.kt b/...s/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/MutableStateParameterCheckTest.kt
@@ -0,0 +1,48 @@
+// Copyright 2023 Nacho Lopez
+// SPDX-License-Identifier: Apache-2.0
+package io.nlopez.compose.rules.detekt
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.SourceLocation
+import io.gitlab.arturbosch.detekt.test.assertThat
+import io.gitlab.arturbosch.detekt.test.lint
+import io.nlopez.compose.rules.MutableStateParameter
+import org.intellij.lang.annotations.Language
+import org.junit.jupiter.api.Test
+
+class MutableStateParameterCheckTest {
+
+    private val rule = MutableStateParameterCheck(Config.empty)
+
+    @Test
+    fun `errors when a Composable has a MutableState parameter`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(a: MutableState<String>) {}
+            """.trimIndent()
+        val errors = rule.lint(code)
+        assertThat(errors)
+            .hasStartSourceLocations(
+                SourceLocation(2, 15),
+            )
+        for (error in errors) {
+            assertThat(error).hasMessage(MutableStateParameter.MutableStateParameterInCompose)
+        }
+    }
+
+    @Test
+    fun `no errors when a Composable has valid parameters`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(a: String, b: (Int) -> Unit) {}
+                @Composable
+                fun Something(a: State<String>) {}
+            """.trimIndent()
+        val errors = rule.lint(code)
+        assertThat(errors).isEmpty()
+    }
+}
diff --git a/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/ComposeRuleSetProvider.kt b/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/ComposeRuleSetProvider.kt
@@ -25,6 +25,7 @@ class ComposeRuleSetProvider : RuleSetProviderV3(
         RuleProvider { ModifierWithoutDefaultCheck() },
         RuleProvider { MultipleContentEmittersCheck() },
         RuleProvider { MutableParametersCheck() },
+        RuleProvider { MutableStateParameterCheck() },
         RuleProvider { NamingCheck() },
         RuleProvider { ParameterOrderCheck() },
         RuleProvider { PreviewAnnotationNamingCheck() },

diff --git a/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/MutableStateParameterCheck.kt b/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/MutableStateParameterCheck.kt
@@ -0,0 +1,11 @@
+// Copyright 2023 Nacho Lopez
+// SPDX-License-Identifier: Apache-2.0
+package io.nlopez.compose.rules.ktlint
+
+import io.nlopez.compose.rules.MutableStateParameter
+import io.nlopez.rules.core.ComposeKtVisitor
+import io.nlopez.rules.core.ktlint.KtlintRule
+
+class MutableStateParameterCheck :
+    KtlintRule("compose:mutable-state-param-check"),
+    ComposeKtVisitor by MutableStateParameter()
diff --git a/rules/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/MutableParametersCheckTest.kt b/rules/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/MutableParametersCheckTest.kt
@@ -17,8 +17,6 @@ class MutableParametersCheckTest {
         @Language("kotlin")
         val code =
             """
-                @Composable
-                fun Something(a: MutableState<String>) {}
                 @Composable
                 fun Something(a: ArrayList<String>) {}
                 @Composable
@@ -42,11 +40,6 @@ class MutableParametersCheckTest {
                 col = 15,
                 detail = MutableParameters.MutableParameterInCompose,
             ),
-            LintViolation(
-                line = 8,
-                col = 15,
-                detail = MutableParameters.MutableParameterInCompose,
-            ),
         )
     }
 

diff --git a/...s/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/MutableStateParameterCheckTest.kt b/...s/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/MutableStateParameterCheckTest.kt
@@ -0,0 +1,44 @@
+// Copyright 2023 Nacho Lopez
+// SPDX-License-Identifier: Apache-2.0
+package io.nlopez.compose.rules.ktlint
+
+import com.pinterest.ktlint.test.KtLintAssertThat.Companion.assertThatRule
+import com.pinterest.ktlint.test.LintViolation
+import io.nlopez.compose.rules.MutableStateParameter
+import org.intellij.lang.annotations.Language
+import org.junit.jupiter.api.Test
+
+class MutableStateParameterCheckTest {
+
+    private val mutableParamRuleAssertThat = assertThatRule { MutableStateParameterCheck() }
+
+    @Test
+    fun `errors when a Composable has a mutable parameter`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(a: MutableState<String>) {}
+            """.trimIndent()
+        mutableParamRuleAssertThat(code).hasLintViolationsWithoutAutoCorrect(
+            LintViolation(
+                line = 2,
+                col = 15,
+                detail = MutableStateParameter.MutableStateParameterInCompose,
+            ),
+        )
+    }
+
+    @Test
+    fun `no errors when a Composable has valid parameters`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(a: String, b: (Int) -> Unit) {}
+                @Composable
+                fun Something(a: State<String>) {}
+            """.trimIndent()
+        mutableParamRuleAssertThat(code).hasNoLintViolations()
+    }
+}