Hello All! I've published a ruleset that ensures c...

# detekt

[Rob]

Hello All!
I've published a ruleset that ensures correct handling of coroutine cancellation in catch blocks. Please try it out and provide feedback. Thanks!
https://github.com/zirman/robs-rules/

[Brais Gabin]

Those rules look really good. I think they could be in the default rule set of detekt. Would you like that? If so open an issue and we can talk about the fit because we have already a rule that does something similar but it's not as powerful as yours.

[Rob]

I think it would be great if it was in the default set. I did use one of the default rules as a base to get started.

[Sebastian Schuberth]

Wouldn't they better fit to https://detekt.dev/docs/rules/coroutines?

[Sebastian Schuberth]

Also, note there's already https://detekt.dev/docs/rules/coroutines#suspendfunswallowedcancellation.

[Rob]

I remember seeing those before but thought they were too restrictive. At work we call suspend fun in runCatching everywhere. I wanted a more flexible option that makes sure we check for cancellation if there is a suspend call inside a catching block.

[Sebastian Schuberth]

I see. I was just quickly checking, and both that rule and yours seem to flag exactly the same code in our code base.

[Rob]

I see that it's very similar. With the ruleset I made, it is more opinionated about how to correctly handle catching CoroutineCancellation. It requires calling ensureActive() in the catch block or onFailure instead of other work arounds.
That is what was recommended by @elizarov in this thread.

[Rob]

I've made some changes to the rule to fail only when there is a suspend call inside the try block but I have yet to publish those changes.

[Rob]

This should have fewer false positives.

[Sebastian Schuberth]

Should the rules /docs also be updated to require the if (e is CancellationException) pattern as explained here?

[Sebastian Schuberth]

Currently, the rules are still triggered when having that conditional before currentCoroutineContext().ensureActive().

[Rob]

Yes, I wanted to make sure the coroutine is still active before doing anything. Can you give me a example use case where an if statement would be needed before the call to ensureActive()?

[Sebastian Schuberth]

AFAIU it's not needed, but recommended:

The pattern without if (e is CancellationException) isn't in line with the prompt cancellation guarantee that a lot of our functions give.

[Rob]

I see. Yeah, I can extend it to accept both or require the check.