Skip to content

Commit efe1882

Browse files
authored
Fix ComposeModifierReused when using nested KtDotQualifiedExpressions (#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.
1 parent 02a2d48 commit efe1882

File tree

3 files changed

+54
-5
lines changed

3 files changed

+54
-5
lines changed

rules/common/src/main/kotlin/com/twitter/compose/rules/ComposeModifierReused.kt

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import org.jetbrains.kotlin.com.intellij.psi.PsiElement
1111
import org.jetbrains.kotlin.psi.KtBlockExpression
1212
import org.jetbrains.kotlin.psi.KtCallExpression
1313
import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
14+
import org.jetbrains.kotlin.psi.KtExpression
1415
import org.jetbrains.kotlin.psi.KtFunction
1516
import org.jetbrains.kotlin.psi.KtProperty
1617
import org.jetbrains.kotlin.psi.KtReferenceExpression
@@ -75,13 +76,24 @@ class ComposeModifierReused : ComposeKtVisitor {
7576
}
7677
// if it's MyComposable(modifier.fillMaxWidth()) or similar
7778
is KtDotQualifiedExpression -> {
78-
modifierNames.contains(expression.receiverExpression.text)
79+
// On cases of multiple nested KtDotQualifiedExpressions (e.g. multiple chained methods)
80+
// we need to iterate until we find the start of the chain
81+
modifierNames.contains(expression.rootExpression.text)
7982
}
8083

8184
else -> false
8285
}
8386
}
8487

88+
private val KtDotQualifiedExpression.rootExpression: KtExpression
89+
get() {
90+
var current: KtExpression = receiverExpression
91+
while (current is KtDotQualifiedExpression) {
92+
current = current.receiverExpression
93+
}
94+
return current
95+
}
96+
8597
private fun KtBlockExpression.obtainAllModifierNames(initialName: String): List<String> {
8698
var lastSize = 0
8799
val tempModifierNames = mutableSetOf(initialName)
@@ -117,8 +129,7 @@ class ComposeModifierReused : ComposeKtVisitor {
117129

118130
companion object {
119131
val ModifierShouldBeUsedOnceOnly = """
120-
Modifiers should only be used once and by the root level layout of a Composable. This is true even if
121-
appended to or with other modifiers e.g. 'modifier.fillMaxWidth()'.
132+
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()'.
122133
123134
Use Modifier (with a capital 'M') to construct a new Modifier that you can pass to other composables.
124135

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,23 @@ class ComposeModifierReusedCheckTest {
4747
SomethingElse(myMod)
4848
}
4949
}
50+
@Composable
51+
fun FoundThisOneInTheWild(modifier: Modifier = Modifier) {
52+
Box(
53+
modifier = modifier
54+
.size(AvatarSize.Default.size)
55+
.clip(CircleShape)
56+
.then(colorModifier)
57+
) {
58+
Box(
59+
modifier = modifier.padding(spacesBorderWidth)
60+
)
61+
}
62+
}
5063
""".trimIndent()
5164

5265
val errors = rule.lint(code)
53-
assertThat(errors).hasSize(9)
66+
assertThat(errors)
5467
.hasSourceLocations(
5568
SourceLocation(3, 5),
5669
SourceLocation(4, 9),
@@ -60,7 +73,9 @@ class ComposeModifierReusedCheckTest {
6073
SourceLocation(19, 5),
6174
SourceLocation(20, 5),
6275
SourceLocation(25, 9),
63-
SourceLocation(26, 9)
76+
SourceLocation(26, 9),
77+
SourceLocation(31, 5),
78+
SourceLocation(37, 9)
6479
)
6580
for (error in errors) {
6681
assertThat(error).hasMessage(ComposeModifierReused.ModifierShouldBeUsedOnceOnly)

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,19 @@ class ComposeModifierReusedCheckTest {
4545
SomethingElse(myMod)
4646
}
4747
}
48+
@Composable
49+
fun FoundThisOneInTheWild(modifier: Modifier = Modifier) {
50+
Box(
51+
modifier = modifier
52+
.size(AvatarSize.Default.size)
53+
.clip(CircleShape)
54+
.then(colorModifier)
55+
) {
56+
Box(
57+
modifier = modifier.padding(spacesBorderWidth)
58+
)
59+
}
60+
}
4861
""".trimIndent()
4962

5063
modifierRuleAssertThat(code).hasLintViolationsWithoutAutoCorrect(
@@ -92,6 +105,16 @@ class ComposeModifierReusedCheckTest {
92105
line = 26,
93106
col = 9,
94107
detail = ComposeModifierReused.ModifierShouldBeUsedOnceOnly
108+
),
109+
LintViolation(
110+
line = 31,
111+
col = 5,
112+
detail = ComposeModifierReused.ModifierShouldBeUsedOnceOnly
113+
),
114+
LintViolation(
115+
line = 37,
116+
col = 9,
117+
detail = ComposeModifierReused.ModifierShouldBeUsedOnceOnly
95118
)
96119
)
97120
}

0 commit comments

Comments
 (0)