Skip to content

Commit

Permalink
Reduce LoopWithTooManyJumpStatements finding scope (#6110)
Browse files Browse the repository at this point in the history
  • Loading branch information
TWiStErRob committed May 20, 2023
1 parent f99ac58 commit 67d2c72
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 38 deletions.
Expand Up @@ -11,11 +11,17 @@ import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.config
import io.gitlab.arturbosch.detekt.api.internal.ActiveByDefault
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.KtBreakExpression
import org.jetbrains.kotlin.psi.KtContinueExpression
import org.jetbrains.kotlin.psi.KtDoWhileExpression
import org.jetbrains.kotlin.psi.KtElement
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtForExpression
import org.jetbrains.kotlin.psi.KtLoopExpression
import org.jetbrains.kotlin.psi.KtPsiUtil
import org.jetbrains.kotlin.psi.KtWhileExpression

/**
* Loops which contain multiple `break` or `continue` statements are hard to read and understand.
Expand Down Expand Up @@ -48,7 +54,7 @@ class LoopWithTooManyJumpStatements(config: Config = Config.empty) : Rule(config

override fun visitLoopExpression(loopExpression: KtLoopExpression) {
if (countBreakAndReturnStatements(loopExpression.body) > maxJumpCount) {
report(CodeSmell(issue, Entity.from(loopExpression), issue.description))
report(CodeSmell(issue, Entity.from(loopExpression.keyword ?: loopExpression), issue.description))
}
super.visitLoopExpression(loopExpression)
}
Expand All @@ -71,3 +77,22 @@ class LoopWithTooManyJumpStatements(config: Config = Config.empty) : Rule(config
return count
}
}

/**
* For some reason not all keyword properties are exposed on [KtLoopExpression] subclasses, so we have to do it manually.
*/
@Suppress("CommentOverPrivateProperty")
private val KtLoopExpression.keyword: PsiElement?
get() =
when (this) {
is KtForExpression -> this.forKeyword
is KtWhileExpression -> this.whileKeyword
is KtDoWhileExpression -> this.doKeyword
else -> null
}

private val KtDoWhileExpression.doKeyword: PsiElement?
get() = KtPsiUtil.findChildByType(this, KtTokens.DO_KEYWORD)

private val KtWhileExpression.whileKeyword: PsiElement?
get() = KtPsiUtil.findChildByType(this, KtTokens.WHILE_KEYWORD)
@@ -1,73 +1,124 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.gitlab.arturbosch.detekt.test.TestConfig
import io.gitlab.arturbosch.detekt.test.lint
import org.assertj.core.api.Assertions.assertThat
import io.gitlab.arturbosch.detekt.test.assertThat
import io.gitlab.arturbosch.detekt.test.compileAndLint
import org.junit.jupiter.api.Test

private const val MAX_JUMP_COUNT = "maxJumpCount"

class LoopWithTooManyJumpStatementsSpec {
val subject = LoopWithTooManyJumpStatements()
private val positiveCode = """
@Suppress("KotlinConstantConditions", "RedundantSuppression")
fun tooManyJumpStatements() {
val i = 0
// reports 1 - too many jump statements
for (j in 1..2) {
if (i > 1) {
break
} else {
continue

@Test
fun `reports for loops with more than 1 break or continue statement`() {
val code = """
fun f(i: Int) {
for (j in 1..2) {
if (i > 1) {
break
} else {
continue
}
}
}
// reports 1 - too many jump statements
while (i < 2) {
if (i > 1) break else continue
""".trimIndent()
assertThat(LoopWithTooManyJumpStatements().compileAndLint(code)).hasTextLocations(20 to 23)
}

@Test
fun `reports while loops with more than 1 break or continue statement`() {
val code = """
fun f(i: Int) {
while (i < 3) {
if (i > 1) break else continue
}
}
// reports 1 - too many jump statements
do {
if (i > 1) break else continue
} while (i < 2)
}
""".trimIndent()
""".trimIndent()
assertThat(LoopWithTooManyJumpStatements().compileAndLint(code)).hasTextLocations(20 to 25)
}

@Test
fun `reports loops with more than 1 break or continue statement`() {
assertThat(subject.lint(positiveCode)).hasSize(3)
fun `reports do loops with more than 1 break or continue statement`() {
val code = """
fun f(i: Int) {
do {
if (i > 2) break else continue
} while (i < 1)
}
""".trimIndent()
assertThat(LoopWithTooManyJumpStatements().compileAndLint(code)).hasTextLocations(20 to 22)
}

@Test
fun `does not report when max count configuration is set to 2`() {
val config = TestConfig(MAX_JUMP_COUNT to "2")
val findings = LoopWithTooManyJumpStatements(config).lint(positiveCode)
fun `does not report for loops when max count configuration is set to 2`() {
val code = """
fun f(i: Int) {
for (j in 1..2) {
if (i > 1) {
break
} else {
continue
}
}
}
""".trimIndent()
val findings = LoopWithTooManyJumpStatements(TestConfig(MAX_JUMP_COUNT to "2")).compileAndLint(code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report loop with less than 1 break or continue statement`() {
fun `does not report while loops when max count configuration is set to 2`() {
val code = """
fun f(i: Int) {
while (i < 3) {
if (i > 1) break else continue
}
}
""".trimIndent()
val findings = LoopWithTooManyJumpStatements(TestConfig(MAX_JUMP_COUNT to "2")).compileAndLint(code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report do loops when max count configuration is set to 2`() {
val code = """
fun f(i: Int) {
do {
if (i > 2) break else continue
} while (i < 1)
}
""".trimIndent()
val findings = LoopWithTooManyJumpStatements(TestConfig(MAX_JUMP_COUNT to "2")).compileAndLint(code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report for loop with less than 1 break statement`() {
val code = """
fun onlyOneJump() {
for (i in 1..2) {
if (i > 1) break
}
}
""".trimIndent()
val findings = LoopWithTooManyJumpStatements().compileAndLint(code)
assertThat(findings).isEmpty()
}

@Test
fun `does not report nested loop with less than 1 break or continue statement`() {
val code = """
fun jumpsInNestedLoops() {
for (i in 1..2) {
if (i > 1) break
for (i in 1..10) {
if (i > 5) break
// jump statements of the inner loop must not be counted in the outer loop
@Suppress("KotlinConstantConditions", "RedundantSuppression")
while (i > 1) {
while (i < 3) {
if (i > 1) continue
}
}
}
""".trimIndent()
val findings = subject.lint(code)
val findings = LoopWithTooManyJumpStatements().compileAndLint(code)
assertThat(findings).isEmpty()
}
}

0 comments on commit 67d2c72

Please sign in to comment.