Improve SuspendFunSwallowedCancellation#9208
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (36.66%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #9208 +/- ##
============================================
- Coverage 84.92% 84.74% -0.19%
- Complexity 4427 4439 +12
============================================
Files 570 569 -1
Lines 12299 12311 +12
Branches 2694 2720 +26
============================================
- Hits 10445 10433 -12
- Misses 683 692 +9
- Partials 1171 1186 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hm I just noticed that this case was not covered: runCatching {
suspendCall()
}.onFailure { error ->
if (error is CancellationException) currentCoroutineContext().ensureActive()
}.getOrNull()I wonder, should we cover it? It seems pretty complex as |
| val exceptionType = typeReference?.type | ||
|
|
||
| val cancellationExceptionType = findTypeAlias(CANCELLATION_EXCEPTION_CLASS_ID)?.defaultType | ||
| ?: error("Cancellation exception type not found. Are coroutines on the classpath?") |
There was a problem hiding this comment.
We can't do this. Someone could has 4 modules in their project, enables the coroutine rule set at all of them but only use coroutines in 3 of them. The forth would always fail.
If we can't find it we should assume that everything is correct.
There was a problem hiding this comment.
Ah, good point. Fixed.
| } | ||
|
|
||
| @Test | ||
| fun `does not report when using ensureActive approach to handle cancellation`() { |
There was a problem hiding this comment.
Could you merge this test and the following one using @ParametrizedTest? They are really similar.
|
|
||
| val ensureActiveExpression = ifAtStart.then as? KtDotQualifiedExpression ?: return false | ||
|
|
||
| return ( |
There was a problem hiding this comment.
To me it's slightly cleaner that way, but not by much. Remove.d
|
|
||
| override fun visitTryExpression(expression: KtTryExpression) { | ||
| super.visitTryExpression(expression) | ||
| analyze(expression) { |
There was a problem hiding this comment.
Wow, this is a bad idea. analyze is really slow. Can we reduce its scope?
There was a problem hiding this comment.
I have moved it to the nested function, but I'm wondering how is analyze cached? Because it's called anyway in the isSuspend() a little further down and now for every catch clause (inside for loop).
Is analysis cached or is every analyze() call slow? If later, it might be more efficient to just call it once on the top level, rather than analyzing multiple times.
PR does three things:
SuspendFunSwallowedCancellation#8902Throwableclass #9190 (comment))CancellationExceptionfromInstanceOfCheckForExceptionrule (otherwiseensureActiveway of handling cancellation just triggers that rule instead)