From 01063dcb158ad720356557b62476af14191ea308 Mon Sep 17 00:00:00 2001 From: marschwar Date: Mon, 1 Aug 2022 10:09:30 +0200 Subject: [PATCH] Fix false negative for UseRequire when thrown in conditional block (#5147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix false negative for UseRequire when thrown in conditional block * do not change api * use assertj instead of junit * Update detekt-psi-utils/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensionsSpec.kt Co-authored-by: Brais Gabín Co-authored-by: Markus Schwarz Co-authored-by: Brais Gabín --- .../detekt/rules/ThrowExtensions.kt | 8 +- .../detekt/rules/ThrowExtensionsSpec.kt | 130 ++++++++++++++++++ .../detekt/rules/style/UseRequire.kt | 13 +- .../detekt/rules/style/UseCheckOrErrorSpec.kt | 13 ++ .../detekt/rules/style/UseRequireSpec.kt | 29 ++++ 5 files changed, 181 insertions(+), 12 deletions(-) create mode 100644 detekt-psi-utils/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensionsSpec.kt diff --git a/detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensions.kt b/detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensions.kt index 98e881dc7ff..274a2b737e2 100644 --- a/detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensions.kt +++ b/detekt-psi-utils/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensions.kt @@ -1,8 +1,8 @@ package io.gitlab.arturbosch.detekt.rules +import org.jetbrains.kotlin.psi.KtBlockExpression import org.jetbrains.kotlin.psi.KtCallExpression import org.jetbrains.kotlin.psi.KtContainerNodeForControlStructureBody -import org.jetbrains.kotlin.psi.KtIfExpression import org.jetbrains.kotlin.psi.KtThrowExpression import org.jetbrains.kotlin.psi.KtValueArgument import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType @@ -21,5 +21,9 @@ val KtThrowExpression.arguments: List get() = findDescendantOfType()?.valueArguments.orEmpty() fun KtThrowExpression.isEnclosedByConditionalStatement(): Boolean { - return parent is KtIfExpression || parent is KtContainerNodeForControlStructureBody + return when (parent) { + is KtContainerNodeForControlStructureBody -> true + is KtBlockExpression -> parent.parent is KtContainerNodeForControlStructureBody + else -> false + } } diff --git a/detekt-psi-utils/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensionsSpec.kt b/detekt-psi-utils/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensionsSpec.kt new file mode 100644 index 00000000000..a0dc4b0839d --- /dev/null +++ b/detekt-psi-utils/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensionsSpec.kt @@ -0,0 +1,130 @@ +package io.gitlab.arturbosch.detekt.rules + +import io.github.detekt.test.utils.compileContentForTest +import org.assertj.core.api.Assertions.assertThat +import org.intellij.lang.annotations.Language +import org.jetbrains.kotlin.psi.KtThrowExpression +import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test + +internal class ThrowExtensionsSpec { + + @Nested + inner class `is enclosed by conditional statement` { + @Test + fun `is true for if statement on same line`() { + val code = """ + fun test() { + if (i == 1) throw IllegalArgumentException() + } + """ + + verifyThrowExpression(code) { + assertThat(isEnclosedByConditionalStatement()).isTrue() + } + } + + @Test + fun `is true for if statement on separate line`() { + val code = """ + fun test() { + if (i == 1) + throw IllegalArgumentException() + } + """ + + verifyThrowExpression(code) { + assertThat(isEnclosedByConditionalStatement()).isTrue() + } + } + + @Test + fun `is true for if statement on in block`() { + val code = """ + fun test() { + if (i == 1) { + println("message") + throw IllegalArgumentException() + } + } + """ + + verifyThrowExpression(code) { + assertThat(isEnclosedByConditionalStatement()).isTrue() + } + } + + @Test + fun `is false if thrown unconditionally`() { + val code = """ + fun test() { + throw IllegalArgumentException() + } + """ + + verifyThrowExpression(code) { + assertThat(isEnclosedByConditionalStatement()).isFalse() + } + } + + @Test + fun `is false for when statement`() { + val code = """ + fun test(a: Int) { + when (a) { + 1 -> throw IllegalArgumentException() + 2 -> println("2") + else -> println("other") + } + } + """ + + verifyThrowExpression(code) { + assertThat(isEnclosedByConditionalStatement()).isFalse() + } + } + + @Test + fun `is true in else clause`() { + val code = """ + fun test(a: Int) { + if (a == 2) println("2") else throw IllegalArgumentException() + } + """ + + verifyThrowExpression(code) { + assertThat(isEnclosedByConditionalStatement()).isTrue() + } + } + + @Test + fun `is true in else clause with curly braces`() { + val code = """ + fun test(a: Int) { + if (a == 2) { + println("2") + } else { + throw IllegalArgumentException() + } + } + """ + + verifyThrowExpression(code) { + assertThat(isEnclosedByConditionalStatement()).isTrue() + } + } + } + + private fun verifyThrowExpression( + @Language("kotlin") code: String, + throwingAssertions: KtThrowExpression.() -> Unit + ) { + val ktFile = compileContentForTest(code) + val ktThrowExpression = ktFile.findDescendantOfType() + assertThat(ktThrowExpression) + .withFailMessage("no throw expression found") + .isNotNull + throwingAssertions(ktThrowExpression!!) + } +} diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequire.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequire.kt index 16ce755f472..94bf1985ffe 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequire.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequire.kt @@ -13,7 +13,6 @@ import io.gitlab.arturbosch.detekt.rules.isEmptyOrSingleStringArgument import io.gitlab.arturbosch.detekt.rules.isEnclosedByConditionalStatement import io.gitlab.arturbosch.detekt.rules.isIllegalArgumentException import org.jetbrains.kotlin.psi.KtBlockExpression -import org.jetbrains.kotlin.psi.KtNamedFunction import org.jetbrains.kotlin.psi.KtThrowExpression /** @@ -42,8 +41,7 @@ class UseRequire(config: Config = Config.empty) : Rule(config) { override fun visitThrowExpression(expression: KtThrowExpression) { if (!expression.isIllegalArgumentException()) return - - if (expression.isOnlyExpressionInBlock()) return + if (expression.hasMoreExpressionsInBlock()) return if (expression.isEnclosedByConditionalStatement() && expression.arguments.isEmptyOrSingleStringArgument(bindingContext) @@ -52,11 +50,6 @@ class UseRequire(config: Config = Config.empty) : Rule(config) { } } - private fun KtThrowExpression.isOnlyExpressionInBlock(): Boolean { - return when (val p = parent) { - is KtBlockExpression -> p.statements.size == 1 - is KtNamedFunction -> true - else -> false - } - } + private fun KtThrowExpression.hasMoreExpressionsInBlock(): Boolean = + (parent as? KtBlockExpression)?.run { statements.size > 1 } ?: false } diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseCheckOrErrorSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseCheckOrErrorSpec.kt index 3e67126601f..6178cb24e54 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseCheckOrErrorSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseCheckOrErrorSpec.kt @@ -25,6 +25,19 @@ class UseCheckOrErrorSpec(val env: KotlinCoreEnvironment) { assertThat(subject.lint(code)).hasStartSourceLocation(3, 16) } + @Test + fun `reports if a an IllegalStateException is thrown conditionally in a block`() { + val code = """ + fun x() { + doSomething() + if (a < 0) { + throw IllegalStateException() + } + } + """ + assertThat(subject.lint(code)).hasStartSourceLocation(4, 9) + } + @Test fun `reports if a an IllegalStateException is thrown with an error message`() { val code = """ diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequireSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequireSpec.kt index a726d9e96c9..f7306b45420 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequireSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequireSpec.kt @@ -25,6 +25,20 @@ class UseRequireSpec(val env: KotlinCoreEnvironment) { assertThat(subject.lint(code)).hasStartSourceLocation(2, 16) } + @Test + fun `reports if a precondition throws an IllegalArgumentException as the only statement in block expression`() { + val code = """ + fun x(a: Int) { + if (a < 0) { + throw IllegalArgumentException() + } + doSomething() + } + """ + + assertThat(subject.lint(code)).hasStartSourceLocation(3, 9) + } + @Test fun `reports if a precondition throws an IllegalArgumentException with more details`() { val code = """ @@ -80,6 +94,21 @@ class UseRequireSpec(val env: KotlinCoreEnvironment) { assertThat(subject.lint(code)).isEmpty() } + @Test + fun `does not report an issue if the exceptions in thrown with more than one statement in block expression`() { + val code = """ + fun x(a: Int) { + if (a < 0) { + println("bang!") + throw IllegalArgumentException() + } + doSomething() + } + """ + + assertThat(subject.lint(code)).isEmpty() + } + @Test fun `does not report an issue if the exception thrown as the only action in a block`() { val code = """