From f7d3e33ee57011bb2adc76ad9dffb273b48463dd Mon Sep 17 00:00:00 2001 From: Nacho Lopez Date: Thu, 19 Sep 2024 17:45:56 +0200 Subject: [PATCH] Add LambdaParameterEventTrailing rule --- docs/detekt.md | 6 ++ docs/rules.md | 45 ++++++++++- .../io/nlopez/compose/core/util/Lambdas.kt | 8 +- .../rules/LambdaParameterEventTrailing.kt | 51 +++++++++++++ .../rules/detekt/ComposeRuleSetProvider.kt | 1 + .../LambdaParameterEventTrailingCheck.kt | 23 ++++++ .../LambdaParameterEventTrailingCheckTest.kt | 76 +++++++++++++++++++ .../rules/ktlint/ComposeRuleSetProvider.kt | 1 + .../LambdaParameterEventTrailingCheck.kt | 14 ++++ 9 files changed, 218 insertions(+), 7 deletions(-) create mode 100644 rules/common/src/main/kotlin/io/nlopez/compose/rules/LambdaParameterEventTrailing.kt create mode 100644 rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheck.kt create mode 100644 rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheckTest.kt create mode 100644 rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/LambdaParameterEventTrailingCheck.kt diff --git a/docs/detekt.md b/docs/detekt.md index 7adde07d..3310850c 100644 --- a/docs/detekt.md +++ b/docs/detekt.md @@ -51,6 +51,12 @@ Compose: # treatAsComposableLambda: MyComposableLambdaType DefaultsVisibility: active: true + LambdaParameterEventTrailing: + active: true + # -- You can optionally add your own composables here + # contentEmitters: MyComposable,MyOtherComposable + # -- You can add composables here that you don't want to count as content emitters (e.g. custom dialogs or modals) + # contentEmittersDenylist: MyNonEmitterComposable LambdaParameterInRestartableEffect: active: true # -- You can optionally have a list of types to be treated as lambdas (e.g. typedefs or fun interfaces not picked up automatically) diff --git a/docs/rules.md b/docs/rules.md index 958e3df5..395d6c04 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -20,7 +20,7 @@ More information: [State and Jetpack Compose](https://developer.android.com/jetp ### State should be remembered in composables -Be careful when using `mutableStateOf` (or any of the other state builders) to make sure that you `remember` the instance. If you don't `remember` the state instance, a new state instance will be created when the function is recomposed. +Be careful when using `mutableStateOf` (or any of the other `State` builders) to make sure that you `remember` the instance. If you don't `remember` the state instance, a new state instance will be created when the function is recomposed. !!! info "" @@ -236,6 +236,45 @@ fun Profile(user: User, modifier: Modifier = Modifier) { :material-chevron-right-box: [compose:content-trailing-lambda](https://github.com/mrmans0n/compose-rules/blob/main/rules/common/src/main/kotlin/io/nlopez/compose/rules/ContentTrailingLambda.kt) ktlint :material-chevron-right-box: [ContentTrailingLambda](https://github.com/mrmans0n/compose-rules/blob/main/rules/common/src/main/kotlin/io/nlopez/compose/rules/ContentTrailingLambda.kt) detekt +### Avoid using the trailing lambda for event lambdas in UI Composables + +In Compose, trailing lambdas in composable functions are typically used for content slots. To avoid confusion and maintain consistency, event lambdas (e.g., `onClick`, `onValueChange`) should generally not be placed in the trailing position. + +Recommendations: + +- **Required** Event Lambdas: Place required event lambdas before the `Modifier` parameter. This clearly distinguishes them from content slots. +- **Optional** Event Lambdas: When possible, avoid placing optional event lambdas as the last parameter. If an optional event lambda must be positioned at the end, consider adding a clarifying comment to the function definition. + +```kotlin +// ❌ Using an event lambda (like onClick) as the trailing lambda when in a composable makes it error prone and awkward to read +@Composable +fun MyButton(modifier: Modifier = Modifier, onClick: () -> Unit) { /* ... */ } + +@Composable +fun SomeUI(modifier: Modifier = Modifier) { + MyButton { + // This is an onClick, but by reading it people would assume it's a content slot + } +} + +// ✅ By moving the event lambda to be before Modifier, we avoid confusion +@Composable +fun MyBetterButton(onClick: () -> Unit, modifier: Modifier = Modifier) { /* ... */ } + +@Composable +fun SomeUI(modifier: Modifier = Modifier) { + MyBetterButton( + onClick = { + // Now this param is straightforward to understand + }, + ) +} +``` + +!!! info "" + + :material-chevron-right-box: [compose:lambda-param-event-trailing](https://github.com/mrmans0n/compose-rules/blob/main/rules/common/src/main/kotlin/io/nlopez/compose/rules/ContentTrailingLambda.kt) ktlint :material-chevron-right-box: [LambdaParameterEventTrailing](https://github.com/mrmans0n/compose-rules/blob/main/rules/common/src/main/kotlin/io/nlopez/compose/rules/LambdaParameterEventTrailing.kt) detekt + ### Naming CompositionLocals properly `CompositionLocal`s should be named by using the adjective `Local` as prefix, followed by a descriptive noun that describes the value they hold. This makes it easier to know when a value comes from a `CompositionLocal`. Given that these are implicit dependencies, we should make them obvious. @@ -328,11 +367,11 @@ To try to enforce common standard, and for consistency’s sake, we'll want to a ```kotlin // ❌ @Composable -fun Avatar(onShown: () -> Unit, onChanged: () -> Unit) { ... } +fun Avatar(onShown: () -> Unit, onChanged: () -> Unit) { /* ... */ } // ✅ @Composable -fun Avatar(onShow: () -> Unit, onChange: () -> Unit) { ... } +fun Avatar(onShow: () -> Unit, onChange: () -> Unit) { /* ... */ } ``` !!! info "" diff --git a/rules/common/src/main/kotlin/io/nlopez/compose/core/util/Lambdas.kt b/rules/common/src/main/kotlin/io/nlopez/compose/core/util/Lambdas.kt index daf345e5..3a9192cf 100644 --- a/rules/common/src/main/kotlin/io/nlopez/compose/core/util/Lambdas.kt +++ b/rules/common/src/main/kotlin/io/nlopez/compose/core/util/Lambdas.kt @@ -13,19 +13,19 @@ import org.jetbrains.kotlin.psi.KtTypeElement import org.jetbrains.kotlin.psi.KtTypeReference import org.jetbrains.kotlin.psi.KtUserType -fun KtTypeElement.isLambda(treatAsLambdaTypes: Set): Boolean = when (this) { +fun KtTypeElement.isLambda(treatAsLambdaTypes: Set = emptySet()): Boolean = when (this) { is KtFunctionType -> true is KtNullableType -> innerType?.isLambda(treatAsLambdaTypes) == true is KtUserType -> referencedName in treatAsLambdaTypes else -> false } -fun KtTypeReference.isLambda(treatAsLambdaTypes: Set): Boolean = +fun KtTypeReference.isLambda(treatAsLambdaTypes: Set = emptySet()): Boolean = typeElement?.isLambda(treatAsLambdaTypes) == true fun KtTypeReference.isComposableLambda( - treatAsLambdaTypes: Set, - treatAsComposableLambdaTypes: Set, + treatAsLambdaTypes: Set = emptySet(), + treatAsComposableLambdaTypes: Set = emptySet(), ): Boolean = when (val element = typeElement) { null -> false is KtFunctionType -> isComposable diff --git a/rules/common/src/main/kotlin/io/nlopez/compose/rules/LambdaParameterEventTrailing.kt b/rules/common/src/main/kotlin/io/nlopez/compose/rules/LambdaParameterEventTrailing.kt new file mode 100644 index 00000000..68a93524 --- /dev/null +++ b/rules/common/src/main/kotlin/io/nlopez/compose/rules/LambdaParameterEventTrailing.kt @@ -0,0 +1,51 @@ +// Copyright 2024 Nacho Lopez +// SPDX-License-Identifier: Apache-2.0 +package io.nlopez.compose.rules + +import io.nlopez.compose.core.ComposeKtConfig +import io.nlopez.compose.core.ComposeKtVisitor +import io.nlopez.compose.core.Emitter +import io.nlopez.compose.core.report +import io.nlopez.compose.core.util.emitsContent +import io.nlopez.compose.core.util.isComposable +import io.nlopez.compose.core.util.isLambda +import io.nlopez.compose.core.util.modifierParameter +import org.jetbrains.kotlin.psi.KtFunction + +class LambdaParameterEventTrailing : ComposeKtVisitor { + override fun visitComposable(function: KtFunction, emitter: Emitter, config: ComposeKtConfig) { + // We want this rule to only run for functions that emit content + if (!function.emitsContent(config)) return + + // We'd also want for it to have a modifier to apply the rule, which serves two purposes: + // - making sure that it's the separation between required and optional parameters + // - the lambda would be able to be moved before the modifier and not be the trailing one + if (function.modifierParameter(config) == null) return + + val trailingParam = function.valueParameters.lastOrNull() ?: return + + // Check if the trailing element... + // - is a lambda + // - is not @Composable + // - doesn't have a default value + // - starts with "on", meaning it's an event + val typeReference = trailingParam.typeReference ?: return + if (!typeReference.isLambda()) return + if (typeReference.isComposable) return + if (trailingParam.hasDefaultValue()) return + val name = trailingParam.name ?: return + if (!name.startsWith("on")) return + + emitter.report(trailingParam, EventLambdaIsTrailingLambda) + } + + companion object { + val EventLambdaIsTrailingLambda = """ + Lambda parameters in a @Composable that are for events (e.g. onClick, onChange, etc) and are required (they don't have a default value) should not be used as the trailing parameter. + + Composable functions that emit content usually reserve the trailing lambda syntax for the content slot, and that can lead to an assumption that other composables can be used in that lambda. + + See https://mrmans0n.github.io/compose-rules/rules/#TODO for more information. + """.trimIndent() + } +} diff --git a/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ComposeRuleSetProvider.kt b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ComposeRuleSetProvider.kt index 397e30d9..6496040b 100644 --- a/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ComposeRuleSetProvider.kt +++ b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ComposeRuleSetProvider.kt @@ -18,6 +18,7 @@ class ComposeRuleSetProvider : RuleSetProvider { ContentEmitterReturningValuesCheck(config), ContentTrailingLambdaCheck(config), DefaultsVisibilityCheck(config), + LambdaParameterEventTrailingCheck(config), LambdaParameterInRestartableEffectCheck(config), Material2Check(config), ModifierClickableOrderCheck(config), diff --git a/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheck.kt b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheck.kt new file mode 100644 index 00000000..8ac13db4 --- /dev/null +++ b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheck.kt @@ -0,0 +1,23 @@ +// Copyright 2024 Nacho Lopez +// SPDX-License-Identifier: Apache-2.0 +package io.nlopez.compose.rules.detekt + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Severity +import io.nlopez.compose.core.ComposeKtVisitor +import io.nlopez.compose.rules.DetektRule +import io.nlopez.compose.rules.LambdaParameterEventTrailing + +class LambdaParameterEventTrailingCheck(config: Config) : + DetektRule(config), + ComposeKtVisitor by LambdaParameterEventTrailing() { + + override val issue: Issue = Issue( + id = "LambdaParameterEventTrailing", + severity = Severity.Style, + description = LambdaParameterEventTrailing.EventLambdaIsTrailingLambda, + debt = Debt.FIVE_MINS, + ) +} diff --git a/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheckTest.kt b/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheckTest.kt new file mode 100644 index 00000000..75659891 --- /dev/null +++ b/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/LambdaParameterEventTrailingCheckTest.kt @@ -0,0 +1,76 @@ +// Copyright 2024 Nacho Lopez +// SPDX-License-Identifier: Apache-2.0 +package io.nlopez.compose.rules.detekt + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.SourceLocation +import io.gitlab.arturbosch.detekt.test.assertThat +import io.gitlab.arturbosch.detekt.test.lint +import io.nlopez.compose.rules.LambdaParameterEventTrailing +import org.intellij.lang.annotations.Language +import org.junit.jupiter.api.Test + +class LambdaParameterEventTrailingCheckTest { + + private val rule = LambdaParameterEventTrailingCheck(Config.empty) + + @Test + fun `error out when detecting a lambda being as trailing`() { + @Language("kotlin") + val code = + """ + @Composable + fun Something(modifier: Modifier = Modifier, onClick: () -> Unit) { + Text("Hello") + } + """.trimIndent() + val errors = rule.lint(code) + assertThat(errors) + .hasStartSourceLocations( + SourceLocation(2, 46), + ) + for (error in errors) { + assertThat(error).hasMessage(LambdaParameterEventTrailing.EventLambdaIsTrailingLambda) + } + } + + @Test + fun `passes when a lambda is required`() { + @Language("kotlin") + val code = + """ + @Composable + fun Something(onClick: () -> Unit, modifier: Modifier = Modifier) { + Text("Hello") + } + """.trimIndent() + val errors = rule.lint(code) + assertThat(errors).isEmpty() + } + + @Test + fun `passes when a lambda is composable`() { + @Language("kotlin") + val code = + """ + @Composable + fun Something(modifier: Modifier = Modifier, on: @Composable () -> Unit) { + Text("Hello") + } + """.trimIndent() + val errors = rule.lint(code) + assertThat(errors).isEmpty() + } + + @Test + fun `passes when the function doesnt emit content`() { + @Language("kotlin") + val code = + """ + @Composable + fun something(onClick: () -> Unit) {} + """.trimIndent() + val errors = rule.lint(code) + assertThat(errors).isEmpty() + } +} diff --git a/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/ComposeRuleSetProvider.kt b/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/ComposeRuleSetProvider.kt index 688b040e..fd67404c 100644 --- a/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/ComposeRuleSetProvider.kt +++ b/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/ComposeRuleSetProvider.kt @@ -18,6 +18,7 @@ class ComposeRuleSetProvider : RuleProvider { ContentEmitterReturningValuesCheck() }, RuleProvider { ContentTrailingLambdaCheck() }, RuleProvider { DefaultsVisibilityCheck() }, + RuleProvider { LambdaParameterEventTrailingCheck() }, RuleProvider { LambdaParameterInRestartableEffectCheck() }, RuleProvider { Material2Check() }, RuleProvider { ModifierClickableOrderCheck() }, diff --git a/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/LambdaParameterEventTrailingCheck.kt b/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/LambdaParameterEventTrailingCheck.kt new file mode 100644 index 00000000..84feacaf --- /dev/null +++ b/rules/ktlint/src/main/kotlin/io/nlopez/compose/rules/ktlint/LambdaParameterEventTrailingCheck.kt @@ -0,0 +1,14 @@ +// Copyright 2024 Nacho Lopez +// SPDX-License-Identifier: Apache-2.0 +package io.nlopez.compose.rules.ktlint + +import io.nlopez.compose.core.ComposeKtVisitor +import io.nlopez.compose.rules.KtlintRule +import io.nlopez.compose.rules.LambdaParameterEventTrailing + +class LambdaParameterEventTrailingCheck : + KtlintRule( + id = "compose:lambda-param-event-trailing", + editorConfigProperties = setOf(contentEmittersProperty, contentEmittersDenylist) + ), + ComposeKtVisitor by LambdaParameterEventTrailing()