Fix false negative for UseRequire when thrown in conditional block (#…

…5147)

* 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 <braisgabin@gmail.com>

Co-authored-by: Markus Schwarz <post@markus-schwarz.net>
Co-authored-by: Brais Gabín <braisgabin@gmail.com>

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<KtValueArgument>
     get() = findDescendantOfType<KtCallExpression>()?.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
+    }
 }

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<KtThrowExpression>()
+        assertThat(ktThrowExpression)
+            .withFailMessage("no throw expression found")
+            .isNotNull
+        throwingAssertions(ktThrowExpression!!)
+    }
+}

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
 }

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 = """

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 = """