From 9832afafe8daa8dd6b6739899891eeefaee79c57 Mon Sep 17 00:00:00 2001 From: Nacho Lopez Date: Thu, 3 Oct 2024 11:21:51 +0200 Subject: [PATCH] Add support to nullable slots in ContentSlotReused --- .../core/util/KtCallableDeclarations.kt | 7 +- .../nlopez/compose/core/util/KtParameters.kt | 9 ++ .../nlopez/compose/rules/ContentSlotReused.kt | 92 +++++-------------- .../rules/detekt/ContentSlotReusedCheck.kt | 2 +- .../detekt/ContentSlotReusedCheckTest.kt | 45 ++++++++- .../ktlint/ContentSlotReusedCheckTest.kt | 63 ++++++++++++- 6 files changed, 138 insertions(+), 80 deletions(-) create mode 100644 rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtParameters.kt diff --git a/rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtCallableDeclarations.kt b/rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtCallableDeclarations.kt index 31ec086d..8f8058db 100644 --- a/rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtCallableDeclarations.kt +++ b/rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtCallableDeclarations.kt @@ -50,6 +50,7 @@ val KnownUnstableCollectionTypesRegex = sequenceOf( fun KtCallableDeclaration.contentSlots( treatAsLambdaTypes: Set, treatAsComposableLambdaTypes: Set, -): List = valueParameters.filter { parameter -> - parameter.typeReference?.isComposableLambda(treatAsLambdaTypes, treatAsComposableLambdaTypes) == true -} +): Sequence = valueParameters.asSequence() + .filter { parameter -> + parameter.typeReference?.isComposableLambda(treatAsLambdaTypes, treatAsComposableLambdaTypes) == true + } diff --git a/rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtParameters.kt b/rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtParameters.kt new file mode 100644 index 00000000..bf375fca --- /dev/null +++ b/rules/common/src/main/kotlin/io/nlopez/compose/core/util/KtParameters.kt @@ -0,0 +1,9 @@ +// Copyright 2024 Nacho Lopez +// SPDX-License-Identifier: Apache-2.0 +package io.nlopez.compose.core.util + +import org.jetbrains.kotlin.psi.KtNullableType +import org.jetbrains.kotlin.psi.KtParameter + +val KtParameter.isTypeNullable: Boolean + get() = typeReference?.typeElement is KtNullableType diff --git a/rules/common/src/main/kotlin/io/nlopez/compose/rules/ContentSlotReused.kt b/rules/common/src/main/kotlin/io/nlopez/compose/rules/ContentSlotReused.kt index 559a7656..1d794b9e 100644 --- a/rules/common/src/main/kotlin/io/nlopez/compose/rules/ContentSlotReused.kt +++ b/rules/common/src/main/kotlin/io/nlopez/compose/rules/ContentSlotReused.kt @@ -10,93 +10,47 @@ import io.nlopez.compose.core.util.composableLambdaTypes import io.nlopez.compose.core.util.contentSlots import io.nlopez.compose.core.util.findChildrenByClass import io.nlopez.compose.core.util.findShadowingRedeclarations +import io.nlopez.compose.core.util.isTypeNullable import io.nlopez.compose.core.util.lambdaTypes -import io.nlopez.compose.core.util.uniquePairs -import org.jetbrains.kotlin.lexer.KtTokens -import org.jetbrains.kotlin.psi.KtBinaryExpression import org.jetbrains.kotlin.psi.KtCallExpression -import org.jetbrains.kotlin.psi.KtElement import org.jetbrains.kotlin.psi.KtFunction -import org.jetbrains.kotlin.psi.KtIfExpression -import org.jetbrains.kotlin.psi.KtWhenExpression -import org.jetbrains.kotlin.psi.psiUtil.parents +import org.jetbrains.kotlin.psi.KtParameter +import org.jetbrains.kotlin.psi.KtSafeQualifiedExpression class ContentSlotReused : ComposeKtVisitor { override fun visitComposable(function: KtFunction, emitter: Emitter, config: ComposeKtConfig) { val lambdaTypes = function.containingKtFile.lambdaTypes(config) val composableLambdaTypes = function.containingKtFile.composableLambdaTypes(config) - val slots = function.contentSlots(lambdaTypes, composableLambdaTypes) - .filter { it.name?.isNotEmpty() == true } + val slotsWithMultipleUsages = function.contentSlots(lambdaTypes, composableLambdaTypes) + .filter { slot -> function.findNotShadowedUsagesOf(slot).count() >= 2 } - val slotsWithMultipleUsages = slots.filter { it.name != null } - .map { slot -> slot to function.findUsages(slot.name!!) } - .filter { (_, usages) -> usages.count() >= 2 } - - // Now that we found some candidates, we need to make sure the slots being reused are in different branches - // in the ast tree. We'll need to search for parent ifs, whens, etc. - val slotsInDifferentBranches = slotsWithMultipleUsages - .filter { (_, usages) -> isSlotUsedInSeparateBranches(usages, function) } - .map { (slot, _) -> slot } - - for (slot in slotsInDifferentBranches) { - emitter.report(slot, ContentSlotReusedInDifferentBranches) + for (slot in slotsWithMultipleUsages) { + emitter.report(slot, ContentSlotsShouldNotBeReused) } } - private fun KtFunction.findUsages(name: String): Sequence = - findChildrenByClass().filter { it.calleeExpression?.text == name } - // Remove shadowed usages - .filter { it.findShadowingRedeclarations(name, this).count() == 0 } - - private fun isSlotUsedInSeparateBranches(slots: Sequence, stopAt: KtFunction): Boolean = - slots.uniquePairs() - .any { (slot1, slot2) -> - findCommonAncestor(slot1, slot2, stopAt).isBranchingElement() - } - - private fun findCommonAncestor(slot1: KtCallExpression, slot2: KtCallExpression, stopAt: KtFunction): KtElement { - val height1 = slot1.parents.takeWhile { it != stopAt }.count() - val height2 = slot2.parents.takeWhile { it != stopAt }.count() - - var current1: KtElement = slot1 - var current2: KtElement = slot2 - - // If the heights are different, we'll need to go up in the one that's deeper until they - when { - height1 > height2 -> { - repeat(height1 - height2) { current1 = current1.parent as KtElement } - } - - height1 < height2 -> { - repeat(height2 - height1) { current2 = current2.parent as KtElement } - } - } - - // Traverse up until they are at the same level - while (current1 != current2) { - current1 = current1.parent as KtElement - current2 = current2.parent as KtElement + private fun KtFunction.findNotShadowedUsagesOf(slot: KtParameter): Sequence { + val slotName = slot.name?.takeIf { it.isNotEmpty() } ?: return emptySequence() + val slots = when { + // content?.invoke() + slot.isTypeNullable -> findChildrenByClass() + .filter { it.receiverExpression.text == slotName } + .mapNotNull { it.selectorExpression as? KtCallExpression } + .filter { it.calleeExpression?.text == "invoke" } + + // content() + else -> findChildrenByClass().filter { it.calleeExpression?.text == slotName } } - - return current1 - } - - private fun KtElement.isBranchingElement(): Boolean = when (val current = this) { - // Always true, if not, the ancestor would have been found in then or in else expressions - is KtIfExpression -> true - // Always true, if not, the ancestor would have been found in a when entry - is KtWhenExpression -> true - // Only branching if it's an elvis operator - is KtBinaryExpression -> current.operationToken == KtTokens.ELVIS - else -> false + // Return and remove shadowed usages + return slots.filter { it.findShadowingRedeclarations(parameterName = slotName, stopAt = this).count() == 0 } } companion object { - val ContentSlotReusedInDifferentBranches = """ - Content slots should not be reused in different code branches of a composable function (e.g. if/when/elvis). + val ContentSlotsShouldNotBeReused = """ + Content slots should not be reused in different code branches/scopes of a composable function, to preserve the slot internal state. - You can wrap the usages in a remember { movableContentOf { ... }} block to make sure their internal state is preserved correctly. + You can wrap the slot in a remember { movableContentOf { ... }} block to make sure their internal state is preserved correctly. See https://mrmans0n.github.io/compose-rules/rules/#content-slots-should-not-be-reused-in-branching-code for more information. """.trimIndent() diff --git a/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheck.kt b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheck.kt index 27b019c6..f849bc0d 100644 --- a/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheck.kt +++ b/rules/detekt/src/main/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheck.kt @@ -16,7 +16,7 @@ class ContentSlotReusedCheck(config: Config) : override val issue: Issue = Issue( id = "ContentSlotReused", severity = Severity.Defect, - description = ContentSlotReused.ContentSlotReusedInDifferentBranches, + description = ContentSlotReused.ContentSlotsShouldNotBeReused, debt = Debt.TEN_MINS, ) } diff --git a/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheckTest.kt b/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheckTest.kt index 60f3f3b8..24d48d06 100644 --- a/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheckTest.kt +++ b/rules/detekt/src/test/kotlin/io/nlopez/compose/rules/detekt/ContentSlotReusedCheckTest.kt @@ -19,7 +19,7 @@ class ContentSlotReusedCheckTest { private val rule = ContentSlotReusedCheck(testConfig) @Test - fun `errors when there is a slot being reused in different branches`() { + fun `errors when there is a slot being reused`() { @Language("kotlin") val code = """ @@ -42,6 +42,11 @@ class ContentSlotReusedCheckTest { fun D(text: String, content: Potato) { potato?.let { content() } ?: content() } + @Composable + fun E(text: String, content: @Composable () -> Unit) { + val content1 = remember { movableContentOf { content() } } + val content2 = remember { movableContentOf { content() } } + } """.trimIndent() val errors = rule.lint(code) @@ -51,9 +56,45 @@ class ContentSlotReusedCheckTest { SourceLocation(6, 21), SourceLocation(13, 21), SourceLocation(17, 21), + SourceLocation(21, 21), + ) + for (error in errors) { + assertThat(error).hasMessage(ContentSlotReused.ContentSlotsShouldNotBeReused) + } + } + + @Test + fun `errors when there is a nullable slot being reused`() { + @Language("kotlin") + val code = + """ + @Composable + fun A(text: String, content: (@Composable () -> Unit)? = null) { + if (x) content?.invoke() else content?.invoke() + } + @Composable + fun B(text: String, content: (@Composable () -> Unit)? = null) { + when { + x -> content?.invoke() + else -> content?.invoke() + } + } + @Composable + fun C(text: String, content: (@Composable () -> Unit)? = null) { + val content1 = remember { movableContentOf { content?.invoke() } } + val content2 = remember { movableContentOf { content?.invoke() } } + } + """.trimIndent() + + val errors = rule.lint(code) + assertThat(errors) + .hasStartSourceLocations( + SourceLocation(2, 21), + SourceLocation(6, 21), + SourceLocation(13, 21), ) for (error in errors) { - assertThat(error).hasMessage(ContentSlotReused.ContentSlotReusedInDifferentBranches) + assertThat(error).hasMessage(ContentSlotReused.ContentSlotsShouldNotBeReused) } } diff --git a/rules/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/ContentSlotReusedCheckTest.kt b/rules/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/ContentSlotReusedCheckTest.kt index 8ba9edbe..893882c4 100644 --- a/rules/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/ContentSlotReusedCheckTest.kt +++ b/rules/ktlint/src/test/kotlin/io/nlopez/compose/rules/ktlint/ContentSlotReusedCheckTest.kt @@ -13,7 +13,7 @@ class ContentSlotReusedCheckTest { private val ruleAssertThat = assertThatRule { ContentSlotReusedCheck() } @Test - fun `errors when there is a slot being reused in different branches`() { + fun `errors when there is a slot being reused`() { @Language("kotlin") val code = """ @@ -36,6 +36,11 @@ class ContentSlotReusedCheckTest { fun D(text: String, content: Potato) { potato?.let { content() } ?: content() } + @Composable + fun E(text: String, content: @Composable () -> Unit) { + val content1 = remember { movableContentOf { content() } } + val content2 = remember { movableContentOf { content() } } + } """.trimIndent() ruleAssertThat(code) @@ -44,22 +49,70 @@ class ContentSlotReusedCheckTest { LintViolation( line = 2, col = 21, - detail = ContentSlotReused.ContentSlotReusedInDifferentBranches, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, ), LintViolation( line = 6, col = 21, - detail = ContentSlotReused.ContentSlotReusedInDifferentBranches, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, ), LintViolation( line = 13, col = 21, - detail = ContentSlotReused.ContentSlotReusedInDifferentBranches, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, ), LintViolation( line = 17, col = 21, - detail = ContentSlotReused.ContentSlotReusedInDifferentBranches, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, + ), + LintViolation( + line = 21, + col = 21, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, + ), + ) + } + + @Test + fun `errors when there is a nullable slot being reused`() { + @Language("kotlin") + val code = + """ + @Composable + fun A(text: String, content: (@Composable () -> Unit)? = null) { + if (x) content?.invoke() else content?.invoke() + } + @Composable + fun B(text: String, content: (@Composable () -> Unit)? = null) { + when { + x -> content?.invoke() + else -> content?.invoke() + } + } + @Composable + fun C(text: String, content: (@Composable () -> Unit)? = null) { + val content1 = remember { movableContentOf { content?.invoke() } } + val content2 = remember { movableContentOf { content?.invoke() } } + } + """.trimIndent() + + ruleAssertThat(code) + .hasLintViolationsWithoutAutoCorrect( + LintViolation( + line = 2, + col = 21, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, + ), + LintViolation( + line = 6, + col = 21, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, + ), + LintViolation( + line = 13, + col = 21, + detail = ContentSlotReused.ContentSlotsShouldNotBeReused, ), ) }