Don't allow runCatching with supend functions inside

[BraisGabin]

Expected Behavior of the rule

Raise any usage of runCatching that calls a suspend function.

Example:

suspend fun foo(): String { /*...*/ }

suspend fun bar(): String? {
  return runCatching { foo() }.getOrNull()
}

Context

From the documentation:

While catching Exception is an anti-pattern, this issue may surface in more subtle ways, like when using the runCatching function, which does not rethrow CancellationException.

Extracted from: https://kotlinlang.org/docs/cancellation-and-timeouts.html#cancellation-is-cooperative

If you catch and don't rethrow all the CancellationException your coroutines are not cancelled even if you cancel their CoroutineScope. And that can create huge issues that are really difficult to fix because the errors and exceptions that you get are not related with that runCatching.

At work we had this issue for several months until we realised what was going on. We already have TooGenericExceptionCaught that works with try/catch but we don't have anything to spot this issue. At work we are considering to forbid runCatching all together but I think that at detekt we should have a rule to spot this.

It's true that you can be careful and check the type of the caught exception and rethrow it in case of CancellationException but that's far to easy to forget.

I think that this rule should be inside coroutines but potential-bugs could be also a good fit.

[TWiStErRob]

What's the solution to a violation like this?
runCatchingExceptCancellationExceptionWhichIsRethrown (with a better name ofc)?

[cortinico]

What's the solution to a violation like this?

I'm also a bit hesitant on forbidding runCatching for all sorts of suspend fun. It feels like we could limit its usage quite a lot.

[BraisGabin]

This is similar to forbid the usage of lateinit var. There are ways to use it properly. But it's risky. This would be the same.

runCacthing is the same as a try/catch(Throwable) and we all agree that the second one is bad.

What's the solution to a violation like this?

Don't use runCacthing. This issue leans to the controversial ones so if you don't agree with it I can create a custom rule. But this rule is backed by oficial documentation talking about this issue. For that reason I think that this rule is a good fit for the default rules of detekt.

[TWiStErRob]

Nicola, I agree with the report, I just didn't know how to fix it.

I think there's no question here whether runCatching should be allowed or not, with suspend. Based on my current understanding, when one uses runCatching { suspend ... }, they're doing it wrong, full stop ¹; as Brais summarized it really well above in OP.

cc @julia-martemianova @Zeliret as far as I remember you reach a similar conclusion before, right?

It's just that most projects don't know they doing it wrong, because it only matters in the resource usage patterns / edge cases / error cases. So it's really hard to monitor/track or even observe the bad effects directly. The bad behavior will come as a weird side-effect that's hard to trace back to this issue, so better nip it in the bud.

I agree that it needs to be very clear why it's problematic, and the rule needs strong guidance of good patterns how to solve this problem.

¹ As usual, exceptions apply (no puns intended): if one actually does checks the failure in the Result object is CancellationException and unwraps it correctly (challenge it itself) and then rethrows it in the right context, then it's all fine. But this requires extreme precision.

[cortinico]

I just didn't know how to fix it

If we go for this rule, then the solution for user would be to either:

1. Don't use runCatching { suspend ... } at all
2. I know what I'm doing with exception -> Local suppress

[TWiStErRob]

3. runCatchingExceptCancellationExceptionWhichIsRethrown (which is trivial to implement in any project, harder to enforce)

I wonder if there's an open issue for Kotlin/Coroutines library to address this in some way.

[BraisGabin]

3. runCatchingExceptCancellationExceptionWhichIsRethrown (which is trivial to implement in any project, harder to enforce)

That's my solution. Adding a @Suppress inside that one because there we will correctly manage the rethrow.

[atulgpt]

I have myself been hit by this issue a few times(and it's really hard to pinpoint. Although I started using a similar function like runCatchingExceptCancellationExceptionWhichIsRethrown which is hard to enforce as mentioned above).
I agree with @cortinico comment to make the rule simple

If we go for this rule, then the solution for user would be to either:
1. Don't use runCatching { suspend ... } at all
2. I know what I'm doing with exception -> Local suppress

But shouldn't we handle try-catch scenario, as well as not everyone, will have the TooGenericExceptionCaught enabled in their project? If we should then what should be the plan to handle try-catch?

[BraisGabin]

I would start with runCacthing. Later we can extend this rule adding the try/catch.

[TWiStErRob]

I agree with Atul, considering the problem is not runCatching itself, but rather how it hides the catching of CancellationException (or any other special exception), I would propose to simply add this handling to the TooGenericExceptionCaught rule with a functionNames: String[] setting. This would future proof and allow other custom (think Arrow library for example) functions to be detected.

Since there needs to be a specific handling for suspend fun, I think the end-result should be:

TooGenericExceptionCaught:
  exceptionNames: [...]
  functionNames: []
  suspendFunctionNames: ['runCatching']

because in the end your OP is essentially describing a false negative for TooGenericExceptionCaught. runCatching is just stdlib's way of saying try-catch. Allowing these two new settings would allow people to find all runCatching (and other similar method) usages with the right rule.

[BraisGabin]

I think that a rule for this case is a better option. We can then add logic to that rule to check the paths of the try catch and ensure that it doesn't catch the exception or it rethrow it directly.

I would not expand the current rule because that would make it way too complex to use.

[atulgpt]

Can I try this issue? I implemented one POC with few passing TCs