diff --git a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelay.kt b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelay.kt index 17895bfcfc2..1501342a6cd 100644 --- a/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelay.kt +++ b/detekt-rules-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelay.kt @@ -15,12 +15,13 @@ import org.jetbrains.kotlin.descriptors.FunctionDescriptor import org.jetbrains.kotlin.psi.KtCallExpression import org.jetbrains.kotlin.psi.KtCallableReferenceExpression import org.jetbrains.kotlin.psi.KtExpression -import org.jetbrains.kotlin.psi.KtFunctionLiteral -import org.jetbrains.kotlin.psi.KtLambdaArgument +import org.jetbrains.kotlin.psi.KtLambdaExpression import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtProperty import org.jetbrains.kotlin.psi.KtValueArgument import org.jetbrains.kotlin.psi.psiUtil.getParentOfType import org.jetbrains.kotlin.psi.psiUtil.getParentOfTypes +import org.jetbrains.kotlin.psi.psiUtil.getParentOfTypesAndPredicate import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.calls.util.getParameterForArgument import org.jetbrains.kotlin.resolve.calls.util.getResolvedCall @@ -66,7 +67,7 @@ class SleepInsteadOfDelay(config: Config = Config.empty) : Rule(config) { } private fun checkAndReport(expression: KtExpression) { - if (expression.isThreadSleepFunction() && expression.isInSuspendBlock()) { + if (expression.isThreadSleepFunction() && shouldReport(expression)) { report(CodeSmell(issue, Entity.from(expression), SUSPEND_FUN_MESSAGE)) } } @@ -90,49 +91,88 @@ class SleepInsteadOfDelay(config: Config = Config.empty) : Rule(config) { } } - private fun PsiElement.isSuspendingBlock(): Boolean { - return when (this) { - is KtFunctionLiteral -> { - val psiParent = this.parent.parent - if (psiParent is KtLambdaArgument) { - psiParent.isSuspendAllowedLambdaArgument() - } else { - this.isSuspendScope() + @Suppress("ReturnCount") + private fun getNearestParentForSuspension(psiElement: PsiElement): PsiElement? { + fun KtValueArgument.isNearestParentForSuspension(): Boolean { + val parent = this.getParentOfTypes(true, KtCallExpression::class.java) ?: return false + val callDescriptor = parent.getResolvedCall(bindingContext) + val valueParameterDescriptor = + parent.getResolvedCall(bindingContext) + ?.getParameterForArgument(this) + ?: return false + val functionDescriptor = callDescriptor?.resultingDescriptor as? FunctionDescriptor ?: return false + return functionDescriptor.isInline.not() || + (valueParameterDescriptor.isNoinline || valueParameterDescriptor.isCrossinline) + } + return psiElement.getParentOfTypesAndPredicate( + false, + KtNamedFunction::class.java, + KtValueArgument::class.java, + KtLambdaExpression::class.java, + ) { + when (it) { + is KtValueArgument -> { + it.isNearestParentForSuspension() } + + is KtNamedFunction -> { + true + } + + is KtLambdaExpression -> { + it.getParentOfType(true, KtValueArgument::class.java) != null + } + + else -> { + false + } + } + } + } + + private fun PsiElement.isSuspendAllowed(): Boolean { + return when (this) { + is KtValueArgument -> { + this.isSuspendAllowed() } + is KtNamedFunction -> { - this.isSuspendScope() + this.isSuspendAllowed() } + + is KtLambdaExpression -> { + this.isSuspendAllowed() + } + else -> { false } } } - private fun KtLambdaArgument.isSuspendAllowedLambdaArgument(): Boolean { - val callDescriptor = getParentOfType(true)?.getResolvedCall(bindingContext) ?: return false - val functionDescriptor = callDescriptor.resultingDescriptor as? FunctionDescriptor ?: return false - val valueParameterDescriptor = callDescriptor.getParameterForArgument(this) ?: return false + private fun KtValueArgument.isSuspendAllowed(): Boolean { + val parent = this.getParentOfTypes(true, KtCallExpression::class.java) ?: return false + val valueParameterDescriptor = + parent.getResolvedCall(bindingContext) + ?.getParameterForArgument(this) + return valueParameterDescriptor?.returnType?.isSuspendFunctionType == true + } - return ( - functionDescriptor.isInline && - (valueParameterDescriptor.isNoinline.not() && valueParameterDescriptor.isCrossinline.not()) - ) || valueParameterDescriptor.returnType?.isSuspendFunctionType == true + private fun KtLambdaExpression.isSuspendAllowed(): Boolean { + val parent = this.getParentOfTypes(true, KtProperty::class.java) + ?: return false + val properDescriptor = bindingContext[BindingContext.VARIABLE, parent] ?: return false + return properDescriptor.returnType?.isSuspendFunctionType ?: false } - private fun PsiElement.isSuspendScope(): Boolean { + private fun KtNamedFunction.isSuspendAllowed(): Boolean { val functionDescriptor = bindingContext[BindingContext.FUNCTION, this] ?: return false return functionDescriptor.isSuspend } - private fun KtExpression.isInSuspendBlock(): Boolean { - val containingBlockExpression = - this.getParentOfTypes( - true, - KtNamedFunction::class.java, - KtFunctionLiteral::class.java - ) ?: return false - return containingBlockExpression.isSuspendingBlock() + private fun shouldReport(expression: KtExpression): Boolean { + val nearestParentForSuspension = getNearestParentForSuspension(expression) ?: return false + return nearestParentForSuspension.isSuspendAllowed() } companion object { diff --git a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelaySpec.kt b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelaySpec.kt index 278fd969ed0..79b02d04201 100644 --- a/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelaySpec.kt +++ b/detekt-rules-coroutines/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/SleepInsteadOfDelaySpec.kt @@ -6,6 +6,7 @@ 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.DisplayName +import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test @Suppress("BlockingMethodInNonBlockingContext", "RedundantSuspendModifier") @@ -18,7 +19,6 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) { fun `should report no issue for delay() in suspend functions`() { val code = """ import kotlinx.coroutines.delay - suspend fun foo() { delay(1000L) } @@ -168,25 +168,33 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) { } @Test - fun `should report Thread sleep() called inside variable declaration`() { + fun `should report Thread sleep() called in custom function inside suspend lambda`() { val code = """ - import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.MainScope import kotlinx.coroutines.launch - val t = GlobalScope.launch { - Thread.sleep(1000L) + fun suspendBlock(lambda: suspend () -> Unit) { + MainScope().launch { + lambda() + } + } + + fun test() { + suspendBlock { + Thread.sleep(1000L) + } } """.trimIndent() assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) } @Test - fun `should report Thread sleep() called in custom function inside suspend lambda`() { + fun `should report Thread sleep() called in custom function inside suspend lambda wth type in braces`() { val code = """ import kotlinx.coroutines.MainScope import kotlinx.coroutines.launch - fun suspendBlock(lambda: suspend () -> Unit) { + fun suspendBlock(lambda: (suspend () -> Unit)) { MainScope().launch { lambda() } @@ -201,6 +209,108 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) { assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) } + @Test + fun `should report Thread sleep() called in custom function inside suspend lambda with lambda in braces`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + fun suspendBlock(lambda: suspend () -> Unit) { + MainScope().launch { + lambda() + } + } + + fun test() { + suspendBlock({ + Thread.sleep(1000L) + }) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() called in custom function inside suspend lambda with named lambda`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + fun suspendBlock(lambda: suspend () -> Unit) { + MainScope().launch { + lambda() + } + } + + fun test() { + suspendBlock(lambda = { + Thread.sleep(1000L) + }) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() called in custom function inside suspend lambda with named lambda with braces`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + fun suspendBlock(lambda: suspend () -> Unit) { + MainScope().launch { + lambda() + } + } + + fun test() { + suspendBlock(lambda = ({ + Thread.sleep(1000L) + })) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() callable ref called in custom function inside suspend lambda`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.launch + + fun suspendBlock(lambda: (suspend (Long) -> Unit)) { + MainScope().launch { + lambda(1000) + } + } + + fun test1() { + suspendBlock(lambda = Thread::sleep) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should not report delay callable ref called in custom function inside suspend lambda`() { + val code = """ + import kotlinx.coroutines.MainScope + import kotlinx.coroutines.delay + import kotlinx.coroutines.launch + + fun suspendBlock(lambda: (suspend (Long) -> Unit)) { + MainScope().launch { + lambda(1000) + } + } + + fun test1() { + suspendBlock(lambda = ::delay) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + @Test fun `should report Thread sleep() called in custom inline function inside suspend crossinline lambda`() { val code = """ @@ -301,24 +411,6 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) { assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) } - @Test - fun `should not report Thread sleep() when used a callable reference variable`() { - @Suppress("DeferredResultUnused") - val code = """ - import kotlinx.coroutines.Dispatchers - import kotlinx.coroutines.GlobalScope - import kotlinx.coroutines.launch - import kotlinx.coroutines.withContext - - fun test() { - GlobalScope.launch { - val funRef: (Long) -> Unit = Thread::sleep - } - } - """.trimIndent() - assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() - } - @Test fun `should not report Thread sleep() when used inside map as callable reference variable`() { @Suppress("DeferredResultUnused") @@ -380,56 +472,66 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) { } @Test - fun `should report Thread sleep() called in when called inside custom inline function`() { + fun `should report Thread sleep() called in nested inline function inside coroutine scope lambda`() { val code = """ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.launch import kotlinx.coroutines.withContext - inline fun inlineFun(lambda: () -> Unit) = lambda() - - suspend fun test() { - inlineFun { Thread.sleep(1000L) } + fun test() { + GlobalScope.launch { + listOf(emptyList()).map { delays -> + delays.map { + Thread.sleep(it) + } + } + } } """.trimIndent() assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) } @Test - fun `should report Thread sleep() called inside suspend lambda variable`() { + fun `should not report Thread sleep() called in nested inline function with noinline function in-between`() { val code = """ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.delay import kotlinx.coroutines.launch + import kotlin.concurrent.thread import kotlinx.coroutines.withContext fun test() { GlobalScope.launch { - val localLambda: suspend () -> Unit = { - Thread.sleep(1000) + listOf(emptyList()).map { delays -> + thread { + delays.map { + Thread.sleep(it) + } + } } } } """.trimIndent() - assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() } @Test - fun `should not report Thread sleep() called inside non-suspend lambda variable`() { + fun `should report Thread sleep() called in when called inside custom inline function`() { val code = """ + import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.launch + import kotlinx.coroutines.withContext + inline fun inlineFun(lambda: () -> Unit) = lambda() + suspend fun test() { - GlobalScope.launch { - val localLambda: () -> Unit = { - Thread.sleep(1000) - } - } + inlineFun { Thread.sleep(1000L) } } """.trimIndent() - assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) } @Test @@ -540,7 +642,7 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) { } @Test - fun `should report Thread sleep() when called isnide for block`() { + fun `should report Thread sleep() when called inside for block`() { val code = """ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.GlobalScope @@ -556,4 +658,191 @@ class SleepInsteadOfDelaySpec(private val env: KotlinCoreEnvironment) { """.trimIndent() assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) } + + @Test + fun `#6176 - should not report when inlined lambda is inside non suspending function`() { + val code = """ + class OhBoy { + private val lock = Object() + + fun get(a: Int, b: Int) { + synchronized(lock) { + Thread.sleep(500) + } + } + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `should not report when inlined lambda is is non suspending inner fun with outer is fun suspend`() { + val code = """ + suspend fun test() { + fun testInner() { + synchronized(Unit) { + Thread.sleep(1000) + } + } + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Nested + inner class `Given suspend function variable` { + @Test + fun `should report Thread sleep() in suspend variable`() { + val code = """ + val foo = suspend { + Thread.sleep(1000) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() in suspend lambda`() { + val code = """ + val foo: suspend () -> Unit = { + Thread.sleep(1000) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() in suspend lambda with type inside braces`() { + val code = """ + val foo: (suspend () -> Unit) = { + Thread.sleep(1000) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() in suspend lambda with expression inside braces`() { + val code = """ + val foo: suspend () -> Unit = ({ + Thread.sleep(1000) + }) + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() in suspend lambda with variable`() { + val code = """ + val foo: suspend (Int, Int) -> Unit = { bar1, bar2 -> + Thread.sleep(1000) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should not report Thread sleep() in non-suspending lambda`() { + val code = """ + val foo: () -> Unit = { + Thread.sleep(1000) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `should not report Thread sleep() in non-suspending lambda inside suspend fun`() { + val code = """ + suspend fun foo() { + val bar: () -> Unit = { + Thread.sleep(1000) + } + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `should report Thread sleep() in suspend lambda inside non-suspending fun`() { + val code = """ + fun foo() { + val bar: suspend () -> Unit = { + Thread.sleep(1000) + } + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Nested + inner class `Given inside launch` { + @Test + fun `should report Thread sleep() called inside variable declaration inside GlobalScope launch`() { + val code = """ + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.launch + + val t = GlobalScope.launch { + Thread.sleep(1000L) + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should report Thread sleep() called inside suspend lambda variable`() { + val code = """ + import kotlinx.coroutines.Dispatchers + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.launch + import kotlinx.coroutines.withContext + + fun test() { + GlobalScope.launch { + val localLambda: suspend () -> Unit = { + Thread.sleep(1000) + } + } + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1) + } + + @Test + fun `should not report Thread sleep() called inside non-suspend lambda variable`() { + val code = """ + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.launch + + suspend fun test() { + GlobalScope.launch { + val localLambda: () -> Unit = { + Thread.sleep(1000) + } + } + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + + @Test + fun `should not report Thread sleep() when used a callable reference variable`() { + @Suppress("DeferredResultUnused") + val code = """ + import kotlinx.coroutines.Dispatchers + import kotlinx.coroutines.GlobalScope + import kotlinx.coroutines.launch + import kotlinx.coroutines.withContext + + fun test() { + GlobalScope.launch { + val funRef: (Long) -> Unit = Thread::sleep + } + } + """.trimIndent() + assertThat(subject.compileAndLintWithContext(env, code)).isEmpty() + } + } + } }