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.

Sign up for GitHub

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

Jump to bottom

Reduce LoopWithTooManyJumpStatements finding scope #6110

Merged
merged 2 commits into from May 20, 2023
Merged

Reduce LoopWithTooManyJumpStatements finding scope #6110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
005ca5a
Split up LoopWithTooManyJumpStatementsSpec into separate test cases
TWiStErRob May 19, 2023
1d532a7
Report loop problems on main loop keyword.
TWiStErRob May 19, 2023
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
27 changes: 26 additions & 1 deletion .../src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/LoopWithTooManyJumpStatements.kt
View file
Open in desktop
Expand Up @@ -11,11 +11,17 @@
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 @@

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 @@
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

Check warning on line 91 in detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/LoopWithTooManyJumpStatements.kt

View check run for this annotation

Codecov / codecov/patch

detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/LoopWithTooManyJumpStatements.kt#L91 

Added line #L91 was not covered by tests
}

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

private val KtWhileExpression.whileKeyword: PsiElement?
get() = KtPsiUtil.findChildByType(this, KtTokens.WHILE_KEYWORD)
125 changes: 88 additions & 37 deletions .../test/kotlin/io/gitlab/arturbosch/detekt/rules/style/LoopWithTooManyJumpStatementsSpec.kt
View file
Open in desktop
@@ -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()
}
}