From 2c75b65a688211c59595d6b7bb48ce5251fec681 Mon Sep 17 00:00:00 2001 From: Andrei Paikin Date: Sat, 11 Dec 2021 02:05:37 +0300 Subject: [PATCH] Fix false positive in RethrowCaughtException for try with more than one catch (#4367) --- .../exceptions/RethrowCaughtException.kt | 35 ++++++-- .../exceptions/RethrowCaughtExceptionSpec.kt | 88 +++++++++++++++++++ 2 files changed, 115 insertions(+), 8 deletions(-) diff --git a/detekt-rules-exceptions/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtException.kt b/detekt-rules-exceptions/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtException.kt index 36bd0cecdd1e..d98c1f763bbd 100644 --- a/detekt-rules-exceptions/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtException.kt +++ b/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. * * * 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) + * } * } * */ @@ -57,13 +69,20 @@ 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() + catchClauses.map { violatingThrowExpressionFrom(it) } + .takeLastWhile { it != null } + .forEach { 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 } } diff --git a/detekt-rules-exceptions/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtExceptionSpec.kt b/detekt-rules-exceptions/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtExceptionSpec.kt index c62cbb5e4dfb..e8811b76c531 100644 --- a/detekt-rules-exceptions/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/exceptions/RethrowCaughtExceptionSpec.kt +++ b/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) + assertThat(result[1].startPosition.line == 7) + } } })