Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UnreachableCatchBlock rule #3478

Merged
merged 2 commits into from Feb 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Expand Up @@ -524,6 +524,8 @@ potential-bugs:
active: true
UnnecessarySafeCall:
active: true
UnreachableCatchBlock:
active: false
UnreachableCode:
active: true
UnsafeCallOnNullableType:
Expand Down
Expand Up @@ -42,7 +42,8 @@ class PotentialBugProvider : DefaultRuleSetProvider {
WrongEqualsTypeParameter(config),
IgnoredReturnValue(config),
ImplicitUnitReturnType(config),
NullableToStringCall(config)
NullableToStringCall(config),
UnreachableCatchBlock(config)
)
)
}
@@ -0,0 +1,80 @@
package io.gitlab.arturbosch.detekt.rules.bugs

import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Debt
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.rules.safeAs
import org.jetbrains.kotlin.descriptors.ClassDescriptor
import org.jetbrains.kotlin.psi.KtCatchClause
import org.jetbrains.kotlin.psi.KtTryExpression
import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.resolve.descriptorUtil.isSubclassOf

/**
* Reports unreachable catch blocks.
* Catch blocks can be unreachable if the exception has already been caught in the block above.
*
* <noncompliant>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to add an extended description and compliant code example in order to have a better understand of the rule from the user point of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* fun test() {
* try {
* foo()
* } catch (t: Throwable) {
* bar()
* } catch (e: Exception) {
* // Unreachable
* baz()
* }
* }
* </noncompliant>
*
* <compliant>
* fun test() {
* try {
* foo()
* } catch (e: Exception) {
* baz()
* } catch (t: Throwable) {
* bar()
* }
* }
* </compliant>
*
* @requiresTypeResolution
*/
class UnreachableCatchBlock(config: Config = Config.empty) : Rule(config) {
override val issue = Issue(
javaClass.simpleName,
Severity.Warning,
"Unreachable catch block detected.",
Debt.FIVE_MINS
)

@Suppress("ReturnCount")
override fun visitCatchSection(catchClause: KtCatchClause) {
super.visitCatchSection(catchClause)
if (bindingContext == BindingContext.EMPTY) return

val tryExpression = catchClause.getStrictParentOfType<KtTryExpression>() ?: return
val prevCatchClauses = tryExpression.catchClauses.takeWhile { it != catchClause }
if (prevCatchClauses.isEmpty()) return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if empty check even necessary considering the any statement underneath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid unnecessary catchClassDescriptor() call in the following case.

try {
} catch (e: Exception) {
}

val catchClassDescriptor = catchClause.catchClassDescriptor() ?: return
if (prevCatchClauses.any { catchClassDescriptor.isSubclassOf(it) }) {
report(CodeSmell(issue, Entity.from(catchClause), "This catch block is unreachable."))
}
}

private fun KtCatchClause.catchClassDescriptor(): ClassDescriptor? {
val typeReference = catchParameter?.typeReference ?: return null
return bindingContext[BindingContext.TYPE, typeReference]?.constructor?.declarationDescriptor?.safeAs()
}

private fun ClassDescriptor.isSubclassOf(catchClause: KtCatchClause): Boolean {
val catchClassDescriptor = catchClause.catchClassDescriptor() ?: return false
return isSubclassOf(catchClassDescriptor)
}
}
@@ -0,0 +1,77 @@
package io.gitlab.arturbosch.detekt.rules.bugs

import io.gitlab.arturbosch.detekt.api.SourceLocation
import io.gitlab.arturbosch.detekt.rules.setupKotlinEnvironment
import io.gitlab.arturbosch.detekt.test.assertThat
import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext
import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment
import org.spekframework.spek2.Spek
import org.spekframework.spek2.style.specification.describe

class UnreachableCatchBlockSpec : Spek({
setupKotlinEnvironment()
val env: KotlinCoreEnvironment by memoized()
val subject by memoized { UnreachableCatchBlock() }

describe("UnreachableCatchBlock rule") {
it("reports a unreachable catch block that is after the super class catch block") {
val code = """
fun test() {
try {
} catch (t: Throwable) {
} catch (e: Exception) {
}
}
"""
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
assertThat(findings).hasSourceLocation(4, 7)
}

it("reports a unreachable catch block that is after the same class catch block") {
val code = """
fun test() {
try {
} catch (e: Exception) {
} catch (e: Exception) {
}
}
"""
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
assertThat(findings).hasSourceLocation(4, 7)
}

it("reports two unreachable catch blocks that is after the super class catch block") {
val code = """
fun test() {
try {
} catch (e: RuntimeException) {
} catch (e: IllegalArgumentException) {
} catch (e: IllegalStateException) {
}
}
"""
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).hasSize(2)
assertThat(findings).hasSourceLocations(
SourceLocation(4, 7),
SourceLocation(5, 7)
)
}

it("does not report unreachable catch block") {
val code = """
fun test() {
try {
} catch (e: IllegalArgumentException) {
} catch (e: IllegalStateException) {
} catch (e: RuntimeException) {
}
}
"""
val findings = subject.compileAndLintWithContext(env, code)
assertThat(findings).isEmpty()
}
}
})