Fix ComposeModifierReused when using nested KtDotQualifiedExpressio…

…ns (#106) `ComposeModifierReused` was ignoring legit violations when reusing modifiers that had deeply nested values, because the `KtDotQualifiedExpression` handling was not traversing the chained methods to get to the KtReferenceExpression leaf.
twitter · Oct 19, 2022 · efe1882 · efe1882
1 parent 02a2d48
commit efe1882
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 5 deletions.
diff --git a/rules/common/src/main/kotlin/com/twitter/compose/rules/ComposeModifierReused.kt b/rules/common/src/main/kotlin/com/twitter/compose/rules/ComposeModifierReused.kt
@@ -11,6 +11,7 @@ import org.jetbrains.kotlin.com.intellij.psi.PsiElement
 import org.jetbrains.kotlin.psi.KtBlockExpression
 import org.jetbrains.kotlin.psi.KtCallExpression
 import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
+import org.jetbrains.kotlin.psi.KtExpression
 import org.jetbrains.kotlin.psi.KtFunction
 import org.jetbrains.kotlin.psi.KtProperty
 import org.jetbrains.kotlin.psi.KtReferenceExpression
@@ -75,13 +76,24 @@ class ComposeModifierReused : ComposeKtVisitor {
                 }
                 // if it's MyComposable(modifier.fillMaxWidth()) or similar
                 is KtDotQualifiedExpression -> {
-                    modifierNames.contains(expression.receiverExpression.text)
+                    // On cases of multiple nested KtDotQualifiedExpressions (e.g. multiple chained methods)
+                    // we need to iterate until we find the start of the chain
+                    modifierNames.contains(expression.rootExpression.text)
                 }
 
                 else -> false
             }
         }
 
+    private val KtDotQualifiedExpression.rootExpression: KtExpression
+        get() {
+            var current: KtExpression = receiverExpression
+            while (current is KtDotQualifiedExpression) {
+                current = current.receiverExpression
+            }
+            return current
+        }
+
     private fun KtBlockExpression.obtainAllModifierNames(initialName: String): List<String> {
         var lastSize = 0
         val tempModifierNames = mutableSetOf(initialName)
@@ -117,8 +129,7 @@ class ComposeModifierReused : ComposeKtVisitor {
 
     companion object {
         val ModifierShouldBeUsedOnceOnly = """
-            Modifiers should only be used once and by the root level layout of a Composable. This is true even if
-            appended to or with other modifiers e.g. 'modifier.fillMaxWidth()'.
+            Modifiers should only be used once and by the root level layout of a Composable. This is true even if appended to or with other modifiers e.g. 'modifier.fillMaxWidth()'.
 
             Use Modifier (with a capital 'M') to construct a new Modifier that you can pass to other composables.
 

diff --git a/...detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposeModifierReusedCheckTest.kt b/...detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposeModifierReusedCheckTest.kt
@@ -47,10 +47,23 @@ class ComposeModifierReusedCheckTest {
                         SomethingElse(myMod)
                     }
                 }
+                @Composable
+                fun FoundThisOneInTheWild(modifier: Modifier = Modifier) {
+                    Box(
+                        modifier = modifier
+                            .size(AvatarSize.Default.size)
+                            .clip(CircleShape)
+                            .then(colorModifier)
+                    ) {
+                        Box(
+                            modifier = modifier.padding(spacesBorderWidth)
+                        )
+                    }
+                }
             """.trimIndent()
 
         val errors = rule.lint(code)
-        assertThat(errors).hasSize(9)
+        assertThat(errors)
             .hasSourceLocations(
                 SourceLocation(3, 5),
                 SourceLocation(4, 9),
@@ -60,7 +73,9 @@ class ComposeModifierReusedCheckTest {
                 SourceLocation(19, 5),
                 SourceLocation(20, 5),
                 SourceLocation(25, 9),
-                SourceLocation(26, 9)
+                SourceLocation(26, 9),
+                SourceLocation(31, 5),
+                SourceLocation(37, 9)
             )
         for (error in errors) {
             assertThat(error).hasMessage(ComposeModifierReused.ModifierShouldBeUsedOnceOnly)

diff --git a/...ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposeModifierReusedCheckTest.kt b/...ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposeModifierReusedCheckTest.kt
@@ -45,6 +45,19 @@ class ComposeModifierReusedCheckTest {
                         SomethingElse(myMod)
                     }
                 }
+                @Composable
+                fun FoundThisOneInTheWild(modifier: Modifier = Modifier) {
+                    Box(
+                        modifier = modifier
+                            .size(AvatarSize.Default.size)
+                            .clip(CircleShape)
+                            .then(colorModifier)
+                    ) {
+                        Box(
+                            modifier = modifier.padding(spacesBorderWidth)
+                        )
+                    }
+                }
             """.trimIndent()
 
         modifierRuleAssertThat(code).hasLintViolationsWithoutAutoCorrect(
@@ -92,6 +105,16 @@ class ComposeModifierReusedCheckTest {
                 line = 26,
                 col = 9,
                 detail = ComposeModifierReused.ModifierShouldBeUsedOnceOnly
+            ),
+            LintViolation(
+                line = 31,
+                col = 5,
+                detail = ComposeModifierReused.ModifierShouldBeUsedOnceOnly
+            ),
+            LintViolation(
+                line = 37,
+                col = 9,
+                detail = ComposeModifierReused.ModifierShouldBeUsedOnceOnly
             )
         )
     }