From 8ec16324146c667025becb17504c26c095ad3198 Mon Sep 17 00:00:00 2001 From: Matt Gulbronson <59572380+mgulbronson@users.noreply.github.com> Date: Wed, 6 Mar 2024 17:48:13 -0500 Subject: [PATCH] Add rule to ban extension functions on nullable receivers (#26) --- README.md | 2 + detekt.yaml | 2 + .../com/faire/detekt/FaireRulesProvider.kt | 2 + .../NoExtensionFunctionOnNullableReceiver.kt | 58 +++++++++++++++++++ ...ExtensionFunctionOnNullableReceiverTest.kt | 56 ++++++++++++++++++ 5 files changed, 120 insertions(+) create mode 100644 src/main/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiver.kt create mode 100644 src/test/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiverTest.kt diff --git a/README.md b/README.md index 9ea51c7..864456a 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,8 @@ FaireRuleSet: active: true GetOrDefaultShouldBeReplacedWithGetOrElse: active: true + NoExtensionFunctionOnNullableReceiver: + active: true NoNonPrivateGlobalVariables: active: true NoNullableLambdaWithDefaultNull: diff --git a/detekt.yaml b/detekt.yaml index d92f6d7..c896d1b 100644 --- a/detekt.yaml +++ b/detekt.yaml @@ -117,6 +117,8 @@ FaireRuleSet: active: true GetOrDefaultShouldBeReplacedWithGetOrElse: active: true + NoExtensionFunctionOnNullableReceiver: + active: true NoNonPrivateGlobalVariables: active: true NoNullableLambdaWithDefaultNull: diff --git a/src/main/kotlin/com/faire/detekt/FaireRulesProvider.kt b/src/main/kotlin/com/faire/detekt/FaireRulesProvider.kt index 457552b..65a22d3 100644 --- a/src/main/kotlin/com/faire/detekt/FaireRulesProvider.kt +++ b/src/main/kotlin/com/faire/detekt/FaireRulesProvider.kt @@ -11,6 +11,7 @@ import com.faire.detekt.rules.DoNotUsePropertyAccessInAssert import com.faire.detekt.rules.DoNotUseSingleOnFilter import com.faire.detekt.rules.DoNotUseSizePropertyInAssert import com.faire.detekt.rules.GetOrDefaultShouldBeReplacedWithGetOrElse +import com.faire.detekt.rules.NoExtensionFunctionOnNullableReceiver import com.faire.detekt.rules.NoNonPrivateGlobalVariables import com.faire.detekt.rules.NoNullableLambdaWithDefaultNull import com.faire.detekt.rules.NoPairWithAmbiguousTypes @@ -43,6 +44,7 @@ internal class FaireRulesProvider : RuleSetProvider { DoNotUseSingleOnFilter(config), DoNotUseSizePropertyInAssert(config), GetOrDefaultShouldBeReplacedWithGetOrElse(config), + NoExtensionFunctionOnNullableReceiver(config), NoNonPrivateGlobalVariables(config), NoNullableLambdaWithDefaultNull(config), NoPairWithAmbiguousTypes(config), diff --git a/src/main/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiver.kt b/src/main/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiver.kt new file mode 100644 index 0000000..16cfb09 --- /dev/null +++ b/src/main/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiver.kt @@ -0,0 +1,58 @@ +package com.faire.detekt.rules + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.psiUtil.isExtensionDeclaration + +/** + * This rule reports extension functions on nullable types. + * Exceptions are made for functions that return non-nullable types since they do not introduce nullability. + * + * The reason for this rule is that extension functions on nullable types can obscure the nullability of the receiver. + * + * ``` + * // Bad + * fun String?.foo(): String? = this + * + * nonNullString.foo() // Return value is nullable which needs to be handled downstream + * + * // Good + * fun String.foo(): String = this + * + * nonNullString.foo() // Return value stays non-null + * nullableString?.foo() // Use null-safe operator to handle nullable receiver + * + * // Okay — function returns non-nullable type, essentially converting a nullable into a non-nullable type + * fun String?.emptyIfNull(): String = this ?: "" + * ``` + */ +internal class NoExtensionFunctionOnNullableReceiver(config: Config = Config.empty) : Rule(config) { + override val issue: Issue = Issue( + id = javaClass.simpleName, + severity = Severity.Warning, + description = "This rule reports extension functions on nullable types.", + debt = Debt.FIVE_MINS, + ) + + override fun visitNamedFunction(function: KtNamedFunction) { + super.visitNamedFunction(function) + + if (!function.isExtensionDeclaration()) return + if (function.receiverTypeReference?.text?.endsWith("?") != true) return + if (function.typeReference?.text?.endsWith("?") != true) return + + report( + CodeSmell( + issue = issue, + entity = Entity.from(function), + message = "No extension functions on nullable types", + ), + ) + } +} diff --git a/src/test/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiverTest.kt b/src/test/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiverTest.kt new file mode 100644 index 0000000..404b2f2 --- /dev/null +++ b/src/test/kotlin/com/faire/detekt/rules/NoExtensionFunctionOnNullableReceiverTest.kt @@ -0,0 +1,56 @@ +package com.faire.detekt.rules + +import io.gitlab.arturbosch.detekt.test.assertThat +import io.gitlab.arturbosch.detekt.test.compileAndLint +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test + +internal class NoExtensionFunctionOnNullableReceiverTest { + private lateinit var rule: NoExtensionFunctionOnNullableReceiver + + @BeforeEach + fun setup() { + rule = NoExtensionFunctionOnNullableReceiver() + } + + @Test + fun `flag non-private extension function on nullable type`() { + val findings = rule.compileAndLint( + """ + fun String?.foo(): String? = this + """.trimIndent(), + ) + assertThat(findings).hasSize(1) + } + + @Test + fun `do not flag extension function on non-nullable type`() { + val findings = rule.compileAndLint( + """ + fun String.foo(): String? = this + """.trimIndent(), + ) + assertThat(findings).isEmpty() + } + + @Test + fun `do not flag extension function that returns non-null type`() { + val findings = rule.compileAndLint( + """ + fun String?.foo(): String = this ?: "" + """.trimIndent(), + ) + assertThat(findings).isEmpty() + } + + @Test + fun `do not flag if manually suppressed`() { + val findings = rule.compileAndLint( + """ + @Suppress("NoExtensionFunctionOnNullableReceiver") + fun String?.foo(): String? = this + """.trimIndent(), + ) + assertThat(findings).isEmpty() + } +}