GuardCluase also matches if-with-body that contains a return #2671

pinkasey · 2020-05-10T09:36:28Z

@arturbosch
I thought opening a PR was better than opening an Issue.
Although, I didn't properly test it yet - I'm new to gradle and Spek.
If you prefere, I can open an issue instead...

What I want -
to consider an if with a body as guard clause as well.
I've added a test-case to demonstrate.

In my company, we almost disabled ReturnCount because it catches guard-clauses, and then I saw guard-clauses - and was disappointed it applies only to ifs without a body.
Our code-style convention don't allow if's without body.

…ns a return as guard-clasue as well

pinkasey · 2020-05-10T09:37:53Z

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ReturnCountSpec.kt

+        context("a file with an if condition guard clause with body and 2 returns") {
+            val code = """
+            fun test(x: Int): Int {
+                if (x < 4) {


I want this to count as a GuardClause.
We have many of these in our code.

In fact, many consider it a bad practice to have an if with no body, so they'll never enjoy the benefit of the GuardClause feature.

codecov · 2020-05-10T09:45:58Z

Codecov Report

Merging #2671 into master will decrease coverage by 0.48%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master    #2671      +/-   ##
============================================
- Coverage     79.98%   79.50%   -0.49%     
+ Complexity     2307     2293      -14     
============================================
  Files           380      380              
  Lines          6781     6782       +1     
  Branches       1229     1229              
============================================
- Hits           5424     5392      -32     
- Misses          740      774      +34     
+ Partials        617      616       -1

Impacted Files	Coverage Δ	Complexity Δ
.../io/gitlab/arturbosch/detekt/rules/GuardClauses.kt	`82.35% <80.00%> (+7.35%)`	`0.00 <0.00> (ø)`
...kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt	`52.52% <100.00%> (-14.15%)`	`11.00 <1.00> (ø)`
...tlab/arturbosch/detekt/DetektCreateBaselineTask.kt	`0.00% <0.00%> (-48.39%)`	`0.00% <0.00%> (-12.00%)`
...tlab/arturbosch/detekt/DetektGenerateConfigTask.kt	`0.00% <0.00%> (-31.25%)`	`0.00% <0.00%> (-2.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02bf569...33306e8. Read the comment docs.

cortinico · 2020-05-10T10:42:53Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/GuardClauses.kt

@@ -24,8 +24,8 @@ fun KtNamedFunction.yieldStatementsSkippingGuardClauses(): Sequence<KtExpression

 fun KtExpression.isGuardClause(): Boolean {

-    fun KtReturnExpression.isIfConditionGuardClause(): Boolean {
-        val ifExpr = this.parent?.parent as? KtIfExpression
+    fun KtReturnExpression.isIfConditionGuardClause(ancestorExpression: KtExpression): Boolean {


Suggested change

fun KtReturnExpression.isIfConditionGuardClause(ancestorExpression: KtExpression): Boolean {

fun isIfConditionGuardClause(ancestorExpression: KtExpression): Boolean {

This can probably be refactored to not be an extension function?

I actually like it that way,
and anyway I don't feel comfortable to make that change to someone else's code 🙂

@cortinico is right here. As you are not using the receiver inside the nested method is it unused.
I would propose an additional change which checks if the return statement is actually the last statement inside the if:

fun KtExpression.isGuardClause(): Boolean { fun isIfConditionGuardClause(returnExpr: KtReturnExpression): Boolean { val ifExpr = this as? KtIfExpression return ifExpr != null && ifExpr.`else` == null && returnExpr === ifExpr.then?.lastBlockStatementOrThis() } fun KtReturnExpression.isElvisOperatorGuardClause(): Boolean { val elvisExpr = this.parent as? KtBinaryExpression return elvisExpr != null && elvisExpr.operationToken == KtTokens.ELVIS } val returnExpr = this.findDescendantOfType<KtReturnExpression>() ?: return false return isIfConditionGuardClause(returnExpr) || returnExpr.isElvisOperatorGuardClause() }

right
I didn't notice that I no longer use the receiver 😕
I'll change the code

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ReturnCountSpec.kt

…/style/ReturnCountSpec.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>

schalkms

I'm okay with that change, although we have to be careful here.
Therefore, this change should be properly tested. The following script is very useful in that regard.

Script reference

arturbosch

Very nice PR @pinkasey . We definitely missed out on checking the braces.
Please see my proposed change, thx!

arturbosch · 2020-05-10T21:20:41Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/GuardClauses.kt

@@ -24,8 +24,8 @@ fun KtNamedFunction.yieldStatementsSkippingGuardClauses(): Sequence<KtExpression

 fun KtExpression.isGuardClause(): Boolean {

-    fun KtReturnExpression.isIfConditionGuardClause(): Boolean {
-        val ifExpr = this.parent?.parent as? KtIfExpression
+    fun KtReturnExpression.isIfConditionGuardClause(ancestorExpression: KtExpression): Boolean {


@cortinico is right here. As you are not using the receiver inside the nested method is it unused.
I would propose an additional change which checks if the return statement is actually the last statement inside the if:

fun KtExpression.isGuardClause(): Boolean { fun isIfConditionGuardClause(returnExpr: KtReturnExpression): Boolean { val ifExpr = this as? KtIfExpression return ifExpr != null && ifExpr.`else` == null && returnExpr === ifExpr.then?.lastBlockStatementOrThis() } fun KtReturnExpression.isElvisOperatorGuardClause(): Boolean { val elvisExpr = this.parent as? KtBinaryExpression return elvisExpr != null && elvisExpr.operationToken == KtTokens.ELVIS } val returnExpr = this.findDescendantOfType<KtReturnExpression>() ?: return false return isIfConditionGuardClause(returnExpr) || returnExpr.isElvisOperatorGuardClause() }

pinkasey · 2020-05-11T15:46:16Z

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ReturnCountSpec.kt

@@ -89,7 +151,10 @@ class ReturnCountSpec : Spek({
                    if(a == null) return
                    val models = b as? Int ?: return
                    val position = c?.takeIf { it != -1 } ?: return
-                    if(b !is String) return
+                    if(b !is String) {


I thought -
why not include one with braces too

pinkasey · 2020-05-11T16:51:21Z

tests fail because my example code (which i copy-pasted from previous it) - did not compile.
I'll fix that, and the rest of the cases while I'm at it.
please wait with review...

pinkasey · 2020-05-11T17:17:19Z

OK
I've fixed my tests,
migrated all tests but the last one from lint() to compileAndLint().
Hope checks pass this time.

pinkasey · 2020-05-11T17:19:08Z

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ReturnCountSpec.kt

                    4 -> return 4
                }
+                return 6


These code-snippets didn't compile, because there wasn't a return in every execution path.

arturbosch

Thanks!

pinkasey · 2020-05-12T06:19:53Z

Thanks!

Thanks @arturbosch
for the good comments and encouragement!

Improve definition of GuardCluase - consider if with body that contai…

fbe2a65

…ns a return as guard-clasue as well

pinkasey commented May 10, 2020

View reviewed changes

cortinico requested changes May 10, 2020

View reviewed changes

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ReturnCountSpec.kt Outdated Show resolved Hide resolved

Update detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules…

8bb7ddd

…/style/ReturnCountSpec.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>

schalkms reviewed May 10, 2020

View reviewed changes

arturbosch requested changes May 10, 2020

View reviewed changes

Attended CR comments and added some tests

d11cc12

pinkasey commented May 11, 2020

View reviewed changes

pinkasey requested review from schalkms and arturbosch May 11, 2020 15:46

fix most compilation errors in ReturnCountSpec.kt's code-snippets

33306e8

pinkasey commented May 11, 2020

View reviewed changes

schalkms approved these changes May 11, 2020

View reviewed changes

arturbosch approved these changes May 11, 2020

View reviewed changes

arturbosch merged commit 47ef2cf into detekt:master May 11, 2020

pinkasey deleted the eyal_GuardClauses_with_body branch May 12, 2020 06:20

arturbosch added this to the 1.9.0 milestone May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GuardCluase also matches if-with-body that contains a return #2671

GuardCluase also matches if-with-body that contains a return #2671

pinkasey commented May 10, 2020 •

edited

Loading

pinkasey May 10, 2020

codecov bot commented May 10, 2020 •

edited

Loading

cortinico May 10, 2020

pinkasey May 10, 2020

schalkms May 10, 2020

arturbosch May 10, 2020

pinkasey May 11, 2020

schalkms left a comment

arturbosch left a comment

arturbosch May 10, 2020

pinkasey May 11, 2020

pinkasey commented May 11, 2020

pinkasey commented May 11, 2020

pinkasey May 11, 2020

arturbosch left a comment

pinkasey commented May 12, 2020 •

edited

Loading

	fun KtReturnExpression.isIfConditionGuardClause(ancestorExpression: KtExpression): Boolean {
	fun isIfConditionGuardClause(ancestorExpression: KtExpression): Boolean {

GuardCluase also matches if-with-body that contains a return #2671

GuardCluase also matches if-with-body that contains a return #2671

Conversation

pinkasey commented May 10, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented May 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkms left a comment

Choose a reason for hiding this comment

arturbosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinkasey commented May 11, 2020

pinkasey commented May 11, 2020

Choose a reason for hiding this comment

arturbosch left a comment

Choose a reason for hiding this comment

pinkasey commented May 12, 2020 • edited Loading

pinkasey commented May 10, 2020 •

edited

Loading

codecov bot commented May 10, 2020 •

edited

Loading

pinkasey commented May 12, 2020 •

edited

Loading