Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a warning when a button is used inside of AlertDialog's content slot instead of actions slot #545

Merged
merged 2 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ 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

@Suppress("UnstableApiUsage")
class OrbitComposeIssueRegistry : IssueRegistry() {
override val issues = listOf(
DialogButtonsInContentSlotDetector.ISSUE,
MaterialDesignInsteadOrbitDesignDetector.ISSUE,
MaterialDialogWithOrbitButtonsDetector.ISSUE,
ScaffoldContentPaddingDetector.ISSUE,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
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.intellij.psi.PsiElement
import com.intellij.psi.PsiMethod
import org.jetbrains.uast.UBlockExpression
import org.jetbrains.uast.UCallExpression
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<String> = 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<UBlockExpression> {
val expressions = mutableListOf<UBlockExpression>()

val uElement = this.toUElement()
if (uElement is UBlockExpression) expressions.add(uElement)
expressions.addAll(children.flatMap { it.findAllBlockExpressionsInHierarchy() })

return expressions
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<ULambdaExpression>()
.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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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("."))
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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<ULambdaExpression>()
.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()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -86,9 +85,4 @@ class ScaffoldContentPaddingDetector :
}
}
}

private fun PsiMethod.isInPackageName(packageName: String): Boolean {
val actual = (containingFile as? PsiJavaFile)?.packageName
return packageName == actual
}
}
Original file line number Diff line number Diff line change
@@ -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(),
)
}
}