Add new rule SleepInsteadOfDelay

[severn-everett]

Added new SleepInsteadOfDelay rule for checking for whether Thread.sleep() is being used in suspend functions and coroutines.

[codecov]

Codecov Report

Merging #3335 (22ba632) into master (77a2d7f) will decrease coverage by 0.18%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master    #3335      +/-   ##
============================================
- Coverage     80.44%   80.25%   -0.19%     
- Complexity     2721     2727       +6     
============================================
  Files           445      446       +1     
  Lines          8250     8262      +12     
  Branches       1566     1568       +2     
============================================
- Hits           6637     6631       -6     
- Misses          772      785      +13     
- Partials        841      846       +5     

Impacted Files	Coverage Δ	Complexity Δ
...sch/detekt/rules/coroutines/SleepInsteadOfDelay.kt	73.91% <73.91%> (ø)	10.00 <10.00> (?)
...osch/detekt/rules/coroutines/CoroutinesProvider.kt	100.00% <100.00%> (ø)	3.00 <0.00> (ø)
...lab/arturbosch/detekt/internal/ClassLoaderCache.kt	31.57% <0.00%> (-58.90%)	0.00% <0.00%> (ø%)
...ekt/rules/style/RedundantVisibilityModifierRule.kt	91.48% <0.00%> (-4.43%)	11.00% <0.00%> (-3.00%)
...io/github/detekt/report/sarif/SarifOutputReport.kt	90.62% <0.00%> (-4.12%)	4.00% <0.00%> (ø%)
.../io/gitlab/arturbosch/detekt/internal/DetektJvm.kt	57.69% <0.00%> (-1.57%)	4.00% <0.00%> (-1.00%)
...n/kotlin/io/github/detekt/report/sarif/SarifDsl.kt	100.00% <0.00%> (ø)	0.00% <0.00%> (ø%)
...tlab/arturbosch/detekt/core/reporting/Reporting.kt	94.11% <0.00%> (ø)	0.00% <0.00%> (ø%)
.../main/kotlin/io/github/detekt/parser/KtCompiler.kt	81.81% <0.00%> (+4.54%)	8.00% <0.00%> (ø%)
... and 1 more

---

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 a17b7f9...9d8d0dc. Read the comment docs.

[BraisGabin]

It looks good :) just one more test and it's good to go for my end.

[picklebento]

I think we should also check async here. This could be in a follow-up PR.

[cortinico]

Is sleepImported needed at all? You're checking the fully qualified name of the Thread.sleep function below (line 92) so it should be irrelevant if sleep was imported or not.

[severn-everett]

sleepImported is there to avoid having to search through all KtCallExpression descendents, given that there will only be dot-qualified expressions of Thread.sleep() if sleepImported is false.

[cortinico]

to avoid having to search through all KtCallExpression descendents

Are you trying to avoid this for performance reasons?

[severn-everett]

Suggestions for additional tests to increase code coverage would be welcome.

[severn-everett]

Yes, basically. If Thread.sleep isn't imported, then there wouldn't be any non dot-qualified expressions of the function, correct? In that case, the rule could skip parsing through expressions that it wouldn't have an expectation of finding the sleep() function. Op wo 6 jan. 2021 21:44 schreef Nicola Corti <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelay.kt <#3335 (comment)>: > + private var sleepImported = false + + override val issue = Issue( + javaClass.simpleName, + Severity.Defect, + "Usage of Thread.sleep() in coroutines can potentially halt multiple coroutines at once.", + Debt.FIVE_MINS + ) + + override fun postVisit(root: KtFile) { + super.postVisit(root) + sleepImported = false + } + + override fun visitImportDirective(importDirective: KtImportDirective) { + sleepImported = importDirective.importedFqName?.asString() in IMPORT_PATHS to avoid having to search through all KtCallExpression descendents Are you trying to avoid this for performance reasons? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3335 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC7SZ4IVJG4422X3XYQF6QDSYTDTRANCNFSM4VOIGSRA> .

[cortinico]

Yes, basically. If Thread.sleep isn't imported, then there wouldn't be any non dot-qualified expressions of the function, correct? In that case, the rule could skip parsing through expressions that it wouldn't have an expectation of finding the sleep() function.

I would say that in this case the readability outweighs the performance benefits. It was not immediately clear to me why you were interested in the checking if sleep() was imported or not.

I would simplify the checkDescendants function with

    private fun PsiElement.checkDescendants(message: String) {
        forEachDescendantOfType<KtCallExpression> { it.verifyExpression(message) }
        forEachDescendantOfType<KtDotQualifiedExpression> { it.verifyExpression(message) }
    }

You're anyway running verifyExpression for all the KtDotQualifiedExpression, I don't see the benefit of optionally skipping the KtCallExpression occurrences only. You can then remove all the postVisit and visitImportDirective code as well as the specific cases in the tests.

[cortinico]

LGTM

[mhernand40]

It seems that although 1.16.0 has not been released yet, this new rule has been published in https://detekt.github.io/detekt/coroutines.html#sleepinsteadofdelay.

For me, this led to

> Run failed with 1 invalid config property.
  	- Property 'coroutines>SleepInsteadOfDelay' is misspelled or does not exist.

When I unknowingly tried to enable this with version 1.15.0.

[cortinico]

It seems that although 1.16.0 has not been released yet, this new rule has been published in detekt.github.io/detekt/coroutines.html#sleepinsteadofdelay.

That's expected as the documentation website follows the status of the current master. If you want to use SleepInsteadOfDelay you can consider using our snapshot version https://detekt.github.io/detekt/snapshots.html or wait for 1.16.0 to be released.