Fix false positives for ComposeContentEmitterReturningValues (#420)

Resolves #419
slackhq · Oct 2, 2024 · a08088b · a08088b
1 parent 21e2260
commit a08088b
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,10 @@ Changelog
 **Unreleased**
 --------------
 
+- **Fix**: Fix false positives reported by `ComposeContentEmitterReturningValues`.
+- **Fix**: Fix `content-emitters` configuration in docs.
+- **Fix**: Fix link to multipreview annotations in docs.
+
 1.4.0
 -----
 

diff --git a/...ose-lint-checks/src/main/java/slack/lint/compose/ContentEmitterReturningValuesDetector.kt b/...ose-lint-checks/src/main/java/slack/lint/compose/ContentEmitterReturningValuesDetector.kt
@@ -102,7 +102,7 @@ constructor(
             .findChildrenByClass<KtFunction>()
             .filter {
               val method = it.toUElementOfType<UMethod>() ?: return@filter false
-              method.isComposable && !method.returnsUnitOrVoid(context.evaluator)
+              method.isComposable
             }
             // We don't want to analyze composables that are extension functions, as they might be
             // things like
@@ -120,21 +120,24 @@ constructor(
 
         // We can start showing errors, for composables that emit at all (from the list of
         // known composables)
-        val directEmissionsReported = composableToEmissionCount.keys
+        val directEmissionsReported = composableToEmissionCount.filterValues { it > 0 }.keys
         for (composable in directEmissionsReported) {
-          context.report(
-            ISSUE,
-            composable,
-            context.getNameLocation(composable),
-            ISSUE.getExplanation(TextFormat.TEXT),
-          )
+          if (
+            composable.toUElementOfType<UMethod>()?.returnsUnitOrVoid(context.evaluator) != true
+          ) {
+            context.report(
+              ISSUE,
+              composable,
+              context.getNameLocation(composable),
+              ISSUE.getExplanation(TextFormat.TEXT),
+            )
+          }
         }
 
         // Now we can give some extra passes through the list of composables, and try to get a more
         // accurate count.
         // We want to make sure that if these composables are using other composables in this file
-        // that emit UI,
-        // those are taken into account too. For example:
+        // that emit UI, those are taken into account too. For example:
         // @Composable fun Comp1() { Text("Hi") }
         // @Composable fun Comp2() { Text("Hola") }
         // @Composable fun Comp3() { Comp1() Comp2() } // This wouldn't be picked up at first, but
@@ -156,16 +159,20 @@ constructor(
         // Here we have the settled data after all the needed passes, so we want to show errors for
         // them, if they were not caught already by the 1st emission loop
         currentMapping
-          .filterValues { it > 1 }
+          .filterValues { it > 0 }
           .filterNot { directEmissionsReported.contains(it.key) }
           .keys
           .forEach { composable ->
-            context.report(
-              ISSUE,
-              composable,
-              context.getNameLocation(composable),
-              ISSUE.getExplanation(TextFormat.TEXT),
-            )
+            if (
+              composable.toUElementOfType<UMethod>()?.returnsUnitOrVoid(context.evaluator) != true
+            ) {
+              context.report(
+                ISSUE,
+                composable,
+                context.getNameLocation(composable),
+                ISSUE.getExplanation(TextFormat.TEXT),
+              )
+            }
           }
       }
     }

diff --git a/...lint-checks/src/test/java/slack/lint/compose/ContentEmitterReturningValuesDetectorTest.kt b/...lint-checks/src/test/java/slack/lint/compose/ContentEmitterReturningValuesDetectorTest.kt
@@ -176,4 +176,23 @@ class ContentEmitterReturningValuesDetectorTest : BaseComposeLintTest() {
         .trimIndent()
     lint().files(*commonStubs, kotlin(code)).run().expectClean()
   }
+
+  // /~https://github.com/slackhq/compose-lints/issues/419
+  @Test
+  fun `ensure happy path returning is valid`() {
+    @Language("kotlin")
+    val code =
+      """
+        import androidx.compose.runtime.Composable
+
+        @Composable
+        fun rememberInsetsController(): WindowInsetsControllerCompat? {
+            val view = LocalView.current
+            val window = remember { view.context.findActivity()?.window } ?: return null
+            return remember { WindowCompat.getInsetsController(window, view) }
+        }
+      """
+        .trimIndent()
+    lint().files(*commonStubs, kotlin(code)).run().expectClean()
+  }
 }
diff --git a/docs/rules.md b/docs/rules.md
@@ -90,7 +90,7 @@ Related rule: [`ComposeContentEmitterReturningValues`](/~https://github.com/slackh
 
     ```xml
     <issue id="ComposeMultipleContentEmitters">
-       <option name="allowed-composition-locals" value="CustomEmitter,AnotherEmitter" />
+       <option name="content-emitters" value="CustomEmitter,AnotherEmitter" />
     </issue>
     ```
 
@@ -144,7 +144,7 @@ Related rule: [`ComposeMultipleContentEmitters`](/~https://github.com/slackhq/comp
 
     ```xml
     <issue id="ComposeMultipleContentEmitters">
-       <option name="allowed-composition-locals" value="CustomEmitter,AnotherEmitter" />
+       <option name="content-emitters" value="CustomEmitter,AnotherEmitter" />
     </issue>
     ```