Create lint rules module with detector for requiring dropUnlessResumed - #190
Create lint rules module with detector for requiring dropUnlessResumed#190benwicks wants to merge 1 commit into
dropUnlessResumed#190Conversation
…d on all Button/clickable/IconButton
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @benwicks, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a new custom Android Lint rule within a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new lint module to enforce the use of dropUnlessResumed for click handlers, which is a great initiative to prevent lifecycle-related issues. I've identified a critical issue in the detector's implementation that could lead to false positives and have suggested a fix. Additionally, I've recommended adding a test case to improve the test suite's robustness by covering this scenario. After addressing these points, this will be a valuable contribution.
| override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: com.intellij.psi.PsiMethod) { | ||
| val methodName = node.methodName ?: return | ||
| if (methodName == "Button" || methodName == "clickable" || methodName == "IconButton") { | ||
| // Check all arguments for a lambda (named or positional) | ||
| val lambdaArgs = node.valueArguments.filterIsInstance<ULambdaExpression>() | ||
| if (lambdaArgs.isEmpty()) { | ||
| // Also check for named arguments | ||
| node.valueArguments.forEach { arg -> | ||
| if (arg is ULambdaExpression) { | ||
| lambdaArgs.plus(arg) | ||
| } | ||
| } | ||
| } | ||
| for (lambda in lambdaArgs) { | ||
| if (!lambdaCallsDropUnlessResumed(lambda)) { | ||
| context.report( | ||
| ISSUE, | ||
| node, | ||
| context.getLocation(node), | ||
| "onClick handler must use dropUnlessResumed" | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of visitMethodCall has several issues:
- It incorrectly checks all lambda arguments of a function call, not just the
onClickhandler. This will cause false positives for composables likeButtonthat have other lambda parameters (e.g.,content). The lint rule would incorrectly flag thecontentlambda for not usingdropUnlessResumed. - The logic to find lambda arguments between lines 39-46 contains dead code. The
plusmethod on an immutableListreturns a new list, which is not being used, so this block has no effect. - The check for method names on line 36 is redundant, as
visitMethodCallis only invoked for methods specified ingetApplicableMethodNames.
To fix this and make the detector more accurate, we should specifically target the onClick parameter and check only its argument. Here is a suggested replacement for the method that addresses these points.
override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: com.intellij.psi.PsiMethod) {
val onClickParameter = method.parameterList.parameters.find { it.name == "onClick" } ?: return
val onClickArgument = node.getArgumentForParameter(onClickParameter) ?: return
if (onClickArgument is ULambdaExpression) {
if (!lambdaCallsDropUnlessResumed(onClickArgument)) {
context.report(
ISSUE,
onClickArgument,
context.getLocation(onClickArgument),
"onClick handler must use dropUnlessResumed"
)
}
}
}
There was a problem hiding this comment.
+1 to this, I tried it out locally and two things to note are:
node.getArgumentForParameter(onClickParameter)should benode.getArgumentForParameter(onClickParameter.parameterIndex())- In the
reportcall, this uses the argument position, not the node position (which I think makes sense to do). So the expected value in the test needs to be updated if you use the argument position.
| ).run() | ||
| result.expectClean() | ||
| } | ||
| } |
There was a problem hiding this comment.
The current test suite doesn't cover cases where a composable has multiple lambda arguments, such as Button with its onClick and content lambdas. This is an important scenario to test to prevent false positives, as highlighted in the review of DropUnlessResumedOnClickDetector.
Please add a test case to ensure that the lint check correctly targets only the onClick lambda and ignores other lambdas like content.
@Test
fun testButtonWithContentLambda() {
lint().files(
buttonStub,
composableStub,
kotlin(
"""
package test
import androidx.compose.material3.Button
import androidx.compose.runtime.Composable
fun dropUnlessResumed(block: () -> Unit) = block()
@Composable
fun MyButton() {
Button(onClick = { dropUnlessResumed { println("Clicked") } }) {
// This content lambda should not be flagged.
}
}
""".trimIndent()
)
).run().expectClean()
}
}There was a problem hiding this comment.
+1, doesn't even need to be a separate test necessarily, can just add an empty trailing lambda to the existing ones.
|
Please address the initial gemini feedback. @bsagmoe for lint_baseline |
| lintOptions { | ||
| baseline(file("lint-baseline.xml")) | ||
| } |
There was a problem hiding this comment.
Could you update this to use the non-deprecated format?
lint {
baseline = file("lint-baseline.xml")
}
| @@ -0,0 +1,29 @@ | |||
| plugins { | |||
| id("java-library") | |||
| id("org.jetbrains.kotlin.jvm") | |||
There was a problem hiding this comment.
Can you add this plugin using the version catalog method?
build.gradle.kts (Project root)
plugins {
...
alias(libs.plugins.kotlin.jvm) apply false
...
}
libs.version.toml
...
[plugins]
...
kotlin-jvm = { id = "org.jetbrains.kotlin.jvm", version.ref = "kotlin" }
...
lint-rules/build.gradle.kts
plugins {
id("java-library")
alias(libs.plugins.kotlin.jvm)
}
|
|
||
| override fun getApplicableUastTypes() = listOf(UCallExpression::class.java) | ||
|
|
||
| override fun getApplicableMethodNames() = listOf("Button", "clickable", "IconButton") |
There was a problem hiding this comment.
Can you also add other button composables like ElevatedButton, FilledTonalButton, OutlinedButton, and TextButton?
| override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: com.intellij.psi.PsiMethod) { | ||
| val methodName = node.methodName ?: return | ||
| if (methodName == "Button" || methodName == "clickable" || methodName == "IconButton") { | ||
| // Check all arguments for a lambda (named or positional) | ||
| val lambdaArgs = node.valueArguments.filterIsInstance<ULambdaExpression>() | ||
| if (lambdaArgs.isEmpty()) { | ||
| // Also check for named arguments | ||
| node.valueArguments.forEach { arg -> | ||
| if (arg is ULambdaExpression) { | ||
| lambdaArgs.plus(arg) | ||
| } | ||
| } | ||
| } | ||
| for (lambda in lambdaArgs) { | ||
| if (!lambdaCallsDropUnlessResumed(lambda)) { | ||
| context.report( | ||
| ISSUE, | ||
| node, | ||
| context.getLocation(node), | ||
| "onClick handler must use dropUnlessResumed" | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
+1 to this, I tried it out locally and two things to note are:
node.getArgumentForParameter(onClickParameter)should benode.getArgumentForParameter(onClickParameter.parameterIndex())- In the
reportcall, this uses the argument position, not the node position (which I think makes sense to do). So the expected value in the test needs to be updated if you use the argument position.
| ).run() | ||
| result.expectClean() | ||
| } | ||
| } |
There was a problem hiding this comment.
+1, doesn't even need to be a separate test necessarily, can just add an empty trailing lambda to the existing ones.
|
In the interim, I've added a rule about this in |
3 participants
on all Button/clickable/IconButton
This is a start at addressing #107