appKODE / detekt-rules-compose

A collection of Detekt rules for Jetpack Compose
MIT License
136 stars 8 forks source link

Extend `UnnecessaryEventHandlerParameter` for constants #3

Open BraisGabin opened 2 years ago

BraisGabin commented 2 years ago

I think that code like this should be also flagged:

@Composable
private fun Foo(
    onSomething: (Int) -> Unit,
) {
    Button(onClick = { onSomething(5) }) { /*...*/ }
}

When 5 could be any kind of constant value/object/data class with contant values on it... Probably it's impossible to handle ALL the cases but probably the more easy ones.

Context

In my project we use MVI so we start with a lambda like this: onUserIntent: (SealedClassIntent) -> Unit. And we propagate that or something similar. I'm not 100% sure that I like that but it's kind of OK. But what I think it's not Ok is when it ends up to a Component like this:

@Composable
private fun Foo(
    onUserIntent: (SealedClassIntent) -> Unit,
) {
    Button(onClick = { onUserIntent(SealedClassIntent.Back) }) { /*...*/ }
}

I think it should be something like this:

@Composable
private fun Foo(
    onBackClick: () -> Unit,
) {
    Button(onClick = onBackClick) { /*...*/ }
}

And it's the caller job to decide which SealedClassIntent should this Composable emit.

dimsuz commented 2 years ago

I agree!

It would be easy to implement for primitive types, but it seems that it would require the type resolution enabled for your example with a sealed class? Otherwise it seems a quite difficult task to find out that SealedClassIntent.Back is actualy an object.

osipxd commented 2 years ago

Maybe it is possible to ignore the rule for lambda if its parameter is not derived from composable arguments? For example:

// GOOD. Lambda parameter created inside the composable
@Composable
private fun Foo(
    onSomething: (Any) -> Unit,
) {
    Button(onClick = { onSomething(5) }) { /*...*/ }
}

// BAD. Lambda parameter derived from composable arguments
@Composable
private fun Foo(
    number: Int,
    onSomething: (Int) -> Unit,
) {
    Button(onClick = { onSomething(number) }) { /*...*/ }
}
dimsuz commented 2 years ago

Hmm. But this rule tries to nudge you to change the first case into letting the parent supply the constant while simplifing the implementation of Foo:

fun main() {
  Foo(onSomething = { doStuff(5) })
}

@Composable
private fun Foo(onSomething: () -> Unit) {
  Button(onClick = onSomething)
}

Wouldn't this work for you?

osipxd commented 2 years ago

Ah. I misread the issue. Sorry :)