Add SuspendFunSwallowedCancellation rule (#5666)

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>

detekt-core/src/main/resources/default-detekt-config.yml

@@ -190,6 +190,8 @@ coroutines:
     active: true
   SleepInsteadOfDelay:
     active: true
+  SuspendFunSwallowedCancellation:
+    active: false
   SuspendFunWithCoroutineScopeReceiver:
     active: false
   SuspendFunWithFlowReturnType:

detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt

@@ -21,7 +21,8 @@ class CoroutinesProvider : DefaultRuleSetProvider {
             RedundantSuspendModifier(config),
             SleepInsteadOfDelay(config),
             SuspendFunWithFlowReturnType(config),
-            SuspendFunWithCoroutineScopeReceiver(config)
+            SuspendFunWithCoroutineScopeReceiver(config),
+            SuspendFunSwallowedCancellation(config),
         )
     )
 }

detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt

@@ -0,0 +1,188 @@
+package io.gitlab.arturbosch.detekt.rules.coroutines
+
+import io.gitlab.arturbosch.detekt.api.CodeSmell
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.Debt
+import io.gitlab.arturbosch.detekt.api.Entity
+import io.gitlab.arturbosch.detekt.api.Issue
+import io.gitlab.arturbosch.detekt.api.Rule
+import io.gitlab.arturbosch.detekt.api.Severity
+import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
+import org.jetbrains.kotlin.builtins.StandardNames.COROUTINES_PACKAGE_FQ_NAME
+import org.jetbrains.kotlin.com.intellij.psi.PsiElement
+import org.jetbrains.kotlin.descriptors.FunctionDescriptor
+import org.jetbrains.kotlin.descriptors.PropertyDescriptor
+import org.jetbrains.kotlin.name.FqName
+import org.jetbrains.kotlin.name.Name
+import org.jetbrains.kotlin.psi.KtCallExpression
+import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.KtForExpression
+import org.jetbrains.kotlin.psi.KtNameReferenceExpression
+import org.jetbrains.kotlin.psi.KtOperationExpression
+import org.jetbrains.kotlin.psi.KtValueArgument
+import org.jetbrains.kotlin.psi.psiUtil.anyDescendantOfType
+import org.jetbrains.kotlin.psi.psiUtil.getParentOfType
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.calls.util.getParameterForArgument
+import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall
+import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe
+import org.jetbrains.kotlin.utils.addToStdlib.ifTrue
+
+/**
+ * `suspend` functions should not be called inside `runCatching`'s lambda block, because `runCatching` catches all the
+ * `Exception`s. For Coroutines to work in all cases, developers should make sure to propagate `CancellationException`
+ * exceptions. This means `CancellationException` should never be:
+ * * caught and swallowed (even if logged)
+ * * caught and propagated to external systems
+ * * caught and shown to the user
+ *
+ * they must always be rethrown in the same context.
+ *
+ * Using `runCatching` increases this risk of mis-handling cancellation. If you catch and don't rethrow all the
+ * `CancellationException`, your coroutines are not cancelled even if you cancel their `CoroutineScope`.
+ *
+ * This can very easily lead to:
+ * * unexpected crashes
+ * * extremely hard to diagnose bugs
+ * * memory leaks
+ * * performance issues
+ * * battery drain
+ *
+ * See reference, [Kotlin doc](https://kotlinlang.org/docs/cancellation-and-timeouts.html#cancellation-is-cooperative).
+ *
+ * If your project wants to use `runCatching` and `Result` objects, it is recommended to write a `coRunCatching`
+ * utility function which immediately re-throws `CancellationException`; and forbid `runCatching` and `suspend`
+ * combinations by activating this rule.
+ *
+ * <noncompliant>
+ * @@Throws(IllegalStateException::class)
+ * suspend fun bar(delay: Long) {
+ *     check(delay <= 1_000L)
+ *     delay(delay)
+ * }
+ *
+ * suspend fun foo() {
+ *     runCatching {
+ *         bar(1_000L)
+ *     }
+ * }
+ * </noncompliant>
+ *
+ * <compliant>
+ * @@Throws(IllegalStateException::class)
+ * suspend fun bar(delay: Long) {
+ *     check(delay <= 1_000L)
+ *     delay(delay)
+ * }
+ *
+ * suspend fun foo() {
+ *     try {
+ *         bar(1_000L)
+ *     } catch (e: IllegalStateException) {
+ *         // handle error
+ *     }
+ * }
+ *
+ * // Alternate
+ * @@Throws(IllegalStateException::class)
+ * suspend fun foo() {
+ *     bar(1_000L)
+ * }
+ * </compliant>
+ *
+ */
+@RequiresTypeResolution
+class SuspendFunSwallowedCancellation(config: Config) : Rule(config) {
+    override val issue = Issue(
+        id = javaClass.simpleName,
+        severity = Severity.Minor,
+        description = "`runCatching` does not propagate `CancellationException`, don't use it with `suspend` lambda " +
+            "blocks.",
+        debt = Debt.TEN_MINS
+    )
+
+    override fun visitCallExpression(expression: KtCallExpression) {
+        super.visitCallExpression(expression)
+
+        val resultingDescriptor = expression.getResolvedCall(bindingContext)?.resultingDescriptor ?: return
+
+        if (resultingDescriptor.fqNameSafe != RUN_CATCHING_FQ) return
+
+        fun shouldTraverseInside(element: PsiElement): Boolean =
+            expression == element || shouldTraverseInside(element, bindingContext)
+
+        expression.anyDescendantOfType<KtExpression>(::shouldTraverseInside) { descendant ->
+            descendant.hasSuspendCalls()
+        }.ifTrue { report(expression) }
+    }
+
+    @Suppress("ReturnCount")
+    private fun shouldTraverseInside(psiElement: PsiElement, bindingContext: BindingContext): Boolean {
+        return when (psiElement) {
+            is KtCallExpression -> {
+                val callableDescriptor =
+                    (psiElement.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor)
+                        ?: return false
+
+                callableDescriptor.fqNameSafe != RUN_CATCHING_FQ && callableDescriptor.isInline
+            }
+            is KtValueArgument -> {
+                val callExpression = psiElement.getParentOfType<KtCallExpression>(true)
+                val valueParameterDescriptor =
+                    callExpression?.getResolvedCall(bindingContext)?.getParameterForArgument(psiElement) ?: return false
+
+                valueParameterDescriptor.isCrossinline.not() && valueParameterDescriptor.isNoinline.not()
+            }
+            else -> true
+        }
+    }
+
+    @Suppress("ReturnCount")
+    private fun KtExpression.hasSuspendCalls(): Boolean {
+        return when (this) {
+            is KtForExpression -> {
+                val loopRangeIterator = bindingContext[BindingContext.LOOP_RANGE_ITERATOR_RESOLVED_CALL, loopRange]
+                val loopRangeHasNext =
+                    bindingContext[BindingContext.LOOP_RANGE_HAS_NEXT_RESOLVED_CALL, loopRange]
+                val loopRangeNext = bindingContext[BindingContext.LOOP_RANGE_NEXT_RESOLVED_CALL, loopRange]
+                listOf(loopRangeIterator, loopRangeHasNext, loopRangeNext).any {
+                    it?.resultingDescriptor?.isSuspend == true
+                }
+            }
+            is KtCallExpression, is KtOperationExpression -> {
+                val resolvedCall = getResolvedCall(bindingContext) ?: return false
+                (resolvedCall.resultingDescriptor as? FunctionDescriptor)?.isSuspend == true
+            }
+            is KtNameReferenceExpression -> {
+                val resolvedCall = getResolvedCall(bindingContext) ?: return false
+                val propertyDescriptor = resolvedCall.resultingDescriptor as? PropertyDescriptor
+                propertyDescriptor?.fqNameSafe == COROUTINE_CONTEXT_FQ_NAME
+            }
+            else -> {
+                false
+            }
+        }
+    }
+
+    private fun report(
+        expression: KtCallExpression,
+    ) {
+        report(
+            CodeSmell(
+                issue,
+                Entity.from((expression.calleeExpression as? PsiElement) ?: expression),
+                "The `runCatching` has suspend call inside. You should either use specific `try-catch` " +
+                    "only catching exception that you are expecting or rethrow the `CancellationException` if " +
+                    "already caught."
+            )
+        )
+    }
+
+    companion object {
+        private val RUN_CATCHING_FQ = FqName("kotlin.runCatching")
+
+        // Based on code from Kotlin project:
+        // https://github.com/JetBrains/kotlin/commit/87bbac9d43e15557a2ff0dc3254fd41a9d5639e1
+        private val COROUTINE_CONTEXT_FQ_NAME = COROUTINES_PACKAGE_FQ_NAME.child(Name.identifier("coroutineContext"))
+    }
+}