Better Support for Guard Clauses in ThrowsCount Rule #2876

amitd291 · 2020-07-15T18:46:14Z

This PR achieves the following:

Make GuardClauses extension functions generic to support both return and throw expressions
Fix ThrowsCount scenarios related to guard clauses and add tests for the same
Reuse GuardClauses code for ThrowsCount rule

…xpressions, fix throw count scenarios related to guard clauses and add tests, reuse guard clause code for throw count rule

…checking for ELVIS guard clause

codecov · 2020-07-15T18:57:11Z

Codecov Report

Merging #2876 into master will decrease coverage by 0.11%.
The diff coverage is 63.63%.

@@             Coverage Diff              @@
##             master    #2876      +/-   ##
============================================
- Coverage     80.28%   80.17%   -0.12%     
  Complexity     2417     2417              
============================================
  Files           421      421              
  Lines          7359     7358       -1     
  Branches       1346     1343       -3     
============================================
- Hits           5908     5899       -9     
- Misses          756      764       +8     
  Partials        695      695

Impacted Files	Coverage Δ	Complexity Δ
.../io/gitlab/arturbosch/detekt/rules/GuardClauses.kt	`25.00% <30.00%> (-62.50%)`	`0.00 <0.00> (ø)`
...itlab/arturbosch/detekt/rules/style/ThrowsCount.kt	`95.45% <90.90%> (-4.55%)`	`6.00 <1.00> (-1.00)`
...itlab/arturbosch/detekt/rules/style/ReturnCount.kt	`95.12% <100.00%> (ø)`	`11.00 <0.00> (+1.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 5a0fe62...b5b838b. Read the comment docs.

arturbosch · 2020-07-15T20:42:16Z

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

+                }
+                throw Exception()
+            }
+        """


It looks like all test code templates need 4 extra spaces

@arturbosch good catch, I have fixed it.

arturbosch · 2020-07-15T20:42:33Z

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

+
+                    throw Exception()
+                }
+            """


Here it is right

arturbosch · 2020-07-15T20:42:52Z

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

-        } else {
-            if (!it.isGuardClause()) {
-                firstNonGuardFound = true
+inline fun <reified T : KtExpression> KtNamedFunction.yieldStatementsSkippingGuardClauses(): Sequence<KtExpression> =


Should this return T instead?

@arturbosch sorry if it was not clear, but the generic type T is for the child expression which can be either return or throw. These expressions can be of the sub-types KtReturnExpression or KtThrowExpression. The generic finds its purpose in the following line of code:

val descendantExpr = this.findDescendantOfType<T>() ?: return false

And on the consumer side the code looks like the following:

// in ReturnCount.kt yieldStatementsSkippingGuardClauses<KtReturnExpression>() // in ThrowsCount.kt yieldStatementsSkippingGuardClauses<KtThrowExpression>()

Whereas the return type of yieldStatementsSkippingGuardClauses method is a Sequence of parent statements, which is always of the type KtExpression.

Thanks for the explanation.

schalkms · 2020-07-15T20:54:10Z

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

+            } else {
+                if (!it.isGuardClause<T>()) {
+                    firstNonGuardFound = true
+                    yield(it)
+                }
            }


Suggested change

} else {

if (!it.isGuardClause<T>()) {

firstNonGuardFound = true

yield(it)

}

}

} else if (!it.isGuardClause<T>()) {

firstNonGuardFound = true

yield(it)

}

@schalkms thanks for the suggestion, it has been applied.

schalkms · 2020-07-15T20:58:30Z

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

 }

 fun KtExpression.isElvisOperatorGuardClause(): Boolean {
-    val elvisExpr = this.parent as? KtBinaryExpression
-    return elvisExpr?.operationToken == KtTokens.ELVIS
+    val elvisExpr = this.findDescendantOfType<KtBinaryExpression>() ?: return false


I'm not sure if I really understand this change. Could you please shed some light into this?

@schalkms
Earlier isElvisOperatorGuardClause was being called as an extension for the child return expression, and now I have modified it to be called as an extension on the parent statement (which contains the return / throw child expression)
Firstly it makes it generic for both return and throw expressions, as it is called on the parent. And secondly it sounds more meaningful, as the the parent statement is a guard clause (and not the child).

amitd291 · 2020-07-16T07:56:05Z

@arturbosch @schalkms
I have a question on the code coverage check. No matter how many spek test cases I try to add (exhaustively covering most of the code branches), it always says coverage has degraded. Any hints on how to fix this?

arturbosch · 2020-07-16T13:03:26Z

@amitdash291 for GuardClauses it seems that jacoco does not probably instrument the bytecode. Maybe some edge case for Kotlin sequence generators. Their Kotlin support is still fresh.
Sometimes we also have some problems with CI caching ^^.
PR looks fine, I'll merge it, thanks!

amitd291 · 2020-07-16T13:07:23Z

@amitdash291 for GuardClauses it seems that jacoco does not probably instrument the bytecode. Maybe some edge case for Kotlin sequence generators. Their Kotlin support is still fresh.
Sometimes we also have some problems with CI caching ^^.
PR looks fine, I'll merge it, thanks!

@arturbosch got it, the clarification about coverage issue.
Thanks for merging this PR 👍

amitd291 added 4 commits July 15, 2020 23:49

Make GuardClause functions generic to support both return and throw e…

eb2b950

…xpressions, fix throw count scenarios related to guard clauses and add tests, reuse guard clause code for throw count rule

Use statement as the reference rather than the return expression for …

73c8719

…checking for ELVIS guard clause

Format GuardClauses file

920a8dc

Merge remote-tracking branch 'upstream/master'

2a9f3e7

amitd291 changed the title ~~Better Support for Guard Clauses in ThrowCount~~ Better Support for Guard Clauses in ThrowsCount Rule Jul 15, 2020

Add test cases

48c5a12

arturbosch reviewed Jul 15, 2020

View reviewed changes

arturbosch approved these changes Jul 15, 2020

View reviewed changes

arturbosch added this to the 1.11.0 milestone Jul 15, 2020

schalkms approved these changes Jul 15, 2020

View reviewed changes

amitd291 added 2 commits July 16, 2020 08:02

Fix test templating

d8c3bb5

Cleanup GuardClauses as per PR suggestions

65f73b6

schalkms approved these changes Jul 16, 2020

View reviewed changes

amitd291 added 2 commits July 16, 2020 13:47

Add test cases for function with empty body

e7cc6af

Add test cases for function without body

b5b838b

arturbosch merged commit 2de31f1 into detekt:master Jul 16, 2020

arturbosch modified the milestones: 1.11.0, 1.10.1 Aug 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better Support for Guard Clauses in ThrowsCount Rule #2876

Better Support for Guard Clauses in ThrowsCount Rule #2876

amitd291 commented Jul 15, 2020 •

edited

Loading

codecov bot commented Jul 15, 2020 •

edited

Loading

arturbosch Jul 15, 2020

amitd291 Jul 16, 2020

arturbosch Jul 15, 2020

arturbosch Jul 15, 2020

amitd291 Jul 16, 2020 •

edited

Loading

arturbosch Jul 16, 2020

schalkms Jul 15, 2020

amitd291 Jul 16, 2020

schalkms Jul 15, 2020

amitd291 Jul 16, 2020 •

edited

Loading

amitd291 commented Jul 16, 2020 •

edited

Loading

arturbosch commented Jul 16, 2020 •

edited

Loading

amitd291 commented Jul 16, 2020

Better Support for Guard Clauses in ThrowsCount Rule #2876

Better Support for Guard Clauses in ThrowsCount Rule #2876

Conversation

amitd291 commented Jul 15, 2020 • edited Loading

codecov bot commented Jul 15, 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

amitd291 Jul 16, 2020 • edited Loading

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

amitd291 Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

amitd291 commented Jul 16, 2020 • edited Loading

arturbosch commented Jul 16, 2020 • edited Loading

amitd291 commented Jul 16, 2020

amitd291 commented Jul 15, 2020 •

edited

Loading

codecov bot commented Jul 15, 2020 •

edited

Loading

amitd291 Jul 16, 2020 •

edited

Loading

amitd291 Jul 16, 2020 •

edited

Loading

amitd291 commented Jul 16, 2020 •

edited

Loading

arturbosch commented Jul 16, 2020 •

edited

Loading