Ignore composables that emit content in their own window. (#110)

* Ignore composables that emit content in their own window. Fixes #109

* Lower case the test names

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

* Refactor the sequence function to be tailrec.

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

Author: PaulWoitaschek

core-common/src/main/kotlin/com/twitter/rules/core/util/Composables.kt

@@ -3,6 +3,7 @@
 package com.twitter.rules.core.util
 
 import com.twitter.rules.core.ComposeKtConfig.Companion.config
+import org.jetbrains.kotlin.com.intellij.psi.PsiElement
 import org.jetbrains.kotlin.psi.KtCallExpression
 import org.jetbrains.kotlin.psi.KtCallableDeclaration
 import org.jetbrains.kotlin.psi.KtFunction
@@ -11,7 +12,36 @@ import org.jetbrains.kotlin.psi.KtProperty
 import org.jetbrains.kotlin.psi.psiUtil.referenceExpression
 
 val KtFunction.emitsContent: Boolean
-    get() = if (isComposable) findChildrenByClass<KtCallExpression>().any { it.emitsContent } else false
+    get() {
+        return if (isComposable) {
+            sequence {
+                tailrec suspend fun SequenceScope<KtCallExpression>.scan(elements: List<PsiElement>) {
+                    if (elements.isEmpty()) return
+                    val toProcess = elements
+                        .mapNotNull { current ->
+                            if (current is KtCallExpression) {
+                                if (current.emitExplicitlyNoContent) {
+                                    null
+                                } else {
+                                    yield(current)
+                                    current
+                                }
+                            } else {
+                                current
+                            }
+                        }
+                        .flatMap { it.children.toList() }
+                    return scan(toProcess)
+                }
+                scan(listOf(this@emitsContent))
+            }.any { it.emitsContent }
+        } else {
+            false
+        }
+    }
+
+private val KtCallExpression.emitExplicitlyNoContent: Boolean
+    get() = calleeExpression?.text in ComposableNonEmittersList
 
 val KtCallExpression.emitsContent: Boolean
     get() {
@@ -28,6 +58,15 @@ private val KtCallExpression.containsComposablesWithModifiers: Boolean
         .filter { it.isNamed() }
         .any { it.getArgumentName()?.text == "modifier" }
 
+/**
+ * This is a denylist with common composables that emit content in their own window. Feel free to add more elements
+ * if you stumble upon them in code reviews that should have triggered an error from this rule.
+ */
+private val ComposableNonEmittersList = setOf(
+    "AlertDialog",
+    "ModalBottomSheetLayout"
+)
+
 /**
  * This is an allowlist with common composables that emit content. Feel free to add more elements if you stumble
  * upon them in code reviews that should have triggered an error from this rule.

core-common/src/main/kotlin/com/twitter/rules/core/util/PsiElements.kt

@@ -5,28 +5,27 @@ package com.twitter.rules.core.util
 import org.jetbrains.kotlin.com.intellij.psi.PsiElement
 import org.jetbrains.kotlin.com.intellij.psi.PsiNameIdentifierOwner
 import org.jetbrains.kotlin.psi.psiUtil.startOffset
-import java.util.*
+import java.util.Deque
+import java.util.LinkedList
 
 inline fun <reified T : PsiElement> PsiElement.findChildrenByClass(): Sequence<T> =
     sequence {
-        val klass = T::class
         val queue: Deque<PsiElement> = LinkedList()
         queue.add(this@findChildrenByClass)
         while (queue.isNotEmpty()) {
             val current = queue.pop()
-            if (klass.isInstance(current)) {
-                yield(current as T)
+            if (current is T) {
+                yield(current)
             }
             queue.addAll(current.children)
         }
     }
 
 inline fun <reified T : PsiElement> PsiElement.findDirectFirstChildByClass(): T? {
-    val klass = T::class
     var current = firstChild
     while (current != null) {
-        if (klass.isInstance(current)) {
-            return current as T
+        if (current is T) {
+            return current
         }
         current = current.nextSibling
     }
@@ -35,11 +34,10 @@ inline fun <reified T : PsiElement> PsiElement.findDirectFirstChildByClass(): T?
 
 inline fun <reified T : PsiElement> PsiElement.findDirectChildrenByClass(): Sequence<T> =
     sequence {
-        val klass = T::class
         var current = firstChild
         while (current != null) {
-            if (klass.isInstance(current)) {
-                yield(current as T)
+            if (current is T) {
+                yield(current)
             }
             current = current.nextSibling
         }

rules/detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposeModifierMissingCheckTest.kt

@@ -200,4 +200,45 @@ class ComposeModifierMissingCheckTest {
         val errors = rule.lint(code)
         assertThat(errors).isEmpty()
     }
+
+    @Test
+    fun `non content emitting root composables are ignored`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun MyDialog() {
+                  AlertDialog(
+                    onDismissRequest = { /*TODO*/ },
+                    buttons = { Text(text = "Button") },
+                    text = { Text(text = "Body") },
+                  )
+                }
+            """.trimIndent()
+
+        val errors = rule.lint(code)
+        assertThat(errors).isEmpty()
+    }
+
+    @Test
+    fun `non content emitter with content emitter not ignored`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun MyDialog() {
+                  Text(text = "Unicorn")
+
+                  AlertDialog(
+                    onDismissRequest = { /*TODO*/ },
+                    buttons = { Text(text = "Button") },
+                    text = { Text(text = "Body") },
+                  )
+                }
+            """.trimIndent()
+
+        val errors = rule.lint(code)
+        assertThat(errors).hasTextLocations("MyDialog")
+        assertThat(errors[0]).hasMessage(ComposeModifierMissing.MissingModifierContentComposable)
+    }
 }

rules/ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposeModifierMissingCheckTest.kt

@@ -214,4 +214,48 @@ class ComposeModifierMissingCheckTest {
 
         modifierRuleAssertThat(code).hasNoLintViolations()
     }
+
+    @Test
+    fun `non content emitting root composables are ignored`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun MyDialog() {
+                  AlertDialog(
+                    onDismissRequest = { /*TODO*/ },
+                    buttons = { Text(text = "Button") },
+                    text = { Text(text = "Body") },
+                  )
+                }
+            """.trimIndent()
+
+        modifierRuleAssertThat(code).hasNoLintViolations()
+    }
+
+    @Test
+    fun `non content emitter with content emitter not ignored`() {
+        @Language("kotlin")
+        val code =
+            """
+                @Composable
+                fun MyDialog() {
+                  Text(text = "Unicorn")
+
+                  AlertDialog(
+                    onDismissRequest = { /*TODO*/ },
+                    buttons = { Text(text = "Button") },
+                    text = { Text(text = "Body") },
+                  )
+                }
+            """.trimIndent()
+
+        modifierRuleAssertThat(code).hasLintViolationsWithoutAutoCorrect(
+            LintViolation(
+                line = 2,
+                col = 5,
+                detail = ComposeModifierMissing.MissingModifierContentComposable
+            )
+        )
+    }
 }