Fix false positive in RethrowCaughtException for try with more than one catch (#4367)

detekt-rules-exceptions/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtException.kt

@@ -10,10 +10,14 @@ import io.gitlab.arturbosch.detekt.api.Severity
 import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
 import org.jetbrains.kotlin.psi.KtCatchClause
 import org.jetbrains.kotlin.psi.KtThrowExpression
+import org.jetbrains.kotlin.psi.KtTryExpression
+import org.jetbrains.kotlin.psi.psiUtil.getChildrenOfType
 
 /**
  * This rule reports all exceptions that are caught and then later re-thrown without modification.
- * It ignores caught exceptions that are rethrown if there is work done before that.
+ * It ignores cases:
+ * 1. When caught exceptions that are rethrown if there is work done before that.
+ * 2. When there are more than one catch in try block and at least one of them has some work.
  *
  * <noncompliant>
  * fun foo() {
@@ -44,6 +48,14 @@ import org.jetbrains.kotlin.psi.KtThrowExpression
  *         print(e.message)
  *         throw e
  *     }
+ *
+ *     try {
+ *         // ...
+ *     } catch (e: IOException) {
+ *         throw e
+ *     } catch (e: Exception) {
+ *         print(e.message)
+ *     }
  * }
  * </compliant>
  */
@@ -57,13 +69,24 @@ class RethrowCaughtException(config: Config = Config.empty) : Rule(config) {
         Debt.FIVE_MINS
     )
 
-    override fun visitCatchSection(catchClause: KtCatchClause) {
-        val exceptionName = catchClause.catchParameter?.name ?: return
-        val statements = catchClause.catchBody?.children ?: return
-        val throwExpression = statements.firstOrNull() as? KtThrowExpression
-        if (throwExpression != null && throwExpression.thrownExpression?.text == exceptionName) {
-            report(CodeSmell(issue, Entity.from(throwExpression), issue.description))
+    override fun visitTryExpression(tryExpr: KtTryExpression) {
+        val catchClauses = tryExpr.getChildrenOfType<KtCatchClause>()
+        catchClauses.map { violatingThrowExpressionFrom(it) }
+            .takeLastWhile { it != null }
+            .forEach { violation ->
+                violation?.let {
+                    report(CodeSmell(issue, Entity.from(it), issue.description))
+                }
+            }
+        super.visitTryExpression(tryExpr)
+    }
+
+    private fun violatingThrowExpressionFrom(catchClause: KtCatchClause): KtThrowExpression? {
+        val exceptionName = catchClause.catchParameter?.name
+        val throwExpression = catchClause.catchBody?.children?.firstOrNull() as? KtThrowExpression
+        if (throwExpression?.thrownExpression?.text == exceptionName) {
+            return throwExpression
         }
-        super.visitCatchSection(catchClause)
+        return null
     }
 }

detekt-rules-exceptions/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtExceptionSpec.kt

@@ -22,6 +22,21 @@ class RethrowCaughtExceptionSpec : Spek({
             assertThat(subject.compileAndLint(code)).hasSize(1)
         }
 
+        it("does not report when the other exception is rethrown with same name") {
+            val code = """
+                class A {
+                    private lateinit var e: Exception
+                    fun f() {
+                        try {
+                        } catch (e: IllegalStateException) {
+                            throw this.e
+                        }
+                    }
+                }
+            """
+            assertThat(subject.compileAndLint(code)).isEmpty()
+        }
+
         it("reports when the same exception succeeded by dead code is rethrown") {
             val code = """
                 fun f() {
@@ -108,5 +123,78 @@ class RethrowCaughtExceptionSpec : Spek({
             """
             assertThat(subject.compileAndLint(code)).isEmpty()
         }
+
+        it("does not report when exception rethrown only in first catch") {
+            val code = """
+                fun f() {
+                    try {
+                    } catch (e: IllegalStateException) {
+                        throw e
+                    } catch (e: Exception) {
+                        print(e)
+                    }
+                }
+            """
+            assertThat(subject.compileAndLint(code)).isEmpty()
+        }
+
+        it("does not report when some work is done in last catch") {
+            val code = """
+                fun f() {
+                    try {
+                    } catch (e: IllegalStateException) {
+                        throw e
+                    } catch (e: Exception) {
+                        print(e)
+                        throw e
+                    }
+                }
+            """
+            assertThat(subject.compileAndLint(code)).isEmpty()
+        }
+
+        it("does not report when there is no catch clauses") {
+            val code = """
+                fun f() {
+                    try {
+                    } finally {
+                    }
+                }
+            """
+            assertThat(subject.compileAndLint(code)).isEmpty()
+        }
+
+        it("reports when exception rethrown in last catch") {
+            val code = """
+                fun f() {
+                    try {
+                    } catch (e: IllegalStateException) {
+                        print(e)
+                    } catch (e: Exception) {
+                        throw e
+                    }
+                }
+            """
+            assertThat(subject.compileAndLint(code)).hasSize(1)
+        }
+
+        it("reports 2 violations for each catch") {
+            val code = """
+                fun f() {
+                    try {
+                    } catch (e: IllegalStateException) {
+                        throw e
+                    } catch (e: Exception) {
+                        // some comment
+                        throw e
+                    }
+                }
+            """
+            val result = subject.compileAndLint(code)
+            assertThat(result).hasSize(2)
+            // ensure correct violation order
+            assertThat(result[0].startPosition.line == 4).isTrue
+            assertThat(result[1].startPosition.line == 7).isTrue
+        }
     }
 })