Add SuspendFunSwallowedCancellation rule

[atulgpt]

Fixes #5468

More details:

This PR tries to detekt, all the suspend fun inside a runCatching block. Currently, I am finding each suspension point(suspend fun) inside the runCatching block and then for each suspension point trying to backpropagate if I can reach back to the original runCatching block or not.
Cases, where I am not able to reach, are as follows (these cases will not be reported a code smell for the current runCatching block)

1. If the suspension point is called by lambda which is not an inline function
2. If suspension point is called by fun which is inline but lambda, where that suspension point is present, is not inlined lambda(in other words lambda is marked with noinline or crossinline)

If any of the above points gets encountered, I break the backpropagation and then check if the returned function descriptor is the same as the current runCatching block(I am not checking against fqName as there could be multiple nested runCatching and then deeper suspension point will be reported against all the parent runCatching)

I have created this draft PR for early feedback and suggestion on improving the propagation around TCs. Although I agree that I am propagation a runCatching..suspend fun from top to bottom then bottom to top, I want to know if should I optimise this(do this in single propagation(This will require custom propagation of child or some other logic)) or is it okay. I'm fine either way

Update:
Now in the commit, I have simplified the traversal logic and only going inside a function for traversal when that function is inline or value parameter of the function is not marked noinline or crossinline(This helped in preventing the backpropagation from the suspension point)

[github-actions]

	Messages
📖	Thanks for adding a new rule to detekt ❤️
✅	We detekted that you added tests, to your rule, that's awesome!
✅	We detekted that you updated the default-detekt-config.yml file, that's awesome!

Generated by 🚫 dangerJS against d7e1309

[TWiStErRob]

Really nice, looks pretty advanced, looking at the issue description I didn't expect this complexity.

[TWiStErRob]

Feels like the code is getting shorter, but definitely simpler, nice one!

[atulgpt]

Feels like the code is getting shorter, but definitely simpler, nice one!

Thanks

[codecov]

Codecov Report

Merging #5666 (d7e1309) into main (6d7e7e6) will decrease coverage by 0.01%.
The diff coverage is 82.69%.

@@             Coverage Diff              @@
##               main    #5666      +/-   ##
============================================
- Coverage     84.59%   84.58%   -0.01%     
- Complexity     3790     3816      +26     
============================================
  Files           546      547       +1     
  Lines         12918    12969      +51     
  Branches       2268     2286      +18     
============================================
+ Hits          10928    10970      +42     
- Misses          861      862       +1     
- Partials       1129     1137       +8     

Impacted Files	Coverage Δ
...ules/coroutines/SuspendFunSwallowedCancellation.kt	82.00% <82.00%> (ø)
...osch/detekt/rules/coroutines/CoroutinesProvider.kt	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[BraisGabin]

I was out for some time and there are a lot of comments on these PR. Is there any blockers to merge this? Can I help to unblock something?

To me the tests feel a bit too long but this rule is a clear win for our users so we can improve that in later PRs. I mean, this implements just "half" issue. The runCatching thing. We need to implement the try/catch too.

[atulgpt]

Hi @BraisGabin try-catch was agreed to be implemented as a separate PR(that's why the generic name SuspendFunSwallowedCancellation). Your input Is needed for the comment(context: Currently I am allowing false negative while addressing that comment will favour false positive instead of false negative) cc @TWiStErRob

[TWiStErRob]

@BraisGabin note: unresolved conversations are unresolved, hence the name ;)

[BraisGabin]

LGTM

[atulgpt]

@BraisGabin note: unresolved conversations are unresolved, hence the name ;)

Hi @TWiStErRob can you check once again? I have added the TC as well for your case and it is passing

[TWiStErRob]

I think it's sufficiently covered with tests, so let's release it and see if false positives/negatives come up. I would add a note in the issue description something like:

* Note: This is a very complex topic, so there are ought to be
* false positive/false negative findings, if you come across one,
* please report in the [detekt issue tracker](https://github.com/detekt/detekt/issues).

I know this is the default expected behavior, but in the case of this rule, it's even more important to not just "suppress it in my case", but discuss usage patterns we didn't think of.

[cortinico]

I think it's sufficiently covered with tests, so let's release it and see if false positives/negatives come up.

Merging as we want this included in 1.23 👍