From b40ccebc53a89c3b3d2f9d9263c5353eaceceeff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mate=C4=8Dn=C3=BD?= Date: Fri, 29 Sep 2023 13:26:06 +0200 Subject: [PATCH 1/2] Add a warning when a button is used inside of AlertDialog's content slot instead of actions slot --- .../compose/lint/OrbitComposeIssueRegistry.kt | 2 + .../DialogButtonsInContentSlotDetector.kt | 112 ++++++++++++++++++ .../DialogButtonsInContentSlotDetectorTest.kt | 101 ++++++++++++++++ 3 files changed, 215 insertions(+) create mode 100644 lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt create mode 100644 lint/src/test/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetectorTest.kt diff --git a/lint/src/main/kotlin/kiwi/orbit/compose/lint/OrbitComposeIssueRegistry.kt b/lint/src/main/kotlin/kiwi/orbit/compose/lint/OrbitComposeIssueRegistry.kt index de206d3ee..2a94465d0 100644 --- a/lint/src/main/kotlin/kiwi/orbit/compose/lint/OrbitComposeIssueRegistry.kt +++ b/lint/src/main/kotlin/kiwi/orbit/compose/lint/OrbitComposeIssueRegistry.kt @@ -3,6 +3,7 @@ package kiwi.orbit.compose.lint import com.android.tools.lint.client.api.IssueRegistry import com.android.tools.lint.client.api.Vendor import com.android.tools.lint.detector.api.CURRENT_API +import kiwi.orbit.compose.lint.detectors.DialogButtonsInContentSlotDetector import kiwi.orbit.compose.lint.detectors.MaterialDesignInsteadOrbitDesignDetector import kiwi.orbit.compose.lint.detectors.MaterialDialogWithOrbitButtonsDetector import kiwi.orbit.compose.lint.detectors.ScaffoldContentPaddingDetector @@ -10,6 +11,7 @@ import kiwi.orbit.compose.lint.detectors.ScaffoldContentPaddingDetector @Suppress("UnstableApiUsage") class OrbitComposeIssueRegistry : IssueRegistry() { override val issues = listOf( + DialogButtonsInContentSlotDetector.ISSUE, MaterialDesignInsteadOrbitDesignDetector.ISSUE, MaterialDialogWithOrbitButtonsDetector.ISSUE, ScaffoldContentPaddingDetector.ISSUE, diff --git a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt new file mode 100644 index 000000000..1cd1af3ab --- /dev/null +++ b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt @@ -0,0 +1,112 @@ +package kiwi.orbit.compose.lint.detectors + +import com.android.tools.lint.detector.api.Category +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.Implementation +import com.android.tools.lint.detector.api.Issue +import com.android.tools.lint.detector.api.JavaContext +import com.android.tools.lint.detector.api.Scope +import com.android.tools.lint.detector.api.Severity +import com.android.tools.lint.detector.api.SourceCodeScanner +import com.android.tools.lint.detector.api.computeKotlinArgumentMapping +import com.intellij.psi.PsiElement +import com.intellij.psi.PsiJavaFile +import com.intellij.psi.PsiMember +import com.intellij.psi.PsiMethod +import org.jetbrains.uast.UBlockExpression +import org.jetbrains.uast.UCallExpression +import org.jetbrains.uast.ULambdaExpression +import org.jetbrains.uast.skipParenthesizedExprDown +import org.jetbrains.uast.toUElement +import org.jetbrains.uast.tryResolveNamed + +class DialogButtonsInContentSlotDetector : Detector(), SourceCodeScanner { + + companion object { + internal val ISSUE: Issue = Issue.create( + id = "DialogButtonsInContentSlotDetector", + briefDescription = "It is a mistake to place CTAs into content slot of AlertDialogs.", + explanation = "Buttons in dialogs should be placed into action slot, where proper sizes and styling is provided.", + category = Category.CUSTOM_LINT_CHECKS, + priority = 7, + severity = Severity.ERROR, + implementation = Implementation( + DialogButtonsInContentSlotDetector::class.java, + Scope.JAVA_FILE_SCOPE, + ), + ) + + private const val ALERT_CONTENT_PARAM = "content" + private const val PACKAGE_ORBIT_ALERT = "kiwi.orbit.compose.ui.controls" + + private const val PACKAGE_ORBIT_BUTTON = "kiwi.orbit.compose.ui.controls" + private const val NAME_ORBIT_BUTTON = "Button" + } + + override fun getApplicableMethodNames(): List = listOf( + "AlertInfo", "AlertSuccess", "AlertWarning", "AlertCritical", + ) + + override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) { + if (method.isInPackageName(PACKAGE_ORBIT_ALERT)) { + val contentArgument = method.getArgument(node, ALERT_CONTENT_PARAM) + val topBlockExpression = contentArgument?.body as? UBlockExpression ?: return + topBlockExpression.sourcePsi?.findAllBlockExpressionsInHierarchy()?.forEach { + it.checkBlockExpression(context) + } + } + } + + private fun UBlockExpression.checkBlockExpression(context: JavaContext) { + expressions.forEach { + val resolvedBodyExpression = it.skipParenthesizedExprDown().tryResolveNamed() ?: return@forEach + val packageName = resolvedBodyExpression.getPackageName() ?: return@forEach + + val expressionName = resolvedBodyExpression.name + val isButton = expressionName?.contains(NAME_ORBIT_BUTTON) == true + val isOrbit = packageName == PACKAGE_ORBIT_BUTTON + + if (isButton && isOrbit) { + context.report( + issue = ISSUE, + scope = it, + location = context.getLocation(it), + message = "$expressionName in Dialog's content slot. Use actions slot for CTAs instead.", + ) + } + } + + } + + private fun PsiElement.findAllBlockExpressionsInHierarchy(): List { + val expressions = mutableListOf() + + val uElement = this.toUElement() + if (uElement is UBlockExpression) expressions.add(uElement) + expressions.addAll(children.flatMap { it.findAllBlockExpressionsInHierarchy() }) + + return expressions + } + + private fun PsiMethod.getArgument( + node: UCallExpression, + argumentName: String, + ): ULambdaExpression? = computeKotlinArgumentMapping(node, this) + .orEmpty() + .filter { (_, parameter) -> + parameter.name == argumentName + } + .keys + .filterIsInstance() + .firstOrNull() + + private fun PsiMethod.isInPackageName(packageName: String): Boolean { + val actual = (containingFile as? PsiJavaFile)?.packageName + return packageName == actual + } + + private fun PsiElement.getPackageName(): String? = when (this) { + is PsiMember -> this.containingClass?.qualifiedName?.let { it.substring(0, it.lastIndexOf(".")) } + else -> null + } +} diff --git a/lint/src/test/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetectorTest.kt b/lint/src/test/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetectorTest.kt new file mode 100644 index 000000000..46432cf87 --- /dev/null +++ b/lint/src/test/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetectorTest.kt @@ -0,0 +1,101 @@ +package kiwi.orbit.compose.lint.detectors + +import com.android.tools.lint.checks.infrastructure.TestFiles +import com.android.tools.lint.checks.infrastructure.TestLintTask +import com.android.tools.lint.detector.api.Detector +import com.android.tools.lint.detector.api.SourceCodeScanner +import org.junit.Test + + +class DialogButtonsInContentSlotDetectorTest: Detector(), SourceCodeScanner { + + companion object { + private val ALERT_INFO_STUB = TestFiles.kotlin( + """ + package kiwi.orbit.compose.ui.controls + + fun AlertInfo( + actions: () -> Unit = {}, + content: () -> Unit, + ) + """.trimIndent(), + ) + + private val ORBIT_BUTTON_PRIMARY_STUB = TestFiles.kotlin( + """ + package kiwi.orbit.compose.ui.controls + + fun ButtonPrimary( + onClick: () -> Unit, + ) + + fun Column( + content: () -> Unit, + ) + """.trimIndent(), + ) + + private val ORBIT_BUTTON_CRITICAL_STUB = TestFiles.kotlin( + """ + package kiwi.orbit.compose.ui.controls + + fun ButtonCritical( + onClick: () -> Unit, + ) + """.trimIndent(), + ) + } + + + @Test + fun testDetector() { + TestLintTask.lint() + .files( + TestFiles.kotlin( + """ + package foo + + import kiwi.orbit.compose.ui.controls.AlertInfo + import kiwi.orbit.compose.ui.controls.ButtonPrimary + import kiwi.orbit.compose.ui.controls.ButtonCritical + import kiwi.orbit.compose.ui.controls.Column + + fun test() { + AlertInfo { ButtonPrimary( { } ) } + AlertInfo { ButtonCritical( { } ) } + AlertInfo { Column { ButtonPrimary( { } ) } } + AlertInfo { + Column { + } + ButtonPrimary( { } ) + } + } + """, + ), + ALERT_INFO_STUB, + ORBIT_BUTTON_PRIMARY_STUB, + ORBIT_BUTTON_CRITICAL_STUB, + ) + .issues(DialogButtonsInContentSlotDetector.ISSUE) + .allowMissingSdk() + .run() + .expectErrorCount(4) + .expect( + """ + src/foo/test.kt:10: Error: ButtonPrimary in Dialog's content slot. Use actions slot for CTAs instead. [DialogButtonsInContentSlotDetector] + AlertInfo { ButtonPrimary( { } ) } + ~~~~~~~~~~~~~~~~~~~~ + src/foo/test.kt:11: Error: ButtonCritical in Dialog's content slot. Use actions slot for CTAs instead. [DialogButtonsInContentSlotDetector] + AlertInfo { ButtonCritical( { } ) } + ~~~~~~~~~~~~~~~~~~~~~ + src/foo/test.kt:12: Error: ButtonPrimary in Dialog's content slot. Use actions slot for CTAs instead. [DialogButtonsInContentSlotDetector] + AlertInfo { Column { ButtonPrimary( { } ) } } + ~~~~~~~~~~~~~~~~~~~~ + src/foo/test.kt:16: Error: ButtonPrimary in Dialog's content slot. Use actions slot for CTAs instead. [DialogButtonsInContentSlotDetector] + ButtonPrimary( { } ) + ~~~~~~~~~~~~~~~~~~~~ + 4 errors, 0 warnings + """.trimIndent(), + ) + } +} From 3252f9aae596e976c3522203f004533146783d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mate=C4=8Dn=C3=BD?= Date: Fri, 29 Sep 2023 13:46:52 +0200 Subject: [PATCH 2/2] Utility functions and convenience functions consolidated and placed to LintUtils.kt --- .../DialogButtonsInContentSlotDetector.kt | 27 ---------------- .../orbit/compose/lint/detectors/LintUtils.kt | 31 ++++++++++++++++++ ...aterialDesignInsteadOrbitDesignDetector.kt | 11 ++----- .../MaterialDialogWithOrbitButtonsDetector.kt | 32 ++----------------- .../ScaffoldContentPaddingDetector.kt | 6 ---- 5 files changed, 35 insertions(+), 72 deletions(-) diff --git a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt index 1cd1af3ab..3da9d3709 100644 --- a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt +++ b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/DialogButtonsInContentSlotDetector.kt @@ -8,14 +8,10 @@ import com.android.tools.lint.detector.api.JavaContext import com.android.tools.lint.detector.api.Scope import com.android.tools.lint.detector.api.Severity import com.android.tools.lint.detector.api.SourceCodeScanner -import com.android.tools.lint.detector.api.computeKotlinArgumentMapping import com.intellij.psi.PsiElement -import com.intellij.psi.PsiJavaFile -import com.intellij.psi.PsiMember import com.intellij.psi.PsiMethod import org.jetbrains.uast.UBlockExpression import org.jetbrains.uast.UCallExpression -import org.jetbrains.uast.ULambdaExpression import org.jetbrains.uast.skipParenthesizedExprDown import org.jetbrains.uast.toUElement import org.jetbrains.uast.tryResolveNamed @@ -75,7 +71,6 @@ class DialogButtonsInContentSlotDetector : Detector(), SourceCodeScanner { ) } } - } private fun PsiElement.findAllBlockExpressionsInHierarchy(): List { @@ -87,26 +82,4 @@ class DialogButtonsInContentSlotDetector : Detector(), SourceCodeScanner { return expressions } - - private fun PsiMethod.getArgument( - node: UCallExpression, - argumentName: String, - ): ULambdaExpression? = computeKotlinArgumentMapping(node, this) - .orEmpty() - .filter { (_, parameter) -> - parameter.name == argumentName - } - .keys - .filterIsInstance() - .firstOrNull() - - private fun PsiMethod.isInPackageName(packageName: String): Boolean { - val actual = (containingFile as? PsiJavaFile)?.packageName - return packageName == actual - } - - private fun PsiElement.getPackageName(): String? = when (this) { - is PsiMember -> this.containingClass?.qualifiedName?.let { it.substring(0, it.lastIndexOf(".")) } - else -> null - } } diff --git a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/LintUtils.kt b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/LintUtils.kt index cdd0d08f7..e3e49a730 100644 --- a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/LintUtils.kt +++ b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/LintUtils.kt @@ -16,14 +16,45 @@ package kiwi.orbit.compose.lint.detectors +import com.android.tools.lint.detector.api.computeKotlinArgumentMapping +import com.intellij.psi.PsiElement +import com.intellij.psi.PsiJavaFile +import com.intellij.psi.PsiMember +import com.intellij.psi.PsiMethod import org.jetbrains.kotlin.psi.KtLambdaExpression import org.jetbrains.kotlin.psi.KtParameter import org.jetbrains.kotlin.psi.KtSimpleNameExpression import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType import org.jetbrains.kotlin.psi.psiUtil.isAncestor +import org.jetbrains.uast.UCallExpression import org.jetbrains.uast.ULambdaExpression import org.jetbrains.uast.toUElement +internal fun PsiMethod.isInPackageName(packageName: String): Boolean { + val actual = (containingFile as? PsiJavaFile)?.packageName + return packageName == actual +} + +internal fun PsiElement.getPackageName(): String? = when (this) { + is PsiMember -> this.containingClass?.qualifiedName?.let { it.substring(0, it.lastIndexOf(".")) } + else -> null +} + +/** + * Get arguments of a method. Useful for retrieving Compose Lambda expression used as arguments. + */ +internal fun PsiMethod.getArgument( + node: UCallExpression, + argumentName: String, +): ULambdaExpression? = computeKotlinArgumentMapping(node, this) + .orEmpty() + .filter { (_, parameter) -> + parameter.name == argumentName + } + .keys + .filterIsInstance() + .firstOrNull() + /** * Returns a list of unreferenced parameters in [this]. If no parameters have been specified, but * there is an implicit `it` parameter, this will return a list containing an diff --git a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDesignInsteadOrbitDesignDetector.kt b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDesignInsteadOrbitDesignDetector.kt index bbe6e1aa3..4fa5bfb24 100644 --- a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDesignInsteadOrbitDesignDetector.kt +++ b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDesignInsteadOrbitDesignDetector.kt @@ -16,7 +16,6 @@ import org.jetbrains.uast.UCallExpression import org.jetbrains.uast.UElement import org.jetbrains.uast.UQualifiedReferenceExpression import org.jetbrains.uast.USimpleNameReferenceExpression -import org.jetbrains.uast.getContainingUClass import org.jetbrains.uast.toUElement import org.jetbrains.uast.tryResolveNamed @@ -33,8 +32,7 @@ class MaterialDesignInsteadOrbitDesignDetector : Detector(), Detector.UastScanne return object : UElementHandler() { override fun visitCallExpression(node: UCallExpression) { val name = node.methodName ?: return - val wrapperName = node.resolve()?.containingClass?.qualifiedName ?: return - val packageName = getPackageName(wrapperName) + val packageName = node.resolve()?.getPackageName()?: return val fqn = "$packageName.$name" val (preferredName) = METHOD_NAMES.entries.firstOrNull { it.value.contains(fqn) } ?: return @@ -44,8 +42,7 @@ class MaterialDesignInsteadOrbitDesignDetector : Detector(), Detector.UastScanne val parentExpression = node.sourcePsi?.parents?.find { it is KtCallExpression } val resolved = parentExpression.toUElement()?.tryResolveNamed() val parentName = resolved?.name - val parentWrapper = resolved.toUElement()?.getContainingUClass()?.qualifiedName ?: "" - val parentPackage = getPackageName(parentWrapper) + val parentPackage = resolved?.getPackageName() ?: "" val parentFqn = "$parentPackage.$parentName" if (allowedEntry.value.find { parentFqn.contains(it) } != null) { return @@ -222,9 +219,5 @@ class MaterialDesignInsteadOrbitDesignDetector : Detector(), Detector.UastScanne "Using $name instead of $preferredName", ) } - - private fun getPackageName(fullyQualifiedName: String): String { - return fullyQualifiedName.substring(0, fullyQualifiedName.lastIndexOf(".")) - } } } diff --git a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDialogWithOrbitButtonsDetector.kt b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDialogWithOrbitButtonsDetector.kt index 4f9ca5779..bb204955f 100644 --- a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDialogWithOrbitButtonsDetector.kt +++ b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/MaterialDialogWithOrbitButtonsDetector.kt @@ -8,15 +8,10 @@ import com.android.tools.lint.detector.api.JavaContext import com.android.tools.lint.detector.api.Scope import com.android.tools.lint.detector.api.Severity import com.android.tools.lint.detector.api.SourceCodeScanner -import com.android.tools.lint.detector.api.computeKotlinArgumentMapping -import com.intellij.psi.PsiElement -import com.intellij.psi.PsiJavaFile -import com.intellij.psi.PsiMember import com.intellij.psi.PsiMethod import com.intellij.psi.PsiNamedElement import org.jetbrains.uast.UBlockExpression import org.jetbrains.uast.UCallExpression -import org.jetbrains.uast.ULambdaExpression import org.jetbrains.uast.skipParenthesizedExprDown import org.jetbrains.uast.tryResolveNamed @@ -51,7 +46,7 @@ class MaterialDialogWithOrbitButtonsDetector : Detector(), SourceCodeScanner { override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) { if (method.isInPackageName(PACKAGE_MATERIAL_ALERT)) { - val confirmButtonArgument = getArgument(node, method, MATERIAL_ALERT_CONFIRM_BUTTON_PARAM) + val confirmButtonArgument = method.getArgument(node, MATERIAL_ALERT_CONFIRM_BUTTON_PARAM) (confirmButtonArgument?.body as? UBlockExpression)?.let { body -> val resolvedBodyExpression = body.resolveFirstBodyExpression() ?: return@let @@ -77,7 +72,7 @@ class MaterialDialogWithOrbitButtonsDetector : Detector(), SourceCodeScanner { } } - val dismissButtonArgument = getArgument(node, method, MATERIAL_ALERT_DISMISS_BUTTON_PARAM) + val dismissButtonArgument = method.getArgument(node, MATERIAL_ALERT_DISMISS_BUTTON_PARAM) (dismissButtonArgument?.body as? UBlockExpression)?.let { body -> val resolvedBodyExpression = body.resolveFirstBodyExpression() ?: return@let @@ -105,29 +100,6 @@ class MaterialDialogWithOrbitButtonsDetector : Detector(), SourceCodeScanner { } } - private fun getArgument( - node: UCallExpression, - method: PsiMethod, - argumentName: String, - ): ULambdaExpression? = computeKotlinArgumentMapping(node, method) - .orEmpty() - .filter { (_, parameter) -> - parameter.name == argumentName - } - .keys - .filterIsInstance() - .firstOrNull() - - private fun PsiMethod.isInPackageName(packageName: String): Boolean { - val actual = (containingFile as? PsiJavaFile)?.packageName - return packageName == actual - } - - private fun PsiElement.getPackageName(): String? = when (this) { - is PsiMember -> this.containingClass?.qualifiedName?.let { it.substring(0, it.lastIndexOf(".")) } - else -> null - } - private fun UBlockExpression.resolveFirstBodyExpression(): PsiNamedElement? { return expressions.firstOrNull()?.skipParenthesizedExprDown()?.tryResolveNamed() } diff --git a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/ScaffoldContentPaddingDetector.kt b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/ScaffoldContentPaddingDetector.kt index 84f997cd8..58bbf9d5f 100644 --- a/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/ScaffoldContentPaddingDetector.kt +++ b/lint/src/main/kotlin/kiwi/orbit/compose/lint/detectors/ScaffoldContentPaddingDetector.kt @@ -25,7 +25,6 @@ import com.android.tools.lint.detector.api.Scope import com.android.tools.lint.detector.api.Severity import com.android.tools.lint.detector.api.SourceCodeScanner import com.android.tools.lint.detector.api.computeKotlinArgumentMapping -import com.intellij.psi.PsiJavaFile import com.intellij.psi.PsiMethod import java.util.EnumSet import org.jetbrains.uast.UCallExpression @@ -86,9 +85,4 @@ class ScaffoldContentPaddingDetector : } } } - - private fun PsiMethod.isInPackageName(packageName: String): Boolean { - val actual = (containingFile as? PsiJavaFile)?.packageName - return packageName == actual - } }