Compose ReusedModifierInstance false positive

[raul19]

Expected Behavior

I have a @Composable that receives a modifier, and the first child is ProvideWindowInsets that has no modifier parameter, and I put the received modifier on the first child of ProvideWindowInsets, then Detekt says that the received modifier instance should be passed only to de first child.

Here is an example:

@Composable
fun MyComposable(
    ....
    modifier: Modifier = Modifier,
    .....
    ) {
          ProvideWindowInsets {
               Surface(
                      modifier = modifier,
                      .....
               ) {
                    .....
               }
          }
    }

Observed Behavior

Detekt says that the received modifier instance should be passed only to de first child.

Steps to Reproduce

Create a composable that receive a modifier and its first child has no modifier parameter (ProvideWindowInsets) so pass the modifier to the first child of ProvideWindowInsets.

Context

Your Environment

• Version of detekt used: 1.20.0
• Operating System and version: Mac OS 12.4

[dimsuz]

Thanks for the report!

Unfortunately without Detekt's "type resolution" feature enabled, there's no way for the detekt rule to know which parameters does a function call have - if they are not used. And "type resolution" is currently an experimental feature (read about it here).

I will try to play with this and hope to fix eventually. For now I'd suggest to put @Suppress("ReusedModifierInstance") on this particular @Composable.

[BraisGabin]

I just published this on the detekt repo: https://github.com/detekt/detekt/discussions/4959 I hope we can remove the "experimental" tag from type resolution. It should be completely safe to use type resolution.

But, even with type resolution, which is your idea here? look for the first node that allows a Modifier and check that is that the only one receiving the modifier value? I think that should do the trick :)

[dimsuz]

Yes, exactly!

But after reading your comment I'm thinking: is it really required to check if outer nodes have the modifier parameter? Maybe it's simply enough to check that modifier is used exactly once as an argument, no matter on what level and with whatever outer composables.

This would work without type resolution even.

Ah, but this will probably still produce some undesired behavior, because then the following wouldn't produce a rule violation

fun MyComosable(modifier: Modifier) {
  Column {
    Row {
      Button(modifier = modifier)
    }
  }
}

and this would probably be not what a "good" composable should do.

But if it would be possible to find a "detektLite" variant of this rule, it would be nice, because a lot of users are using it (although I guess this will gradually change towards "detektFull") and this rule is one of those which can really help catch some bugs.

[BraisGabin]

But if it would be possible to find a "detektLite" variant of this rule, it would be nice, because a lot of users are using it (although I guess this will gradually change towards "detektFull") and this rule is one of those which can really help catch some bugs.

If we find a way to do it completely with "detektLite" would be nice. But I don't recommend to give this rule a mixed behaviour: https://github.com/detekt/detekt/issues/2994

The problem is, how do we know if we should skip a node? Creating a list of nodes to skip? That could be hard to maintain.