Fix LambdaParameterInRestartableEffect conflict with onDispose lambda (#254)

Composables which accept a parameter named `onDispose` will always raise
a warning for its use in `DisposableEffect`, from the call to
`DisposableEffectScope.onDispose`. This is a rare edge case but doesn't
allow any workarounds other than renaming the lambda or suppressing.

To fix, I've implemented simple plain text matching to ignore any usages
of `onDispose` in a `DisposableEffect`. This may not be perfect (as the
TODO in test cases shows) but in the absence of type resolution it might
be a reasonable workaround. Very open to more elegant solutions!

Co-authored-by: Nacho Lopez <[email protected]>

Author: dzirbel

rules/common/src/main/kotlin/io/nlopez/compose/rules/LambdaParameterInRestartableEffect.kt

@@ -39,15 +39,26 @@ class LambdaParameterInRestartableEffect : ComposeKtVisitor {
 
             if (lambdaParameterNames.isEmpty()) continue
 
-            // Then, we just want the lambda parameters that are actually used inside in any of the found effects'
+            // Then, we just want the lambda parameters that are actually used inside any of the found effects'
             // trailing lambda code
-            val usedLambdaParameterNames = effects.mapNotNull { it.lambdaArguments.lastOrNull() }
-                .mapNotNull { it.getLambdaExpression()?.bodyExpression }
-                .flatMap { body ->
+            val usedLambdaParameterNames = effects
+                .flatMap { effect ->
+                    val body = effect.lambdaArguments.lastOrNull()?.getLambdaExpression()?.bodyExpression
+                        ?: return@flatMap emptySequence()
+
                     val callExpressions = body.findChildrenByClass<KtCallExpression>()
+                    val isDisposableEffect = effect.calleeExpression?.text == "DisposableEffect"
 
                     // Lambdas used directly: myLambda()
-                    val invoked = callExpressions.mapNotNull { it.calleeExpression?.text }
+                    val invoked = callExpressions
+                        .let { expressions ->
+                            if (isDisposableEffect) {
+                                expressions.filter { it.calleeExpression?.text != "onDispose" }
+                            } else {
+                                expressions
+                            }
+                        }
+                        .mapNotNull { it.calleeExpression?.text }
                         .filter { it in lambdaParameterNames }
 
                     // Lambdas being tossed around to other methods

rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterInRestartableEffectCheckTest.kt

@@ -78,4 +78,65 @@ class LambdaParameterInRestartableEffectCheckTest {
         val errors = rule.lint(code)
         assertThat(errors).isEmpty()
     }
+
+    @Test
+    fun `error out when detecting a lambda named onDispose used in a non-DisposableEffect`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    LaunchedEffect(Unit) {
+                        onDispose()
+                    }
+                }
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    DisposableEffect(Unit) {
+                        onDispose(onDispose)
+                    }
+                }
+
+                // TODO ideally these would also be caught, but may require type resolution
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    DisposableEffect(Unit) {
+                        onDispose { onDispose() }
+                    }
+                }
+                @Composable
+                fun Something(onDispose: (Int) -> Unit) {
+                    DisposableEffect(Unit) {
+                        onDispose(0)
+                        onDispose {}
+                    }
+                }
+            """.trimIndent()
+        val errors = rule.lint(code)
+        assertThat(errors)
+            .hasStartSourceLocations(
+                SourceLocation(2, 15),
+                SourceLocation(8, 15),
+            )
+        for (error in errors) {
+            assertThat(error).hasMessage(LambdaParameterInRestartableEffect.LambdaUsedInRestartableEffect)
+        }
+    }
+
+    @Test
+    fun `passes when a lambda named onDispose is present but unused in DisposableEffect`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    val latestOnDispose by rememberUpdatedState(onDispose)
+                    DisposableEffect(Unit) {
+                        onDispose(latestOnDispose)
+                    }
+                }
+            """.trimIndent()
+        val errors = rule.lint(code)
+        assertThat(errors).isEmpty()
+    }
 }

rules/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/LambdaParameterInRestartableEffectCheckTest.kt

@@ -86,4 +86,68 @@ class LambdaParameterInRestartableEffectCheckTest {
             )
             .hasNoLintViolations()
     }
+
+    @Test
+    fun `error out when detecting a lambda named onDispose used in a non-DisposableEffect`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    LaunchedEffect(Unit) {
+                        onDispose()
+                    }
+                }
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    DisposableEffect(Unit) {
+                        onDispose(onDispose)
+                    }
+                }
+
+                // TODO ideally these would also be caught, but may require type resolution
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    DisposableEffect(Unit) {
+                        onDispose { onDispose() }
+                    }
+                }
+                @Composable
+                fun Something(onDispose: (Int) -> Unit) {
+                    DisposableEffect(Unit) {
+                        onDispose(0)
+                        onDispose {}
+                    }
+                }
+            """.trimIndent()
+        ruleAssertThat(code)
+            .hasLintViolationsWithoutAutoCorrect(
+                LintViolation(
+                    line = 2,
+                    col = 15,
+                    detail = LambdaParameterInRestartableEffect.LambdaUsedInRestartableEffect,
+                ),
+                LintViolation(
+                    line = 8,
+                    col = 15,
+                    detail = LambdaParameterInRestartableEffect.LambdaUsedInRestartableEffect,
+                ),
+            )
+    }
+
+    @Test
+    fun `passes when a lambda named onDispose is present but unused in DisposableEffect`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun Something(onDispose: () -> Unit) {
+                    val latestOnDispose by rememberUpdatedState(onDispose)
+                    DisposableEffect(Unit) {
+                        onDispose(latestOnDispose)
+                    }
+                }
+            """.trimIndent()
+        ruleAssertThat(code).hasNoLintViolations()
+    }
 }