Rule suggestion: In Compose, forbid var with mutableStateOf (whether wrapped in remember or not)

[rocketraman]

Expected Behavior of the rule

In Compose, MutableState can be accessed in a variety of ways, see https://developer.android.com/jetpack/compose/state#state-in-composables.

One of these ways is to declare the MutableState as a var and then access the state value directly via property delegation:

var foo by mutableStateOf(something)

However, this introduces the use of vars into Composables, which can easily be avoided by deconstructing the getter and setter properties and declaring them as val:

val (foo, setFoo) = mutableStateOf(something)

Examples that would trigger the rule:

var foo by mutableStateOf(something)
var foo by remember { mutableStateOf(something) }

Context

I believe this results in cleaner code and better compile-time safety through the use of vals rather than vars.

[BraisGabin]

We have a similar rule DoubleMutabilityForCollection. It doesn't do what you are asking for but I think that maybe we could generalize that rule so it doesn't do that only with mutable collections but allow to define which types are consider "mutable" so they can't be stored in a var. @cortinico you implemented that rule, what do you think about this?

[cortinico]

@cortinico you implemented that rule, what do you think about this?

That's a nice addition but really Compose specific (as the mutableStateOf implies another layer of mutability). If we ever decide to have an android ruleset, this rule should live there 👍

[BraisGabin]

Could we make the rule no Android dependant? I mean, allow to setup a list of mutable classes and forbid to store those in a var

[rocketraman]

Could we make the rule no Android dependant? I mean, allow to setup a list of mutable classes and forbid to store those in a var

In general, this makes a lot of sense to me. However, one complexity with Compose specifically is that often state is wrapped in a remember call, and I think the rule would need to "know" that. Unless we can somehow configure that too.

If we ever decide to have an android ruleset, this rule should live there

Just one correction here -- Compose, thanks to JB, is now on Desktop and Web as well :-) However, your wider point is taken.

[cortinico]

one complexity with Compose specifically is that often state is wrapped in a remember call, and I think the rule would need to "know" that

That is not a problem if the rule is extended to allow a list of mutable types. remember resolves to the return type of the inner lambda.

I'm more concerned that this will potentially overcomplicate the rule. Happy to see a PR if someone wants to jump on it though.

[rocketraman]

The other option is to create a separate rule "DoubleMutabilityForState", which would work very similarly to "DoubleMutabilityForCollection", except that it would contain a list of known state types that introduce double mutability.

It doesn't have to be configurable, though there are other rules that are configurable in similar ways e.g. "WildcardImport" so I don't personally see that as a big deal in terms of complexity.

[cortinico]

Yup you're right. I recalled the rule being quite complicated. However it's fairly simple. I don't think we need a new rule, we can just make this list configurable. Do you want to try to submit a PR for it?

detekt/detekt-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/DoubleMutabilityForCollection.kt

Lines 69 to 78 in 58e95e6

	val mutableTypes = setOf(
	FqName("kotlin.collections.MutableList"),
	FqName("kotlin.collections.MutableMap"),
	FqName("kotlin.collections.MutableSet"),
	FqName("java.util.ArrayList"),
	FqName("java.util.LinkedHashSet"),
	FqName("java.util.HashSet"),
	FqName("java.util.LinkedHashMap"),
	FqName("java.util.HashMap"),
	)

[rocketraman]

The main reason I'd lean towards creating a new rule is that the name DoubleMutabilityForCollection is a bit misleading, as a state object isn't a collection per say.

Plus, a user may very well want to enable or disable these in isolation i.e. keep DoubleMutabilityForCollection enabled while disabling DoubleMutabilityForState or vice-versa.

[cortinico]

The main reason I'd lean towards creating a new rule is that the name DoubleMutabilityForCollection is a bit misleading, as a state object isn't a collection per say.

I think this can be easily handled using the alias feature.
Specifically, DoubleMutabilityForState would not be a good name, if that rule is configurable. You're assuming that the user will provide a state* function in the rule name while it can be used for any "mutable" type.
Potentially aliasing the rule to DoubleMutability could be fine as well.

Plus, a user may very well want to enable or disable these in isolation i.e. keep DoubleMutabilityForCollection enabled while disabling DoubleMutabilityForState or vice-versa.

That can be achieved by customizing the list of mutable types.

[rocketraman]

PR submitted.

[mhernand40]

Looking at the following code sample from https://developer.android.com/jetpack/compose/state#viewmodels-source-of-truth

data class ExampleUiState(
    dataToDisplayOnScreen: List<Example> = emptyList(),
    userMessages: List<Message> = emptyList(),
    loading: Boolean = false
)

class ExampleViewModel(
    private val repository: MyRepository,
    private val savedState: SavedStateHandle
) : ViewModel() {

    var uiState by mutableStateOf<ExampleUiState>(...)
        private set

    // Business logic
    fun somethingRelatedToBusinessLogic() { ... }
}

@Composable
fun ExampleScreen(viewModel: ExampleViewModel = viewModel()) {

    val uiState = viewModel.uiState
    ...

    Button(onClick = { viewModel.somethingRelatedToBusinessLogic() }) {
        Text("Do something")
    }
}

Would this rule discourage the use of this var uiState + private set?

[cortinico]

Would this rule discourage the use of this var uiState + private set?

Currently not. The rule would not even flag this specific case as it's just checking for assignments val uiState = mutableStateOf(...) and not property delevates (i.e. usage of by).

[drinkthestars]

Is this a proposal to encourage destructuring declaration over the other methods of declaring mutableStateOfs?

If so, we might want to pause on that since the destructuring declaration is being considered as a foot-gun by some in the community + compose team, especially when used inside Composables with remember { }.

References:

• original reddit thread
• twitter thread
• issuetracker #212452424

The issuetracker issue's latest comment from the team wonders if the destructuring syntax implementation is currently a bug or not.

[rocketraman]

Currently not. The rule would not even flag this specific case as it's just checking for assignments val uiState = mutableStateOf(...) and not property delevates (i.e. usage of by).

Hmm, I actually just realized that is true.

Is this a proposal to encourage destructuring declaration over the other methods of declaring mutableStateOfs?

My original intent was indeed to encourage either Option 1 or Option 3 below, over Option 2 which uses a var:

// Option 1: ok
val mutableState = remember { mutableStateOf(default) }

// Option 2: discourage
var value by remember { mutableStateOf(default) }

// Option 3: ok
val (value, setValue) = remember { mutableStateOf(default) }

My initial thinking when I saw Option 2 was "yuck" because vars in Kotlin are generally not idiomatic, and are usually used in tight scopes if at all. That being said, given that the var here is a delegate, I suppose it is actually "safe" in the sense that it is impossible to mistakenly break the state by assigning something else to it -- the delegate ensures that the underlying state is modified as should always be the case.

Hopefully the Compose team can get this sorted out and provide better recommendations / documentation / api improvements. The last comment in the issue tracker that the destructuring declaration should probably return a getter, not the current value, is not a knock on the destructuring approach per say, but rather than component1() value returned from it is a value rather than a function.

What this discussion tells me is that any Detekt rules that target Compose state usage should wait until the preferred idioms are sorted out.

All that being said, the PR being implemented here for now cares nothing for any of this because all it does is make the DoubleMutabilityForCollection configurable, and doesn't address the Compose use case directly at all. However, I personally don't need it any more given the discussion above, so up to the team whether the PR I submitted still makes sense or not. I'm happy to bring it to completion.

[mhernand40]

I think a rule that prevents the following makes total sense:

var value1 = mutableStateOf(default)

// as well as

var value2 = remember { mutableStateOf(default) }

The reason being because the above assign a mutable type to a mutable variable/field, hence the double mutability.

However discouraging the use of vars by delegation, e.g.,

var value1 by mutableStateOf(default)

// or

var value2 by remember { mutableStateOf(default) }

seems like we are fighting against a valuable language feature. Not only Compose, but Kotlin also has its own built-in delegates, such as Delegates.observable<T>(), Delegates.notNull<T>(), and Delegates.vetoable<T>(). All IMO are valid use cases for var.

[drinkthestars]

What this discussion tells me is that any Detekt rules that target Compose state usage should wait until the preferred idioms are sorted out.

Agreed. Hopefully updates to that issue will add clarity.

I think a rule that prevents the following makes total sense:

Agreed. Additionally, this one is also problematic (explained here):

// DON'T
var list by mutableStateOf(mutableListOf("a")) // changing inner mutable list will not update state
// Or, Worse
var list = mutableStateOf(mutableListOf("a"))

// DO
var list by mutableStateOf(listOf("a"))
// OR
val list = mutableStateListOf("a")

Update:
Looks like the mutableState + mutableList already has a custom android lint rule for it that warns in the IDE.

Given that^, to prevent the following, not sure if a custom android lint rule is better suited (at least right now you still get warned if value1 is never written to again):

var value1 = remember { mutableStateOf("default") }

Regardless, if we're adding any new customizability to rules related to Compose, then probably good to also update https://detekt.github.io/detekt/compose.html so that folks can see all Compose-related detekt things in one place.

[cortinico]

What this discussion tells me is that any Detekt rules that target Compose state usage should wait until the preferred idioms are sorted out.

As a rule of thumbs, we're trying to keep the tool as platform/framework-agnostic as possible. We're generally ok making changes to rules to make them more extensible and easier to adapt to work with a specific framework (e.g., make a rule configurable to work with Jetpack Compose).

However, if you need a specific detection logic for a use case like the one you're suggesting @drinkthestars, that would probably better fit a custom rule.

[rocketraman]

If so, we might want to pause on that since the destructuring declaration is being considered as a foot-gun by some in the community + compose team, especially when used inside Composables with remember { }.

@drinkthestars At this point pretty unrelated to this issue, but for interest sake: part of that Twitter thread has comments from Leland Richardson which are relevant. Example:

I think there might be a just-as-compelling argument that the other forms are actually more foot-gun-like than this?

https://twitter.com/Louis_CAD/status/1476180047476232192