From ad461a07e208c096310416e29c5f1ac0f1c502b9 Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Thu, 5 Jan 2023 04:35:23 +0530 Subject: [PATCH 1/9] Add SuspendFunInsideRunCatching rule --- .../main/resources/default-detekt-config.yml | 2 + .../rules/coroutines/CoroutinesProvider.kt | 3 +- .../coroutines/SuspendFunInsideRunCatching.kt | 142 +++++ .../SuspendFunInsideRunCatchingSpec.kt | 531 ++++++++++++++++++ 4 files changed, 677 insertions(+), 1 deletion(-) create mode 100644 detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt create mode 100644 detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 1cb67feb020..abb29ef2f22 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -190,6 +190,8 @@ coroutines: active: true SleepInsteadOfDelay: active: true + SuspendFunInsideRunCatching: + active: false SuspendFunWithCoroutineScopeReceiver: active: false SuspendFunWithFlowReturnType: diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt index fe9ade01a85..79b477b77f8 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt +++ b/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), + SuspendFunInsideRunCatching(config), ) ) } diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt new file mode 100644 index 00000000000..e5b2a6f4eb2 --- /dev/null +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt @@ -0,0 +1,142 @@ +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.backend.common.descriptors.isSuspend +import org.jetbrains.kotlin.descriptors.CallableDescriptor +import org.jetbrains.kotlin.descriptors.FunctionDescriptor +import org.jetbrains.kotlin.name.FqName +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType +import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType +import org.jetbrains.kotlin.psi.psiUtil.getParentOfTypesAndPredicate +import org.jetbrains.kotlin.resolve.BindingContext +import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall +import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe + +/** + * Suspend functions should not be called inside `runCatching` as `runCatching` catches + * all the exception while for Coroutine cooperative cancellation to work, we have to + * never catch the `CancellationException` exception or rethrowing it again if caught + * + * See https://kotlinlang.org/docs/cancellation-and-timeouts.html#cancellation-is-cooperative + * + * + * @@Throws(IllegalStateException::class) + * suspend fun bar(delay: Long) { + * check(delay <= 1_000L) + * delay(delay) + * } + * + * suspend fun foo() { + * runCatching { + * bar(1_000L) + * } + * } + * + * + * + * @@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 + * suspend fun foo() { + * bar(1_000L) + * } + * + * + */ +@RequiresTypeResolution +class SuspendFunInsideRunCatching(config: Config) : Rule(config) { + override val issue = Issue( + id = "SuspendFunInsideRunCatching", + severity = Severity.Minor, + description = "The `suspend` functions should be called inside `runCatching` block as it also swallows " + + "`CancellationException` which is important for cooperative cancellation." + + "You should either use specific `try-catch` only catching exception that you are expecting" + + " or rethrow the `CancellationException` if already caught", + debt = Debt.TEN_MINS + ) + + override fun visitCallExpression(expression: KtCallExpression) { + super.visitCallExpression(expression) + + val resultingDescriptor = expression.getResolvedCall(bindingContext)?.resultingDescriptor + resultingDescriptor ?: return + if (resultingDescriptor.fqNameSafe != RUN_CATCHING_FQ) return + + expression.forEachDescendantOfType { descendant -> + if (descendant.getResolvedCall(bindingContext)?.resultingDescriptor?.isSuspend == true && shouldReport( + resultingDescriptor, + descendant, + bindingContext, + ) + ) { + val message = + "The suspend function call ${descendant.text} is inside `runCatching`. You should either " + + "use specific `try-catch` only catching exception that you are expecting or rethrow the " + + "`CancellationException` if already caught" + report( + CodeSmell( + issue, + Entity.from(expression), + message + ) + ) + } + } + } + + private fun shouldReport( + runCatchingCallableDescriptor: CallableDescriptor, + callExpression: KtCallExpression, + bindingContext: BindingContext, + ): Boolean { + val firstNonInlineOrRunCatchingParent = + callExpression.getParentOfTypesAndPredicate(true, KtCallExpression::class.java) { parentCallExp -> + val parentCallFunctionDescriptor = + parentCallExp.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor + parentCallFunctionDescriptor ?: return@getParentOfTypesAndPredicate false + + val isParentRunCatching = parentCallFunctionDescriptor.fqNameSafe == RUN_CATCHING_FQ + val isInline = parentCallFunctionDescriptor.isInline + val noInlineAndCrossInlineValueParametersIndex = + parentCallFunctionDescriptor.valueParameters.filter { valueParameterDescriptor -> + valueParameterDescriptor.isCrossinline || valueParameterDescriptor.isNoinline + }.map { + it.index + } + val callExpressionIndexInParentCall = parentCallExp.valueArguments.indexOfFirst { valueArgument -> + valueArgument?.findDescendantOfType { + it == callExpression + } != null + } + isParentRunCatching || + isInline.not() || + noInlineAndCrossInlineValueParametersIndex.contains(callExpressionIndexInParentCall) + } + return firstNonInlineOrRunCatchingParent.getResolvedCall(bindingContext)?.resultingDescriptor == + runCatchingCallableDescriptor + } + + companion object { + private val RUN_CATCHING_FQ = FqName("kotlin.runCatching") + } +} diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt new file mode 100644 index 00000000000..aad89f0109b --- /dev/null +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt @@ -0,0 +1,531 @@ +package io.gitlab.arturbosch.detekt.rules.coroutines + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Finding +import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest +import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext +import org.assertj.core.api.Assertions.assertThat +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.junit.jupiter.api.Test + +@KotlinCoreEnvironmentTest +class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { + + private val subject = SuspendFunInsideRunCatching(Config.empty) + + @Test + fun `does report suspend function call in runCatching`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + runCatching { + delay(1000L) + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay(1000L)") + } + + @Test + fun `does report for in case of nested runCatching`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + runCatching { + delay(1000L) + runCatching { + delay(2000L) + } + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay(1000L)", "delay(2000L)") + } + + @Test + fun `does report for delay() in suspend functions`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + runCatching { + delay(1000L) + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay(1000L)") + } + + @Test + fun `does not report no suspending function is used inside runBlocking`() { + val code = """ + suspend fun foo() { + runCatching { + Thread.sleep(1000L) + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does not report when try catch is used`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + try { + delay(1000L) + } catch (e: IllegalStateException) { + // handle error + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does report when suspend fun is called inside inline function`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + runCatching { + listOf(1L, 2L, 3L).map { + delay(it) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay(it)") + } + + @Test + fun `does report when inside inline function with noinline and cross inline parameters in same order`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + import kotlinx.coroutines.runBlocking + + inline fun foo( + noinline noinlineBlock: suspend () -> Unit, + inlineBlock: () -> Unit, + crossinline crossinlineBlock: suspend () -> Unit, + ) = inlineBlock().toString() + MainScope().launch { + noinlineBlock() + }.toString() + runBlocking { + crossinlineBlock() + }.toString() + noinlineBlock() + + suspend fun bar() + { + runCatching { + foo( + noinlineBlock = { + delay(2000L) + }, + inlineBlock = { delay(1000L) }, + ) { + + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay(1000L)") + } + + // Failing + @Test + fun `does report when inside inline function with noinline and cross inline parameters not in same order`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + import kotlinx.coroutines.runBlocking + + inline fun foo( + noinline noinlineBlock: suspend () -> Unit, + inlineBlock: () -> Unit, + crossinline crossinlineBlock: suspend () -> Unit, + ) = inlineBlock().toString() + MainScope().launch { + noinlineBlock() + }.toString() + runBlocking { + crossinlineBlock() + }.toString() + noinlineBlock() + + suspend fun bar() + { + runCatching { + foo( + inlineBlock = { delay(1000L) }, + noinlineBlock = { + delay(2000L) + }, + ) { + + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay(1000L)") + } + + @Test + fun `does report when lambda in inline function is passed as crossinline`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun foo(crossinline block: suspend () -> R) = MainScope().launch { + block() + } + suspend fun bar() { + runCatching { + foo { + delay(1000L) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does not report when lambda in inline function is passed as noinline`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun foo(noinline block: suspend () -> R) = MainScope().launch { + block() + } + suspend fun suspendFun() = delay(1000) + suspend fun bar() { + runCatching { + foo { + delay(1000L) + } + + val baz = suspend { + delay(1000L) + } + foo(baz) + foo(::suspendFun) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does report when suspend fun is called as extension function`() { + val code = """ + import kotlinx.coroutines.delay + + private suspend fun List.await() = delay(100L) + + suspend fun foo() { + runCatching { + listOf(1L, 2L, 3L).await() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + assertFindings(findings, "await()") + } + + // Failing + @Test + fun `does report when suspending iterator is used`() { + val code = """ + import kotlinx.coroutines.delay + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + for (x in SuspendingIterator()) { + println(x) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "iterator.next()") + } + + @Test + fun `does report when suspend function is invoked`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + val suspendBlock = suspend { } + runCatching { + suspendBlock() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "suspendBlock()") + } + + @Test + fun `does report in case of suspendCancellableCoroutine`() { + val code = """ + import kotlinx.coroutines.delay + import kotlinx.coroutines.suspendCancellableCoroutine + + suspend fun foo() { + runCatching { + suspendCancellableCoroutine { + it.resume(Unit) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + assertFindings( + findings, + """ + suspendCancellableCoroutine { + it.resume(Unit) + } + """.trimIndent() + ) + } + + @Test + fun `does report in case suspend callable refernce is invoked`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun bar() = delay(1000) + suspend fun foo() { + runCatching { + ::bar.invoke() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "invoke()") + } + + @Test + fun `does report in case suspend local function is invoked`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + suspend fun localFun() = delay(1000L) + runCatching { + localFun() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + assertFindings(findings, "localFun()") + } + + @Test + fun `does report in suspend operator is invoked`() { + val code = """ + import kotlinx.coroutines.delay + + class C { + suspend operator fun invoke() = delay(1000L) + } + + suspend fun foo() { + runCatching { + C()() + C().invoke() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "C()()", "invoke()") + } + + @Test + fun `does not report coroutine is launched`() { + val code = """ + import kotlinx.coroutines.delay + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + suspend fun foo() { + runCatching { + MainScope().launch { + delay(1000L) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does report when job is joined`() { + val code = """ + import kotlinx.coroutines.delay + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + suspend fun foo() { + runCatching { + MainScope().launch { + delay(1000L) + }.join() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "join()") + } + + @Test + fun `does not report async is used`() { + val code = """ + import kotlinx.coroutines.async + import kotlinx.coroutines.delay + import kotlinx.coroutines.MainScope + + suspend fun foo() { + runCatching { + MainScope().async { + delay(1000L) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does report async is awaited`() { + val code = """ + import kotlinx.coroutines.async + import kotlinx.coroutines.delay + import kotlinx.coroutines.MainScope + + suspend fun foo() { + runCatching { + MainScope().async { + delay(1000L) + }.await() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "await()") + } + + @Test + fun `does not report when suspend block is passed to non inline function`() { + val code = """ + import kotlinx.coroutines.delay + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + fun bar(lambda: suspend () -> Unit) { + MainScope().launch { lambda() } + } + + suspend fun foo() { + runCatching { + bar { + delay(1000L) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does not report when suspend fun is called inside runBlocking`() { + val code = """ + import kotlinx.coroutines.delay + import kotlinx.coroutines.runBlocking + + suspend fun foo() { + runCatching { + runBlocking { + delay(1000L) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does report when suspend fun is called in string interpolation`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + runCatching { + val string = "${'$'}{delay(1000)}" + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay(1000)") + } + + private fun assertFindings(findings: List, vararg funCallExpression: String) { + assertThat(findings).hasSize(funCallExpression.size) + assertThat(findings.map { it.message }).containsExactlyInAnyOrder( + *funCallExpression.map { + "The suspend function call $it is inside `runCatching`. You should either " + + "use specific `try-catch` only catching exception that you are expecting or rethrow the " + + "`CancellationException` if already caught" + }.toTypedArray() + ) + } +} From 58608a82ca6c3ba102d07f2465438e28d5b12675 Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Fri, 6 Jan 2023 00:03:19 +0530 Subject: [PATCH 2/9] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add suggestion from code review Co-authored-by: Róbert Papp --- .../coroutines/SuspendFunInsideRunCatching.kt | 26 ++++++++++++++----- .../SuspendFunInsideRunCatchingSpec.kt | 13 +++++----- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt index e5b2a6f4eb2..f464538023c 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt @@ -21,11 +21,25 @@ import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe /** - * Suspend functions should not be called inside `runCatching` as `runCatching` catches - * all the exception while for Coroutine cooperative cancellation to work, we have to - * never catch the `CancellationException` exception or rethrowing it again if caught - * - * See https://kotlinlang.org/docs/cancellation-and-timeouts.html#cancellation-is-cooperative +`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 + +For reference, see [Kotlin documentation](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. * * * @@Throws(IllegalStateException::class) @@ -92,7 +106,7 @@ class SuspendFunInsideRunCatching(config: Config) : Rule(config) { val message = "The suspend function call ${descendant.text} is inside `runCatching`. You should either " + "use specific `try-catch` only catching exception that you are expecting or rethrow the " + - "`CancellationException` if already caught" + "`CancellationException` if already caught." report( CodeSmell( issue, diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt index aad89f0109b..7fb0171f893 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt @@ -127,8 +127,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { crossinlineBlock() }.toString() + noinlineBlock() - suspend fun bar() - { + suspend fun bar() { runCatching { foo( noinlineBlock = { @@ -159,11 +158,11 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { noinline noinlineBlock: suspend () -> Unit, inlineBlock: () -> Unit, crossinline crossinlineBlock: suspend () -> Unit, - ) = inlineBlock().toString() + MainScope().launch { - noinlineBlock() - }.toString() + runBlocking { - crossinlineBlock() - }.toString() + noinlineBlock() + ) = + inlineBlock().toString() + + MainScope().launch { noinlineBlock() }.toString() + + runBlocking { crossinlineBlock() }.toString() + + noinlineBlock() suspend fun bar() { From bcc0062338482e2c6cacdc4a3c6c1659159f52d0 Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Fri, 6 Jan 2023 04:33:27 +0530 Subject: [PATCH 3/9] Simplify logic to have single traversal Rename rule to SuspendFunSwallowedCancellation Add new TCs --- .../main/resources/default-detekt-config.yml | 2 +- .../rules/coroutines/CoroutinesProvider.kt | 2 +- .../coroutines/SuspendFunInsideRunCatching.kt | 156 --------------- .../SuspendFunSwallowedCancellation.kt | 180 +++++++++++++++++ ...=> SuspendFunSwallowedCancellationSpec.kt} | 182 +++++++++++++----- 5 files changed, 321 insertions(+), 201 deletions(-) delete mode 100644 detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt create mode 100644 detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt rename detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/{SuspendFunInsideRunCatchingSpec.kt => SuspendFunSwallowedCancellationSpec.kt} (73%) diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index abb29ef2f22..737999beb9b 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -190,7 +190,7 @@ coroutines: active: true SleepInsteadOfDelay: active: true - SuspendFunInsideRunCatching: + SuspendFunSwallowedCancellation: active: false SuspendFunWithCoroutineScopeReceiver: active: false diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt index 79b477b77f8..b4661c86d04 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/CoroutinesProvider.kt @@ -22,7 +22,7 @@ class CoroutinesProvider : DefaultRuleSetProvider { SleepInsteadOfDelay(config), SuspendFunWithFlowReturnType(config), SuspendFunWithCoroutineScopeReceiver(config), - SuspendFunInsideRunCatching(config), + SuspendFunSwallowedCancellation(config), ) ) } diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt deleted file mode 100644 index f464538023c..00000000000 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatching.kt +++ /dev/null @@ -1,156 +0,0 @@ -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.backend.common.descriptors.isSuspend -import org.jetbrains.kotlin.descriptors.CallableDescriptor -import org.jetbrains.kotlin.descriptors.FunctionDescriptor -import org.jetbrains.kotlin.name.FqName -import org.jetbrains.kotlin.psi.KtCallExpression -import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType -import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType -import org.jetbrains.kotlin.psi.psiUtil.getParentOfTypesAndPredicate -import org.jetbrains.kotlin.resolve.BindingContext -import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall -import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe - -/** -`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 - -For reference, see [Kotlin documentation](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. - * - * - * @@Throws(IllegalStateException::class) - * suspend fun bar(delay: Long) { - * check(delay <= 1_000L) - * delay(delay) - * } - * - * suspend fun foo() { - * runCatching { - * bar(1_000L) - * } - * } - * - * - * - * @@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 - * suspend fun foo() { - * bar(1_000L) - * } - * - * - */ -@RequiresTypeResolution -class SuspendFunInsideRunCatching(config: Config) : Rule(config) { - override val issue = Issue( - id = "SuspendFunInsideRunCatching", - severity = Severity.Minor, - description = "The `suspend` functions should be called inside `runCatching` block as it also swallows " + - "`CancellationException` which is important for cooperative cancellation." + - "You should either use specific `try-catch` only catching exception that you are expecting" + - " or rethrow the `CancellationException` if already caught", - debt = Debt.TEN_MINS - ) - - override fun visitCallExpression(expression: KtCallExpression) { - super.visitCallExpression(expression) - - val resultingDescriptor = expression.getResolvedCall(bindingContext)?.resultingDescriptor - resultingDescriptor ?: return - if (resultingDescriptor.fqNameSafe != RUN_CATCHING_FQ) return - - expression.forEachDescendantOfType { descendant -> - if (descendant.getResolvedCall(bindingContext)?.resultingDescriptor?.isSuspend == true && shouldReport( - resultingDescriptor, - descendant, - bindingContext, - ) - ) { - val message = - "The suspend function call ${descendant.text} is inside `runCatching`. You should either " + - "use specific `try-catch` only catching exception that you are expecting or rethrow the " + - "`CancellationException` if already caught." - report( - CodeSmell( - issue, - Entity.from(expression), - message - ) - ) - } - } - } - - private fun shouldReport( - runCatchingCallableDescriptor: CallableDescriptor, - callExpression: KtCallExpression, - bindingContext: BindingContext, - ): Boolean { - val firstNonInlineOrRunCatchingParent = - callExpression.getParentOfTypesAndPredicate(true, KtCallExpression::class.java) { parentCallExp -> - val parentCallFunctionDescriptor = - parentCallExp.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor - parentCallFunctionDescriptor ?: return@getParentOfTypesAndPredicate false - - val isParentRunCatching = parentCallFunctionDescriptor.fqNameSafe == RUN_CATCHING_FQ - val isInline = parentCallFunctionDescriptor.isInline - val noInlineAndCrossInlineValueParametersIndex = - parentCallFunctionDescriptor.valueParameters.filter { valueParameterDescriptor -> - valueParameterDescriptor.isCrossinline || valueParameterDescriptor.isNoinline - }.map { - it.index - } - val callExpressionIndexInParentCall = parentCallExp.valueArguments.indexOfFirst { valueArgument -> - valueArgument?.findDescendantOfType { - it == callExpression - } != null - } - isParentRunCatching || - isInline.not() || - noInlineAndCrossInlineValueParametersIndex.contains(callExpressionIndexInParentCall) - } - return firstNonInlineOrRunCatchingParent.getResolvedCall(bindingContext)?.resultingDescriptor == - runCatchingCallableDescriptor - } - - companion object { - private val RUN_CATCHING_FQ = FqName("kotlin.runCatching") - } -} diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt new file mode 100644 index 00000000000..47ff5dd6ff1 --- /dev/null +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt @@ -0,0 +1,180 @@ +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.backend.common.descriptors.isSuspend +import org.jetbrains.kotlin.com.intellij.psi.PsiElement +import org.jetbrains.kotlin.descriptors.FunctionDescriptor +import org.jetbrains.kotlin.name.FqName +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtForExpression +import org.jetbrains.kotlin.psi.KtValueArgument +import org.jetbrains.kotlin.psi.psiUtil.forEachDescendantOfType +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 + +/** +`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 + +For reference, see [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. + * + * + * @@Throws(IllegalStateException::class) + * suspend fun bar(delay: Long) { + * check(delay <= 1_000L) + * delay(delay) + * } + * + * suspend fun foo() { + * runCatching { + * bar(1_000L) + * } + * } + * + * + * + * @@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) + * } + * + * + */ +@RequiresTypeResolution +class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { + override val issue = Issue( + id = "SuspendFunInsideRunCatching", + severity = Severity.Minor, + description = "`suspend` functions should not be called inside `runCatching`'s lambda block, because " + + "`runCatching` catches all the `Exception`s.", + debt = Debt.TEN_MINS + ) + + override fun visitCallExpression(expression: KtCallExpression) { + super.visitCallExpression(expression) + + val resultingDescriptor = expression.getResolvedCall(bindingContext)?.resultingDescriptor + resultingDescriptor ?: return + if (resultingDescriptor.fqNameSafe != RUN_CATCHING_FQ) return + + expression.forEachDescendantOfType({ + expression == it || shouldTraverseInside(it, bindingContext) + }) { descendant -> + val callableDescriptor = descendant.getResolvedCall(bindingContext)?.resultingDescriptor + if (callableDescriptor?.isSuspend == true) { + report( + message = "The suspend function call ${callableDescriptor.fqNameSafe.shortName()} called inside " + + "`runCatching`. You should either use specific `try-catch` only catching exception that you " + + "are expecting or rethrow the `CancellationException` if already caught.", + expression + ) + } + } + + expression.forEachDescendantOfType { descendant -> + if (descendant.doesLoopHasSuspendingIterators()) { + report( + message = "The for-loop expression has suspending operator which is called inside " + + "`runCatching`. You should either use specific `try-catch` only catching exception that you " + + "are expecting or rethrow the `CancellationException` if already caught.", + expression + ) + } + } + } + + @Suppress("ReturnCount") + private fun shouldTraverseInside(psiElement: PsiElement, bindingContext: BindingContext): Boolean { + if (psiElement is KtCallExpression) { + val callableDescriptor = + psiElement.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor + callableDescriptor ?: return false + + if (callableDescriptor.fqNameSafe == RUN_CATCHING_FQ) return false + + if (callableDescriptor.isInline.not()) return false + } + + if (psiElement is KtValueArgument) { + val callExpression = psiElement.getParentOfType(true) + callExpression ?: return false + val valueParameterDescriptor = + callExpression.getResolvedCall(bindingContext)?.getParameterForArgument(psiElement) + valueParameterDescriptor ?: return false + if (valueParameterDescriptor.isCrossinline || valueParameterDescriptor.isNoinline) return false + } + return true + } + + private fun KtForExpression.doesLoopHasSuspendingIterators(): Boolean { + val iteratorResolvedCall = bindingContext[BindingContext.LOOP_RANGE_ITERATOR_RESOLVED_CALL, this.loopRange] + val loopRangeHasNextResolvedCall = + bindingContext[BindingContext.LOOP_RANGE_HAS_NEXT_RESOLVED_CALL, this.loopRange] + val loopRangeNextResolvedCall = bindingContext[BindingContext.LOOP_RANGE_NEXT_RESOLVED_CALL, this.loopRange] + return listOf(iteratorResolvedCall, loopRangeHasNextResolvedCall, loopRangeNextResolvedCall).any { + it?.resultingDescriptor?.isSuspend == true + } + } + + private fun report( + message: String, + expression: KtCallExpression, + ) { + report( + CodeSmell( + issue, + Entity.from(expression), + message + ) + ) + } + + companion object { + private val RUN_CATCHING_FQ = FqName("kotlin.runCatching") + } +} diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt similarity index 73% rename from detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt rename to detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt index 7fb0171f893..c5eb349fe08 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunInsideRunCatchingSpec.kt +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt @@ -9,14 +9,14 @@ import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment import org.junit.jupiter.api.Test @KotlinCoreEnvironmentTest -class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { +class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment) { - private val subject = SuspendFunInsideRunCatching(Config.empty) + private val subject = SuspendFunSwallowedCancellation(Config.empty) @Test fun `does report suspend function call in runCatching`() { val code = """ - import kotlinx.coroutines.delay + import kotlinx.coroutines.delay suspend fun foo() { runCatching { @@ -25,31 +25,33 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay(1000L)") + assertFindings(findings, "delay") } @Test fun `does report for in case of nested runCatching`() { val code = """ - import kotlinx.coroutines.delay + import kotlinx.coroutines.delay + + suspend fun bar() = delay(2000) suspend fun foo() { runCatching { delay(1000L) runCatching { - delay(2000L) + bar() } } } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay(1000L)", "delay(2000L)") + assertFindings(findings, "delay", "bar") } @Test fun `does report for delay() in suspend functions`() { val code = """ - import kotlinx.coroutines.delay + import kotlinx.coroutines.delay suspend fun foo() { runCatching { @@ -58,7 +60,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay(1000L)") + assertFindings(findings, "delay") } @Test @@ -74,10 +76,54 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { assertFindings(findings) } + @Test + fun `does report when _when_ is used in result`() { + val code = """ + import kotlinx.coroutines.delay + suspend fun bar() = delay(1000L) + suspend fun foo(): Result<*> { + val result = runCatching { bar() } + when(result.isSuccess) { + true -> TODO() + false -> TODO() + } + return result + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "bar") + } + + @Test + fun `does report when onSuccess is used in result`() { + val code = """ + import kotlinx.coroutines.delay + suspend fun bar() = delay(1000L) + suspend fun foo() { + runCatching { bar() }.onSuccess { + TODO() + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "bar") + } + + @Test + fun `does report when runCatching is used as function expression`() { + val code = """ + import kotlinx.coroutines.delay + suspend fun bar() = delay(1000L) + suspend fun foo() = runCatching { bar() } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "bar") + } + @Test fun `does not report when try catch is used`() { val code = """ - import kotlinx.coroutines.delay + import kotlinx.coroutines.delay suspend fun foo() { try { @@ -94,7 +140,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { @Test fun `does report when suspend fun is called inside inline function`() { val code = """ - import kotlinx.coroutines.delay + import kotlinx.coroutines.delay suspend fun foo() { runCatching { @@ -106,7 +152,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay(it)") + assertFindings(findings, "delay") } @Test @@ -123,9 +169,9 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { crossinline crossinlineBlock: suspend () -> Unit, ) = inlineBlock().toString() + MainScope().launch { noinlineBlock() - }.toString() + runBlocking { + } + runBlocking { crossinlineBlock() - }.toString() + noinlineBlock() + }.toString() suspend fun bar() { runCatching { @@ -142,10 +188,9 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay(1000L)") + assertFindings(findings, "delay") } - // Failing @Test fun `does report when inside inline function with noinline and cross inline parameters not in same order`() { val code = """ @@ -158,11 +203,11 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { noinline noinlineBlock: suspend () -> Unit, inlineBlock: () -> Unit, crossinline crossinlineBlock: suspend () -> Unit, - ) = - inlineBlock().toString() + - MainScope().launch { noinlineBlock() }.toString() + - runBlocking { crossinlineBlock() }.toString() + - noinlineBlock() + ) = inlineBlock().toString() + MainScope().launch { + noinlineBlock() + } + runBlocking { + crossinlineBlock() + }.toString() suspend fun bar() { @@ -180,7 +225,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay(1000L)") + assertFindings(findings, "delay") } @Test @@ -206,6 +251,60 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { assertFindings(findings) } + @Test + fun `does not report when lambda parameter chain has noinline function call`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun inline(block: () -> R) = block() + fun noInline(block: suspend () -> R) = MainScope().launch { block() } + + suspend fun bar() + { + runCatching { + inline { + noInline { + inline { + delay(1000) + } + } + + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings) + } + + @Test + fun `does report when lambda parameter chain has all inlined function call`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun inline(block: () -> R) = block() + + suspend fun bar() + { + runCatching { + inline { + inline { + delay(1000) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindings(findings, "delay") + } + @Test fun `does not report when lambda in inline function is passed as noinline`() { val code = """ @@ -252,10 +351,9 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) - assertFindings(findings, "await()") + assertFindings(findings, "await") } - // Failing @Test fun `does report when suspending iterator is used`() { val code = """ @@ -275,7 +373,12 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "iterator.next()") + assertThat(findings).hasSize(1) + assertThat(findings[0].message).isEqualTo( + "The for-loop expression has suspending operator which is called " + + "inside `runCatching`. You should either use specific `try-catch` only catching exception that you are " + + "expecting or rethrow the `CancellationException` if already caught." + ) } @Test @@ -292,7 +395,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "suspendBlock()") + assertFindings(findings, "invoke") } @Test @@ -312,14 +415,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) - assertFindings( - findings, - """ - suspendCancellableCoroutine { - it.resume(Unit) - } - """.trimIndent() - ) + assertFindings(findings, "suspendCancellableCoroutine") } @Test @@ -336,7 +432,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "invoke()") + assertFindings(findings, "invoke") } @Test @@ -354,7 +450,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) - assertFindings(findings, "localFun()") + assertFindings(findings, "localFun") } @Test @@ -375,7 +471,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "C()()", "invoke()") + assertFindings(findings, "invoke", "invoke") } @Test @@ -415,7 +511,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "join()") + assertFindings(findings, "join") } @Test @@ -455,7 +551,7 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "await()") + assertFindings(findings, "await") } @Test @@ -514,16 +610,16 @@ class SuspendFunInsideRunCatchingSpec(private val env: KotlinCoreEnvironment) { """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay(1000)") + assertFindings(findings, "delay") } private fun assertFindings(findings: List, vararg funCallExpression: String) { assertThat(findings).hasSize(funCallExpression.size) assertThat(findings.map { it.message }).containsExactlyInAnyOrder( *funCallExpression.map { - "The suspend function call $it is inside `runCatching`. You should either " + - "use specific `try-catch` only catching exception that you are expecting or rethrow the " + - "`CancellationException` if already caught" + "The suspend function call $it called inside " + + "`runCatching`. You should either use specific `try-catch` only catching exception that you " + + "are expecting or rethrow the `CancellationException` if already caught." }.toTypedArray() ) } From 4a961b02b76ee1aab15f378dab4cdf7db3f02e09 Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Wed, 11 Jan 2023 20:20:43 +0530 Subject: [PATCH 4/9] Address PR review comments Check shouldPropagate inside while checking for loop Simplify if else block --- .../SuspendFunSwallowedCancellation.kt | 93 ++++--- .../SuspendFunSwallowedCancellationSpec.kt | 228 +++++++++++++++--- 2 files changed, 238 insertions(+), 83 deletions(-) diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt index 47ff5dd6ff1..04ca6da0733 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt @@ -23,30 +23,30 @@ import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe /** -`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 - -For reference, see [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. + * `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. * * * @@Throws(IllegalStateException::class) @@ -90,8 +90,8 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { override val issue = Issue( id = "SuspendFunInsideRunCatching", severity = Severity.Minor, - description = "`suspend` functions should not be called inside `runCatching`'s lambda block, because " + - "`runCatching` catches all the `Exception`s.", + description = "`runCatching` does not propagate `CancellationException`, don't use it with `suspend` lambda " + + "blocks.", debt = Debt.TEN_MINS ) @@ -102,9 +102,10 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { resultingDescriptor ?: return if (resultingDescriptor.fqNameSafe != RUN_CATCHING_FQ) return - expression.forEachDescendantOfType({ + val shouldTraverseInside: (PsiElement) -> Boolean = { expression == it || shouldTraverseInside(it, bindingContext) - }) { descendant -> + } + expression.forEachDescendantOfType(shouldTraverseInside) { descendant -> val callableDescriptor = descendant.getResolvedCall(bindingContext)?.resultingDescriptor if (callableDescriptor?.isSuspend == true) { report( @@ -116,8 +117,8 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { } } - expression.forEachDescendantOfType { descendant -> - if (descendant.doesLoopHasSuspendingIterators()) { + expression.forEachDescendantOfType(shouldTraverseInside) { descendant -> + if (descendant.hasSuspendingIterators()) { report( message = "The for-loop expression has suspending operator which is called inside " + "`runCatching`. You should either use specific `try-catch` only catching exception that you " + @@ -130,28 +131,26 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { @Suppress("ReturnCount") private fun shouldTraverseInside(psiElement: PsiElement, bindingContext: BindingContext): Boolean { - if (psiElement is KtCallExpression) { - val callableDescriptor = - psiElement.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor - callableDescriptor ?: return false - - if (callableDescriptor.fqNameSafe == RUN_CATCHING_FQ) return false + return when (psiElement) { + is KtCallExpression -> { + val callableDescriptor = + (psiElement.getResolvedCall(bindingContext)?.resultingDescriptor as? FunctionDescriptor) + ?: return false - if (callableDescriptor.isInline.not()) return false - } + callableDescriptor.fqNameSafe != RUN_CATCHING_FQ && callableDescriptor.isInline + } + is KtValueArgument -> { + val callExpression = psiElement.getParentOfType(true) ?: return false + val valueParameterDescriptor = + callExpression.getResolvedCall(bindingContext)?.getParameterForArgument(psiElement) ?: return false - if (psiElement is KtValueArgument) { - val callExpression = psiElement.getParentOfType(true) - callExpression ?: return false - val valueParameterDescriptor = - callExpression.getResolvedCall(bindingContext)?.getParameterForArgument(psiElement) - valueParameterDescriptor ?: return false - if (valueParameterDescriptor.isCrossinline || valueParameterDescriptor.isNoinline) return false + valueParameterDescriptor.isCrossinline.not() && valueParameterDescriptor.isNoinline.not() + } + else -> true } - return true } - private fun KtForExpression.doesLoopHasSuspendingIterators(): Boolean { + private fun KtForExpression.hasSuspendingIterators(): Boolean { val iteratorResolvedCall = bindingContext[BindingContext.LOOP_RANGE_ITERATOR_RESOLVED_CALL, this.loopRange] val loopRangeHasNextResolvedCall = bindingContext[BindingContext.LOOP_RANGE_HAS_NEXT_RESOLVED_CALL, this.loopRange] diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt index c5eb349fe08..ae05fbaa608 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt @@ -25,7 +25,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay") + assertFindingsForSuspendCall(findings, "delay") } @Test @@ -45,7 +45,55 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay", "bar") + assertFindingsForSuspendCall(findings, "delay", "bar") + } + + @Test + fun `does report for in case of nested runCatching with suspend fun call in inner runCatching`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + suspend fun bar() = delay(2000) + + suspend fun foo() { + runCatching { + MainScope().launch { + delay(1000L) + runCatching { + bar() + } + } + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall(findings, "bar") + } + + @Test + fun `does report for in case of nested runCatching with suspend fun call in outer runCatching`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + suspend fun bar() = delay(2000) + + suspend fun foo() { + runCatching { + delay(1000L) + runCatching { + MainScope().launch { + bar() + } + } + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall(findings, "delay") } @Test @@ -60,7 +108,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay") + assertFindingsForSuspendCall(findings, "delay") } @Test @@ -73,7 +121,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -91,7 +139,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "bar") + assertFindingsForSuspendCall(findings, "bar") } @Test @@ -106,7 +154,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "bar") + assertFindingsForSuspendCall(findings, "bar") } @Test @@ -117,7 +165,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment suspend fun foo() = runCatching { bar() } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "bar") + assertFindingsForSuspendCall(findings, "bar") } @Test @@ -134,7 +182,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -152,7 +200,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay") + assertFindingsForSuspendCall(findings, "delay") } @Test @@ -188,7 +236,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay") + assertFindingsForSuspendCall(findings, "delay") } @Test @@ -225,7 +273,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay") + assertFindingsForSuspendCall(findings, "delay") } @Test @@ -248,7 +296,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -277,7 +325,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -302,7 +350,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay") + assertFindingsForSuspendCall(findings, "delay") } @Test @@ -332,7 +380,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -351,7 +399,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) - assertFindings(findings, "await") + assertFindingsForSuspendCall(findings, "await") } @Test @@ -373,12 +421,111 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).hasSize(1) - assertThat(findings[0].message).isEqualTo( - "The for-loop expression has suspending operator which is called " + - "inside `runCatching`. You should either use specific `try-catch` only catching exception that you are " + - "expecting or rethrow the `CancellationException` if already caught." - ) + assertFindingsForSuspendingIterator(findings, 1) + } + + @Test + fun `does report when nested suspending iterator is used`() { + val code = """ + import kotlinx.coroutines.delay + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + for (x in SuspendingIterator()) { + for (y in SuspendingIterator()) { + println(x + y) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendingIterator(findings, 2) + } + + @Test + fun `does report when nested iterator with one suspending iterator is used`() { + val code = """ + import kotlinx.coroutines.delay + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + for (x in 1..10) { + for (y in SuspendingIterator()) { + println(x.toString() + y) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendingIterator(findings, 1) + } + + @Test + fun `does report when suspending iterator is used withing inlined block`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + inline fun foo(lambda: () -> Unit) { + lambda() + } + + suspend fun bar() { + runCatching { + foo { + for (x in SuspendingIterator()) { + println(x) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendingIterator(findings, 1) + } + + @Test + fun `does not report when suspending iterator is used withing non inlined block`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + MainScope().launch { + for (x in SuspendingIterator()) { + println(x) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendingIterator(findings, 0) } @Test @@ -395,7 +542,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "invoke") + assertFindingsForSuspendCall(findings, "invoke") } @Test @@ -415,11 +562,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) - assertFindings(findings, "suspendCancellableCoroutine") + assertFindingsForSuspendCall(findings, "suspendCancellableCoroutine") } @Test - fun `does report in case suspend callable refernce is invoked`() { + fun `does report in case suspend callable reference is invoked`() { val code = """ import kotlinx.coroutines.delay @@ -432,7 +579,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "invoke") + assertFindingsForSuspendCall(findings, "invoke") } @Test @@ -450,7 +597,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) - assertFindings(findings, "localFun") + assertFindingsForSuspendCall(findings, "localFun") } @Test @@ -471,7 +618,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "invoke", "invoke") + assertFindingsForSuspendCall(findings, "invoke", "invoke") } @Test @@ -491,7 +638,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -511,7 +658,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "join") + assertFindingsForSuspendCall(findings, "join") } @Test @@ -531,7 +678,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -551,7 +698,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "await") + assertFindingsForSuspendCall(findings, "await") } @Test @@ -575,7 +722,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -594,7 +741,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings) + assertFindingsForSuspendCall(findings) } @Test @@ -610,10 +757,10 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindings(findings, "delay") + assertFindingsForSuspendCall(findings, "delay") } - private fun assertFindings(findings: List, vararg funCallExpression: String) { + private fun assertFindingsForSuspendCall(findings: List, vararg funCallExpression: String) { assertThat(findings).hasSize(funCallExpression.size) assertThat(findings.map { it.message }).containsExactlyInAnyOrder( *funCallExpression.map { @@ -623,4 +770,13 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment }.toTypedArray() ) } + + private fun assertFindingsForSuspendingIterator(findings: List, size: Int) { + assertThat(findings).hasSize(size) + assertThat(findings.map { it.message }).allMatch { + return@allMatch it == "The for-loop expression has suspending operator which is called " + + "inside `runCatching`. You should either use specific `try-catch` only catching exception that you " + + "are expecting or rethrow the `CancellationException` if already caught." + } + } } From 76c320e9be53039d3a3f8152f36757451bafe190 Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Thu, 12 Jan 2023 01:17:32 +0530 Subject: [PATCH 5/9] Fix PR comments Make the local function Early return of the declaration of the variable --- .../coroutines/SuspendFunSwallowedCancellation.kt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt index 04ca6da0733..79cc602795d 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt @@ -98,14 +98,14 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { override fun visitCallExpression(expression: KtCallExpression) { super.visitCallExpression(expression) - val resultingDescriptor = expression.getResolvedCall(bindingContext)?.resultingDescriptor - resultingDescriptor ?: return + val resultingDescriptor = expression.getResolvedCall(bindingContext)?.resultingDescriptor ?: return + if (resultingDescriptor.fqNameSafe != RUN_CATCHING_FQ) return - val shouldTraverseInside: (PsiElement) -> Boolean = { - expression == it || shouldTraverseInside(it, bindingContext) - } - expression.forEachDescendantOfType(shouldTraverseInside) { descendant -> + fun shouldTraverseInside(element: PsiElement): Boolean = + expression == element || shouldTraverseInside(element, bindingContext) + + expression.forEachDescendantOfType(::shouldTraverseInside) { descendant -> val callableDescriptor = descendant.getResolvedCall(bindingContext)?.resultingDescriptor if (callableDescriptor?.isSuspend == true) { report( @@ -117,7 +117,7 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { } } - expression.forEachDescendantOfType(shouldTraverseInside) { descendant -> + expression.forEachDescendantOfType(::shouldTraverseInside) { descendant -> if (descendant.hasSuspendingIterators()) { report( message = "The for-loop expression has suspending operator which is called inside " + From 1adaa732b3fc952920a6f092e6abde7d444d4290 Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Thu, 12 Jan 2023 02:47:26 +0530 Subject: [PATCH 6/9] Fix inconsistent class name and issue id --- .../detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt index 79cc602795d..94a1fd840d1 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt @@ -88,7 +88,7 @@ import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe @RequiresTypeResolution class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { override val issue = Issue( - id = "SuspendFunInsideRunCatching", + id = javaClass.simpleName, severity = Severity.Minor, description = "`runCatching` does not propagate `CancellationException`, don't use it with `suspend` lambda " + "blocks.", From 8405a19a026a42d6ae9fbd9f3cbd6efe132c8f6a Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Thu, 12 Jan 2023 20:20:24 +0530 Subject: [PATCH 7/9] Handle suspend operators Report violating runCatching block instead of suspend call Assert location in tests --- .../SuspendFunSwallowedCancellation.kt | 81 +-- .../SuspendFunSwallowedCancellationSpec.kt | 541 +++++++++++++++--- 2 files changed, 504 insertions(+), 118 deletions(-) diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt index 94a1fd840d1..45265bf1b48 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellation.kt @@ -8,19 +8,25 @@ 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.backend.common.descriptors.isSuspend +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.forEachDescendantOfType +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 @@ -105,28 +111,9 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { fun shouldTraverseInside(element: PsiElement): Boolean = expression == element || shouldTraverseInside(element, bindingContext) - expression.forEachDescendantOfType(::shouldTraverseInside) { descendant -> - val callableDescriptor = descendant.getResolvedCall(bindingContext)?.resultingDescriptor - if (callableDescriptor?.isSuspend == true) { - report( - message = "The suspend function call ${callableDescriptor.fqNameSafe.shortName()} called inside " + - "`runCatching`. You should either use specific `try-catch` only catching exception that you " + - "are expecting or rethrow the `CancellationException` if already caught.", - expression - ) - } - } - - expression.forEachDescendantOfType(::shouldTraverseInside) { descendant -> - if (descendant.hasSuspendingIterators()) { - report( - message = "The for-loop expression has suspending operator which is called inside " + - "`runCatching`. You should either use specific `try-catch` only catching exception that you " + - "are expecting or rethrow the `CancellationException` if already caught.", - expression - ) - } - } + expression.anyDescendantOfType(::shouldTraverseInside) { descendant -> + descendant.hasSuspendCalls() + }.ifTrue { report(expression) } } @Suppress("ReturnCount") @@ -140,9 +127,9 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { callableDescriptor.fqNameSafe != RUN_CATCHING_FQ && callableDescriptor.isInline } is KtValueArgument -> { - val callExpression = psiElement.getParentOfType(true) ?: return false + val callExpression = psiElement.getParentOfType(true) val valueParameterDescriptor = - callExpression.getResolvedCall(bindingContext)?.getParameterForArgument(psiElement) ?: return false + callExpression?.getResolvedCall(bindingContext)?.getParameterForArgument(psiElement) ?: return false valueParameterDescriptor.isCrossinline.not() && valueParameterDescriptor.isNoinline.not() } @@ -150,30 +137,52 @@ class SuspendFunSwallowedCancellation(config: Config) : Rule(config) { } } - private fun KtForExpression.hasSuspendingIterators(): Boolean { - val iteratorResolvedCall = bindingContext[BindingContext.LOOP_RANGE_ITERATOR_RESOLVED_CALL, this.loopRange] - val loopRangeHasNextResolvedCall = - bindingContext[BindingContext.LOOP_RANGE_HAS_NEXT_RESOLVED_CALL, this.loopRange] - val loopRangeNextResolvedCall = bindingContext[BindingContext.LOOP_RANGE_NEXT_RESOLVED_CALL, this.loopRange] - return listOf(iteratorResolvedCall, loopRangeHasNextResolvedCall, loopRangeNextResolvedCall).any { - it?.resultingDescriptor?.isSuspend == 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( - message: String, expression: KtCallExpression, ) { report( CodeSmell( issue, - Entity.from(expression), - message + 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")) } } diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt index ae05fbaa608..b69ecc93dab 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt @@ -2,10 +2,12 @@ package io.gitlab.arturbosch.detekt.rules.coroutines import io.gitlab.arturbosch.detekt.api.Config import io.gitlab.arturbosch.detekt.api.Finding +import io.gitlab.arturbosch.detekt.api.SourceLocation import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest +import io.gitlab.arturbosch.detekt.test.assertThat import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext -import org.assertj.core.api.Assertions.assertThat import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test @KotlinCoreEnvironmentTest @@ -25,7 +27,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 5)), + listOf(SourceLocation(4, 16)) + ) } @Test @@ -45,7 +51,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay", "bar") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5), SourceLocation(8, 9)), + listOf(SourceLocation(6, 16), SourceLocation(8, 20)) + ) } @Test @@ -69,7 +79,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "bar") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(11, 13)), + listOf(SourceLocation(11, 24)) + ) } @Test @@ -93,7 +107,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) } @Test @@ -108,7 +126,31 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 5)), + listOf(SourceLocation(4, 16)) + ) + } + + @Test + fun `does report for coroutineContext in suspend functions`() { + val code = """ + import kotlinx.coroutines.delay + import kotlin.coroutines.coroutineContext + + suspend fun foo() { + runCatching { + coroutineContext + } + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(5, 5)), + listOf(SourceLocation(5, 16)) + ) } @Test @@ -121,7 +163,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -139,7 +181,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "bar") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 18)), + listOf(SourceLocation(4, 29)) + ) } @Test @@ -154,7 +200,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "bar") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 5)), + listOf(SourceLocation(4, 16)) + ) } @Test @@ -165,7 +215,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment suspend fun foo() = runCatching { bar() } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "bar") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(3, 21)), + listOf(SourceLocation(3, 32)) + ) } @Test @@ -182,7 +236,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -200,7 +254,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 5)), + listOf(SourceLocation(4, 16)) + ) } @Test @@ -236,7 +294,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(17, 5)), + listOf(SourceLocation(17, 16)) + ) } @Test @@ -257,8 +319,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment crossinlineBlock() }.toString() - suspend fun bar() - { + suspend fun bar() { runCatching { foo( inlineBlock = { delay(1000L) }, @@ -273,7 +334,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(17, 5)), + listOf(SourceLocation(17, 16)) + ) } @Test @@ -296,7 +361,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -309,8 +374,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment inline fun inline(block: () -> R) = block() fun noInline(block: suspend () -> R) = MainScope().launch { block() } - suspend fun bar() - { + suspend fun bar() { runCatching { inline { noInline { @@ -325,7 +389,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -337,8 +401,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment inline fun inline(block: () -> R) = block() - suspend fun bar() - { + suspend fun bar() { runCatching { inline { inline { @@ -350,7 +413,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) } @Test @@ -380,7 +447,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -388,7 +455,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val code = """ import kotlinx.coroutines.delay - private suspend fun List.await() = delay(100L) + suspend fun List.await() = delay(100L) suspend fun foo() { runCatching { @@ -399,7 +466,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertThat(findings).hasSize(1) - assertFindingsForSuspendCall(findings, "await") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) } @Test @@ -421,7 +492,33 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendingIterator(findings, 1) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) + } + + @Test + fun `does report when suspend fun in for subject is used`() { + val code = """ + suspend fun bar() = 10 + + suspend fun foo() { + runCatching { + for (i in 1..bar()) { + println(i) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 5)), + listOf(SourceLocation(4, 16)) + ) } @Test @@ -430,7 +527,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.delay class SuspendingIterator { - suspend operator fun iterator(): Iterator = iterator { yield("value") } + suspend operator fun iterator(): Iterator = iterator { yield("value") } } suspend fun bar() { @@ -445,7 +542,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendingIterator(findings, 2) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) } @Test @@ -469,7 +570,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendingIterator(findings, 1) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) } @Test @@ -499,7 +604,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendingIterator(findings, 1) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(14, 5)), + listOf(SourceLocation(14, 16)) + ) } @Test @@ -525,7 +634,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendingIterator(findings, 0) + assertThat(findings).isEmpty() } @Test @@ -542,7 +651,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "invoke") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(5, 5)), + listOf(SourceLocation(5, 16)) + ) } @Test @@ -550,6 +663,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val code = """ import kotlinx.coroutines.delay import kotlinx.coroutines.suspendCancellableCoroutine + import kotlin.coroutines.resume suspend fun foo() { runCatching { @@ -561,8 +675,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).hasSize(1) - assertFindingsForSuspendCall(findings, "suspendCancellableCoroutine") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) } @Test @@ -579,7 +696,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "invoke") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(5, 5)), + listOf(SourceLocation(5, 16)) + ) } @Test @@ -596,29 +717,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).hasSize(1) - assertFindingsForSuspendCall(findings, "localFun") - } - - @Test - fun `does report in suspend operator is invoked`() { - val code = """ - import kotlinx.coroutines.delay - - class C { - suspend operator fun invoke() = delay(1000L) - } - - suspend fun foo() { - runCatching { - C()() - C().invoke() - } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "invoke", "invoke") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(5, 5)), + listOf(SourceLocation(5, 16)) + ) } @Test @@ -638,7 +741,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -658,7 +761,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "join") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) } @Test @@ -678,7 +785,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -698,7 +805,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "await") + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) } @Test @@ -722,7 +833,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -741,7 +852,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings) + assertThat(findings).isEmpty() } @Test @@ -757,26 +868,292 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall(findings, "delay") - } - - private fun assertFindingsForSuspendCall(findings: List, vararg funCallExpression: String) { - assertThat(findings).hasSize(funCallExpression.size) - assertThat(findings.map { it.message }).containsExactlyInAnyOrder( - *funCallExpression.map { - "The suspend function call $it called inside " + - "`runCatching`. You should either use specific `try-catch` only catching exception that you " + - "are expecting or rethrow the `CancellationException` if already caught." - }.toTypedArray() + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 5)), + listOf(SourceLocation(4, 16)) ) } - private fun assertFindingsForSuspendingIterator(findings: List, size: Int) { - assertThat(findings).hasSize(size) - assertThat(findings.map { it.message }).allMatch { - return@allMatch it == "The for-loop expression has suspending operator which is called " + - "inside `runCatching`. You should either use specific `try-catch` only catching exception that you " + - "are expecting or rethrow the `CancellationException` if already caught." + @Nested + inner class `Containing suspending operator` { + @Test + fun `does report in case of suspend invoked operator`() { + val code = """ + import kotlinx.coroutines.delay + + class C { + suspend operator fun invoke() = delay(1000L) + } + + suspend fun foo() { + runCatching { + C()() + C().invoke() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) + } + + @Test + fun `does report in case of suspend inc operator`() { + val code = """ + class OperatorClass { + suspend operator fun inc(): OperatorClass = OperatorClass() + suspend operator fun dec(): OperatorClass = OperatorClass() + } + + suspend fun foo() { + runCatching { + var operatorClass = OperatorClass() + operatorClass++ + } + runCatching { + var operatorClass = OperatorClass() + operatorClass-- + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(7, 5), SourceLocation(11, 5)), + listOf(SourceLocation(7, 16), SourceLocation(11, 16)) + ) + } + + @Test + fun `does report in case of suspend not operator`() { + val code = """ + class OperatorClass { + suspend operator fun not() = false + } + + suspend fun foo() { + runCatching { + val operatorClass = OperatorClass() + println(!operatorClass) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + + @Test + fun `does report in case of suspend unaryPlus operator`() { + val code = """ + class OperatorClass { + suspend operator fun unaryPlus() = OperatorClass() + } + + suspend fun foo() { + runCatching { + val operatorClass = OperatorClass() + println(+operatorClass) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + + @Test + fun `does report in case of suspend plus operator`() { + val code = """ + class OperatorClass { + suspend operator fun plus(test: OperatorClass) = test + } + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + println(operatorClass1 + operatorClass2) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + + @Test + fun `does report in case of suspend div operator`() { + val code = """ + class OperatorClass { + suspend operator fun div(value: Int) = OperatorClass() + } + + suspend fun foo() { + runCatching { + val operatorClass = OperatorClass() + println(operatorClass / 2) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + + @Test + fun `does report in case of suspend compareTo operator`() { + val code = """ + class OperatorClass { + suspend operator fun compareTo(operatorClass: OperatorClass) = 0 + } + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + println(operatorClass1 < operatorClass2) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + + @Test + fun `does report in case of suspend plusAssign operator`() { + val code = """ + class OperatorClass { + suspend operator fun plusAssign(operatorClass: OperatorClass) { } + } + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + operatorClass1 += (operatorClass2) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + + @Test + fun `does report in case of suspend divAssign operator`() { + val code = """ + class OperatorClass { + suspend operator fun divAssign(operatorClass: OperatorClass) { + delay(1000) + } + } + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + operatorClass1 /= (operatorClass2) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) } + + @Test + fun `does report in case of suspend rangeTo operator`() { + val code = """ + class OperatorClass { + suspend operator fun rangeTo(operatorClass: OperatorClass) = OperatorClass() + } + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + println(operatorClass1..operatorClass2) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + + @Test + fun `does report in case of suspend times operator`() { + val code = """ + class OperatorClass { + suspend operator fun times(operatorClass: OperatorClass) = OperatorClass() + } + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + println(operatorClass1 * operatorClass2) + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } + } + + private fun assertFindingsForSuspendCall( + findings: List, + listOfStartLocation: List, + listOfEndLocation: List, + ) { + check(listOfEndLocation.size == listOfStartLocation.size) + assertThat(findings).hasSize(listOfStartLocation.size) + assertThat(findings).hasStartSourceLocations(*listOfStartLocation.toTypedArray()) + assertThat(findings).hasEndSourceLocations(*listOfEndLocation.toTypedArray()) } } From 1217a4de232f9e1f885bc305393a8dc80bf3ef1b Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Mon, 16 Jan 2023 20:35:31 +0530 Subject: [PATCH 8/9] Add two TC around operators Add TCs for suspend fun noinline and crossinline lambda --- .../SuspendFunSwallowedCancellationSpec.kt | 978 ++++++++++-------- 1 file changed, 549 insertions(+), 429 deletions(-) diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt index b69ecc93dab..5987f0b6b3d 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt @@ -156,6 +156,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment @Test fun `does not report no suspending function is used inside runBlocking`() { val code = """ + @Suppress("RedundantSuspendModifier") suspend fun foo() { runCatching { Thread.sleep(1000L) @@ -239,413 +240,302 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment assertThat(findings).isEmpty() } - @Test - fun `does report when suspend fun is called inside inline function`() { - val code = """ - import kotlinx.coroutines.delay - - suspend fun foo() { - runCatching { - listOf(1L, 2L, 3L).map { - delay(it) + @Nested + inner class WithLambda { + @Test + fun `does report when suspend fun is called inside inline function`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + runCatching { + listOf(1L, 2L, 3L).map { + delay(it) + } } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(4, 5)), - listOf(SourceLocation(4, 16)) - ) - } + """.trimIndent() - @Test - fun `does report when inside inline function with noinline and cross inline parameters in same order`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch - import kotlinx.coroutines.runBlocking + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(4, 5)), + listOf(SourceLocation(4, 16)) + ) + } - inline fun foo( - noinline noinlineBlock: suspend () -> Unit, - inlineBlock: () -> Unit, - crossinline crossinlineBlock: suspend () -> Unit, - ) = inlineBlock().toString() + MainScope().launch { - noinlineBlock() - } + runBlocking { - crossinlineBlock() - }.toString() - - suspend fun bar() { - runCatching { - foo( - noinlineBlock = { - delay(2000L) - }, - inlineBlock = { delay(1000L) }, - ) { - - } + @Test + fun `does not report when lambda in non suspending inline function is passed as crossinline`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun foo(crossinline block: suspend () -> R) = MainScope().launch { + block() } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(17, 5)), - listOf(SourceLocation(17, 16)) - ) - } - - @Test - fun `does report when inside inline function with noinline and cross inline parameters not in same order`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch - import kotlinx.coroutines.runBlocking - - inline fun foo( - noinline noinlineBlock: suspend () -> Unit, - inlineBlock: () -> Unit, - crossinline crossinlineBlock: suspend () -> Unit, - ) = inlineBlock().toString() + MainScope().launch { - noinlineBlock() - } + runBlocking { - crossinlineBlock() - }.toString() - - suspend fun bar() { - runCatching { - foo( - inlineBlock = { delay(1000L) }, - noinlineBlock = { - delay(2000L) - }, - ) { - + suspend fun bar() { + runCatching { + foo { + delay(1000L) + } } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(17, 5)), - listOf(SourceLocation(17, 16)) - ) - } + """.trimIndent() - @Test - fun `does report when lambda in inline function is passed as crossinline`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } - inline fun foo(crossinline block: suspend () -> R) = MainScope().launch { - block() - } - suspend fun bar() { - runCatching { - foo { - delay(1000L) + @Test + fun `does report when lambda in suspend inline function is passed as crossinline`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + suspend inline fun foo(crossinline block: suspend () -> R) = block() + suspend fun bar() { + runCatching { + foo { + delay(1000L) + } } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).isEmpty() - } + """.trimIndent() - @Test - fun `does not report when lambda parameter chain has noinline function call`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) + ) + } - inline fun inline(block: () -> R) = block() - fun noInline(block: suspend () -> R) = MainScope().launch { block() } - - suspend fun bar() { - runCatching { - inline { - noInline { - inline { - delay(1000) + @Test + fun `does not report when lambda parameter chain has noinline function call`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun inline(block: () -> R) = block() + fun noInline(block: suspend () -> R) = MainScope().launch { block() } + + suspend fun bar() { + runCatching { + inline { + noInline { + inline { + delay(1000) + } } + } - } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).isEmpty() - } + """.trimIndent() - @Test - fun `does report when lambda parameter chain has all inlined function call`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } - inline fun inline(block: () -> R) = block() - - suspend fun bar() { - runCatching { - inline { + @Test + fun `does report when lambda parameter chain has all inlined function call`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun inline(block: () -> R) = block() + + suspend fun bar() { + runCatching { inline { - delay(1000) + inline { + delay(1000) + } } } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(8, 5)), - listOf(SourceLocation(8, 16)) - ) - } - - @Test - fun `does not report when lambda in inline function is passed as noinline`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch - - inline fun foo(noinline block: suspend () -> R) = MainScope().launch { - block() - } - suspend fun suspendFun() = delay(1000) - suspend fun bar() { - runCatching { - foo { - delay(1000L) - } - - val baz = suspend { - delay(1000L) - } - foo(baz) - foo(::suspendFun) - } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).isEmpty() - } - - @Test - fun `does report when suspend fun is called as extension function`() { - val code = """ - import kotlinx.coroutines.delay - - suspend fun List.await() = delay(100L) - - suspend fun foo() { - runCatching { - listOf(1L, 2L, 3L).await() - } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).hasSize(1) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) - ) - } + """.trimIndent() - @Test - fun `does report when suspending iterator is used`() { - val code = """ - import kotlinx.coroutines.delay + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) + } - class SuspendingIterator { - suspend operator fun iterator(): Iterator = iterator { yield("value") } - } - - suspend fun bar() { - runCatching { - for (x in SuspendingIterator()) { - println(x) - } + @Test + fun `does not report when lambda in non suspending inline function is passed as noinline`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + inline fun foo(noinline block: suspend () -> R) = MainScope().launch { + block() } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(8, 5)), - listOf(SourceLocation(8, 16)) - ) - } - - @Test - fun `does report when suspend fun in for subject is used`() { - val code = """ - suspend fun bar() = 10 - - suspend fun foo() { - runCatching { - for (i in 1..bar()) { - println(i) + suspend fun suspendFun() = delay(1000) + suspend fun bar() { + runCatching { + foo { + delay(1000L) + } + + val baz = suspend { + delay(1000L) + } + foo(baz) + foo(::suspendFun) } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(4, 5)), - listOf(SourceLocation(4, 16)) - ) - } + """.trimIndent() - @Test - fun `does report when nested suspending iterator is used`() { - val code = """ - import kotlinx.coroutines.delay + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } - class SuspendingIterator { - suspend operator fun iterator(): Iterator = iterator { yield("value") } - } - - suspend fun bar() { - runCatching { - for (x in SuspendingIterator()) { - for (y in SuspendingIterator()) { - println(x + y) + @Test + fun `does report when lambda in suspend inline function is passed as noinline`() { + val code = """ + import kotlinx.coroutines.delay + + suspend inline fun foo(noinline block: suspend () -> R) = block() + suspend fun bar() { + runCatching { + foo { + delay(1000L) } } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(8, 5)), - listOf(SourceLocation(8, 16)) - ) - } + """.trimIndent() - @Test - fun `does report when nested iterator with one suspending iterator is used`() { - val code = """ - import kotlinx.coroutines.delay + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(5, 5)), + listOf(SourceLocation(5, 16)) + ) + } - class SuspendingIterator { - suspend operator fun iterator(): Iterator = iterator { yield("value") } - } - - suspend fun bar() { - runCatching { - for (x in 1..10) { - for (y in SuspendingIterator()) { - println(x.toString() + y) - } + @Test + fun `does report when suspend fun is called as extension function`() { + val code = """ + import kotlinx.coroutines.delay + @Suppress("RedundantSuspendModifier") + suspend fun List.await() = delay(this.size) + + suspend fun foo() { + runCatching { + listOf(1L, 2L, 3L).await() } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(8, 5)), - listOf(SourceLocation(8, 16)) - ) - } + """.trimIndent() - @Test - fun `does report when suspending iterator is used withing inlined block`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).hasSize(1) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(6, 5)), + listOf(SourceLocation(6, 16)) + ) + } - class SuspendingIterator { - suspend operator fun iterator(): Iterator = iterator { yield("value") } - } + @Test + fun `does report when inside inline function with noinline and cross inline parameters in same order`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + import kotlinx.coroutines.runBlocking + + inline fun foo( + noinline noinlineBlock: suspend () -> Unit, + inlineBlock: () -> Unit, + crossinline crossinlineBlock: suspend () -> Unit, + ) = inlineBlock().toString() + MainScope().launch { + noinlineBlock() + } + runBlocking { + crossinlineBlock() + }.toString() + + suspend fun bar() { + runCatching { + foo( + noinlineBlock = { + delay(2000L) + }, + inlineBlock = { delay(1000L) }, + ) { - inline fun foo(lambda: () -> Unit) { - lambda() - } - - suspend fun bar() { - runCatching { - foo { - for (x in SuspendingIterator()) { - println(x) } } } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertFindingsForSuspendCall( - findings, - listOf(SourceLocation(14, 5)), - listOf(SourceLocation(14, 16)) - ) - } + """.trimIndent() - @Test - fun `does not report when suspending iterator is used withing non inlined block`() { - val code = """ - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.delay - import kotlinx.coroutines.launch + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(17, 5)), + listOf(SourceLocation(17, 16)) + ) + } - class SuspendingIterator { - suspend operator fun iterator(): Iterator = iterator { yield("value") } - } - - suspend fun bar() { - runCatching { - MainScope().launch { - for (x in SuspendingIterator()) { - println(x) + @Test + fun `does report when inside inline function with noinline and cross inline parameters not in same order`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + import kotlinx.coroutines.runBlocking + + inline fun foo( + noinline noinlineBlock: suspend () -> Unit, + inlineBlock: () -> Unit, + crossinline crossinlineBlock: suspend () -> Unit, + ) = inlineBlock().toString() + MainScope().launch { + noinlineBlock() + } + runBlocking { + crossinlineBlock() + }.toString() + + suspend fun bar() { + runCatching { + foo( + inlineBlock = { delay(1000L) }, + noinlineBlock = { + delay(2000L) + }, + ) { + } } } - } - """.trimIndent() + """.trimIndent() - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).isEmpty() + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(17, 5)), + listOf(SourceLocation(17, 16)) + ) + } } @Test - fun `does report when suspend function is invoked`() { + fun `does report when suspend fun in for subject is used`() { val code = """ - import kotlinx.coroutines.delay - + @Suppress("RedundantSuspendModifier") + suspend fun bar() = 10 + suspend fun foo() { - val suspendBlock = suspend { } runCatching { - suspendBlock() + for (i in 1..bar()) { + println(i) + } } } """.trimIndent() @@ -797,6 +687,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment suspend fun foo() { runCatching { + @Suppress("RedundantAsync") MainScope().async { delay(1000L) }.await() @@ -812,30 +703,6 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment ) } - @Test - fun `does not report when suspend block is passed to non inline function`() { - val code = """ - import kotlinx.coroutines.delay - import kotlinx.coroutines.MainScope - import kotlinx.coroutines.launch - - fun bar(lambda: suspend () -> Unit) { - MainScope().launch { lambda() } - } - - suspend fun foo() { - runCatching { - bar { - delay(1000L) - } - } - } - """.trimIndent() - - val findings = subject.compileAndLintWithContext(env, code) - assertThat(findings).isEmpty() - } - @Test fun `does not report when suspend fun is called inside runBlocking`() { val code = """ @@ -859,7 +726,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report when suspend fun is called in string interpolation`() { val code = """ import kotlinx.coroutines.delay - + @Suppress("RedundantSuspendModifier") suspend fun foo() { runCatching { val string = "${'$'}{delay(1000)}" @@ -876,7 +743,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } @Nested - inner class `Containing suspending operator` { + inner class WithOperators { @Test fun `does report in case of suspend invoked operator`() { val code = """ @@ -903,8 +770,9 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } @Test - fun `does report in case of suspend inc operator`() { + fun `does report in case of suspend inc and dec operator`() { val code = """ + @Suppress("RedundantSuspendModifier") class OperatorClass { suspend operator fun inc(): OperatorClass = OperatorClass() suspend operator fun dec(): OperatorClass = OperatorClass() @@ -925,8 +793,37 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(7, 5), SourceLocation(11, 5)), - listOf(SourceLocation(7, 16), SourceLocation(11, 16)) + listOf(SourceLocation(8, 5), SourceLocation(12, 5)), + listOf(SourceLocation(8, 16), SourceLocation(12, 16)) + ) + } + + @Test + fun `does report in case of suspend inc and dec operators called as function`() { + val code = """ + @Suppress("RedundantSuspendModifier") + class OperatorClass { + suspend operator fun inc(): OperatorClass = OperatorClass() + suspend operator fun dec(): OperatorClass = OperatorClass() + } + + suspend fun foo() { + runCatching { + val operatorClass = OperatorClass() + operatorClass.inc() + } + runCatching { + val operatorClass = OperatorClass() + operatorClass.dec() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5), SourceLocation(12, 5)), + listOf(SourceLocation(8, 16), SourceLocation(12, 16)) ) } @@ -934,6 +831,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report in case of suspend not operator`() { val code = """ class OperatorClass { + @Suppress("RedundantSuspendModifier") suspend operator fun not() = false } @@ -948,8 +846,8 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) ) } @@ -957,6 +855,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report in case of suspend unaryPlus operator`() { val code = """ class OperatorClass { + @Suppress("RedundantSuspendModifier") suspend operator fun unaryPlus() = OperatorClass() } @@ -971,8 +870,8 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) ) } @@ -980,6 +879,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report in case of suspend plus operator`() { val code = """ class OperatorClass { + @Suppress("RedundantSuspendModifier") suspend operator fun plus(test: OperatorClass) = test } @@ -995,8 +895,8 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) ) } @@ -1004,6 +904,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report in case of suspend div operator`() { val code = """ class OperatorClass { + @Suppress("RedundantSuspendModifier") suspend operator fun div(value: Int) = OperatorClass() } @@ -1018,8 +919,8 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) ) } @@ -1027,6 +928,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report in case of suspend compareTo operator`() { val code = """ class OperatorClass { + @Suppress("RedundantSuspendModifier") suspend operator fun compareTo(operatorClass: OperatorClass) = 0 } @@ -1042,8 +944,8 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) ) } @@ -1051,6 +953,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report in case of suspend plusAssign operator`() { val code = """ class OperatorClass { + @Suppress("RedundantSuspendModifier") suspend operator fun plusAssign(operatorClass: OperatorClass) { } } @@ -1063,6 +966,30 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) + ) + } + + @Test + fun `does report in case of suspend plus called as plusAssign operator`() { + val code = """ + class C + @Suppress("RedundantSuspendModifier") + suspend operator fun C.plus(i: Int): C = TODO() + + suspend fun f() { + runCatching { + var x = C() + x += 1 + } + + } + """.trimIndent() + val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, @@ -1074,20 +1001,20 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment @Test fun `does report in case of suspend divAssign operator`() { val code = """ - class OperatorClass { - suspend operator fun divAssign(operatorClass: OperatorClass) { - delay(1000) + class OperatorClass { + suspend operator fun divAssign(operatorClass: OperatorClass) { + delay(1000) + } } - } - - suspend fun foo() { - runCatching { - val operatorClass1 = OperatorClass() - val operatorClass2 = OperatorClass() - operatorClass1 /= (operatorClass2) + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + operatorClass1 /= (operatorClass2) + } } - } - """.trimIndent() + """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( @@ -1100,50 +1027,243 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment @Test fun `does report in case of suspend rangeTo operator`() { val code = """ - class OperatorClass { - suspend operator fun rangeTo(operatorClass: OperatorClass) = OperatorClass() - } - - suspend fun foo() { - runCatching { - val operatorClass1 = OperatorClass() - val operatorClass2 = OperatorClass() - println(operatorClass1..operatorClass2) + class OperatorClass { + @Suppress("RedundantSuspendModifier") + suspend operator fun rangeTo(operatorClass: OperatorClass) = OperatorClass() } - } - """.trimIndent() + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + println(operatorClass1..operatorClass2) + } + } + """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) ) } @Test fun `does report in case of suspend times operator`() { val code = """ - class OperatorClass { - suspend operator fun times(operatorClass: OperatorClass) = OperatorClass() - } - - suspend fun foo() { - runCatching { - val operatorClass1 = OperatorClass() - val operatorClass2 = OperatorClass() - println(operatorClass1 * operatorClass2) + class OperatorClass { + @Suppress("RedundantSuspendModifier") + suspend operator fun times(operatorClass: OperatorClass) = OperatorClass() } - } - """.trimIndent() + + suspend fun foo() { + runCatching { + val operatorClass1 = OperatorClass() + val operatorClass2 = OperatorClass() + println(operatorClass1 * operatorClass2) + } + } + """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(6, 5)), - listOf(SourceLocation(6, 16)) + listOf(SourceLocation(7, 5)), + listOf(SourceLocation(7, 16)) ) } + + @Nested + inner class WithSuspendingIterator { + + @Test + fun `does report when suspending iterator is used`() { + val code = """ + import kotlinx.coroutines.delay + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + for (x in SuspendingIterator()) { + println(x) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) + } + + @Test + fun `does report when nested suspending iterator is used`() { + val code = """ + import kotlinx.coroutines.delay + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + for (x in SuspendingIterator()) { + for (y in SuspendingIterator()) { + println(x + y) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) + } + + @Test + fun `does report when nested iterator with one suspending iterator is used`() { + val code = """ + import kotlinx.coroutines.delay + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + for (x in 1..10) { + for (y in SuspendingIterator()) { + println(x.toString() + y) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(8, 5)), + listOf(SourceLocation(8, 16)) + ) + } + + @Test + fun `does report when suspending iterator is used withing inlined block`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + inline fun foo(lambda: () -> Unit) { + lambda() + } + + suspend fun bar() { + runCatching { + foo { + for (x in SuspendingIterator()) { + println(x) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(14, 5)), + listOf(SourceLocation(14, 16)) + ) + } + + @Test + fun `does not report when suspending iterator is used withing non inlined block`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + class SuspendingIterator { + suspend operator fun iterator(): Iterator = iterator { yield("value") } + } + + suspend fun bar() { + runCatching { + MainScope().launch { + for (x in SuspendingIterator()) { + println(x) + } + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } + + @Test + fun `does report when suspend function is invoked`() { + val code = """ + import kotlinx.coroutines.delay + + suspend fun foo() { + val suspendBlock = suspend { } + runCatching { + suspendBlock() + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertFindingsForSuspendCall( + findings, + listOf(SourceLocation(5, 5)), + listOf(SourceLocation(5, 16)) + ) + } + + @Test + fun `does not report when suspend block is passed to non inline function`() { + val code = """ + import kotlinx.coroutines.delay + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + fun bar(lambda: suspend () -> Unit) { + MainScope().launch { lambda() } + } + + suspend fun foo() { + runCatching { + bar { + delay(1000L) + } + } + } + """.trimIndent() + + val findings = subject.compileAndLintWithContext(env, code) + assertThat(findings).isEmpty() + } + } } private fun assertFindingsForSuspendCall( From d7e13093c3d46509384770e0895435c5db28edaa Mon Sep 17 00:00:00 2001 From: Atul Gupta Date: Sat, 25 Feb 2023 04:40:19 +0530 Subject: [PATCH 9/9] Fix detekt error and fix test error --- .../SuspendFunSwallowedCancellationSpec.kt | 75 ++++++++++--------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt index 5987f0b6b3d..708bc67f957 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SuspendFunSwallowedCancellationSpec.kt @@ -246,7 +246,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report when suspend fun is called inside inline function`() { val code = """ import kotlinx.coroutines.delay - + suspend fun foo() { runCatching { listOf(1L, 2L, 3L).map { @@ -270,7 +270,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.MainScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch - + inline fun foo(crossinline block: suspend () -> R) = MainScope().launch { block() } @@ -293,7 +293,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.MainScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch - + suspend inline fun foo(crossinline block: suspend () -> R) = block() suspend fun bar() { runCatching { @@ -318,10 +318,10 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.MainScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch - + inline fun inline(block: () -> R) = block() fun noInline(block: suspend () -> R) = MainScope().launch { block() } - + suspend fun bar() { runCatching { inline { @@ -346,9 +346,9 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.MainScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch - + inline fun inline(block: () -> R) = block() - + suspend fun bar() { runCatching { inline { @@ -374,7 +374,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.MainScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch - + inline fun foo(noinline block: suspend () -> R) = MainScope().launch { block() } @@ -384,7 +384,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment foo { delay(1000L) } - + val baz = suspend { delay(1000L) } @@ -402,7 +402,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report when lambda in suspend inline function is passed as noinline`() { val code = """ import kotlinx.coroutines.delay - + suspend inline fun foo(noinline block: suspend () -> R) = block() suspend fun bar() { runCatching { @@ -426,8 +426,8 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment val code = """ import kotlinx.coroutines.delay @Suppress("RedundantSuspendModifier") - suspend fun List.await() = delay(this.size) - + suspend fun List.await() = delay(this.size.toLong()) + suspend fun foo() { runCatching { listOf(1L, 2L, 3L).await() @@ -451,7 +451,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking - + inline fun foo( noinline noinlineBlock: suspend () -> Unit, inlineBlock: () -> Unit, @@ -461,7 +461,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } + runBlocking { crossinlineBlock() }.toString() - + suspend fun bar() { runCatching { foo( @@ -470,7 +470,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment }, inlineBlock = { delay(1000L) }, ) { - + } } } @@ -491,7 +491,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking - + inline fun foo( noinline noinlineBlock: suspend () -> Unit, inlineBlock: () -> Unit, @@ -501,7 +501,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment } + runBlocking { crossinlineBlock() }.toString() - + suspend fun bar() { runCatching { foo( @@ -510,7 +510,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment delay(2000L) }, ) { - + } } } @@ -748,7 +748,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report in case of suspend invoked operator`() { val code = """ import kotlinx.coroutines.delay - + class C { suspend operator fun invoke() = delay(1000L) } @@ -980,7 +980,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment class C @Suppress("RedundantSuspendModifier") suspend operator fun C.plus(i: Int): C = TODO() - + suspend fun f() { runCatching { var x = C() @@ -1001,6 +1001,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment @Test fun `does report in case of suspend divAssign operator`() { val code = """ + import kotlinx.coroutines.delay class OperatorClass { suspend operator fun divAssign(operatorClass: OperatorClass) { delay(1000) @@ -1014,13 +1015,13 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment operatorClass1 /= (operatorClass2) } } - """.trimIndent() + """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( findings, - listOf(SourceLocation(8, 5)), - listOf(SourceLocation(8, 16)) + listOf(SourceLocation(9, 5)), + listOf(SourceLocation(9, 16)) ) } @@ -1039,7 +1040,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment println(operatorClass1..operatorClass2) } } - """.trimIndent() + """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( @@ -1064,7 +1065,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment println(operatorClass1 * operatorClass2) } } - """.trimIndent() + """.trimIndent() val findings = subject.compileAndLintWithContext(env, code) assertFindingsForSuspendCall( @@ -1081,11 +1082,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report when suspending iterator is used`() { val code = """ import kotlinx.coroutines.delay - + class SuspendingIterator { suspend operator fun iterator(): Iterator = iterator { yield("value") } } - + suspend fun bar() { runCatching { for (x in SuspendingIterator()) { @@ -1107,11 +1108,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report when nested suspending iterator is used`() { val code = """ import kotlinx.coroutines.delay - + class SuspendingIterator { suspend operator fun iterator(): Iterator = iterator { yield("value") } } - + suspend fun bar() { runCatching { for (x in SuspendingIterator()) { @@ -1135,11 +1136,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report when nested iterator with one suspending iterator is used`() { val code = """ import kotlinx.coroutines.delay - + class SuspendingIterator { suspend operator fun iterator(): Iterator = iterator { yield("value") } } - + suspend fun bar() { runCatching { for (x in 1..10) { @@ -1165,7 +1166,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.MainScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch - + class SuspendingIterator { suspend operator fun iterator(): Iterator = iterator { yield("value") } } @@ -1173,7 +1174,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment inline fun foo(lambda: () -> Unit) { lambda() } - + suspend fun bar() { runCatching { foo { @@ -1199,11 +1200,11 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.MainScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch - + class SuspendingIterator { suspend operator fun iterator(): Iterator = iterator { yield("value") } } - + suspend fun bar() { runCatching { MainScope().launch { @@ -1223,7 +1224,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment fun `does report when suspend function is invoked`() { val code = """ import kotlinx.coroutines.delay - + suspend fun foo() { val suspendBlock = suspend { } runCatching { @@ -1246,7 +1247,7 @@ class SuspendFunSwallowedCancellationSpec(private val env: KotlinCoreEnvironment import kotlinx.coroutines.delay import kotlinx.coroutines.MainScope import kotlinx.coroutines.launch - + fun bar(lambda: suspend () -> Unit) { MainScope().launch { lambda() } }