From b11c35777b5b9243c47303b341bd5b4a0fee32fe Mon Sep 17 00:00:00 2001 From: Severn Everett Date: Sun, 12 Dec 2021 18:29:45 +0100 Subject: [PATCH 1/7] Allow a function to declare a Unit return type when it uses a generic function initializer --- .../detekt/rules/style/optional/OptionalUnit.kt | 13 ++++++++++--- .../rules/style/optional/OptionalUnitSpec.kt | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt index 5771771e40b..3b6380e1689 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt @@ -9,6 +9,7 @@ import io.gitlab.arturbosch.detekt.api.Rule import io.gitlab.arturbosch.detekt.api.Severity import io.gitlab.arturbosch.detekt.rules.isOverride import org.jetbrains.kotlin.cfg.WhenChecker +import org.jetbrains.kotlin.js.translate.callTranslator.getReturnType import org.jetbrains.kotlin.psi.KtBlockExpression import org.jetbrains.kotlin.psi.KtExpression import org.jetbrains.kotlin.psi.KtIfExpression @@ -18,8 +19,10 @@ import org.jetbrains.kotlin.psi.KtWhenExpression import org.jetbrains.kotlin.psi.psiUtil.siblings import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.bindingContextUtil.isUsedAsExpression +import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall import org.jetbrains.kotlin.resolve.calls.callUtil.getType import org.jetbrains.kotlin.types.typeUtil.isNothing +import org.jetbrains.kotlin.types.typeUtil.isTypeParameter import org.jetbrains.kotlin.types.typeUtil.isUnit import org.jetbrains.kotlin.utils.addToStdlib.firstIsInstanceOrNull @@ -109,7 +112,7 @@ class OptionalUnit(config: Config = Config.empty) : Rule(config) { val typeReference = function.typeReference val typeElementText = typeReference?.typeElement?.text if (typeElementText == UNIT) { - if (function.initializer.isNothingType()) return + if (function.initializer.isGenericOrNothingType()) return report(CodeSmell(issue, Entity.from(typeReference), createMessage(function))) } } @@ -124,8 +127,12 @@ class OptionalUnit(config: Config = Config.empty) : Rule(config) { private fun createMessage(function: KtNamedFunction) = "The function ${function.name} " + "defines a return type of Unit. This is unnecessary and can safely be removed." - private fun KtExpression?.isNothingType() = - bindingContext != BindingContext.EMPTY && this?.getType(bindingContext)?.isNothing() == true + private fun KtExpression?.isGenericOrNothingType(): Boolean { + if (bindingContext == BindingContext.EMPTY) return false + val isGenericType = this?.getResolvedCall(bindingContext)?.getReturnType()?.isTypeParameter() == true + val isNothingType = this?.getType(bindingContext)?.isNothing() == true + return isGenericType || isNothingType + } companion object { private const val UNIT = "Unit" diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt index 3fe49e0ab9a..0d36ac09164 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt @@ -305,5 +305,19 @@ class OptionalUnitSpec : Spek({ val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).isEmpty() } + + it("should not report when the function initializer requires a type") { + val code = """ + fun foo(block: (List) -> Unit): T { + val list = listOf() + block(list) + return list.first() + } + + fun doFoo(): Unit = foo {} + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } } }) From 14892487c731c3cdbf38a66c758cf2b5f3562cfe Mon Sep 17 00:00:00 2001 From: Severn Everett Date: Sun, 12 Dec 2021 22:00:14 +0100 Subject: [PATCH 2/7] Added check for when redundant function return typing and generic typing are provided; Slight optimization in OptionalUnit for determining whether a function has an explicit return type --- .../detekt/rules/style/optional/OptionalUnit.kt | 15 +++++++++------ .../rules/style/optional/OptionalUnitSpec.kt | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt index 3b6380e1689..ecefea66a15 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt @@ -15,6 +15,7 @@ import org.jetbrains.kotlin.psi.KtExpression import org.jetbrains.kotlin.psi.KtIfExpression import org.jetbrains.kotlin.psi.KtNameReferenceExpression import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtTypeReference import org.jetbrains.kotlin.psi.KtWhenExpression import org.jetbrains.kotlin.psi.psiUtil.siblings import org.jetbrains.kotlin.resolve.BindingContext @@ -59,8 +60,9 @@ class OptionalUnit(config: Config = Config.empty) : Rule(config) { ) override fun visitNamedFunction(function: KtNamedFunction) { - if (function.hasDeclaredReturnType()) { - checkFunctionWithExplicitReturnType(function) + val typeReference = function.typeReference + if (typeReference != null) { + checkFunctionWithExplicitReturnType(function, typeReference) } else if (!function.isOverride()) { checkFunctionWithInferredReturnType(function) } @@ -108,9 +110,8 @@ class OptionalUnit(config: Config = Config.empty) : Rule(config) { } } - private fun checkFunctionWithExplicitReturnType(function: KtNamedFunction) { - val typeReference = function.typeReference - val typeElementText = typeReference?.typeElement?.text + private fun checkFunctionWithExplicitReturnType(function: KtNamedFunction, typeReference: KtTypeReference) { + val typeElementText = typeReference.typeElement?.text if (typeElementText == UNIT) { if (function.initializer.isGenericOrNothingType()) return report(CodeSmell(issue, Entity.from(typeReference), createMessage(function))) @@ -131,7 +132,9 @@ class OptionalUnit(config: Config = Config.empty) : Rule(config) { if (bindingContext == BindingContext.EMPTY) return false val isGenericType = this?.getResolvedCall(bindingContext)?.getReturnType()?.isTypeParameter() == true val isNothingType = this?.getType(bindingContext)?.isNothing() == true - return isGenericType || isNothingType + // Either the function initializer returns Nothing or it is a generic function + // into which Unit is passed, but not both. + return (isGenericType && !isNothingType) || (isNothingType && !isGenericType) } companion object { diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt index 0d36ac09164..8a822dae8af 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt @@ -319,5 +319,19 @@ class OptionalUnitSpec : Spek({ val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).isEmpty() } + + it("should report when the function initializer takes in the type Nothing") { + val code = """ + fun foo(block: (List) -> Unit): T { + val list = listOf() + block(list) + return list.first() + } + + fun doFoo(): Unit = foo {} + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + } } }) From b0bd724292b4bbcc8fd78ad6fcfbe55289d02fb4 Mon Sep 17 00:00:00 2001 From: Severn Everett Date: Sun, 12 Dec 2021 22:17:59 +0100 Subject: [PATCH 3/7] Added test to expand code coverage --- .../detekt/rules/style/optional/OptionalUnitSpec.kt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt index 8a822dae8af..d27c05f751e 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt @@ -333,5 +333,13 @@ class OptionalUnitSpec : Spek({ val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) } + + it("should report when a function has an explicit Unit return type with context") { + val code = """ + fun foo(): Unit { } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + } } }) From a96f4a0314a11f732295c82c76267d6f64a3d5a1 Mon Sep 17 00:00:00 2001 From: Severn Everett Date: Sun, 12 Dec 2021 22:31:19 +0100 Subject: [PATCH 4/7] Optimizing check for whether a function contains an initializer --- .../detekt/rules/style/optional/OptionalUnit.kt | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt index ecefea66a15..1218bb3e7b5 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnit.kt @@ -113,7 +113,8 @@ class OptionalUnit(config: Config = Config.empty) : Rule(config) { private fun checkFunctionWithExplicitReturnType(function: KtNamedFunction, typeReference: KtTypeReference) { val typeElementText = typeReference.typeElement?.text if (typeElementText == UNIT) { - if (function.initializer.isGenericOrNothingType()) return + val initializer = function.initializer + if (initializer?.isGenericOrNothingType() == true) return report(CodeSmell(issue, Entity.from(typeReference), createMessage(function))) } } @@ -128,10 +129,10 @@ class OptionalUnit(config: Config = Config.empty) : Rule(config) { private fun createMessage(function: KtNamedFunction) = "The function ${function.name} " + "defines a return type of Unit. This is unnecessary and can safely be removed." - private fun KtExpression?.isGenericOrNothingType(): Boolean { + private fun KtExpression.isGenericOrNothingType(): Boolean { if (bindingContext == BindingContext.EMPTY) return false - val isGenericType = this?.getResolvedCall(bindingContext)?.getReturnType()?.isTypeParameter() == true - val isNothingType = this?.getType(bindingContext)?.isNothing() == true + val isGenericType = getResolvedCall(bindingContext)?.getReturnType()?.isTypeParameter() == true + val isNothingType = getType(bindingContext)?.isNothing() == true // Either the function initializer returns Nothing or it is a generic function // into which Unit is passed, but not both. return (isGenericType && !isNothingType) || (isNothingType && !isGenericType) From 4e15b137ad10e498b3a97eeda6ed3eaa2555b89d Mon Sep 17 00:00:00 2001 From: Severn Everett Date: Sun, 12 Dec 2021 23:24:56 +0100 Subject: [PATCH 5/7] Adding more tests to improve code coverage --- .../rules/style/optional/OptionalUnitSpec.kt | 109 ++++++++++++------ 1 file changed, 73 insertions(+), 36 deletions(-) diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt index d27c05f751e..edaa59ecfee 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt @@ -18,6 +18,21 @@ class OptionalUnitSpec : Spek({ val env: KotlinCoreEnvironment by memoized() describe("OptionalUnit rule") { + it("should report when a function has an explicit Unit return type with context") { + val code = """ + fun foo(): Unit { } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + } + + it("should not report when a function has a non-unit body expression") { + val code = """ + fun foo() = String + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } context("several functions which return Unit") { @@ -296,50 +311,72 @@ class OptionalUnitSpec : Spek({ val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) } + + it("another object is used as the last expression") { + val code = """ + fun foo() { + String + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } } - it("should not report when function initializer is Nothing") { - val code = """ + context("function initializers") { + it("should not report when function initializer is Nothing") { + val code = """ fun test(): Unit = throw UnsupportedOperationException() """ - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).isEmpty() - } - - it("should not report when the function initializer requires a type") { - val code = """ - fun foo(block: (List) -> Unit): T { - val list = listOf() - block(list) - return list.first() - } + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } - fun doFoo(): Unit = foo {} - """.trimIndent() - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).isEmpty() - } + it("should not report when the function initializer requires a type") { + val code = """ + fun foo(block: (List) -> Unit): T { + val list = listOf() + block(list) + return list.first() + } + + fun doFoo(): Unit = foo {} + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } - it("should report when the function initializer takes in the type Nothing") { - val code = """ - fun foo(block: (List) -> Unit): T { - val list = listOf() - block(list) - return list.first() - } + it("should report on function initializers when there is no context") { + val code = """ + fun test(): Unit = throw UnsupportedOperationException() + """ + val findings = subject.compileAndLint(code) + assertThat(findings).hasSize(1) + } - fun doFoo(): Unit = foo {} - """.trimIndent() - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).hasSize(1) - } + it("should report when the function initializer takes in the type Nothing") { + val code = """ + fun foo(block: (List) -> Unit): T { + val list = listOf() + block(list) + return list.first() + } + + fun doFoo(): Unit = foo {} + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + } - it("should report when a function has an explicit Unit return type with context") { - val code = """ - fun foo(): Unit { } - """.trimIndent() - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).hasSize(1) + it("should report when the function initializer does not provide a different type") { + val code = """ + fun foo() {} + + fun doFoo(): Unit = foo() + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + } } } }) From 71ffd13f510779cc1a9fd8650063ca0e23157788 Mon Sep 17 00:00:00 2001 From: Severn Everett Date: Mon, 13 Dec 2021 08:10:35 +0100 Subject: [PATCH 6/7] Adding another test to improve code coverage --- .../detekt/rules/style/optional/OptionalUnitSpec.kt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt index edaa59ecfee..636b5d0b20e 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt @@ -346,6 +346,14 @@ class OptionalUnitSpec : Spek({ assertThat(findings).isEmpty() } + it("should report when it cannot resolve the initializer call") { + val code = """ + fun doFoo(): Unit = unknownCall {} + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + } + it("should report on function initializers when there is no context") { val code = """ fun test(): Unit = throw UnsupportedOperationException() From cc5c1f293289fe896c4f4816bc76e9b4c5dae16c Mon Sep 17 00:00:00 2001 From: Severn Everett Date: Mon, 13 Dec 2021 08:34:24 +0100 Subject: [PATCH 7/7] Removing test due to it failing compilation check --- .../detekt/rules/style/optional/OptionalUnitSpec.kt | 8 -------- 1 file changed, 8 deletions(-) diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt index 636b5d0b20e..edaa59ecfee 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/optional/OptionalUnitSpec.kt @@ -346,14 +346,6 @@ class OptionalUnitSpec : Spek({ assertThat(findings).isEmpty() } - it("should report when it cannot resolve the initializer call") { - val code = """ - fun doFoo(): Unit = unknownCall {} - """.trimIndent() - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).hasSize(1) - } - it("should report on function initializers when there is no context") { val code = """ fun test(): Unit = throw UnsupportedOperationException()