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
Fix ReturnCount false positive when excludeReturnFromLambda is enabled #5459
Changes from 2 commits
a86069a
0fd7f90
7549459
1b806d8
6e1d6aa
540634e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,7 @@ import io.gitlab.arturbosch.detekt.api.internal.Configuration | |
import io.gitlab.arturbosch.detekt.api.simplePatternToRegex | ||
import io.gitlab.arturbosch.detekt.rules.parentsOfTypeUntil | ||
import io.gitlab.arturbosch.detekt.rules.yieldStatementsSkippingGuardClauses | ||
import org.jetbrains.kotlin.psi.KtCallExpression | ||
import org.jetbrains.kotlin.psi.KtNameReferenceExpression | ||
import org.jetbrains.kotlin.psi.KtLambdaExpression | ||
import org.jetbrains.kotlin.psi.KtNamedFunction | ||
import org.jetbrains.kotlin.psi.KtReturnExpression | ||
import org.jetbrains.kotlin.psi.psiUtil.collectDescendantsOfType | ||
|
@@ -65,7 +64,7 @@ class ReturnCount(config: Config = Config.empty) : Rule(config) { | |
@Configuration("if labeled return statements should be ignored") | ||
private val excludeLabeled: Boolean by config(false) | ||
|
||
@Configuration("if labeled return from a lambda should be ignored") | ||
@Configuration("if labeled return from a lambda should be ignored (takes precedence over excludeLabeled.") | ||
private val excludeReturnFromLambda: Boolean by config(true) | ||
|
||
@Configuration("if true guard clauses at the beginning of a method should be ignored") | ||
|
@@ -94,8 +93,8 @@ class ReturnCount(config: Config = Config.empty) : Rule(config) { | |
|
||
private fun countReturnStatements(function: KtNamedFunction): Int { | ||
fun KtReturnExpression.isExcluded(): Boolean = when { | ||
excludeLabeled && labeledExpression != null -> true | ||
excludeReturnFromLambda && isNamedReturnFromLambda() -> true | ||
excludeLabeled && labeledExpression != null -> true | ||
else -> false | ||
} | ||
|
||
|
@@ -113,11 +112,9 @@ class ReturnCount(config: Config = Config.empty) : Rule(config) { | |
private fun KtReturnExpression.isNamedReturnFromLambda(): Boolean { | ||
val label = this.labeledExpression | ||
if (label != null) { | ||
return this.parentsOfTypeUntil<KtCallExpression, KtNamedFunction>() | ||
.map { it.calleeExpression } | ||
.filterIsInstance<KtNameReferenceExpression>() | ||
.map { it.text } | ||
.any { it in label.text } | ||
return this.parentsOfTypeUntil<KtLambdaExpression, KtNamedFunction>() | ||
.none() | ||
.not() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .none().not() is sort of confusing here. Can we rewrite with any or count() >= 1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a Sequence, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
return false | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, the configurations for excluding seem to be
or
conditions. (The added explanation is only making more confusion for the user). So I revert it.1b806d8