We have released a set of Detekt rules which help ...

# compose

[dimsuz]

We have released a set of Detekt rules which help us find common errors while working on Compose code. For now there are few rules there, which seem more or less generally applicable (of course, they can be turned on/off individually if they don't work for you):

https://github.com/appKODE/detekt-rules-compose

[Filip Wiesner]

ModifierArgumentPosition and ModifierParameterPosition ensure that modifier is declared and passed as a first parameter/argument

Why would you want Modifier being passed as a first argument to a Composable? 🤔 I personally pass it always as a last parameter because some modifier chains are long and I want to see the "more important" model parameters first.
Nothing against your conventions but I would be interested in the reasoning.

[dimsuz]

This seems to be the convention which Material components also use. I guess this was inspired by that and we had this from the start. I thought about whether to include these rules, but then decided that they can be turned off if not desirable...

Alternatively I guess we can also provide an option to that rule which either forces it to be the first argument or the last one: those seem to be the most commonly used presets.

[Filip Wiesner]

Yeah, I've been looking through the material components and I see it. Weird, I've never noticed it 😅
And i agree that it's better to have it with ability to turn it off rather than not having it at all. I was just surprised that someone would make a lint about something I explicitly try not to do 😄

Anyways, thank you for making the rules public. I will consider using them 👍 Especially the ReusedModifierInstance is nice.

[dimsuz]

Yeah, we had a lot of inconsistencies in larger teams where everyone was placing modifier parameters randomly, so we've decided to stick to something 🙂

Thank you, please report any issues or suggestions!

ReusedModifierInstance is indeed the most useful one of those. Others are more or less minor, but occasionally helpful.

[Adam Powell]

The official convention is that the modifier param is the first optional parameter in the list. The idea is that you never have to type modifier = in the parameter list, it can always be specified positionally if the only other parameters you're using are required

[dimsuz]

Yep, this check only checks the calls/declarations which have it. I.e. if no modifier is mentioned, it won't be enforced at all. This is just to stay consistent when it's used.

[Filip Wiesner]

Yes, that is the composable definition. But is there a convention on position of modifier parameter when using the composable?
I 100% agree with the composable definition parameter order convention

[Adam Powell]

When calling it the convention is to match the parameter order of the function definition, but I'd probably shake my head at anyone trying to formally enforce that 🙂

[Adam Powell]

Doubly so at someone trying to formally enforce something other than the defined order of the function

[Adam Powell]

If you're using an order other than the function definition's, you're also using named parameters at the call site so making further rules around it can't really use misunderstanding for a reader as a reason

[dimsuz]

🙂 Indeed the one which checks an argument order at the call site is probably not as useful (I actually don't remember it ever firing), perhaps it should be removed. I guess it was conceived out of calls for perfect symmetry :)

[Filip Wiesner]

I wouldn't mind having a rule to enforce using named parameters 😄 🙃
But yeah, I asked about it in the office and we didn't agree on anything. Someone wants it as first parameter, it does not matter for others and I like it as a last parameter. So it's probably highly subjective and situational.

[ste]

Personally, I put modifier as the first optional parameter (this can be arguable btw) when it comes to signature, but I always pass it as last (but before any @Composable param) when it comes to function invocation - I completely agree with Filip.

[dimsuz]

Agreed. I will remove the ModifierArgumentPosition then and leave only the one which enforces the parameter: ModifierParameterPosition. It's also arguably subjective, but can be enabled/disabled depending on codebase conventions.

[dimsuz]

Following the comments of Adam and ste, will also update ModifierParameterPosition to check that modifier is the first optional parameter, rather than always-first as it is now.

[Adam Powell]

We considered modifier always first but the counterexample is that having to specify the named parameter in Text(text = "hello") as opposed to Text("hello") is silly

[hfhbd]

Notice to myself: Never use var showButton = remember { mutableStateOf(false) }.value. Although .value is nice for val, because it supports data flow analysis (eg smart casts), with var the state won't update 🤦‍♂️
Adding this rule would be nice too 😄

[dimsuz]

I wonder how widespread this error is?...

by the way, you can always use var showButton by remember { ... } (note the by)

[hfhbd]

Yes, you could use by, beside val (showButton, setValue) =. In my codebase, I found it only once, but it easy to make the same mistake again, I think.